Allow multiple linters to be removed from nolint statement by nolint …

…fixer Also ensure that //nolint:nolintlint can be used to disable nolintlint
golangci · Dec 27, 2020 · ba47cb7 · ba47cb7
1 parent aeb9830
commit ba47cb7
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 51 deletions.
diff --git 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
@@ -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,28 +216,60 @@ func foo() {
 					&result.Replacement{
 						Inline: &result.InlineFix{
 							StartCol:  17,
-							Length:    22,
-							NewString: "linter2,linter3",
+							Length:    8,
+							NewString: "",
 						},
 					},
 				},
 				{
 					"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: "",
 						},
 					},
 				},
 				{
 					"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
@@ -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
@@ -290,6 +290,17 @@ func TestNolintUnused(t *testing.T) {
 		ExpectNoLint:         true,
 		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"})
@@ -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
@@ -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
@@ -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
@@ -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
 }