From 5f7f2b9d7c66f4334c16211f2b5c46b92d16663a Mon Sep 17 00:00:00 2001 From: Aaron Pradhan Date: Tue, 19 Mar 2024 16:48:29 -0400 Subject: [PATCH 1/7] Refactor nolint directive handling and update tests --- pkg/golinters/nolintlint/nolintlint.go | 77 ++++++++------------- pkg/golinters/nolintlint/nolintlint_test.go | 10 ++- 2 files changed, 38 insertions(+), 49 deletions(-) diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index 1bce5ef5d2d8..6e1fc2e4dbe8 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -12,28 +12,24 @@ import ( "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) } @@ -41,10 +37,9 @@ func (i ExtraLeadingSpace) Details() string { func (i ExtraLeadingSpace) String() string { return toString(i) } type NotMachine struct { - BaseIssue + 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`", @@ -54,32 +49,29 @@ func (i NotMachine) Details() string { 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 } @@ -92,7 +84,7 @@ func (i NoExplanation) Details() string { func (i NoExplanation) String() string { return toString(i) } type UnusedCandidate struct { - BaseIssue + baseIssue ExpectedLinter string } @@ -131,7 +123,7 @@ const ( 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(`^//\s*nolint(?::([\w-]+(?:,[\w-]+)*))?(?: (//.*))?\n?$`) type Linter struct { needs Needs // indicates which linter checks to perform @@ -180,21 +172,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, + base := baseIssue{ + fullDirective: comment.Text, + position: pos, } // check for, report and eliminate leading spaces, so we can check for other issues @@ -207,20 +190,20 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { }, } if (l.needs & NeedsMachineOnly) != 0 { - issue := NotMachine{BaseIssue: base} - issue.BaseIssue.replacement = removeWhitespace + 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 + issue := ExtraLeadingSpace{baseIssue: base} + issue.baseIssue.replacement = removeWhitespace + issue.baseIssue.replacement.Inline.NewString = " " // assume a single space was intended issues = append(issues, issue) } } fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) if len(fullMatches) == 0 { - issues = append(issues, ParseError{BaseIssue: base}) + issues = append(issues, ParseError{baseIssue: base}) continue } @@ -246,7 +229,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 +244,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 +274,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..121b193182c9 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -127,11 +127,17 @@ 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"}, }, }, { From f46df266d66f8fd1a049031d326c040226ab5d0f Mon Sep 17 00:00:00 2001 From: Aaron Pradhan Date: Tue, 19 Mar 2024 17:13:22 -0400 Subject: [PATCH 2/7] Deprecate NeedsMachineOnly and remove all references in code --- pkg/golinters/nolintlint/nolintlint.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index 6e1fc2e4dbe8..e1c72b3a3f9f 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -113,11 +113,14 @@ 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`) @@ -138,7 +141,7 @@ func NewLinter(needs Needs, excludes []string) (*Linter, error) { } return &Linter{ - needs: needs | NeedsMachineOnly, + needs: needs, excludeByLinter: excludeByName, }, nil } @@ -189,16 +192,9 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { 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) - } + issue := NotMachine{baseIssue: base} + issue.baseIssue.replacement = removeWhitespace + issues = append(issues, issue) } fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) From 4417bc1f8161358b1d1f8e29a2ee3011d7f65957 Mon Sep 17 00:00:00 2001 From: Aaron Pradhan Date: Tue, 19 Mar 2024 17:19:59 -0400 Subject: [PATCH 3/7] Remove NotMachine issue --- pkg/golinters/nolintlint/nolintlint.go | 27 -------------------------- 1 file changed, 27 deletions(-) diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index e1c72b3a3f9f..3daaebd4a735 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -7,7 +7,6 @@ import ( "go/token" "regexp" "strings" - "unicode" "github.com/golangci/golangci-lint/pkg/result" ) @@ -36,18 +35,6 @@ func (i ExtraLeadingSpace) Details() string { func (i ExtraLeadingSpace) String() string { return toString(i) } -type NotMachine struct { - baseIssue -} - -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 } @@ -183,20 +170,6 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { 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: "", - }, - } - issue := NotMachine{baseIssue: base} - issue.baseIssue.replacement = removeWhitespace - issues = append(issues, issue) - } - fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) if len(fullMatches) == 0 { issues = append(issues, ParseError{baseIssue: base}) From 8ce53a903c906ad1ab70e751be6922617604f2ac Mon Sep 17 00:00:00 2001 From: Aaron Pradhan Date: Tue, 19 Mar 2024 17:20:47 -0400 Subject: [PATCH 4/7] Disallow leading whitespace in fullDirectivePattern regular expression --- pkg/golinters/nolintlint/nolintlint.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index 3daaebd4a735..410db5ab94e9 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -113,7 +113,7 @@ const ( var commentPattern = regexp.MustCompile(`^//\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\b`) // matches a complete nolint directive -var fullDirectivePattern = regexp.MustCompile(`^//\s*nolint(?::([\w-]+(?:,[\w-]+)*))?(?: (//.*))?\n?$`) +var fullDirectivePattern = regexp.MustCompile(`^//nolint(?::([\w-]+(?:,[\w-]+)*))?(?: (//.*))?\n?$`) type Linter struct { needs Needs // indicates which linter checks to perform From df61cf50e45b29644ab09ee38d5dcdfb22dad782 Mon Sep 17 00:00:00 2001 From: Aaron Pradhan Date: Tue, 19 Mar 2024 17:23:26 -0400 Subject: [PATCH 5/7] Update test cases with leading space to use the ParseError issue --- pkg/golinters/nolintlint/nolintlint_test.go | 22 ++------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index 121b193182c9..f06dee612371 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -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[:] [// ]` at testing.go:5:9"}, + {issue: "directive `// nolint` should match `//nolint[:] [// ]` at testing.go:6:9"}, }, }, { From 2c5999ff34ff3c2e6eb125fa28cdffce619cea22 Mon Sep 17 00:00:00 2001 From: Aaron Pradhan Date: Tue, 19 Mar 2024 17:24:05 -0400 Subject: [PATCH 6/7] Fix unit test description for comma-separated linter list --- pkg/golinters/nolintlint/nolintlint_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index f06dee612371..895127204a80 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -102,7 +102,7 @@ func foo() { }, }, { - desc: "spaces are allowed in comma-separated list of linters", + desc: "spaces are not allowed in comma-separated list of linters", contents: ` package bar From a3643541e47dc63f0a6f16a050595cbc88e8306a Mon Sep 17 00:00:00 2001 From: Aaron Pradhan Date: Tue, 19 Mar 2024 17:30:48 -0400 Subject: [PATCH 7/7] Update NoExplanation details to provide a generic directive template rather than a specific suggestion; update unit tests accordingly --- pkg/golinters/nolintlint/nolintlint.go | 4 ++-- pkg/golinters/nolintlint/nolintlint_test.go | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index 410db5ab94e9..b746e6f3b2bc 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -64,8 +64,8 @@ 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) + 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) } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index 895127204a80..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"}, }, }, {