diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index 6da31c60fd5e..d20ac30f0951 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/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[:] [// ]`", + return fmt.Sprintf("directive `%s` should match `%s[:] [// ]`", i.fullDirective, i.directiveWithOptionalLeadingSpace) } @@ -112,8 +112,7 @@ const ( NeedsAll = NeedsMachineOnly | NeedsSpecific | NeedsExplanation ) -// matches lines starting with the nolint directive -var directiveOnlyPattern = regexp.MustCompile(`^\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\b`) +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?$`) @@ -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() - matches := directiveOnlyPattern.FindStringSubmatch(text) - if len(matches) == 0 { - continue - } - directive := matches[1] + for _, comment := range c.List { + if !commentPattern.MatchString(comment.Text) { + continue + } - // check for a space between the "//" and the directive - leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(c.List[0].Text) // c.Text() doesn't have all leading space - if len(leadingSpaceMatches) == 0 { - continue - } - leadingSpace := leadingSpaceMatches[1] + // check for a space between the "//" and the directive + leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text) - directiveWithOptionalLeadingSpace := directive - if len(leadingSpace) > 0 { - directiveWithOptionalLeadingSpace = " " + directive - } + var leadingSpace string + if len(leadingSpaceMatches) > 0 { + leadingSpace = leadingSpaceMatches[1] + } - base := BaseIssue{ - fullDirective: c.List[0].Text, - directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace, - position: fset.Position(c.Pos()), - } + directiveWithOptionalLeadingSpace := comment.Text + if len(leadingSpace) > 0 { + split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//") + directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1]) + } - // check for, report and eliminate leading spaces so we can check for other issues - if leadingSpace != "" && leadingSpace != " " { - issues = append(issues, ExtraLeadingSpace{ - BaseIssue: base, - }) - } + base := BaseIssue{ + fullDirective: comment.Text, + directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace, + position: fset.Position(comment.Pos()), + } - if (l.needs&NeedsMachineOnly) != 0 && strings.HasPrefix(directiveWithOptionalLeadingSpace, " ") { - issues = append(issues, NotMachine{BaseIssue: base}) - } + // check for, report and eliminate leading spaces so we can check for other issues + if len(leadingSpace) > 1 { + issues = append(issues, ExtraLeadingSpace{BaseIssue: base}) + } - fullMatches := fullDirectivePattern.FindStringSubmatch(c.List[0].Text) - if len(fullMatches) == 0 { - issues = append(issues, ParseError{BaseIssue: base}) - continue - } - lintersText, explanation := fullMatches[1], fullMatches[2] - var linters []string - if len(lintersText) > 0 { - 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&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 { + issues = append(issues, NotMachine{BaseIssue: base}) } - } - if (l.needs & NeedsSpecific) != 0 { - if len(linters) == 0 { - issues = append(issues, NotSpecific{BaseIssue: base}) + + fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) + if len(fullMatches) == 0 { + issues = append(issues, ParseError{BaseIssue: base}) + continue } - } - // when detecting unused directives, we send all the directives through and filter them out in the nolint processor - if l.needs&NeedsUnused != 0 { - if len(linters) == 0 { - issues = append(issues, UnusedCandidate{BaseIssue: base}) - } else { - for _, linter := range linters { - issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}) + lintersText, explanation := fullMatches[1], fullMatches[2] + var linters []string + if len(lintersText) > 0 { + 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&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") { - needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation - // otherwise, check if we are excluding all of the mentioned linters - for _, ll := range linters { - if !l.excludeByLinter[ll] { // if a linter does require explanation - needsExplanation = true - break + if (l.needs & NeedsSpecific) != 0 { + if len(linters) == 0 { + issues = append(issues, NotSpecific{BaseIssue: base}) } } - if needsExplanation { - fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(c.List[0].Text, "") - issues = append(issues, NoExplanation{ - BaseIssue: base, - fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation, - }) + + // when detecting unused directives, we send all the directives through and filter them out in the nolint processor + if (l.needs & NeedsUnused) != 0 { + if len(linters) == 0 { + 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) == "//") { + needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation + // otherwise, check if we are excluding all of the mentioned linters + 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 } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index 4eb72e8dafae..b4bf4fbac546 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/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) { - linter, _ := NewLinter(NeedsExplanation, nil) - expectIssues(t, linter, ` + testCases := []struct { + desc string + 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", - "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:7:9", - ) - }) - - t.Run("when no explanation is needed for a specific linter", func(t *testing.T) { - linter, _ := NewLinter(NeedsExplanation, []string{"lll"}) - expectIssues(t, linter, ` + 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", + }, + }, + { + desc: "when multiple directives on multiple lines", + needs: NeedsExplanation, + contents: ` +package bar + +// example +//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", + }, + }, + { + 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) { - linter, _ := NewLinter(NeedsSpecific, nil) - expectIssues(t, linter, ` +}`, + }, + { + desc: "when no specific linter is mentioned", + needs: NeedsSpecific, + 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", - "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, ` + 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", + }, + }, + { + 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") - }) - - t.Run("extra spaces in front of directive are reported", func(t *testing.T) { - linter, _ := NewLinter(0, nil) - expectIssues(t, linter, ` +}`, + expected: []string{ + "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9", + }, + }, + { + 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") - }) - - t.Run("spaces are allowed in comma-separated list of linters", func(t *testing.T) { - linter, _ := NewLinter(0, nil) - expectIssues(t, linter, ` +}`, + expected: []string{ + "directive `// nolint` should not have more than one leading space at testing.go:5:9", + }, + }, + { + 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[:] [// ]` 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) - expectIssues(t, linter, ` + expected: []string{ + "directive `// nolint:linter1 linter2` should match `// nolint[:] [// ]` at testing.go:6:9", //nolint:lll // this is a string + }, + }, + { + desc: "multi-line comments don't confuse parser", + contents: ` package bar func foo() { //nolint:test // something else -}`) - }) -} - -func expectIssues(t *testing.T, linter *Linter, contents string, issues ...string) { - actualIssues := parseFile(t, linter, contents) - actualIssueStrs := make([]string, 0, len(actualIssues)) - for _, i := range actualIssues { - 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 { - fset := token.NewFileSet() - expr, err := parser.ParseFile(fset, "testing.go", contents, parser.ParseComments) - if err != nil { - t.Fatalf("unable to parse file contents: %s", err) - } - issues, err := linter.Run(fset, expr) - if err != nil { - t.Fatalf("unable to parse file: %s", err) + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + linter, _ := NewLinter(test.needs, test.excludes) + + fset := token.NewFileSet() + expr, err := parser.ParseFile(fset, "testing.go", test.contents, parser.ParseComments) + 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) + }) } - return issues } diff --git a/test/errchk.go b/test/errchk.go index 962f05a783c9..ae2c42d759d9 100644 --- a/test/errchk.go +++ b/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)