diff --git a/pkg/golinters/nolintlint/nolintlint.go b/pkg/golinters/nolintlint/nolintlint.go index e9b11f1c76a8..26de1dcb98aa 100644 --- a/pkg/golinters/nolintlint/nolintlint.go +++ b/pkg/golinters/nolintlint/nolintlint.go @@ -19,7 +19,9 @@ type BaseIssue struct { replacement *result.Replacement } -func (b BaseIssue) Position() token.Position { return b.position } +func (b BaseIssue) Position() token.Position { + return b.position +} func (b BaseIssue) Replacement() *result.Replacement { return b.replacement @@ -149,135 +151,138 @@ 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 { - for _, comment := range c.List { - if !commentPattern.MatchString(comment.Text) { - continue - } + file, ok := node.(*ast.File) + if !ok { + continue + } - // check for a space between the "//" and the directive - leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text) + for _, c := range file.Comments { + for _, comment := range c.List { + if !commentPattern.MatchString(comment.Text) { + continue + } - var leadingSpace string - if len(leadingSpaceMatches) > 0 { - leadingSpace = leadingSpaceMatches[1] - } + // check for a space between the "//" and the directive + leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text) - directiveWithOptionalLeadingSpace := comment.Text - if len(leadingSpace) > 0 { - split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//") - directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1]) - } + var leadingSpace string + if len(leadingSpaceMatches) > 0 { + leadingSpace = leadingSpaceMatches[1] + } - pos := fset.Position(comment.Pos()) - end := fset.Position(comment.End()) + directiveWithOptionalLeadingSpace := comment.Text + if len(leadingSpace) > 0 { + split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//") + directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1]) + } - base := BaseIssue{ - fullDirective: comment.Text, - directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace, - position: pos, - } + pos := fset.Position(comment.Pos()) + end := fset.Position(comment.End()) - // check for, report and eliminate leading spaces so we can check for other issues - if len(leadingSpace) > 0 { - 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, + directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace, + position: pos, + } - fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) - if len(fullMatches) == 0 { - issues = append(issues, ParseError{BaseIssue: base}) - continue + // check for, report and eliminate leading spaces so we can check for other issues + if len(leadingSpace) > 0 { + 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) + } + } - lintersText, explanation := fullMatches[1], fullMatches[2] - var linters []string - var linterRange []result.Range - if len(lintersText) > 0 { - lls := strings.Split(lintersText, ",") - linters = make([]string, 0, len(lls)) - rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:") - for i, ll := range lls { - rangeEnd := rangeStart + len(ll) - if i < len(lls)-1 { - rangeEnd++ // include trailing comma - } - trimmedLinterName := strings.TrimSpace(ll) - if trimmedLinterName != "" { - linters = append(linters, trimmedLinterName) - linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd}) - } - rangeStart = rangeEnd + fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text) + if len(fullMatches) == 0 { + issues = append(issues, ParseError{BaseIssue: base}) + continue + } + + lintersText, explanation := fullMatches[1], fullMatches[2] + var linters []string + var linterRange []result.Range + if len(lintersText) > 0 { + lls := strings.Split(lintersText, ",") + linters = make([]string, 0, len(lls)) + rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:") + for i, ll := range lls { + rangeEnd := rangeStart + len(ll) + if i < len(lls)-1 { + rangeEnd++ // include trailing comma } + trimmedLinterName := strings.TrimSpace(ll) + if trimmedLinterName != "" { + linters = append(linters, trimmedLinterName) + linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd}) + } + rangeStart = rangeEnd } + } - if (l.needs & NeedsSpecific) != 0 { - if len(linters) == 0 { - issues = append(issues, NotSpecific{BaseIssue: base}) - } + if (l.needs & NeedsSpecific) != 0 { + if len(linters) == 0 { + issues = append(issues, NotSpecific{BaseIssue: base}) } + } - // when detecting unused directives, we send all the directives through and filter them out in the nolint processor - if (l.needs & NeedsUnused) != 0 { - removeNolintCompletely := &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: pos.Column - 1, - Length: end.Column - pos.Column, - NewString: "", - }, - } + // when detecting unused directives, we send all the directives through and filter them out in the nolint processor + if (l.needs & NeedsUnused) != 0 { + removeNolintCompletely := &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: pos.Column - 1, + Length: end.Column - pos.Column, + NewString: "", + }, + } - if len(linters) == 0 { - issue := UnusedCandidate{BaseIssue: base} - issue.replacement = removeNolintCompletely - issues = append(issues, issue) - } else { - for _, linter := range linters { - 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 - if len(linters) == 1 { - issue.replacement = removeNolintCompletely - } - issues = append(issues, issue) + if len(linters) == 0 { + issue := UnusedCandidate{BaseIssue: base} + issue.replacement = removeNolintCompletely + issues = append(issues, issue) + } else { + for _, linter := range linters { + 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 + if len(linters) == 1 { + issue.replacement = removeNolintCompletely } + issues = append(issues, issue) } } + } - 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&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, - }) - } + if needsExplanation { + fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "") + issues = append(issues, NoExplanation{ + BaseIssue: base, + fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation, + }) } } } diff --git a/pkg/golinters/nolintlint/nolintlint_test.go b/pkg/golinters/nolintlint/nolintlint_test.go index 0cc699c370af..dd6da110a41b 100644 --- a/pkg/golinters/nolintlint/nolintlint_test.go +++ b/pkg/golinters/nolintlint/nolintlint_test.go @@ -40,10 +40,10 @@ func foo() { other() //nolintother }`, expected: []issueWithReplacement{ - {"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", nil}, - {"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", nil}, - {"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", nil}, - {"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", nil}, + {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"}, }, }, { @@ -57,7 +57,7 @@ package bar //nolint:dupl func foo() {}`, expected: []issueWithReplacement{ - {"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil}, + {issue: "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1"}, }, }, { @@ -83,8 +83,8 @@ func foo() { bad() // nolint // because }`, expected: []issueWithReplacement{ - {"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9", nil}, - {"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9", nil}, + {issue: "directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9"}, + {issue: "directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9"}, }, }, { @@ -99,8 +99,8 @@ func foo() { }`, expected: []issueWithReplacement{ { - "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9", - &result.Replacement{ + 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, @@ -121,8 +121,8 @@ func foo() { }`, expected: []issueWithReplacement{ { - "directive `// nolint` should not have more than one leading space at testing.go:5:9", - &result.Replacement{ + issue: "directive `// nolint` should not have more than one leading space at testing.go:5:9", + replacement: &result.Replacement{ Inline: &result.InlineFix{ StartCol: 10, Length: 2, @@ -144,7 +144,7 @@ func foo() { good() // nolint: linter1, linter2 }`, expected: []issueWithReplacement{ - {"directive `// nolint:linter1 linter2` should match `// nolint[:] [// ]` at testing.go:6:9", nil}, //nolint:lll // this is a string + {issue: "directive `// nolint:linter1 linter2` should match `// nolint[:] [// ]` at testing.go:6:9"}, //nolint:lll // this is a string }, }, { @@ -168,8 +168,8 @@ func foo() { }`, expected: []issueWithReplacement{ { - "directive `//nolint` is unused at testing.go:5:9", - &result.Replacement{ + issue: "directive `//nolint` is unused at testing.go:5:9", + replacement: &result.Replacement{ Inline: &result.InlineFix{ StartCol: 8, Length: 8, @@ -190,8 +190,8 @@ func foo() { }`, expected: []issueWithReplacement{ { - "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9", - &result.Replacement{ + issue: "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9", + replacement: &result.Replacement{ Inline: &result.InlineFix{ StartCol: 8, Length: 19, @@ -212,12 +212,10 @@ func foo() { }`, expected: []issueWithReplacement{ { - "directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9", - nil, + issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9", }, { - "directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9", - nil, + issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9", }, }, },