Skip to content

nolintlint: enforce directive and linter list formatting #4544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

nolintlint: enforce directive and linter list formatting #4544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c73fc43
Refactor nolint directive handling and update tests
aaronpradhan2 Mar 19, 2024
852cc82
Deprecate NeedsMachineOnly and remove all references in code
aaronpradhan2 Mar 19, 2024
8de2361
Remove NotMachine issue
aaronpradhan2 Mar 19, 2024
5a0e9d7
Disallow leading whitespace in fullDirectivePattern regular expression
aaronpradhan2 Mar 19, 2024
0e91ea9
Update test cases with leading space to use the ParseError issue
aaronpradhan2 Mar 19, 2024
a6eb533
Fix unit test description for comma-separated linter list
aaronpradhan2 Mar 19, 2024
29dda05
Update NoExplanation details to provide a generic directive template …
aaronpradhan2 Mar 19, 2024
476902e
Make the ParseError issue more liberal to allow spaces in the linter …
aaronpradhan2 Mar 21, 2024
a17bca8
Export BaseIssue struct
aaronpradhan2 Mar 21, 2024
acc7823
Split string literal in NoExplanation issue details
aaronpradhan2 Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 18 additions & 65 deletions pkg/golinters/nolintlint/nolintlint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,20 @@ import (
"go/token"
"regexp"
"strings"
"unicode"

"github.com/golangci/golangci-lint/pkg/result"
)

type BaseIssue struct {
fullDirective string
directiveWithOptionalLeadingSpace string
position token.Position
replacement *result.Replacement
fullDirective string
position token.Position
replacement *result.Replacement
}

//nolint:gocritic // TODO(ldez) must be change in the future.
func (b BaseIssue) Position() token.Position {
return b.position
}

//nolint:gocritic // TODO(ldez) must be change in the future.
func (b BaseIssue) Replacement() *result.Replacement {
return b.replacement
}
Expand All @@ -33,34 +29,19 @@ type ExtraLeadingSpace struct {
BaseIssue
}

//nolint:gocritic // TODO(ldez) must be change in the future.
func (i ExtraLeadingSpace) Details() string {
return fmt.Sprintf("directive `%s` should not have more than one leading space", i.fullDirective)
}

func (i ExtraLeadingSpace) String() string { return toString(i) }

type NotMachine struct {
BaseIssue
}

//nolint:gocritic // TODO(ldez) must be change in the future.
func (i NotMachine) Details() string {
expected := i.fullDirective[:2] + strings.TrimLeftFunc(i.fullDirective[2:], unicode.IsSpace)
return fmt.Sprintf("directive `%s` should be written without leading space as `%s`",
i.fullDirective, expected)
}

func (i NotMachine) String() string { return toString(i) }

type NotSpecific struct {
BaseIssue
}

//nolint:gocritic // TODO(ldez) must be change in the future.
func (i NotSpecific) Details() string {
return fmt.Sprintf("directive `%s` should mention specific linter such as `%s:my-linter`",
i.fullDirective, i.directiveWithOptionalLeadingSpace)
return fmt.Sprintf("directive `%s` should mention specific linter such as `//nolint:my-linter`",
i.fullDirective)
}

func (i NotSpecific) String() string { return toString(i) }
Expand All @@ -69,11 +50,9 @@ type ParseError struct {
BaseIssue
}

//nolint:gocritic // TODO(ldez) must be change in the future.
func (i ParseError) Details() string {
return fmt.Sprintf("directive `%s` should match `%s[:<comma-separated-linters>] [// <explanation>]`",
i.fullDirective,
i.directiveWithOptionalLeadingSpace)
return fmt.Sprintf("directive `%s` should match `//nolint[:<comma-separated-linters>] [// <explanation>]`",
i.fullDirective)
}

func (i ParseError) String() string { return toString(i) }
Expand All @@ -85,8 +64,9 @@ type NoExplanation struct {

//nolint:gocritic // TODO(ldez) must be change in the future.
func (i NoExplanation) Details() string {
return fmt.Sprintf("directive `%s` should provide explanation such as `%s // this is why`",
i.fullDirective, i.fullDirectiveWithoutExplanation)
baseMsg := "directive `%s` is missing an explanation; "
explanationMsg := "it should follow the format `//nolint[:<comma-separated-linters>] // <explanation>`"
return fmt.Sprintf(baseMsg+explanationMsg, i.fullDirective)
}

func (i NoExplanation) String() string { return toString(i) }
Expand Down Expand Up @@ -121,17 +101,20 @@ type Issue interface {
type Needs uint

const (
// Deprecated: NeedsMachineOnly is deprecated as leading spaces are no longer allowed,
// making this condition always true. Consumers should adjust their code to assume
// this as the default behavior and no longer rely on NeedsMachineOnly.
NeedsMachineOnly Needs = 1 << iota
NeedsSpecific
NeedsExplanation
NeedsUnused
NeedsAll = NeedsMachineOnly | NeedsSpecific | NeedsExplanation
NeedsAll = NeedsSpecific | NeedsExplanation
)

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(`^//nolint(?::(\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*))?\s*(//.*)?\s*\n?$`)

type Linter struct {
needs Needs // indicates which linter checks to perform
Expand All @@ -146,7 +129,7 @@ func NewLinter(needs Needs, excludes []string) (*Linter, error) {
}

return &Linter{
needs: needs | NeedsMachineOnly,
needs: needs,
excludeByLinter: excludeByName,
}, nil
}
Expand Down Expand Up @@ -180,42 +163,12 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
leadingSpace = leadingSpaceMatches[1]
}

directiveWithOptionalLeadingSpace := "//"
if leadingSpace != "" {
directiveWithOptionalLeadingSpace += " "
}

split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//")
directiveWithOptionalLeadingSpace += strings.TrimSpace(split[1])

pos := fset.Position(comment.Pos())
end := fset.Position(comment.End())

base := BaseIssue{
fullDirective: comment.Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: pos,
}

// check for, report and eliminate leading spaces, so we can check for other issues
if leadingSpace != "" {
removeWhitespace := &result.Replacement{
Inline: &result.InlineFix{
StartCol: pos.Column + 1,
Length: len(leadingSpace),
NewString: "",
},
}
if (l.needs & NeedsMachineOnly) != 0 {
issue := NotMachine{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issues = append(issues, issue)
} else if len(leadingSpace) > 1 {
issue := ExtraLeadingSpace{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
issues = append(issues, issue)
}
fullDirective: comment.Text,
position: pos,
}

fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
Expand Down
36 changes: 11 additions & 25 deletions pkg/golinters/nolintlint/nolintlint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ func foo() {
other() //nolintother
}`,
expected: []issueWithReplacement{
{issue: "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1"},
{issue: "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9"},
{issue: "directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9"},
{issue: "directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9"},
{issue: "directive `//nolint` is missing an explanation; it should follow the format `//nolint[:<comma-separated-linters>] // <explanation>` at testing.go:5:1"},
{issue: "directive `//nolint` is missing an explanation; it should follow the format `//nolint[:<comma-separated-linters>] // <explanation>` at testing.go:7:9"},
{issue: "directive `//nolint //` is missing an explanation; it should follow the format `//nolint[:<comma-separated-linters>] // <explanation>` at testing.go:8:9"},
{issue: "directive `//nolint // ` is missing an explanation; it should follow the format `//nolint[:<comma-separated-linters>] // <explanation>` at testing.go:9:9"},
},
},
{
Expand All @@ -56,7 +56,7 @@ package bar
//nolint:dupl
func foo() {}`,
expected: []issueWithReplacement{
{issue: "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1"},
{issue: "directive `//nolint:dupl` is missing an explanation; it should follow the format `//nolint[:<comma-separated-linters>] // <explanation>` at testing.go:6:1"},
},
},
{
Expand Down Expand Up @@ -97,26 +97,8 @@ func foo() {
good() //nolint
}`,
expected: []issueWithReplacement{
{
issue: "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 10,
Length: 1,
NewString: "",
},
},
},
{
issue: "directive `// nolint` should be written without leading space as `//nolint` at testing.go:6:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 10,
Length: 3,
NewString: "",
},
},
},
{issue: "directive `// nolint` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:5:9"},
{issue: "directive `// nolint` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"},
},
},
{
Expand All @@ -129,9 +111,13 @@ func foo() {
bad() //nolint:linter1 linter2
good() //nolint: linter1,linter2
good() //nolint: linter1, linter2
bad() //nolint / description
bad() //nolint, // hi
}`,
expected: []issueWithReplacement{
{issue: "directive `//nolint:linter1 linter2` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"},
{issue: "directive `//nolint / description` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:9:9"},
{issue: "directive `//nolint, // hi` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:10:9"},
},
},
{
Expand Down
Loading