Revert "Update nolintlint to fix nolint formatting and remove unused nolint statements (#1573)" (#1584)

This reverts commit aeb9830.

There are some cases that nolinter fixer wasn't handling properly or expectedly (#1579, #1580, #1581) so we'll fix those in a new attempt.

Author: ashanbrown

pkg/golinters/nolintlint.go

@@ -72,7 +72,6 @@ func NewNoLintLint() *goanalysis.Linter {
 					Pos:                  i.Position(),
 					ExpectNoLint:         expectNoLint,
 					ExpectedNoLintLinter: expectedNolintLinter,
-					Replacement:          i.Replacement(),
 				}
 				res = append(res, goanalysis.NewIssue(issue, pass))
 			}

pkg/golinters/nolintlint/nolintlint.go

@@ -8,21 +8,16 @@ 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) Replacement() *result.Replacement {
-	return b.replacement
+func (b BaseIssue) Position() token.Position {
+	return b.position
 }
 
 type ExtraLeadingSpace struct {
@@ -90,7 +85,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 %q", i.ExpectedLinter)
+		details += fmt.Sprintf(" for linter %s", i.ExpectedLinter)
 	}
 	return details
 }
@@ -105,7 +100,6 @@ type Issue interface {
 	Details() string
 	Position() token.Position
 	String() string
-	Replacement() *result.Replacement
 }
 
 type Needs uint
@@ -121,7 +115,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
@@ -170,34 +164,19 @@ 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:                          pos,
+						position:                          fset.Position(comment.Pos()),
 					}
 
 					// check for, report and eliminate leading spaces so we can check for other issues
-					if len(leadingSpace) > 0 {
-						machineReadableReplacement := &result.Replacement{
-							Inline: &result.InlineFix{
-								StartCol:  pos.Column - 1,
-								Length:    len(leadingSpace) + 2,
-								NewString: "//",
-							},
-						}
+					if len(leadingSpace) > 1 {
+						issues = append(issues, ExtraLeadingSpace{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)
-						}
+					if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
+						issues = append(issues, NotMachine{BaseIssue: base})
 					}
 
 					fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
@@ -209,7 +188,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, ",")
+						lls := strings.Split(lintersText[1:], ",")
 						linters = make([]string, 0, len(lls))
 						for _, ll := range lls {
 							ll = strings.TrimSpace(ll)
@@ -227,34 +206,11 @@ 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 {
-							issue := UnusedCandidate{BaseIssue: base}
-							issue.replacement = removeNolintCompletely
-							issues = append(issues, issue)
+							issues = append(issues, UnusedCandidate{BaseIssue: base})
 						} else {
-							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)
+							for _, linter := range linters {
+								issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
 							}
 						}
 					}

pkg/golinters/nolintlint/nolintlint_test.go

@@ -7,22 +7,16 @@ 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 []issueWithReplacement
+		expected []string
 	}{
 		{
 			desc:  "when no explanation is provided",
@@ -39,11 +33,11 @@ func foo() {
   good() //nolint // this is ok
 	other() //nolintother
 }`,
-			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},
+			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",
 			},
 		},
 		{
@@ -56,8 +50,8 @@ package bar
 //nolint // this is ok
 //nolint:dupl
 func foo() {}`,
-			expected: []issueWithReplacement{
-				{"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil},
+			expected: []string{
+				"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1",
 			},
 		},
 		{
@@ -82,9 +76,9 @@ func foo() {
   bad() //nolint
   bad() // nolint // because
 }`,
-			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},
+			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",
 			},
 		},
 		{
@@ -97,17 +91,8 @@ func foo() {
   bad() // nolint
   good() //nolint
 }`,
-			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: "//",
-						},
-					},
-				},
+			expected: []string{
+				"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
 			},
 		},
 		{
@@ -119,17 +104,8 @@ func foo() {
   bad() //  nolint
   good() // nolint
 }`,
-			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: "//",
-						},
-					},
-				},
+			expected: []string{
+				"directive `//  nolint` should not have more than one leading space at testing.go:5:9",
 			},
 		},
 		{
@@ -143,8 +119,8 @@ func foo() {
   good() // nolint: linter1,linter2
   good() // nolint: linter1, linter2
 }`,
-			expected: []issueWithReplacement{
-				{"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", nil}, //nolint:lll // this is a string
+			expected: []string{
+				"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", //nolint:lll // this is a string
 			},
 		},
 		{
@@ -157,92 +133,6 @@ 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 {
@@ -259,16 +149,12 @@ func foo() {
 			actualIssues, err := linter.Run(fset, expr)
 			require.NoError(t, err)
 
-			actualIssuesWithReplacements := make([]issueWithReplacement, 0, len(actualIssues))
+			actualIssueStrs := make([]string, 0, len(actualIssues))
 			for _, i := range actualIssues {
-				actualIssuesWithReplacements = append(actualIssuesWithReplacements, issueWithReplacement{
-					issue:       i.String(),
-					replacement: i.Replacement(),
-				})
+				actualIssueStrs = append(actualIssueStrs, i.String())
 			}
 
-			assert.ElementsMatch(t, test.expected, actualIssuesWithReplacements,
-				"expected %s \nbut got %s", test.expected, actualIssuesWithReplacements)
+			assert.ElementsMatch(t, test.expected, actualIssueStrs, "expected %s \nbut got %s", test.expected, actualIssues)
 		})
 	}
 }

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 // if NeedDelete is false it's the replacement lines
+	NewLines       []string // is NeedDelete is false it's the replacement lines
 	Inline         *InlineFix
 }