diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index ee2d5c13d89a..1389dd31c433 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -208,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, ",") 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 } } @@ -244,12 +252,11 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { 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, ","), + StartCol: linterRange[i].From, + Length: linterRange[i].To - linterRange[i].From, + NewString: "", }, } } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index c2abf409ebe0..373da1332078 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -202,7 +202,7 @@ func foo() { }, }, { - desc: "needs unused with multiple specific linter generates replacement for each linter", + desc: "needs unused with multiple specific linters generates a replacement for each linter", needs: NeedsUnused, contents: ` package bar @@ -216,8 +216,8 @@ func foo() { &result.Replacement{ Inline: &result.InlineFix{ StartCol: 17, - Length: 22, - NewString: "linter2,linter3", + Length: 8, + NewString: "", }, }, }, @@ -225,9 +225,9 @@ func foo() { "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", + StartCol: 25, + Length: 8, + NewString: "", }, }, }, @@ -235,9 +235,41 @@ func foo() { "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", + StartCol: 33, + Length: 7, + NewString: "", + }, + }, + }, + }, + }, + { + desc: "needs unused with multiple specific linters generates a replacement preserving space after commas", + needs: NeedsUnused, + contents: ` +package bar + +func foo() { + good() //nolint:linter1, linter2 +}`, + expected: []issueWithReplacement{ + { + "directive `//nolint:linter1, linter2` is unused for linter \"linter1\" at testing.go:5:10", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 18, + Length: 8, + NewString: "", + }, + }, + }, + { + "directive `//nolint:linter1, linter2` is unused for linter \"linter2\" at testing.go:5:10", + &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: 26, + Length: 8, + NewString: "", }, }, }, diff --git a/pkg/result/processors/nolint.go b/pkg/result/processors/nolint.go index e0fde921ac2c..06af04ea19cf 100644 --- a/pkg/result/processors/nolint.go +++ b/pkg/result/processors/nolint.go @@ -30,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 @@ -142,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) diff --git a/pkg/result/processors/nolint_test.go b/pkg/result/processors/nolint_test.go index 294add383b06..4c856889fa85 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, it is not 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 index 3187989320bf..ca1ef165509b 100644 --- a/test/testdata/fix/in/nolintlint.go +++ b/test/testdata/fix/in/nolintlint.go @@ -2,12 +2,15 @@ //config: linters-settings.nolintlint.allow-leading-space=false package p +import "fmt" + func nolintlint() { - run() // nolint:bob // leading space should be dropped - run() // nolint:bob // leading spaces should be dropped + 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 - 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 + fmt.Println() //nolint // nolint should be dropped + fmt.Println() //nolint:lll // nolint should be dropped + fmt.Println() //nolint:alice,lll,bob // enabled linter should be dropped + fmt.Println() //nolint:alice,lll, bob // enabled linter should be dropped but whitespace preserved + fmt.Println() //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 index 01b5333e8360..cfa23fa6491e 100644 --- a/test/testdata/fix/out/nolintlint.go +++ b/test/testdata/fix/out/nolintlint.go @@ -2,12 +2,15 @@ //config: linters-settings.nolintlint.allow-leading-space=false package p +import "fmt" + func nolintlint() { - run() //nolint:bob // leading space should be dropped - run() //nolint:bob // leading spaces should be dropped + 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 - 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 + fmt.Println() + fmt.Println() + fmt.Println() //nolint:alice,bob // enabled linter should be dropped + fmt.Println() //nolint:alice, bob // enabled linter should be dropped but whitespace preserved + fmt.Println() //nolint:alice,lll,bob,nolintlint // enabled linter should not be dropped when nolintlint is nolinted }