From c69dde582e6c16cf4b1391d75e90096560c587e3 Mon Sep 17 00:00:00 2001 From: "Andrew S. Brown" Date: Sat, 20 Feb 2021 14:17:39 -0800 Subject: [PATCH] Add fix option for nolintlint Repair whitespace issues and remove nolint statements that aren't being triggered --- pkg/golinters/nolintlint.go | 1 + pkg/golinters/nolintlint/nolintlint.go | 82 ++++++++--- pkg/golinters/nolintlint/nolintlint_test.go | 132 +++++++++++++++--- pkg/result/issue.go | 2 +- pkg/result/processors/nolint.go | 55 +++++--- pkg/result/processors/nolint_test.go | 18 +++ .../processors/testdata/nolint_unused.go | 2 + test/testdata/fix/in/nolintlint.go | 16 +++ test/testdata/fix/out/nolintlint.go | 16 +++ test/testdata/nolintlint.go | 11 +- 10 files changed, 271 insertions(+), 64 deletions(-) create mode 100644 test/testdata/fix/in/nolintlint.go create mode 100644 test/testdata/fix/out/nolintlint.go diff --git a/pkg/golinters/nolintlint.go b/pkg/golinters/nolintlint.go index d98b50cf8a0c..889cff864c89 100644 --- a/pkg/golinters/nolintlint.go +++ b/pkg/golinters/nolintlint.go @@ -72,6 +72,7 @@ func NewNoLintLint() *goanalysis.Linter { Pos: i.Position(), ExpectNoLint: expectNoLint, ExpectedNoLintLinter: expectedNolintLinter, + Replacement: i.Replacement(), } res = append(res, goanalysis.NewIssue(issue, pass)) } diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index d20ac30f0951..e9b11f1c76a8 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -8,16 +8,21 @@ import ( "regexp" "strings" "unicode" + + "github.com/golangci/golangci-lint/pkg/result" ) type BaseIssue struct { fullDirective string directiveWithOptionalLeadingSpace string position token.Position + replacement *result.Replacement } -func (b BaseIssue) Position() token.Position { - return b.position +func (b BaseIssue) Position() token.Position { return b.position } + +func (b BaseIssue) Replacement() *result.Replacement { + return b.replacement } type ExtraLeadingSpace struct { @@ -85,7 +90,7 @@ type UnusedCandidate struct { 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) + details += fmt.Sprintf(" for linter %q", i.ExpectedLinter) } return details } @@ -100,6 +105,7 @@ type Issue interface { Details() string Position() token.Position String() string + Replacement() *result.Replacement } type Needs uint @@ -115,7 +121,7 @@ const ( var commentPattern = 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?$`) +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 @@ -164,19 +170,34 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1]) } + pos := fset.Position(comment.Pos()) + end := fset.Position(comment.End()) + base := BaseIssue{ fullDirective: comment.Text, directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace, - position: fset.Position(comment.Pos()), + position: pos, } // check for, report and eliminate leading spaces so we can check for other issues - if len(leadingSpace) > 1 { - issues = append(issues, ExtraLeadingSpace{BaseIssue: base}) - } - - if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 { - issues = append(issues, NotMachine{BaseIssue: base}) + if len(leadingSpace) > 0 { + removeWhitespace := &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: pos.Column + 1, + Length: len(leadingSpace), + NewString: "", + }, + } + if (l.needs & NeedsMachineOnly) != 0 { + issue := NotMachine{BaseIssue: base} + issue.BaseIssue.replacement = removeWhitespace + issues = append(issues, issue) + } else if len(leadingSpace) > 1 { + issue := ExtraLeadingSpace{BaseIssue: base} + issue.BaseIssue.replacement = removeWhitespace + issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended + issues = append(issues, issue) + } } fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) @@ -187,14 +208,22 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { lintersText, explanation := fullMatches[1], fullMatches[2] var linters []string + var linterRange []result.Range if len(lintersText) > 0 { - lls := strings.Split(lintersText[1:], ",") + lls := strings.Split(lintersText, ",") linters = make([]string, 0, len(lls)) - for _, ll := range lls { - ll = strings.TrimSpace(ll) - if ll != "" { - linters = append(linters, ll) + rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:") + for i, ll := range lls { + rangeEnd := rangeStart + len(ll) + if i < len(lls)-1 { + rangeEnd++ // include trailing comma + } + trimmedLinterName := strings.TrimSpace(ll) + if trimmedLinterName != "" { + linters = append(linters, trimmedLinterName) + linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd}) } + rangeStart = rangeEnd } } @@ -206,11 +235,28 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { // when detecting unused directives, we send all the directives through and filter them out in the nolint processor if (l.needs & NeedsUnused) != 0 { + removeNolintCompletely := &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: pos.Column - 1, + Length: end.Column - pos.Column, + NewString: "", + }, + } + if len(linters) == 0 { - issues = append(issues, UnusedCandidate{BaseIssue: base}) + issue := UnusedCandidate{BaseIssue: base} + issue.replacement = removeNolintCompletely + issues = append(issues, issue) } else { for _, linter := range linters { - issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}) + issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter} + // only offer replacement if there is a single linter + // because of issues around commas and the possibility of all + // linters being removed + if len(linters) == 1 { + issue.replacement = removeNolintCompletely + } + issues = append(issues, issue) } } } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index b4bf4fbac546..0cc699c370af 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -7,16 +7,22 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/golangci/golangci-lint/pkg/result" ) //nolint:funlen func TestNoLintLint(t *testing.T) { + type issueWithReplacement struct { + issue string + replacement *result.Replacement + } testCases := []struct { desc string needs Needs excludes []string contents string - expected []string + expected []issueWithReplacement }{ { desc: "when no explanation is provided", @@ -33,11 +39,11 @@ func foo() { good() //nolint // this is ok other() //nolintother }`, - expected: []string{ - "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", - "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", - "directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", - "directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", + expected: []issueWithReplacement{ + {"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", nil}, + {"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", nil}, + {"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", nil}, + {"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", nil}, }, }, { @@ -50,8 +56,8 @@ package bar //nolint // this is ok //nolint:dupl func foo() {}`, - expected: []string{ - "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", + expected: []issueWithReplacement{ + {"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil}, }, }, { @@ -76,9 +82,9 @@ func foo() { bad() //nolint bad() // nolint // because }`, - expected: []string{ - "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", + expected: []issueWithReplacement{ + {"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9", nil}, + {"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9", nil}, }, }, { @@ -91,8 +97,17 @@ func foo() { bad() // nolint good() //nolint }`, - expected: []string{ - "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9", + expected: []issueWithReplacement{ + { + "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 10, + Length: 1, + NewString: "", + }, + }, + }, }, }, { @@ -104,8 +119,17 @@ func foo() { bad() // nolint good() // nolint }`, - expected: []string{ - "directive `// nolint` should not have more than one leading space at testing.go:5:9", + expected: []issueWithReplacement{ + { + "directive `// nolint` should not have more than one leading space at testing.go:5:9", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 10, + Length: 2, + NewString: " ", + }, + }, + }, }, }, { @@ -119,8 +143,8 @@ func foo() { good() // nolint: linter1,linter2 good() // nolint: linter1, linter2 }`, - expected: []string{ - "directive `// nolint:linter1 linter2` should match `// nolint[:] [// ]` at testing.go:6:9", //nolint:lll // this is a string + expected: []issueWithReplacement{ + {"directive `// nolint:linter1 linter2` should match `// nolint[:] [// ]` at testing.go:6:9", nil}, //nolint:lll // this is a string }, }, { @@ -133,6 +157,70 @@ func foo() { // something else }`, }, + { + desc: "needs unused without specific linter generates replacement", + needs: NeedsUnused, + contents: ` +package bar + +func foo() { + bad() //nolint +}`, + expected: []issueWithReplacement{ + { + "directive `//nolint` is unused at testing.go:5:9", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 8, + Length: 8, + NewString: "", + }, + }, + }, + }, + }, + { + desc: "needs unused with one specific linter generates replacement", + needs: NeedsUnused, + contents: ` +package bar + +func foo() { + bad() //nolint:somelinter +}`, + expected: []issueWithReplacement{ + { + "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 8, + Length: 19, + NewString: "", + }, + }, + }, + }, + }, + { + desc: "needs unused with multiple specific linters does not generate replacements", + needs: NeedsUnused, + contents: ` +package bar + +func foo() { + bad() //nolint:linter1,linter2 +}`, + expected: []issueWithReplacement{ + { + "directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9", + nil, + }, + { + "directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9", + nil, + }, + }, + }, } for _, test := range testCases { @@ -149,12 +237,16 @@ func foo() { actualIssues, err := linter.Run(fset, expr) require.NoError(t, err) - actualIssueStrs := make([]string, 0, len(actualIssues)) + actualIssuesWithReplacements := make([]issueWithReplacement, 0, len(actualIssues)) for _, i := range actualIssues { - actualIssueStrs = append(actualIssueStrs, i.String()) + actualIssuesWithReplacements = append(actualIssuesWithReplacements, issueWithReplacement{ + issue: i.String(), + replacement: i.Replacement(), + }) } - assert.ElementsMatch(t, test.expected, actualIssueStrs, "expected %s \nbut got %s", test.expected, actualIssues) + assert.ElementsMatch(t, test.expected, actualIssuesWithReplacements, + "expected %s \nbut got %s", test.expected, actualIssuesWithReplacements) }) } } diff --git a/pkg/result/issue.go b/pkg/result/issue.go index eafdbc4a958a..707a2b17cd95 100644 --- a/pkg/result/issue.go +++ b/pkg/result/issue.go @@ -14,7 +14,7 @@ type Range struct { type Replacement struct { NeedOnlyDelete bool // need to delete all lines of the issue without replacement with new lines - NewLines []string // is NeedDelete is false it's the replacement lines + NewLines []string // if NeedDelete is false it's the replacement lines Inline *InlineFix } diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index 9b292eda32ff..06af04ea19cf 100644 --- a/pkg/result/processors/nolint.go +++ b/pkg/result/processors/nolint.go @@ -21,7 +21,8 @@ type ignoredRange struct { linters []string matchedIssueFromLinter map[string]bool result.Range - col int + col int + originalRange *ignoredRange // pre-expanded range (used to match nolintlint issues) } func (i *ignoredRange) doesMatch(issue *result.Issue) bool { @@ -29,23 +30,27 @@ 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] + // only allow selective nolinting of nolintlint + nolintFoundForLinter := len(i.linters) == 0 && issue.FromLinter != golinters.NolintlintName + + for _, linterName := range i.linters { + if linterName == issue.FromLinter { + nolintFoundForLinter = true + break } - return len(i.matchedIssueFromLinter) > 0 } - if len(i.linters) == 0 { + if nolintFoundForLinter { return true } - for _, linterName := range i.linters { - if linterName == issue.FromLinter { - return true + // handle possible unused nolint directives + // nolintlint generates potential issues for every nolint directive and they are filtered out here + if issue.FromLinter == golinters.NolintlintName && issue.ExpectNoLint { + if issue.ExpectedNoLintLinter != "" { + return i.matchedIssueFromLinter[issue.ExpectedNoLintLinter] } + return len(i.matchedIssueFromLinter) > 0 } return false @@ -141,19 +146,14 @@ 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] == nil { - return false, nil - } - nolintDebugf("checking that lint issue was used for %s: %v", i.ExpectedNoLintLinter, i) + + if i.FromLinter == golinters.NolintlintName && i.ExpectNoLint && i.ExpectedNoLintLinter != "" { + // don't expect disabled linters to cover their nolint statements + nolintDebugf("enabled linters: %v", p.enabledLinters) + if p.enabledLinters[i.ExpectedNoLintLinter] == nil { + return false, nil } + nolintDebugf("checking that lint issue was used for %s: %v", i.ExpectedNoLintLinter, i) } fd, err := p.getOrCreateFileData(i) @@ -163,7 +163,11 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) { for _, ir := range fd.ignoredRanges { if ir.doesMatch(i) { + nolintDebugf("found ignored range for issue %v: %v", i, ir) ir.matchedIssueFromLinter[i.FromLinter] = true + if ir.originalRange != nil { + ir.originalRange.matchedIssueFromLinter[i.FromLinter] = true + } return false, nil } } @@ -199,9 +203,14 @@ func (e *rangeExpander) Visit(node ast.Node) ast.Visitor { } expandedRange := *foundRange + // store the original unexpanded range for matching nolintlint issues + if expandedRange.originalRange == nil { + expandedRange.originalRange = foundRange + } if expandedRange.To < nodeEndLine { expandedRange.To = nodeEndLine } + nolintDebugf("found range is %v for node %#v [%d;%d], expanded range is %v", *foundRange, node, nodeStartLine, nodeEndLine, expandedRange) e.expandedRanges = append(e.expandedRanges, expandedRange) diff --git a/pkg/result/processors/nolint_test.go b/pkg/result/processors/nolint_test.go index 294add383b06..c54909f2643c 100644 --- a/pkg/result/processors/nolint_test.go +++ b/pkg/result/processors/nolint_test.go @@ -291,6 +291,17 @@ func TestNolintUnused(t *testing.T) { ExpectedNoLintLinter: "varcheck", } + // the issue below is another nolintlint issue that would be generated for the test file + nolintlintIssueVarcheckUnusedOK := result.Issue{ + Pos: token.Position{ + Filename: fileName, + Line: 5, + }, + 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() @@ -298,6 +309,13 @@ func TestNolintUnused(t *testing.T) { processAssertSame(t, p, nolintlintIssueVarcheck) }) + t.Run("when an issue does not occur but nolintlint is nolinted, it is removed from the nolintlint issues", func(t *testing.T) { + p := createProcessor(t, log, []string{"nolintlint", "varcheck"}) + defer p.Finish() + + processAssertEmpty(t, p, nolintlintIssueVarcheckUnusedOK) + }) + 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() diff --git a/pkg/result/processors/testdata/nolint_unused.go b/pkg/result/processors/testdata/nolint_unused.go index 6ca35f3cd913..dc8eb49e6da1 100644 --- a/pkg/result/processors/testdata/nolint_unused.go +++ b/pkg/result/processors/testdata/nolint_unused.go @@ -1,3 +1,5 @@ package testdata var nolintVarcheck int // nolint:varcheck + +var nolintVarcheckUnusedOK int // nolint:varcheck,nolintlint diff --git a/test/testdata/fix/in/nolintlint.go b/test/testdata/fix/in/nolintlint.go new file mode 100644 index 000000000000..ae7b73589bce --- /dev/null +++ b/test/testdata/fix/in/nolintlint.go @@ -0,0 +1,16 @@ +//args: -Enolintlint -Elll +//config: linters-settings.nolintlint.allow-leading-space=false +package p + +import "fmt" + +func nolintlint() { + fmt.Println() // nolint:bob // leading space should be dropped + fmt.Println() // nolint:bob // leading spaces should be dropped + + // note that the next lines will retain trailing whitespace when fixed + fmt.Println() //nolint // nolint should be dropped + fmt.Println() //nolint:lll // nolint should be dropped + + fmt.Println() //nolint:alice,lll // we don't drop individual linters from lists +} diff --git a/test/testdata/fix/out/nolintlint.go b/test/testdata/fix/out/nolintlint.go new file mode 100644 index 000000000000..9ec6800a7d62 --- /dev/null +++ b/test/testdata/fix/out/nolintlint.go @@ -0,0 +1,16 @@ +//args: -Enolintlint -Elll +//config: linters-settings.nolintlint.allow-leading-space=false +package p + +import "fmt" + +func nolintlint() { + fmt.Println() //nolint:bob // leading space should be dropped + fmt.Println() //nolint:bob // leading spaces should be dropped + + // note that the next lines will retain trailing whitespace when fixed + fmt.Println() + fmt.Println() + + fmt.Println() //nolint:alice,lll // we don't drop individual linters from lists +} diff --git a/test/testdata/nolintlint.go b/test/testdata/nolintlint.go index 2294acc7e0e1..66785e228ea8 100644 --- a/test/testdata/nolintlint.go +++ b/test/testdata/nolintlint.go @@ -1,7 +1,7 @@ -//args: -Enolintlint +//args: -Enolintlint -Emisspell //config: linters-settings.nolintlint.require-explanation=true //config: linters-settings.nolintlint.require-specific=true -//config: linters-settings.nolintlint.allowing-leading-space=false +//config: linters-settings.nolintlint.allow-leading-space=false package testdata import "fmt" @@ -10,4 +10,11 @@ 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" + + // test expanded range + //nolint:misspell // deliberate misspelling to trigger nolintlint + func() { + mispell := true + fmt.Println(mispell) + }() }