fix: nolintlint comment analysis. (#1571)

pkg/golinters/nolintlint/nolintlint.go

@@ -47,7 +47,7 @@ type NotSpecific struct {
 }
 
 func (i NotSpecific) Details() string {
-	return fmt.Sprintf("directive `%s` should mention specific linter such as `//%s:my-linter`",
+	return fmt.Sprintf("directive `%s` should mention specific linter such as `%s:my-linter`",
 		i.fullDirective, i.directiveWithOptionalLeadingSpace)
 }
 
@@ -58,7 +58,7 @@ type ParseError struct {
 }
 
 func (i ParseError) Details() string {
-	return fmt.Sprintf("directive `%s` should match `//%s[:<comma-separated-linters>] [// <explanation>]`",
+	return fmt.Sprintf("directive `%s` should match `%s[:<comma-separated-linters>] [// <explanation>]`",
 		i.fullDirective,
 		i.directiveWithOptionalLeadingSpace)
 }
@@ -112,8 +112,7 @@ const (
 	NeedsAll = NeedsMachineOnly | NeedsSpecific | NeedsExplanation
 )
 
-// matches lines starting with the nolint directive
+var commentPattern = regexp.MustCompile(`^//\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\b`)
-var directiveOnlyPattern = 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?$`)
@@ -142,98 +141,102 @@ var trailingBlankExplanation = regexp.MustCompile(`\s*(//\s*)?$`)
 
 func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
 	var issues []Issue
+
 	for _, node := range nodes {
 		if file, ok := node.(*ast.File); ok {
 			for _, c := range file.Comments {
-				text := c.Text()
+				for _, comment := range c.List {
-				matches := directiveOnlyPattern.FindStringSubmatch(text)
+					if !commentPattern.MatchString(comment.Text) {
-				if len(matches) == 0 {
+						continue
-					continue
+					}
-				}
-				directive := matches[1]
 
-				// check for a space between the "//" and the directive
+					// check for a space between the "//" and the directive
-				leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(c.List[0].Text) // c.Text() doesn't have all leading space
+					leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)
-				if len(leadingSpaceMatches) == 0 {
-					continue
-				}
-				leadingSpace := leadingSpaceMatches[1]
 
-				directiveWithOptionalLeadingSpace := directive
+					var leadingSpace string
-				if len(leadingSpace) > 0 {
+					if len(leadingSpaceMatches) > 0 {
-					directiveWithOptionalLeadingSpace = " " + directive
+						leadingSpace = leadingSpaceMatches[1]
-				}
+					}
 
-				base := BaseIssue{
+					directiveWithOptionalLeadingSpace := comment.Text
-					fullDirective:                     c.List[0].Text,
+					if len(leadingSpace) > 0 {
-					directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
+						split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//")
-					position:                          fset.Position(c.Pos()),
+						directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1])
-				}
+					}
 
-				// check for, report and eliminate leading spaces so we can check for other issues
+					base := BaseIssue{
-				if leadingSpace != "" && leadingSpace != " " {
+						fullDirective:                     comment.Text,
-					issues = append(issues, ExtraLeadingSpace{
+						directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
-						BaseIssue: base,
+						position:                          fset.Position(comment.Pos()),
-					})
+					}
-				}
 
-				if (l.needs&NeedsMachineOnly) != 0 && strings.HasPrefix(directiveWithOptionalLeadingSpace, " ") {
+					// check for, report and eliminate leading spaces so we can check for other issues
-					issues = append(issues, NotMachine{BaseIssue: base})
+					if len(leadingSpace) > 1 {
-				}
+						issues = append(issues, ExtraLeadingSpace{BaseIssue: base})
+					}
 
-				fullMatches := fullDirectivePattern.FindStringSubmatch(c.List[0].Text)
+					if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
-				if len(fullMatches) == 0 {
+						issues = append(issues, NotMachine{BaseIssue: base})
-					issues = append(issues, ParseError{BaseIssue: base})
+					}
-					continue
+
-				}
+					fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
-				lintersText, explanation := fullMatches[1], fullMatches[2]
+					if len(fullMatches) == 0 {
-				var linters []string
+						issues = append(issues, ParseError{BaseIssue: base})
-				if len(lintersText) > 0 {
+						continue
-					lls := strings.Split(lintersText[1:], ",")
+					}
-					linters = make([]string, 0, len(lls))
+
-					for _, ll := range lls {
+					lintersText, explanation := fullMatches[1], fullMatches[2]
-						ll = strings.TrimSpace(ll)
+					var linters []string
-						if ll != "" {
+					if len(lintersText) > 0 {
-							linters = append(linters, ll)
+						lls := strings.Split(lintersText[1:], ",")
+						linters = make([]string, 0, len(lls))
+						for _, ll := range lls {
+							ll = strings.TrimSpace(ll)
+							if ll != "" {
+								linters = append(linters, ll)
+							}
 						}
 					}
-				}
-				if (l.needs & NeedsSpecific) != 0 {
-					if len(linters) == 0 {
-						issues = append(issues, NotSpecific{BaseIssue: base})
-					}
-				}
 
-				// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
+					if (l.needs & NeedsSpecific) != 0 {
-				if l.needs&NeedsUnused != 0 {
+						if len(linters) == 0 {
-					if len(linters) == 0 {
+							issues = append(issues, NotSpecific{BaseIssue: base})
-						issues = append(issues, UnusedCandidate{BaseIssue: base})
-					} else {
-						for _, linter := range linters {
-							issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
 						}
 					}
-				}
 
-				if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
+					// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
-					needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
+					if (l.needs & NeedsUnused) != 0 {
-					// otherwise, check if we are excluding all of the mentioned linters
+						if len(linters) == 0 {
-					for _, ll := range linters {
+							issues = append(issues, UnusedCandidate{BaseIssue: base})
-						if !l.excludeByLinter[ll] { // if a linter does require explanation
+						} else {
-							needsExplanation = true
+							for _, linter := range linters {
-							break
+								issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
+							}
 						}
 					}
-					if needsExplanation {
+
-						fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(c.List[0].Text, "")
+					if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
-						issues = append(issues, NoExplanation{
+						needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
-							BaseIssue:                       base,
+						// otherwise, check if we are excluding all of the mentioned linters
-							fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
+						for _, ll := range linters {
-						})
+							if !l.excludeByLinter[ll] { // if a linter does require explanation
+								needsExplanation = true
+								break
+							}
+						}
+
+						if needsExplanation {
+							fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
+							issues = append(issues, NoExplanation{
+								BaseIssue:                       base,
+								fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
+							})
+						}
 					}
 				}
 			}
 		}
 	}
+
 	return issues, nil
 }

pkg/golinters/nolintlint/nolintlint_test.go

@@ -6,14 +6,26 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+//nolint:funlen
 func TestNoLintLint(t *testing.T) {
-	t.Run("when no explanation is provided", func(t *testing.T) {
+	testCases := []struct {
-		linter, _ := NewLinter(NeedsExplanation, nil)
+		desc     string
-		expectIssues(t, linter, `
+		needs    Needs
+		excludes []string
+		contents string
+		expected []string
+	}{
+		{
+			desc:  "when no explanation is provided",
+			needs: NeedsExplanation,
+			contents: `
 package bar
 
+// example
+//nolint
 func foo() {
   bad() //nolint
   bad() //nolint //
@@ -21,25 +33,42 @@ func foo() {
   good() //nolint // this is ok
 	other() //nolintother
 }`,
-			"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:9",
+			expected: []string{
-			"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:6:9",
+				"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: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",
+			},
+		},
+		{
+			desc:  "when multiple directives on multiple lines",
+			needs: NeedsExplanation,
+			contents: `
+package bar
 
-	t.Run("when no explanation is needed for a specific linter", func(t *testing.T) {
+// example
-		linter, _ := NewLinter(NeedsExplanation, []string{"lll"})
+//nolint // this is ok
-		expectIssues(t, linter, `
+//nolint:dupl
+func foo() {}`,
+			expected: []string{
+				"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1",
+			},
+		},
+		{
+			desc:     "when no explanation is needed for a specific linter",
+			needs:    NeedsExplanation,
+			excludes: []string{"lll"},
+			contents: `
 package bar
 
 func foo() {
 	thisIsAReallyLongLine() //nolint:lll
-}`)
+}`,
-	})
+		},
-
+		{
-	t.Run("when no specific linter is mentioned", func(t *testing.T) {
+			desc:  "when no specific linter is mentioned",
-		linter, _ := NewLinter(NeedsSpecific, nil)
+			needs: NeedsSpecific,
-		expectIssues(t, linter, `
+			contents: `
 package bar
 
 func foo() {
@@ -47,35 +76,41 @@ func foo() {
   bad() //nolint
   bad() // nolint // because
 }`,
-			"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9",
+			expected: []string{
-			"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9")
+				"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",
-
+			},
-	t.Run("when machine-readable style isn't used", func(t *testing.T) {
+		},
-		linter, _ := NewLinter(NeedsMachineOnly, nil)
+		{
-		expectIssues(t, linter, `
+			desc:  "when machine-readable style isn't used",
+			needs: NeedsMachineOnly,
+			contents: `
 package bar
 
 func foo() {
   bad() // nolint
   good() //nolint
-}`, "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9")
+}`,
-	})
+			expected: []string{
-
+				"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
-	t.Run("extra spaces in front of directive are reported", func(t *testing.T) {
+			},
-		linter, _ := NewLinter(0, nil)
+		},
-		expectIssues(t, linter, `
+		{
+			desc: "extra spaces in front of directive are reported",
+			contents: `
 package bar
 
 func foo() {
   bad() //  nolint
   good() // nolint
-}`, "directive `//  nolint` should not have more than one leading space at testing.go:5:9")
+}`,
-	})
+			expected: []string{
-
+				"directive `//  nolint` should not have more than one leading space at testing.go:5:9",
-	t.Run("spaces are allowed in comma-separated list of linters", func(t *testing.T) {
+			},
-		linter, _ := NewLinter(0, nil)
+		},
-		expectIssues(t, linter, `
+		{
+			desc: "spaces are allowed in comma-separated list of linters",
+			contents: `
 package bar
 
 func foo() {
@@ -84,40 +119,42 @@ func foo() {
   good() // nolint: linter1,linter2
   good() // nolint: linter1, linter2
 }`,
-			"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", //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
-	})
+			},
-
+		},
-	t.Run("multi-line comments don't confuse parser", func(t *testing.T) {
+		{
-		linter, _ := NewLinter(0, nil)
+			desc: "multi-line comments don't confuse parser",
-		expectIssues(t, linter, `
+			contents: `
 package bar
 
 func foo() {
   //nolint:test
   // something else
-}`)
+}`,
-	})
+		},
-}
+	}
 
-func expectIssues(t *testing.T, linter *Linter, contents string, issues ...string) {
+	for _, test := range testCases {
-	actualIssues := parseFile(t, linter, contents)
+		test := test
-	actualIssueStrs := make([]string, 0, len(actualIssues))
+		t.Run(test.desc, func(t *testing.T) {
-	for _, i := range actualIssues {
+			t.Parallel()
-		actualIssueStrs = append(actualIssueStrs, i.String())
-	}
-	assert.ElementsMatch(t, issues, actualIssueStrs, "expected %s but got %s", issues, actualIssues)
-}
 
-func parseFile(t *testing.T, linter *Linter, contents string) []Issue {
+			linter, _ := NewLinter(test.needs, test.excludes)
-	fset := token.NewFileSet()
+
-	expr, err := parser.ParseFile(fset, "testing.go", contents, parser.ParseComments)
+			fset := token.NewFileSet()
-	if err != nil {
+			expr, err := parser.ParseFile(fset, "testing.go", test.contents, parser.ParseComments)
-		t.Fatalf("unable to parse file contents: %s", err)
+			require.NoError(t, err)
+
+			actualIssues, err := linter.Run(fset, expr)
+			require.NoError(t, err)
+
+			actualIssueStrs := make([]string, 0, len(actualIssues))
+			for _, i := range actualIssues {
+				actualIssueStrs = append(actualIssueStrs, i.String())
+			}
+
+			assert.ElementsMatch(t, test.expected, actualIssueStrs, "expected %s \nbut got %s", test.expected, actualIssues)
+		})
 	}
-	issues, err := linter.Run(fset, expr)
-	if err != nil {
-		t.Fatalf("unable to parse file: %s", err)
-	}
-	return issues
 }

test/errchk.go

@@ -22,7 +22,7 @@ import (
 //
 // Sources files are supplied as fullshort slice.
 // It consists of pairs: full path to source file and its base name.
-//nolint:gocyclo,funlen
+//nolint:gocyclo
 func errorCheck(outStr string, wantAuto bool, fullshort ...string) (err error) {
 	var errs []error
 	out := splitOutput(outStr, wantAuto)
@@ -160,7 +160,7 @@ var (
 )
 
 // wantedErrors parses expected errors from comments in a file.
-//nolint:nakedret,gocyclo,funlen
+//nolint:nakedret
 func wantedErrors(file, short string) (errs []wantedError) {
 	cache := make(map[string]*regexp.Regexp)