Skip to content
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

Jump to bottom

Refactor nolintlint handling: Enforce directive and linter list formatting #4543

Closed
wants to merge 7 commits into from
Closed

Refactor nolintlint handling: Enforce directive and linter list formatting #4543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5f7f2b9
Refactor nolint directive handling and update tests
Mar 19, 2024
f46df26
Deprecate NeedsMachineOnly and remove all references in code
Mar 19, 2024
4417bc1
Remove NotMachine issue
Mar 19, 2024
8ce53a9
Disallow leading whitespace in fullDirectivePattern regular expression
Mar 19, 2024
df61cf5
Update test cases with leading space to use the ParseError issue
Mar 19, 2024
2c5999f
Fix unit test description for comma-separated linter list
Mar 19, 2024
a364354
Update NoExplanation details to provide a generic directive template …
Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
110 changes: 31 additions & 79 deletions pkg/golinters/nolintlint/nolintlint.go
View file
Open in desktop
Expand Up @@ -7,92 +7,71 @@ 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
type baseIssue struct {
fullDirective string
position token.Position
replacement *result.Replacement
}

//nolint:gocritic // TODO(ldez) must be change in the future.
func (b BaseIssue) Position() token.Position {
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 {
func (b baseIssue) Replacement() *result.Replacement {
return b.replacement
}

type ExtraLeadingSpace struct {
BaseIssue
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
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) }

type ParseError struct {
BaseIssue
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) }

type NoExplanation struct {
BaseIssue
baseIssue
fullDirectiveWithoutExplanation string
}

//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)
return fmt.Sprintf("directive `%s` is missing an explanation; it should follow the format `//nolint[:<comma-separated-linters>] // <explanation>`",
i.fullDirective)
}

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

type UnusedCandidate struct {
BaseIssue
baseIssue
ExpectedLinter string
}

Expand Down Expand Up @@ -121,17 +100,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(?::([\w-]+(?:,[\w-]+)*))?(?: (//.*))?\n?$`)

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

return &Linter{
needs: needs | NeedsMachineOnly,
needs: needs,
excludeByLinter: excludeByName,
}, nil
}
Expand Down Expand Up @@ -180,47 +162,17 @@ 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)
}
base := baseIssue{
fullDirective: comment.Text,
position: pos,
}

fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
issues = append(issues, ParseError{baseIssue: base})
continue
}

Expand All @@ -246,7 +198,7 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {

if (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})
issues = append(issues, NotSpecific{baseIssue: base})
}
}

Expand All @@ -261,12 +213,12 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
}

if len(linters) == 0 {
issue := UnusedCandidate{BaseIssue: base}
issue := UnusedCandidate{baseIssue: base}
issue.replacement = removeNolintCompletely
issues = append(issues, issue)
} else {
for _, linter := range linters {
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
issue := UnusedCandidate{baseIssue: base, ExpectedLinter: linter}
// only offer replacement if there is a single linter
// because of issues around commas and the possibility of all
// linters being removed
Expand All @@ -291,7 +243,7 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
if needsExplanation {
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
issues = append(issues, NoExplanation{
BaseIssue: base,
baseIssue: base,
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
})
}
Expand Down
44 changes: 16 additions & 28 deletions pkg/golinters/nolintlint/nolintlint_test.go
View file
Open in desktop
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,41 +97,29 @@ 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"},
},
},
{
desc: "spaces are allowed in comma-separated list of linters",
desc: "spaces are not allowed in comma-separated list of linters",
contents: `
package bar

func foo() {
good() //nolint:linter1,linter-two
bad() //nolint:linter1 linter2
good() //nolint: linter1,linter2
good() //nolint: linter1, linter2
bad() //nolint: linter1,linter2
bad() //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: linter1,linter2` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:7:9"},
{issue: "directive `//nolint: linter1, linter2` should match `//nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:8: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