From 909f628d750126f8e084855954de0f92181febdc Mon Sep 17 00:00:00 2001 From: Andrew Shannon Brown Date: Sun, 26 Apr 2020 19:30:14 -0700 Subject: [PATCH] Add linter for nolint Linter can check that nolint statements are properly formatted and also that all nolint statements are used. --- .golangci.example.yml | 11 + .golangci.yml | 6 + README.md | 20 ++ pkg/commands/run.go | 2 +- pkg/config/config.go | 16 +- pkg/golinters/deadcode.go | 2 +- pkg/golinters/depguard.go | 2 +- pkg/golinters/dupl.go | 2 +- pkg/golinters/errcheck.go | 2 +- pkg/golinters/funlen.go | 2 +- pkg/golinters/goanalysis/issue.go | 12 +- pkg/golinters/goanalysis/linter.go | 26 +- pkg/golinters/goanalysis/runner.go | 2 +- pkg/golinters/gocognit.go | 2 +- pkg/golinters/goconst.go | 2 +- pkg/golinters/gocyclo.go | 2 +- pkg/golinters/godox.go | 2 +- pkg/golinters/gofmt_common.go | 1 - pkg/golinters/gomodguard.go | 2 +- pkg/golinters/gosec.go | 2 +- pkg/golinters/ineffassign.go | 2 +- pkg/golinters/interfacer.go | 2 +- pkg/golinters/maligned.go | 2 +- pkg/golinters/nestif.go | 2 +- pkg/golinters/nolintlint.go | 92 +++++++ pkg/golinters/nolintlint/README.md | 31 +++ pkg/golinters/nolintlint/nolintlint.go | 239 ++++++++++++++++++ pkg/golinters/nolintlint/nolintlint_test.go | 123 +++++++++ pkg/golinters/prealloc.go | 2 +- pkg/golinters/structcheck.go | 2 +- pkg/golinters/unconvert.go | 2 +- pkg/golinters/unparam.go | 2 +- pkg/golinters/unused.go | 2 +- pkg/golinters/varcheck.go | 2 +- pkg/golinters/whitespace.go | 2 +- pkg/golinters/wsl.go | 2 +- pkg/lint/lintersdb/enabled_set.go | 2 +- pkg/lint/lintersdb/manager.go | 4 + pkg/lint/runner.go | 9 +- pkg/result/issue.go | 4 + .../processors/autogenerated_exclude.go | 2 +- pkg/result/processors/fixer.go | 2 +- .../processors/max_per_file_from_linter.go | 2 +- pkg/result/processors/nolint.go | 68 ++++- pkg/result/processors/nolint_test.go | 72 +++++- .../processors/testdata/nolint_unused.go | 3 + .../processors/testdata/nolint_whole_file.go | 4 +- test/bench/bench_test.go | 2 +- test/errchk.go | 4 +- test/testdata/nolintlint.go | 13 + test/testdata/nolintlint_unused.go | 11 + 51 files changed, 766 insertions(+), 63 deletions(-) create mode 100644 pkg/golinters/nolintlint.go create mode 100644 pkg/golinters/nolintlint/README.md create mode 100644 pkg/golinters/nolintlint/nolintlint.go create mode 100644 pkg/golinters/nolintlint/nolintlint_test.go create mode 100644 pkg/result/processors/testdata/nolint_unused.go create mode 100644 test/testdata/nolintlint.go create mode 100644 test/testdata/nolintlint_unused.go diff --git a/.golangci.example.yml b/.golangci.example.yml index 36f8ed40..d7f89f78 100644 --- a/.golangci.example.yml +++ b/.golangci.example.yml @@ -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 diff --git a/.golangci.yml b/.golangci.yml index e5843395..c59782da 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -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 diff --git a/README.md b/README.md index c2e445cf..5b212cd8 100644 --- a/README.md +++ b/README.md @@ -231,6 +231,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] @@ -496,6 +497,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 @@ -843,6 +845,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 @@ -1032,6 +1045,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. @@ -1062,6 +1080,7 @@ linters: - lll - misspell - nakedret + - nolintlint - rowserrcheck - scopelint - staticcheck @@ -1298,6 +1317,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 diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 434b85d5..4149a00c 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -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 } diff --git a/pkg/config/config.go b/pkg/config/config.go index d3401839..48ea13f3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -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`, }, diff --git a/pkg/golinters/deadcode.go b/pkg/golinters/deadcode.go index 9889dad4..6ff38909 100644 --- a/pkg/golinters/deadcode.go +++ b/pkg/golinters/deadcode.go @@ -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, diff --git a/pkg/golinters/depguard.go b/pkg/golinters/depguard.go index 611f6d49..3bd85481 100644 --- a/pkg/golinters/depguard.go +++ b/pkg/golinters/depguard.go @@ -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, diff --git a/pkg/golinters/dupl.go b/pkg/golinters/dupl.go index d6dc67fb..eaac928e 100644 --- a/pkg/golinters/dupl.go +++ b/pkg/golinters/dupl.go @@ -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(), diff --git a/pkg/golinters/errcheck.go b/pkg/golinters/errcheck.go index bf6b9a45..7df11fc8 100644 --- a/pkg/golinters/errcheck.go +++ b/pkg/golinters/errcheck.go @@ -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, diff --git a/pkg/golinters/funlen.go b/pkg/golinters/funlen.go index 3031da48..29cb6b7e 100644 --- a/pkg/golinters/funlen.go +++ b/pkg/golinters/funlen.go @@ -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, diff --git a/pkg/golinters/goanalysis/issue.go b/pkg/golinters/goanalysis/issue.go index b90a2912..f331a3ab 100644 --- a/pkg/golinters/goanalysis/issue.go +++ b/pkg/golinters/goanalysis/issue.go @@ -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 } diff --git a/pkg/golinters/goanalysis/linter.go b/pkg/golinters/goanalysis/linter.go index 778a1ef6..f1cfcca8 100644 --- a/pkg/golinters/goanalysis/linter.go +++ b/pkg/golinters/goanalysis/linter.go @@ -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 diff --git a/pkg/golinters/goanalysis/runner.go b/pkg/golinters/goanalysis/runner.go index 2805ed2c..64087c28 100644 --- a/pkg/golinters/goanalysis/runner.go +++ b/pkg/golinters/goanalysis/runner.go @@ -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() diff --git a/pkg/golinters/gocognit.go b/pkg/golinters/gocognit.go index 71301180..8044e3e5 100644 --- a/pkg/golinters/gocognit.go +++ b/pkg/golinters/gocognit.go @@ -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), diff --git a/pkg/golinters/goconst.go b/pkg/golinters/goconst.go index c77dc64e..8e91feef 100644 --- a/pkg/golinters/goconst.go +++ b/pkg/golinters/goconst.go @@ -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, diff --git a/pkg/golinters/gocyclo.go b/pkg/golinters/gocyclo.go index 4fb28898..f9c0a799 100644 --- a/pkg/golinters/gocyclo.go +++ b/pkg/golinters/gocyclo.go @@ -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), diff --git a/pkg/golinters/godox.go b/pkg/golinters/godox.go index 78d13f3b..2a4dd9fa 100644 --- a/pkg/golinters/godox.go +++ b/pkg/golinters/godox.go @@ -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, diff --git a/pkg/golinters/gofmt_common.go b/pkg/golinters/gofmt_common.go index ae152ad7..fb1e3f66 100644 --- a/pkg/golinters/gofmt_common.go +++ b/pkg/golinters/gofmt_common.go @@ -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, "-") diff --git a/pkg/golinters/gomodguard.go b/pkg/golinters/gomodguard.go index aae78f81..29cc3c30 100644 --- a/pkg/golinters/gomodguard.go +++ b/pkg/golinters/gomodguard.go @@ -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, diff --git a/pkg/golinters/gosec.go b/pkg/golinters/gosec.go index 0b84824c..97442958 100644 --- a/pkg/golinters/gosec.go +++ b/pkg/golinters/gosec.go @@ -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, diff --git a/pkg/golinters/ineffassign.go b/pkg/golinters/ineffassign.go index 7d755809..601252f7 100644 --- a/pkg/golinters/ineffassign.go +++ b/pkg/golinters/ineffassign.go @@ -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, diff --git a/pkg/golinters/interfacer.go b/pkg/golinters/interfacer.go index 96f6b5b9..1edbe894 100644 --- a/pkg/golinters/interfacer.go +++ b/pkg/golinters/interfacer.go @@ -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, diff --git a/pkg/golinters/maligned.go b/pkg/golinters/maligned.go index 4f34f0ea..22422b8c 100644 --- a/pkg/golinters/maligned.go +++ b/pkg/golinters/maligned.go @@ -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, diff --git a/pkg/golinters/nestif.go b/pkg/golinters/nestif.go index 841a22d4..0998a8ce 100644 --- a/pkg/golinters/nestif.go +++ b/pkg/golinters/nestif.go @@ -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, diff --git a/pkg/golinters/nolintlint.go b/pkg/golinters/nolintlint.go new file mode 100644 index 00000000..d98b50cf --- /dev/null +++ b/pkg/golinters/nolintlint.go @@ -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) +} diff --git a/pkg/golinters/nolintlint/README.md b/pkg/golinters/nolintlint/README.md new file mode 100644 index 00000000..3d440d5a --- /dev/null +++ b/pkg/golinters/nolintlint/README.md @@ -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`. + diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go new file mode 100644 index 00000000..6da31c60 --- /dev/null +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -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[:] [// ]`", + 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 +} diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go new file mode 100644 index 00000000..4eb72e8d --- /dev/null +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -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[:] [// ]` 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 +} diff --git a/pkg/golinters/prealloc.go b/pkg/golinters/prealloc.go index 168dc171..534a6925 100644 --- a/pkg/golinters/prealloc.go +++ b/pkg/golinters/prealloc.go @@ -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, diff --git a/pkg/golinters/structcheck.go b/pkg/golinters/structcheck.go index 8acfc403..7c16f8ec 100644 --- a/pkg/golinters/structcheck.go +++ b/pkg/golinters/structcheck.go @@ -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, diff --git a/pkg/golinters/unconvert.go b/pkg/golinters/unconvert.go index 14757017..456f6836 100644 --- a/pkg/golinters/unconvert.go +++ b/pkg/golinters/unconvert.go @@ -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, diff --git a/pkg/golinters/unparam.go b/pkg/golinters/unparam.go index 866d0663..fabb6b52 100644 --- a/pkg/golinters/unparam.go +++ b/pkg/golinters/unparam.go @@ -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, diff --git a/pkg/golinters/unused.go b/pkg/golinters/unused.go index 5f6d8371..982d6759 100644 --- a/pkg/golinters/unused.go +++ b/pkg/golinters/unused.go @@ -33,7 +33,7 @@ func NewUnused() *goanalysis.Linter { for _, ur := range u.Result() { p := u.ProblemObject(lintCtx.Packages[0].Fset, ur) pkg := typesToPkg[ur.Pkg()] - issues = append(issues, goanalysis.NewIssue(&result.Issue{ //nolint:scopelint + issues = append(issues, goanalysis.NewIssue(&result.Issue{ FromLinter: name, Text: p.Message, Pos: p.Pos, diff --git a/pkg/golinters/varcheck.go b/pkg/golinters/varcheck.go index 3c650d8c..dcf2e7de 100644 --- a/pkg/golinters/varcheck.go +++ b/pkg/golinters/varcheck.go @@ -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, diff --git a/pkg/golinters/whitespace.go b/pkg/golinters/whitespace.go index 4a2ccce5..d475465a 100644 --- a/pkg/golinters/whitespace.go +++ b/pkg/golinters/whitespace.go @@ -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() diff --git a/pkg/golinters/wsl.go b/pkg/golinters/wsl.go index 32fb00c4..f44d09f2 100644 --- a/pkg/golinters/wsl.go +++ b/pkg/golinters/wsl.go @@ -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, diff --git a/pkg/lint/lintersdb/enabled_set.go b/pkg/lint/lintersdb/enabled_set.go index a3b73e70..73e3bcfe 100644 --- a/pkg/lint/lintersdb/enabled_set.go +++ b/pkg/lint/lintersdb/enabled_set.go @@ -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 } diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index e94b4dd8..c70729f8 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -266,6 +266,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") == "" diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index 22859afc..e8f427e0 100644 --- a/pkg/lint/runner.go +++ b/pkg/lint/runner.go @@ -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), diff --git a/pkg/result/issue.go b/pkg/result/issue.go index 83ba705e..16d9a8a8 100644 --- a/pkg/result/issue.go +++ b/pkg/result/issue.go @@ -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 { diff --git a/pkg/result/processors/autogenerated_exclude.go b/pkg/result/processors/autogenerated_exclude.go index 8bdaae66..249ba9d4 100644 --- a/pkg/result/processors/autogenerated_exclude.go +++ b/pkg/result/processors/autogenerated_exclude.go @@ -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") { diff --git a/pkg/result/processors/fixer.go b/pkg/result/processors/fixer.go index 3f7abbc8..98a4bf5c 100644 --- a/pkg/result/processors/fixer.go +++ b/pkg/result/processors/fixer.go @@ -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() }) diff --git a/pkg/result/processors/max_per_file_from_linter.go b/pkg/result/processors/max_per_file_from_linter.go index e9842966..e83c569e 100644 --- a/pkg/result/processors/max_per_file_from_linter.go +++ b/pkg/result/processors/max_per_file_from_linter.go @@ -27,7 +27,7 @@ func NewMaxPerFileFromLinter(cfg *config.Config) *MaxPerFileFromLinter { } return &MaxPerFileFromLinter{ - flc: fileToLinterToCountMap{}, //nolint:goimports,gofmt + flc: fileToLinterToCountMap{}, maxPerFileFromLinterConfig: maxPerFileFromLinterConfig, } } diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index da7abacd..2f1e48db 100644 --- a/pkg/result/processors/nolint.go +++ b/pkg/result/processors/nolint.go @@ -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] +} diff --git a/pkg/result/processors/nolint_test.go b/pkg/result/processors/nolint_test.go index b547fbd0..051b1ebc 100644 --- a/pkg/result/processors/nolint_test.go +++ b/pkg/result/processors/nolint_test.go @@ -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) + }) +} diff --git a/pkg/result/processors/testdata/nolint_unused.go b/pkg/result/processors/testdata/nolint_unused.go new file mode 100644 index 00000000..6ca35f3c --- /dev/null +++ b/pkg/result/processors/testdata/nolint_unused.go @@ -0,0 +1,3 @@ +package testdata + +var nolintVarcheck int // nolint:varcheck diff --git a/pkg/result/processors/testdata/nolint_whole_file.go b/pkg/result/processors/testdata/nolint_whole_file.go index 649923b2..e0f93c83 100644 --- a/pkg/result/processors/testdata/nolint_whole_file.go +++ b/pkg/result/processors/testdata/nolint_whole_file.go @@ -1,4 +1,4 @@ -//nolint: unparam +//nolint: varcheck package testdata -var nolintUnparam int +var nolintVarcheck int diff --git a/test/bench/bench_test.go b/test/bench/bench_test.go index 42caffd7..77e4e5dc 100644 --- a/test/bench/bench_test.go +++ b/test/bench/bench_test.go @@ -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") diff --git a/test/errchk.go b/test/errchk.go index e1b90e6a..6cf12629 100644 --- a/test/errchk.go +++ b/test/errchk.go @@ -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, "") { continue diff --git a/test/testdata/nolintlint.go b/test/testdata/nolintlint.go new file mode 100644 index 00000000..2294acc7 --- /dev/null +++ b/test/testdata/nolintlint.go @@ -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" +} diff --git a/test/testdata/nolintlint_unused.go b/test/testdata/nolintlint_unused.go new file mode 100644 index 00000000..184a2116 --- /dev/null +++ b/test/testdata/nolintlint_unused.go @@ -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 +}