Merge pull request #837 from ashanbrown/asb/nolintlint-internal

Add nolintlint linter as internal linter
This commit is contained in:
Aleksandr Razumov 2020-04-30 03:20:28 +03:00 committed by GitHub
commit eeff3902d4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
50 changed files with 765 additions and 62 deletions

View File

@ -224,6 +224,17 @@ linters-settings:
simple: true
range-loops: true # Report preallocation suggestions on range loops, true by default
for-loops: false # Report preallocation suggestions on for loops, false by default
nolintlint:
# Enable to ensure that nolint directives are all used. Default is true.
allow-unused: false
# Disable to ensure that nolint directives don't have a leading space. Default is true.
allow-leading-space: true
# Exclude following linters from requiring an explanation. Default is [].
allow-no-explanation: []
# Enable to require an explanation after each nolint directive. Default is false.
require-explanation: true
# Enable to require an explanation after each nolint directive. Default is false.
require-specific: true
rowserrcheck:
packages:
- github.com/jmoiron/sqlx

View File

@ -54,6 +54,11 @@ linters-settings:
suggest-new: true
misspell:
locale: US
nolintlint:
allow-leading-space: true # don't require machine-readable nolint directives (i.e. with no leading space)
allow-unused: false # report any unused nolint directives
require-explanation: false # don't require an explanation for nolint directives
require-specific: false # don't require nolint directives to be specific about which linter is being skipped
linters:
# please, do not use `enable-all`: it's deprecated and will be removed soon.
@ -84,6 +89,7 @@ linters:
- lll
- misspell
- nakedret
- nolintlint
- rowserrcheck
- scopelint
- staticcheck

View File

@ -232,6 +232,7 @@ maligned: Tool to detect Go structs that would take less memory if their fields
misspell: Finds commonly misspelled English words in comments [fast: true, auto-fix: true]
nakedret: Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false]
nestif: Reports deeply nested if statements [fast: true, auto-fix: false]
nolintlint: Reports ill-formed or insufficient nolint directives [fast: true, auto-fix: false]
prealloc: Finds slice declarations that could potentially be preallocated [fast: true, auto-fix: false]
rowserrcheck: checks whether Err of rows is checked successfully [fast: true, auto-fix: false]
scopelint: Scopelint checks for unpinned variables in go programs [fast: true, auto-fix: false]
@ -498,6 +499,7 @@ golangci-lint help linters
- [godot](https://github.com/tetafro/godot) - Check if comments end in a period
- [testpackage](https://github.com/maratori/testpackage) - linter that makes you use a separate _test package
- [nestif](https://github.com/nakabonne/nestif) - Reports deeply nested if statements
- [nolintlint](https://github.com/golangci-lint/pkg/golinters/nolintlint) - Reports ill-formed or insufficient nolint directives
## Configuration
@ -845,6 +847,17 @@ linters-settings:
simple: true
range-loops: true # Report preallocation suggestions on range loops, true by default
for-loops: false # Report preallocation suggestions on for loops, false by default
nolintlint:
# Enable to ensure that nolint directives are all used. Default is true.
allow-unused: false
# Disable to ensure that nolint directives don't have a leading space. Default is true.
allow-leading-space: true
# Exclude following linters from requiring an explanation. Default is [].
allow-no-explanation: []
# Enable to require an explanation after each nolint directive. Default is false.
require-explanation: true
# Enable to require an explanation after each nolint directive. Default is false.
require-specific: true
rowserrcheck:
packages:
- github.com/jmoiron/sqlx
@ -1034,6 +1047,11 @@ linters-settings:
suggest-new: true
misspell:
locale: US
nolintlint:
allow-leading-space: true # don't require machine-readable nolint directives (i.e. with no leading space)
allow-unused: false # report any unused nolint directives
require-explanation: false # don't require an explanation for nolint directives
require-specific: false # don't require nolint directives to be specific about which linter is being skipped
linters:
# please, do not use `enable-all`: it's deprecated and will be removed soon.
@ -1064,6 +1082,7 @@ linters:
- lll
- misspell
- nakedret
- nolintlint
- rowserrcheck
- scopelint
- staticcheck
@ -1301,6 +1320,7 @@ Thanks to developers and authors of used linters:
- [tetafro](https://github.com/tetafro)
- [maratori](https://github.com/maratori)
- [nakabonne](https://github.com/nakabonne)
- [golangci-lint](https://github.com/golangci-lint)
## Changelog

View File

@ -441,7 +441,7 @@ func (e *Executor) executeRun(_ *cobra.Command, args []string) {
// to be removed when deadline is finally decommissioned
func (e *Executor) setTimeoutToDeadlineIfOnlyDeadlineIsSet() {
//lint:ignore SA1019 We want to promoted the deprecated config value when needed
deadlineValue := e.cfg.Run.Deadline // nolint: staticcheck
deadlineValue := e.cfg.Run.Deadline // nolint:staticcheck
if deadlineValue != 0 && e.cfg.Run.Timeout == defaultTimeout {
e.cfg.Run.Timeout = deadlineValue
}

View File

@ -235,6 +235,7 @@ type LintersSettings struct {
Godot GodotSettings
Testpackage TestpackageSettings
Nestif NestifSettings
NoLintLint NoLintLintSettings
Custom map[string]CustomLinterSettings
}
@ -316,6 +317,14 @@ type GodotSettings struct {
CheckAll bool `mapstructure:"check-all"`
}
type NoLintLintSettings struct {
RequireExplanation bool `mapstructure:"require-explanation"`
AllowLeadingSpace bool `mapstructure:"allow-leading-space"`
RequireSpecific bool `mapstructure:"require-specific"`
AllowNoExplanation []string `mapstructure:"allow-no-explanation"`
AllowUnused bool `mapstructure:"allow-unused"`
}
type TestpackageSettings struct {
SkipRegexp string `mapstructure:"skip-regexp"`
}
@ -324,7 +333,6 @@ type NestifSettings struct {
MinComplexity int `mapstructure:"min-complexity"`
}
//nolint:gomnd
var defaultLintersSettings = LintersSettings{
Lll: LllSettings{
LineLength: 120,
@ -363,6 +371,12 @@ var defaultLintersSettings = LintersSettings{
ForceCuddleErrCheckAndAssign: false,
ForceCaseTrailingWhitespaceLimit: 0,
},
NoLintLint: NoLintLintSettings{
RequireExplanation: false,
AllowLeadingSpace: true,
RequireSpecific: false,
AllowUnused: false,
},
Testpackage: TestpackageSettings{
SkipRegexp: `(export|internal)_test\.go`,
},

View File

@ -28,7 +28,7 @@ func NewDeadcode() *goanalysis.Linter {
}
res := make([]goanalysis.Issue, 0, len(issues))
for _, i := range issues {
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: i.Pos,
Text: fmt.Sprintf("%s is unused", formatCode(i.UnusedIdentName, nil)),
FromLinter: linterName,

View File

@ -94,7 +94,7 @@ func NewDepguard() *goanalysis.Linter {
if userSuppliedMsgSuffix != "" {
userSuppliedMsgSuffix = ": " + userSuppliedMsgSuffix
}
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: i.Position,
Text: fmt.Sprintf("%s %s%s", formatCode(i.PackageName, lintCtx.Cfg), msgSuffix, userSuppliedMsgSuffix),
FromLinter: linterName,

View File

@ -57,7 +57,7 @@ func NewDupl() *goanalysis.Linter {
text := fmt.Sprintf("%d-%d lines are duplicate of %s",
i.From.LineStart(), i.From.LineEnd(),
formatCode(dupl, lintCtx.Cfg))
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: token.Position{
Filename: i.From.Filename(),
Line: i.From.LineStart(),

View File

@ -59,7 +59,7 @@ func NewErrcheck() *goanalysis.Linter {
} else {
text = "Error return value is not checked"
}
issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
issues = append(issues, goanalysis.NewIssue(&result.Issue{
FromLinter: linterName,
Text: text,
Pos: i.Pos,

View File

@ -42,7 +42,7 @@ func NewFunlen() *goanalysis.Linter {
res := make([]goanalysis.Issue, len(issues))
for k, i := range issues {
res[k] = goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res[k] = goanalysis.NewIssue(&result.Issue{
Pos: token.Position{
Filename: i.Pos.Filename,
Line: i.Pos.Line,

View File

@ -21,9 +21,11 @@ func NewIssue(i *result.Issue, pass *analysis.Pass) Issue {
}
type EncodingIssue struct {
FromLinter string
Text string
Pos token.Position
LineRange *result.Range
Replacement *result.Replacement
FromLinter string
Text string
Pos token.Position
LineRange *result.Range
Replacement *result.Replacement
ExpectNoLint bool
ExpectedNoLintLinter string
}

View File

@ -323,11 +323,13 @@ func saveIssuesToCache(allPkgs []*packages.Package, pkgsFromCache map[*packages.
for ind := range pkgIssues {
i := &pkgIssues[ind]
encodedIssues = append(encodedIssues, EncodingIssue{
FromLinter: i.FromLinter,
Text: i.Text,
Pos: i.Pos,
LineRange: i.LineRange,
Replacement: i.Replacement,
FromLinter: i.FromLinter,
Text: i.Text,
Pos: i.Pos,
LineRange: i.LineRange,
Replacement: i.Replacement,
ExpectNoLint: i.ExpectNoLint,
ExpectedNoLintLinter: i.ExpectedNoLintLinter,
})
}
@ -392,12 +394,14 @@ func loadIssuesFromCache(pkgs []*packages.Package, lintCtx *linter.Context,
issues := make([]result.Issue, 0, len(pkgIssues))
for _, i := range pkgIssues {
issues = append(issues, result.Issue{
FromLinter: i.FromLinter,
Text: i.Text,
Pos: i.Pos,
LineRange: i.LineRange,
Replacement: i.Replacement,
Pkg: pkg,
FromLinter: i.FromLinter,
Text: i.Text,
Pos: i.Pos,
LineRange: i.LineRange,
Replacement: i.Replacement,
Pkg: pkg,
ExpectNoLint: i.ExpectNoLint,
ExpectedNoLintLinter: i.ExpectedNoLintLinter,
})
}
cacheRes.issues = issues

View File

@ -312,7 +312,7 @@ func (r *runner) analyze(pkgs []*packages.Package, analyzers []*analysis.Analyze
debugf("There are %d initial and %d total packages", len(initialPkgs), len(loadingPackages))
for _, lp := range loadingPackages {
if lp.isInitial {
wg.Add(1) //nolint:gomnd
wg.Add(1)
go func(lp *loadingPackage) {
lp.analyzeRecursive(r.loadMode, loadSem)
wg.Done()

View File

@ -49,7 +49,7 @@ func NewGocognit() *goanalysis.Linter {
break // Break as the stats is already sorted from greatest to least
}
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: s.Pos,
Text: fmt.Sprintf("cognitive complexity %d of func %s is high (> %d)",
s.Complexity, formatCode(s.FuncName, lintCtx.Cfg), lintCtx.Settings().Gocognit.MinComplexity),

View File

@ -70,7 +70,7 @@ func checkConstants(pass *analysis.Pass, lintCtx *linter.Context) ([]goanalysis.
} else {
textEnd = fmt.Sprintf(", but such constant %s already exists", formatCode(i.MatchingConst, lintCtx.Cfg))
}
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: i.Pos,
Text: textBegin + textEnd,
FromLinter: goconstName,

View File

@ -49,7 +49,7 @@ func NewGocyclo() *goanalysis.Linter {
break // Break as the stats is already sorted from greatest to least
}
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: s.Pos,
Text: fmt.Sprintf("cyclomatic complexity %d of func %s is high (> %d)",
s.Complexity, formatCode(s.FuncName, lintCtx.Cfg), lintCtx.Settings().Gocyclo.MinComplexity),

View File

@ -41,7 +41,7 @@ func NewGodox() *goanalysis.Linter {
res := make([]goanalysis.Issue, len(issues))
for k, i := range issues {
res[k] = goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res[k] = goanalysis.NewIssue(&result.Issue{
Pos: token.Position{
Filename: i.Pos.Filename,
Line: i.Pos.Line,

View File

@ -59,7 +59,6 @@ func (p *hunkChangesParser) parseDiffLines(h *diffpkg.Hunk) {
lineStr := string(line)
//nolint:gocritic
if strings.HasPrefix(lineStr, "-") {
dl.typ = diffLineDeleted
dl.data = strings.TrimPrefix(lineStr, "-")

View File

@ -73,7 +73,7 @@ func NewGomodguard() *goanalysis.Linter {
defer mu.Unlock()
for _, err := range gomodguardErrors {
issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
issues = append(issues, goanalysis.NewIssue(&result.Issue{
FromLinter: gomodguardName,
Pos: err.Position,
Text: err.Reason,

View File

@ -73,7 +73,7 @@ func NewGosec() *goanalysis.Linter {
continue
}
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: token.Position{
Filename: i.File,
Line: line,

View File

@ -42,7 +42,7 @@ func NewIneffassign() *goanalysis.Linter {
res := make([]goanalysis.Issue, 0, len(issues))
for _, i := range issues {
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: i.Pos,
Text: fmt.Sprintf("ineffectual assignment to %s", formatCode(i.IdentName, lintCtx.Cfg)),
FromLinter: ineffassignName,

View File

@ -48,7 +48,7 @@ func NewInterfacer() *goanalysis.Linter {
res := make([]goanalysis.Issue, 0, len(issues))
for _, i := range issues {
pos := pass.Fset.Position(i.Pos())
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: pos,
Text: i.Message(),
FromLinter: interfacerName,

View File

@ -40,7 +40,7 @@ func NewMaligned() *goanalysis.Linter {
if lintCtx.Settings().Maligned.SuggestNewOrder {
text += fmt.Sprintf(":\n%s", formatCodeBlock(i.NewStructDef, lintCtx.Cfg))
}
issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
issues = append(issues, goanalysis.NewIssue(&result.Issue{
Pos: i.Pos,
Text: text,
FromLinter: linterName,

View File

@ -46,7 +46,7 @@ func NewNestif() *goanalysis.Linter {
res := make([]goanalysis.Issue, 0, len(issues))
for _, i := range issues {
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: i.Pos,
Text: i.Message,
FromLinter: nestifName,

View File

@ -0,0 +1,92 @@
package golinters
import (
"fmt"
"go/ast"
"sync"
"golang.org/x/tools/go/analysis"
"github.com/golangci/golangci-lint/pkg/golinters/goanalysis"
"github.com/golangci/golangci-lint/pkg/golinters/nolintlint"
"github.com/golangci/golangci-lint/pkg/lint/linter"
"github.com/golangci/golangci-lint/pkg/result"
)
const NolintlintName = "nolintlint"
func NewNoLintLint() *goanalysis.Linter {
var mu sync.Mutex
var resIssues []goanalysis.Issue
analyzer := &analysis.Analyzer{
Name: NolintlintName,
Doc: goanalysis.TheOnlyanalyzerDoc,
}
return goanalysis.NewLinter(
NolintlintName,
"Reports ill-formed or insufficient nolint directives",
[]*analysis.Analyzer{analyzer},
nil,
).WithContextSetter(func(lintCtx *linter.Context) {
analyzer.Run = func(pass *analysis.Pass) (interface{}, error) {
var needs nolintlint.Needs
settings := lintCtx.Settings().NoLintLint
if settings.RequireExplanation {
needs |= nolintlint.NeedsExplanation
}
if !settings.AllowLeadingSpace {
needs |= nolintlint.NeedsMachineOnly
}
if settings.RequireSpecific {
needs |= nolintlint.NeedsSpecific
}
if !settings.AllowUnused {
needs |= nolintlint.NeedsUnused
}
lnt, err := nolintlint.NewLinter(needs, settings.AllowNoExplanation)
if err != nil {
return nil, err
}
nodes := make([]ast.Node, 0, len(pass.Files))
for _, n := range pass.Files {
nodes = append(nodes, n)
}
issues, err := lnt.Run(pass.Fset, nodes...)
if err != nil {
return nil, fmt.Errorf("linter failed to run: %s", err)
}
var res []goanalysis.Issue
for _, i := range issues {
expectNoLint := false
var expectedNolintLinter string
if ii, ok := i.(nolintlint.UnusedCandidate); ok {
expectedNolintLinter = ii.ExpectedLinter
expectNoLint = true
}
issue := &result.Issue{
FromLinter: NolintlintName,
Text: i.Details(),
Pos: i.Position(),
ExpectNoLint: expectNoLint,
ExpectedNoLintLinter: expectedNolintLinter,
}
res = append(res, goanalysis.NewIssue(issue, pass))
}
if len(res) == 0 {
return nil, nil
}
mu.Lock()
resIssues = append(resIssues, res...)
mu.Unlock()
return nil, nil
}
}).WithIssuesReporter(func(*linter.Context) []goanalysis.Issue {
return resIssues
}).WithLoadMode(goanalysis.LoadModeSyntax)
}

View File

@ -0,0 +1,31 @@
# nolintlint
nolintlint is a Go static analysis tool to find ill-formed or insufficiently explained `// nolint` directives for golangci
(or any other linter, using th )
## Purpose
To ensure that lint exceptions have explanations. Consider the case below:
```Go
import "crypto/md5" //nolint
func hash(data []byte) []byte {
return md5.New().Sum(data) //nolint
}
```
In the above case, nolint directives are present but the user has no idea why this is being done or which linter
is being suppressed (in this case, gosec recommends against use of md5). `nolintlint` can require that the code provide an explanation, which might look as follows:
```Go
import "crypto/md5" //nolint:gosec // this is not used in a secure application
func hash(data []byte) []byte {
return md5.New().Sum(data) //nolint:gosec // this result is not used in a secure application
}
```
`nolintlint` can also identify cases where you may have written `// nolint`. Finally `nolintlint`, can also enforce that you
use the machine-readable nolint directive format `//nolint` and that you mention what linter is being suppressed, as shown above when we write `//nolint:gosec`.

View File

@ -0,0 +1,239 @@
// nolintlint provides a linter to ensure that all //nolint directives are followed by explanations
package nolintlint
import (
"fmt"
"go/ast"
"go/token"
"regexp"
"strings"
"unicode"
)
type BaseIssue struct {
fullDirective string
directiveWithOptionalLeadingSpace string
position token.Position
}
func (b BaseIssue) Position() token.Position {
return b.position
}
type ExtraLeadingSpace struct {
BaseIssue
}
func (i ExtraLeadingSpace) Details() string {
return fmt.Sprintf("directive `%s` should not have more than one leading space", i.fullDirective)
}
func (i ExtraLeadingSpace) String() string { return toString(i) }
type NotMachine struct {
BaseIssue
}
func (i NotMachine) Details() string {
expected := i.fullDirective[:2] + strings.TrimLeftFunc(i.fullDirective[2:], unicode.IsSpace)
return fmt.Sprintf("directive `%s` should be written without leading space as `%s`",
i.fullDirective, expected)
}
func (i NotMachine) String() string { return toString(i) }
type NotSpecific struct {
BaseIssue
}
func (i NotSpecific) Details() string {
return fmt.Sprintf("directive `%s` should mention specific linter such as `//%s:my-linter`",
i.fullDirective, i.directiveWithOptionalLeadingSpace)
}
func (i NotSpecific) String() string { return toString(i) }
type ParseError struct {
BaseIssue
}
func (i ParseError) Details() string {
return fmt.Sprintf("directive `%s` should match `//%s[:<comma-separated-linters>] [// <explanation>]`",
i.fullDirective,
i.directiveWithOptionalLeadingSpace)
}
func (i ParseError) String() string { return toString(i) }
type NoExplanation struct {
BaseIssue
fullDirectiveWithoutExplanation string
}
func (i NoExplanation) Details() string {
return fmt.Sprintf("directive `%s` should provide explanation such as `%s // this is why`",
i.fullDirective, i.fullDirectiveWithoutExplanation)
}
func (i NoExplanation) String() string { return toString(i) }
type UnusedCandidate struct {
BaseIssue
ExpectedLinter string
}
func (i UnusedCandidate) Details() string {
details := fmt.Sprintf("directive `%s` is unused", i.fullDirective)
if i.ExpectedLinter != "" {
details += fmt.Sprintf(" for linter %s", i.ExpectedLinter)
}
return details
}
func (i UnusedCandidate) String() string { return toString(i) }
func toString(i Issue) string {
return fmt.Sprintf("%s at %s", i.Details(), i.Position())
}
type Issue interface {
Details() string
Position() token.Position
String() string
}
type Needs uint
const (
NeedsMachineOnly Needs = 1 << iota
NeedsSpecific
NeedsExplanation
NeedsUnused
NeedsAll = NeedsMachineOnly | NeedsSpecific | NeedsExplanation
)
// matches lines starting with the nolint directive
var directiveOnlyPattern = regexp.MustCompile(`^\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\b`)
// matches a complete nolint directive
var fullDirectivePattern = regexp.MustCompile(`^//\s*nolint(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\s*(//.*)?\s*\n?$`)
type Linter struct {
excludes []string // lists individual linters that don't require explanations
needs Needs // indicates which linter checks to perform
excludeByLinter map[string]bool
}
// NewLinter creates a linter that enforces that the provided directives fulfill the provided requirements
func NewLinter(needs Needs, excludes []string) (*Linter, error) {
excludeByName := make(map[string]bool)
for _, e := range excludes {
excludeByName[e] = true
}
return &Linter{
needs: needs,
excludeByLinter: excludeByName,
}, nil
}
var leadingSpacePattern = regexp.MustCompile(`^//(\s*)`)
var trailingBlankExplanation = regexp.MustCompile(`\s*(//\s*)?$`)
func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
var issues []Issue
for _, node := range nodes {
if file, ok := node.(*ast.File); ok {
for _, c := range file.Comments {
text := c.Text()
matches := directiveOnlyPattern.FindStringSubmatch(text)
if len(matches) == 0 {
continue
}
directive := matches[1]
// check for a space between the "//" and the directive
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(c.List[0].Text) // c.Text() doesn't have all leading space
if len(leadingSpaceMatches) == 0 {
continue
}
leadingSpace := leadingSpaceMatches[1]
directiveWithOptionalLeadingSpace := directive
if len(leadingSpace) > 0 {
directiveWithOptionalLeadingSpace = " " + directive
}
base := BaseIssue{
fullDirective: c.List[0].Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: fset.Position(c.Pos()),
}
// check for, report and eliminate leading spaces so we can check for other issues
if leadingSpace != "" && leadingSpace != " " {
issues = append(issues, ExtraLeadingSpace{
BaseIssue: base,
})
}
if (l.needs&NeedsMachineOnly) != 0 && strings.HasPrefix(directiveWithOptionalLeadingSpace, " ") {
issues = append(issues, NotMachine{BaseIssue: base})
}
fullMatches := fullDirectivePattern.FindStringSubmatch(c.List[0].Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
continue
}
lintersText, explanation := fullMatches[1], fullMatches[2]
var linters []string
if len(lintersText) > 0 {
lls := strings.Split(lintersText[1:], ",")
linters = make([]string, 0, len(lls))
for _, ll := range lls {
ll = strings.TrimSpace(ll)
if ll != "" {
linters = append(linters, ll)
}
}
}
if (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})
}
}
// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
if l.needs&NeedsUnused != 0 {
if len(linters) == 0 {
issues = append(issues, UnusedCandidate{BaseIssue: base})
} else {
for _, linter := range linters {
issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
}
}
}
if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
// otherwise, check if we are excluding all of the mentioned linters
for _, ll := range linters {
if !l.excludeByLinter[ll] { // if a linter does require explanation
needsExplanation = true
break
}
}
if needsExplanation {
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(c.List[0].Text, "")
issues = append(issues, NoExplanation{
BaseIssue: base,
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
})
}
}
}
}
}
return issues, nil
}

View File

@ -0,0 +1,123 @@
package nolintlint
import (
"go/parser"
"go/token"
"testing"
"github.com/stretchr/testify/assert"
)
func TestNoLintLint(t *testing.T) {
t.Run("when no explanation is provided", func(t *testing.T) {
linter, _ := NewLinter(NeedsExplanation, nil)
expectIssues(t, linter, `
package bar
func foo() {
bad() //nolint
bad() //nolint //
bad() //nolint //
good() //nolint // this is ok
other() //nolintother
}`,
"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:9",
"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:6:9",
"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:7:9",
)
})
t.Run("when no explanation is needed for a specific linter", func(t *testing.T) {
linter, _ := NewLinter(NeedsExplanation, []string{"lll"})
expectIssues(t, linter, `
package bar
func foo() {
thisIsAReallyLongLine() //nolint:lll
}`)
})
t.Run("when no specific linter is mentioned", func(t *testing.T) {
linter, _ := NewLinter(NeedsSpecific, nil)
expectIssues(t, linter, `
package bar
func foo() {
good() //nolint:my-linter
bad() //nolint
bad() // nolint // because
}`,
"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9",
"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9")
})
t.Run("when machine-readable style isn't used", func(t *testing.T) {
linter, _ := NewLinter(NeedsMachineOnly, nil)
expectIssues(t, linter, `
package bar
func foo() {
bad() // nolint
good() //nolint
}`, "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9")
})
t.Run("extra spaces in front of directive are reported", func(t *testing.T) {
linter, _ := NewLinter(0, nil)
expectIssues(t, linter, `
package bar
func foo() {
bad() // nolint
good() // nolint
}`, "directive `// nolint` should not have more than one leading space at testing.go:5:9")
})
t.Run("spaces are allowed in comma-separated list of linters", func(t *testing.T) {
linter, _ := NewLinter(0, nil)
expectIssues(t, linter, `
package bar
func foo() {
good() // nolint:linter1,linter-two
bad() // nolint:linter1 linter2
good() // nolint: linter1,linter2
good() // nolint: linter1, linter2
}`,
"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", //nolint:lll // this is a string
)
})
t.Run("multi-line comments don't confuse parser", func(t *testing.T) {
linter, _ := NewLinter(0, nil)
expectIssues(t, linter, `
package bar
func foo() {
//nolint:test
// something else
}`)
})
}
func expectIssues(t *testing.T, linter *Linter, contents string, issues ...string) {
actualIssues := parseFile(t, linter, contents)
actualIssueStrs := make([]string, 0, len(actualIssues))
for _, i := range actualIssues {
actualIssueStrs = append(actualIssueStrs, i.String())
}
assert.ElementsMatch(t, issues, actualIssueStrs, "expected %s but got %s", issues, actualIssues)
}
func parseFile(t *testing.T, linter *Linter, contents string) []Issue {
fset := token.NewFileSet()
expr, err := parser.ParseFile(fset, "testing.go", contents, parser.ParseComments)
if err != nil {
t.Fatalf("unable to parse file contents: %s", err)
}
issues, err := linter.Run(fset, expr)
if err != nil {
t.Fatalf("unable to parse file: %s", err)
}
return issues
}

View File

@ -34,7 +34,7 @@ func NewPrealloc() *goanalysis.Linter {
var res []goanalysis.Issue
hints := prealloc.Check(pass.Files, s.Simple, s.RangeLoops, s.ForLoops)
for _, hint := range hints {
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: pass.Fset.Position(hint.Pos),
Text: fmt.Sprintf("Consider preallocating %s", formatCode(hint.DeclaredSliceName, lintCtx.Cfg)),
FromLinter: preallocName,

View File

@ -37,7 +37,7 @@ func NewStructcheck() *goanalysis.Linter {
issues := make([]goanalysis.Issue, 0, len(structcheckIssues))
for _, i := range structcheckIssues {
issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
issues = append(issues, goanalysis.NewIssue(&result.Issue{
Pos: i.Pos,
Text: fmt.Sprintf("%s is unused", formatCode(i.FieldName, lintCtx.Cfg)),
FromLinter: linterName,

View File

@ -35,7 +35,7 @@ func NewUnconvert() *goanalysis.Linter {
issues := make([]goanalysis.Issue, 0, len(positions))
for _, pos := range positions {
issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
issues = append(issues, goanalysis.NewIssue(&result.Issue{
Pos: pos,
Text: "unnecessary conversion",
FromLinter: linterName,

View File

@ -58,7 +58,7 @@ func NewUnparam() *goanalysis.Linter {
var res []goanalysis.Issue
for _, i := range unparamIssues {
res = append(res, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
res = append(res, goanalysis.NewIssue(&result.Issue{
Pos: pass.Fset.Position(i.Pos()),
Text: i.Message(),
FromLinter: linterName,

View File

@ -37,7 +37,7 @@ func NewVarcheck() *goanalysis.Linter {
issues := make([]goanalysis.Issue, 0, len(varcheckIssues))
for _, i := range varcheckIssues {
issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
issues = append(issues, goanalysis.NewIssue(&result.Issue{
Pos: i.Pos,
Text: fmt.Sprintf("%s is unused", formatCode(i.VarName, lintCtx.Cfg)),
FromLinter: linterName,

View File

@ -70,7 +70,7 @@ func NewWhitespace() *goanalysis.Linter {
}
issue.Replacement.NewLines = []string{bracketLine}
res[k] = goanalysis.NewIssue(&issue, pass) //nolint:scopelint
res[k] = goanalysis.NewIssue(&issue, pass)
}
mu.Lock()

View File

@ -66,7 +66,7 @@ func NewWSL() *goanalysis.Linter {
defer mu.Unlock()
for _, err := range wslErrors {
issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint
issues = append(issues, goanalysis.NewIssue(&result.Issue{
FromLinter: name,
Pos: err.Position,
Text: err.Reason,

View File

@ -113,7 +113,7 @@ func (es EnabledSet) combineGoAnalysisLinters(linters map[string]*linter.Config)
}
}
if len(goanalysisLinters) <= 1 { //nolint:gomnd
if len(goanalysisLinters) <= 1 {
es.debugf("Didn't combine go/analysis linters: got only %d linters", len(goanalysisLinters))
return
}

View File

@ -269,6 +269,10 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
linter.NewConfig(golinters.NewNestif()).
WithPresets(linter.PresetComplexity).
WithURL("https://github.com/nakabonne/nestif"),
// nolintlint must be last because it looks at the results of all the previous linters for unused nolint directives
linter.NewConfig(golinters.NewNoLintLint()).
WithPresets(linter.PresetStyle).
WithURL("https://github.com/golangci-lint/pkg/golinters/nolintlint"),
}
isLocalRun := os.Getenv("GOLANGCI_COM_RUN") == ""

View File

@ -77,6 +77,13 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env,
excludeRulesProcessor = processors.NewExcludeRules(excludeRules, lineCache, log.Child("exclude_rules"))
}
enabledLintersSet := lintersdb.NewEnabledSet(dbManager,
lintersdb.NewValidator(dbManager), log.Child("enabledLinters"), cfg)
lcs, err := enabledLintersSet.Get(false)
if err != nil {
return nil, err
}
return &Runner{
Processors: []processors.Processor{
processors.NewCgo(goenv),
@ -96,7 +103,7 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env,
excludeProcessor,
excludeRulesProcessor,
processors.NewNolint(log.Child("nolint"), dbManager),
processors.NewNolint(log.Child("nolint"), dbManager, lcs),
processors.NewUniqByLine(cfg),
processors.NewDiff(icfg.Diff, icfg.DiffFromRevision, icfg.DiffPatchFilePath),

View File

@ -41,6 +41,10 @@ type Issue struct {
// HunkPos is used only when golangci-lint is run over a diff
HunkPos int `json:",omitempty"`
// If we are expecting a nolint (because this is from nolintlint), record the expected linter
ExpectNoLint bool
ExpectedNoLintLinter string
}
func (i *Issue) FilePath() string {

View File

@ -132,7 +132,7 @@ func getDoc(filePath string) (string, error) {
var docLines []string
for scanner.Scan() {
line := strings.TrimSpace(scanner.Text())
if strings.HasPrefix(line, "//") { //nolint:gocritic
if strings.HasPrefix(line, "//") {
text := strings.TrimSpace(strings.TrimPrefix(line, "//"))
docLines = append(docLines, text)
} else if line == "" || strings.HasPrefix(line, "package") {

View File

@ -185,7 +185,7 @@ func (f Fixer) applyInlineFixes(lineIssues []result.Issue, origLine []byte, line
func (f Fixer) findNotIntersectingIssues(issues []result.Issue) []result.Issue {
sort.SliceStable(issues, func(i, j int) bool {
a, b := issues[i], issues[j] //nolint:scopelint
a, b := issues[i], issues[j]
return a.Line() < b.Line()
})

View File

@ -27,7 +27,7 @@ func NewMaxPerFileFromLinter(cfg *config.Config) *MaxPerFileFromLinter {
}
return &MaxPerFileFromLinter{
flc: fileToLinterToCountMap{}, //nolint:goimports,gofmt
flc: fileToLinterToCountMap{},
maxPerFileFromLinterConfig: maxPerFileFromLinterConfig,
}
}

View File

@ -8,6 +8,8 @@ import (
"sort"
"strings"
"github.com/golangci/golangci-lint/pkg/golinters"
"github.com/golangci/golangci-lint/pkg/lint/linter"
"github.com/golangci/golangci-lint/pkg/lint/lintersdb"
"github.com/golangci/golangci-lint/pkg/logutils"
"github.com/golangci/golangci-lint/pkg/result"
@ -16,7 +18,8 @@ import (
var nolintDebugf = logutils.Debug("nolint")
type ignoredRange struct {
linters []string
linters []string
matchedIssueFromLinter map[string]bool
result.Range
col int
}
@ -26,6 +29,15 @@ func (i *ignoredRange) doesMatch(issue *result.Issue) bool {
return false
}
// handle possible unused nolint directives
// nolintlint generates potential issues for every nolint directive and they are filtered out here
if issue.ExpectNoLint {
if issue.ExpectedNoLintLinter != "" {
return i.matchedIssueFromLinter[issue.ExpectedNoLintLinter]
}
return len(i.matchedIssueFromLinter) > 0
}
if len(i.linters) == 0 {
return true
}
@ -46,17 +58,24 @@ type fileData struct {
type filesCache map[string]*fileData
type Nolint struct {
cache filesCache
dbManager *lintersdb.Manager
log logutils.Log
cache filesCache
dbManager *lintersdb.Manager
enabledLinters map[string]bool
log logutils.Log
unknownLintersSet map[string]bool
}
func NewNolint(log logutils.Log, dbManager *lintersdb.Manager) *Nolint {
func NewNolint(log logutils.Log, dbManager *lintersdb.Manager, enabledLCs []*linter.Config) *Nolint {
enabledLinters := make(map[string]bool, len(enabledLCs))
for _, lc := range enabledLCs {
enabledLinters[lc.Name()] = true
}
return &Nolint{
cache: filesCache{},
dbManager: dbManager,
enabledLinters: enabledLinters,
log: log,
unknownLintersSet: map[string]bool{},
}
@ -69,6 +88,8 @@ func (p Nolint) Name() string {
}
func (p *Nolint) Process(issues []result.Issue) ([]result.Issue, error) {
// put nolintlint issues last because we process other issues first to determine which nolint directives are unused
sort.Stable(sortWithNolintlintLast(issues))
return filterIssuesErr(issues, p.shouldPassIssue)
}
@ -124,6 +145,22 @@ func (p *Nolint) buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet, fil
}
func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
nolintDebugf("got issue: %v", *i)
if i.FromLinter == golinters.NolintlintName {
// always pass nolintlint issues except ones trying find unused nolint directives
if !i.ExpectNoLint {
return true, nil
}
if i.ExpectedNoLintLinter != "" {
// don't expect disabled linters to cover their nolint statements
nolintDebugf("enabled linters: %v", p.enabledLinters)
if !p.enabledLinters[i.ExpectedNoLintLinter] {
return false, nil
}
nolintDebugf("checking that lint issue was used for %s: %v", i.ExpectedNoLintLinter, i)
}
}
fd, err := p.getOrCreateFileData(i)
if err != nil {
return false, err
@ -131,6 +168,7 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
for _, ir := range fd.ignoredRanges {
if ir.doesMatch(i) {
ir.matchedIssueFromLinter[i.FromLinter] = true
return false, nil
}
}
@ -203,8 +241,9 @@ func (p *Nolint) extractInlineRangeFromComment(text string, g ast.Node, fset *to
From: pos.Line,
To: fset.Position(g.End()).Line,
},
col: pos.Column,
linters: linters,
col: pos.Column,
linters: linters,
matchedIssueFromLinter: make(map[string]bool),
}
}
@ -253,3 +292,18 @@ func (p Nolint) Finish() {
p.log.Warnf("Found unknown linters in //nolint directives: %s", strings.Join(unknownLinters, ", "))
}
// put nolintlint last
type sortWithNolintlintLast []result.Issue
func (issues sortWithNolintlintLast) Len() int {
return len(issues)
}
func (issues sortWithNolintlintLast) Less(i, j int) bool {
return issues[i].FromLinter != golinters.NolintlintName && issues[j].FromLinter == golinters.NolintlintName
}
func (issues sortWithNolintlintLast) Swap(i, j int) {
issues[j], issues[i] = issues[i], issues[j]
}

View File

@ -9,6 +9,8 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/golangci/golangci-lint/pkg/config"
"github.com/golangci/golangci-lint/pkg/golinters"
"github.com/golangci/golangci-lint/pkg/lint/lintersdb"
"github.com/golangci/golangci-lint/pkg/logutils"
"github.com/golangci/golangci-lint/pkg/result"
@ -31,7 +33,7 @@ func newNolint2FileIssue(line int) result.Issue {
}
func newTestNolintProcessor(log logutils.Log) *Nolint {
return NewNolint(log, lintersdb.NewManager(nil, nil))
return NewNolint(log, lintersdb.NewManager(nil, nil), nil)
}
func getMockLog() *logutils.MockLog {
@ -40,7 +42,6 @@ func getMockLog() *logutils.MockLog {
return log
}
//nolint:funlen
func TestNolint(t *testing.T) {
p := newTestNolintProcessor(getMockLog())
defer p.Finish()
@ -229,7 +230,7 @@ func TestNolintWholeFile(t *testing.T) {
Filename: fileName,
Line: 4,
},
FromLinter: "unparam",
FromLinter: "varcheck",
})
processAssertSame(t, p, result.Issue{
Pos: token.Position{
@ -239,3 +240,68 @@ func TestNolintWholeFile(t *testing.T) {
FromLinter: "deadcode",
})
}
func TestNolintUnused(t *testing.T) {
fileName := filepath.Join("testdata", "nolint_unused.go")
log := getMockLog()
log.On("Warnf", "Found unknown linters in //nolint directives: %s", "blah")
createProcessor := func(t *testing.T, log *logutils.MockLog, enabledLinters []string) *Nolint {
enabledSetLog := logutils.NewMockLog()
enabledSetLog.On("Infof", "Active %d linters: %s", len(enabledLinters), enabledLinters)
cfg := &config.Config{Linters: config.Linters{DisableAll: true, Enable: enabledLinters}}
dbManager := lintersdb.NewManager(cfg, nil)
enabledLintersSet := lintersdb.NewEnabledSet(dbManager, lintersdb.NewValidator(dbManager), enabledSetLog, cfg)
lcs, err := enabledLintersSet.Get(false)
assert.NoError(t, err)
return NewNolint(log, dbManager, lcs)
}
// the issues below the nolintlint issues that would be generated for the test file
nolintlintIssueVarcheck := result.Issue{
Pos: token.Position{
Filename: fileName,
Line: 3,
},
FromLinter: golinters.NolintlintName,
ExpectNoLint: true,
ExpectedNoLintLinter: "varcheck",
}
t.Run("when an issue does not occur, it is not removed from the nolintlint issues", func(t *testing.T) {
p := createProcessor(t, log, []string{"nolintlint", "varcheck"})
defer p.Finish()
processAssertSame(t, p, nolintlintIssueVarcheck)
})
t.Run("when an issue occurs, it is removed from the nolintlint issues", func(t *testing.T) {
p := createProcessor(t, log, []string{"nolintlint", "varcheck"})
defer p.Finish()
processAssertEmpty(t, p, []result.Issue{{
Pos: token.Position{
Filename: fileName,
Line: 3,
},
FromLinter: "varcheck",
}, nolintlintIssueVarcheck}...)
})
t.Run("when a linter is not enabled, it is removed from the nolintlint unused issues", func(t *testing.T) {
enabledSetLog := logutils.NewMockLog()
enabledSetLog.On("Infof", "Active %d linters: %s", 1, []string{"nolintlint"})
cfg := &config.Config{Linters: config.Linters{DisableAll: true, Enable: []string{"nolintlint"}}}
dbManager := lintersdb.NewManager(cfg, nil)
enabledLintersSet := lintersdb.NewEnabledSet(dbManager, lintersdb.NewValidator(dbManager), enabledSetLog, cfg)
lcs, err := enabledLintersSet.Get(false)
assert.NoError(t, err)
p := NewNolint(log, dbManager, lcs)
defer p.Finish()
processAssertEmpty(t, p, nolintlintIssueVarcheck)
})
}

View File

@ -0,0 +1,3 @@
package testdata
var nolintVarcheck int // nolint:varcheck

View File

@ -1,4 +1,4 @@
//nolint: unparam
//nolint: varcheck
package testdata
var nolintUnparam int
var nolintVarcheck int

View File

@ -211,7 +211,7 @@ type runResult struct {
duration time.Duration
}
func compare(b *testing.B, gometalinterRun, golangciLintRun func(*testing.B), repoName, mode string, kLOC int) { // nolint
func compare(b *testing.B, gometalinterRun, golangciLintRun func(*testing.B), repoName, mode string, kLOC int) {
gometalinterRes := runOne(b, gometalinterRun, "gometalinter")
golangciLintRes := runOne(b, golangciLintRun, "golangci-lint")

View File

@ -85,7 +85,7 @@ func errorCheck(outStr string, wantAuto bool, fullshort ...string) (err error) {
if len(errs) == 0 {
return nil
}
if len(errs) == 1 { //nolint:gomnd
if len(errs) == 1 {
return errs[0]
}
var buf bytes.Buffer
@ -103,7 +103,7 @@ func splitOutput(out string, wantAuto bool) []string {
var res []string
for _, line := range strings.Split(out, "\n") {
line = strings.TrimSuffix(line, "\r") // normalize Windows output
if strings.HasPrefix(line, "\t") { //nolint:gocritic
if strings.HasPrefix(line, "\t") {
res[len(res)-1] += "\n" + line
} else if strings.HasPrefix(line, "go tool") || strings.HasPrefix(line, "#") || !wantAuto && strings.HasPrefix(line, "<autogenerated>") {
continue

13
test/testdata/nolintlint.go vendored Normal file
View File

@ -0,0 +1,13 @@
//args: -Enolintlint
//config: linters-settings.nolintlint.require-explanation=true
//config: linters-settings.nolintlint.require-specific=true
//config: linters-settings.nolintlint.allowing-leading-space=false
package testdata
import "fmt"
func Foo() {
fmt.Println("not specific") //nolint // ERROR "directive `.*` should mention specific linter such as `//nolint:my-linter`"
fmt.Println("not machine readable") // nolint // ERROR "directive `.*` should be written as `//nolint`"
fmt.Println("extra spaces") // nolint:deadcode // because // ERROR "directive `.*` should not have more than one leading space"
}

11
test/testdata/nolintlint_unused.go vendored Normal file
View File

@ -0,0 +1,11 @@
//args: -Enolintlint -Evarcheck
//config: linters-settings.nolintlint.allow-unused=false
package testdata
import "fmt"
func Foo() {
fmt.Println("unused") //nolint // ERROR "directive `//nolint .*` is unused"
fmt.Println("unused,specific") //nolint:varcheck // ERROR "directive `//nolint:varcheck .*` is unused for linter varcheck"
fmt.Println("not run") //nolint:unparam // unparam is not run so this is ok
}