From aeb98303293570ba682ea933a4e9501a11a3aa99 Mon Sep 17 00:00:00 2001 From: Andrew Shannon Brown Date: Sun, 27 Dec 2020 06:18:02 -0800 Subject: [PATCH] Update nolintlint to fix nolint formatting and remove unused nolint statements (#1573) Also allow multiple ranges to satisfy a nolint statement as having been used. --- pkg/golinters/nolintlint.go | 1 + pkg/golinters/nolintlint/nolintlint.go | 72 +++++++-- pkg/golinters/nolintlint/nolintlint_test.go | 154 +++++++++++++++++--- pkg/result/issue.go | 2 +- pkg/result/processors/nolint.go | 12 +- test/testdata/fix/in/nolintlint.go | 13 ++ test/testdata/fix/out/nolintlint.go | 13 ++ test/testdata/nolintlint.go | 11 +- 8 files changed, 240 insertions(+), 38 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..ee2d5c13d89a 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 len(leadingSpace) > 0 { + machineReadableReplacement := &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: pos.Column - 1, + Length: len(leadingSpace) + 2, + NewString: "//", + }, + } - if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 { - issues = append(issues, NotMachine{BaseIssue: base}) + if (l.needs & NeedsMachineOnly) != 0 { + issue := NotMachine{BaseIssue: base} + issue.BaseIssue.replacement = machineReadableReplacement + issues = append(issues, issue) + } else if len(leadingSpace) > 1 { + issue := ExtraLeadingSpace{BaseIssue: base} + issue.BaseIssue.replacement = machineReadableReplacement + issues = append(issues, issue) + } } fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) @@ -188,7 +209,7 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { lintersText, explanation := fullMatches[1], fullMatches[2] var linters []string 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) @@ -206,11 +227,34 @@ 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}) + for i, linter := range linters { + issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter} + replacement := removeNolintCompletely + if len(linters) > 1 { + otherLinters := append(append([]string(nil), linters[0:i]...), linters[i+1:]...) + replacement = &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:"), + Length: len(lintersText) - 1, + NewString: strings.Join(otherLinters, ","), + }, + } + } + issue.replacement = replacement + issues = append(issues, issue) } } } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index b4bf4fbac546..c2abf409ebe0 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: 8, + Length: 3, + 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: 8, + Length: 4, + 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,92 @@ 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 linter generates replacement for each linter", + needs: NeedsUnused, + contents: ` +package bar + +func foo() { + bad() //nolint:linter1,linter2,linter3 +}`, + expected: []issueWithReplacement{ + { + "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter1\" at testing.go:5:9", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 17, + Length: 22, + NewString: "linter2,linter3", + }, + }, + }, + { + "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter2\" at testing.go:5:9", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 17, + Length: 22, + NewString: "linter1,linter3", + }, + }, + }, + { + "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter3\" at testing.go:5:9", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 17, + Length: 22, + NewString: "linter1,linter2", + }, + }, + }, + }, + }, } for _, test := range testCases { @@ -149,12 +259,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..e0fde921ac2c 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 { @@ -163,7 +164,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 +204,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/test/testdata/fix/in/nolintlint.go b/test/testdata/fix/in/nolintlint.go new file mode 100644 index 000000000000..3187989320bf --- /dev/null +++ b/test/testdata/fix/in/nolintlint.go @@ -0,0 +1,13 @@ +//args: -Enolintlint -Elll +//config: linters-settings.nolintlint.allow-leading-space=false +package p + +func nolintlint() { + run() // nolint:bob // leading space should be dropped + run() // nolint:bob // leading spaces should be dropped + // note that the next lines will retain trailing whitespace when fixed + run() //nolint // nolint should be dropped + run() //nolint:lll // nolint should be dropped + run() //nolint:alice,lll,bob // enabled linter should be dropped + run() //nolint:alice,lll,bob,nolintlint // enabled linter should not be dropped when nolintlint is nolinted +} diff --git a/test/testdata/fix/out/nolintlint.go b/test/testdata/fix/out/nolintlint.go new file mode 100644 index 000000000000..01b5333e8360 --- /dev/null +++ b/test/testdata/fix/out/nolintlint.go @@ -0,0 +1,13 @@ +//args: -Enolintlint -Elll +//config: linters-settings.nolintlint.allow-leading-space=false +package p + +func nolintlint() { + run() //nolint:bob // leading space should be dropped + run() //nolint:bob // leading spaces should be dropped + // note that the next lines will retain trailing whitespace when fixed + run() + run() + run() //nolint:alice,bob // enabled linter should be dropped + run() //nolint:alice,lll,bob,nolintlint // enabled linter should not be dropped when nolintlint is nolinted +} 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) + }() }