diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index 1bce5ef5d2d8..b746e6f3b2bc 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -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[:] [// ]`", - i.fullDirective, - i.directiveWithOptionalLeadingSpace) + return fmt.Sprintf("directive `%s` should match `//nolint[:] [// ]`", + 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[:] // `", + i.fullDirective) } func (i NoExplanation) String() string { return toString(i) } type UnusedCandidate struct { - BaseIssue + baseIssue ExpectedLinter string } @@ -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 @@ -146,7 +128,7 @@ func NewLinter(needs Needs, excludes []string) (*Linter, error) { } return &Linter{ - needs: needs | NeedsMachineOnly, + needs: needs, excludeByLinter: excludeByName, }, nil } @@ -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 } @@ -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}) } } @@ -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 @@ -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, }) } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index a483c7ea66b1..2daee30c81b6 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -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[:] // ` at testing.go:5:1"}, + {issue: "directive `//nolint` is missing an explanation; it should follow the format `//nolint[:] // ` at testing.go:7:9"}, + {issue: "directive `//nolint //` is missing an explanation; it should follow the format `//nolint[:] // ` at testing.go:8:9"}, + {issue: "directive `//nolint // ` is missing an explanation; it should follow the format `//nolint[:] // ` at testing.go:9:9"}, }, }, { @@ -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[:] // ` at testing.go:6:1"}, }, }, { @@ -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[:] [// ]` at testing.go:5:9"}, + {issue: "directive `// nolint` should match `//nolint[:] [// ]` 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[:] [// ]` at testing.go:6:9"}, + {issue: "directive `//nolint: linter1,linter2` should match `//nolint[:] [// ]` at testing.go:7:9"}, + {issue: "directive `//nolint: linter1, linter2` should match `//nolint[:] [// ]` at testing.go:8:9"}, + {issue: "directive `//nolint / description` should match `//nolint[:] [// ]` at testing.go:9:9"}, + {issue: "directive `//nolint, // hi` should match `//nolint[:] [// ]` at testing.go:10:9"}, }, }, {