eeafj/golangci-lint
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 1 Wiki Activity

nolintlint: allow to fix //nolint lines (#1583)

Browse Source
This commit is contained in:
Andrew Shannon Brown 2021-03-12 20:11:05 -08:00 committed by GitHub
parent d28c66672e
commit e1a734e559
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 345 additions and 132 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pkg/golinters/nolintlint.go
View File

@ -72,6 +72,7 @@ func NewNoLintLint() *goanalysis.Linter {
Pos: i.Position(),
ExpectNoLint: expectNoLint,
ExpectedNoLintLinter: expectedNolintLinter,
Replacement: i.Replacement(),
}
res = append(res, goanalysis.NewIssue(issue, pass))
}

221
pkg/golinters/nolintlint/nolintlint.go
View File

@ -8,18 +8,25 @@ import (
"regexp"
"strings"
"unicode"
"github.com/golangci/golangci-lint/pkg/result"
)
type BaseIssue struct {
fullDirective string
directiveWithOptionalLeadingSpace string
position token.Position
replacement *result.Replacement
}
func (b BaseIssue) Position() token.Position {
return b.position
}
func (b BaseIssue) Replacement() *result.Replacement {
return b.replacement
}
type ExtraLeadingSpace struct {
BaseIssue
}
@ -85,7 +92,7 @@ type UnusedCandidate struct {
func (i UnusedCandidate) Details() string {
details := fmt.Sprintf("directive `%s` is unused", i.fullDirective)
if i.ExpectedLinter != "" {
details += fmt.Sprintf(" for linter %s", i.ExpectedLinter)
details += fmt.Sprintf(" for linter %q", i.ExpectedLinter)
}
return details
}
@ -100,6 +107,7 @@ type Issue interface {
Details() string
Position() token.Position
String() string
Replacement() *result.Replacement
}
type Needs uint
@ -115,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(?::(\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*))?\s*(//.*)?\s*\n?$`)
type Linter struct {
excludes []string // lists individual linters that don't require explanations
@ -143,95 +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
}
for _, c := range file.Comments {
for _, comment := range c.List {
if !commentPattern.MatchString(comment.Text) {
continue
}
// check for a space between the "//" and the directive
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)
var leadingSpace string
if len(leadingSpaceMatches) > 0 {
leadingSpace = leadingSpaceMatches[1]
}
directiveWithOptionalLeadingSpace := comment.Text
if len(leadingSpace) > 0 {
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 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)
}
}
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})
}
}
// 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: "",
},
}
// check for a space between the "//" and the directive
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)
var leadingSpace string
if len(leadingSpaceMatches) > 0 {
leadingSpace = leadingSpaceMatches[1]
}
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: fset.Position(comment.Pos()),
}
// check for, report and eliminate leading spaces so we can check for other issues
if len(leadingSpace) > 1 {
issues = append(issues, ExtraLeadingSpace{BaseIssue: base})
}
if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
issues = append(issues, NotMachine{BaseIssue: base})
}
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
if len(lintersText) > 0 {
lls := strings.Split(lintersText[1:], ",")
linters = make([]string, 0, len(lls))
for _, ll := range lls {
ll = strings.TrimSpace(ll)
if ll != "" {
linters = append(linters, ll)
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 & 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 {
if len(linters) == 0 {
issues = append(issues, UnusedCandidate{BaseIssue: base})
} else {
for _, linter := range linters {
issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
}
}
}
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,
})
}
}
}

130
pkg/golinters/nolintlint/nolintlint_test.go
View File

@ -7,16 +7,22 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/golangci/golangci-lint/pkg/result"
)
//nolint:funlen
func TestNoLintLint(t *testing.T) {
type issueWithReplacement struct {
issue string
replacement *result.Replacement
}
testCases := []struct {
desc string
needs Needs
excludes []string
contents string
expected []string
expected []issueWithReplacement
}{
{
desc: "when no explanation is provided",
@ -33,11 +39,11 @@ func foo() {
good() //nolint // this is ok
other() //nolintother
}`,
expected: []string{
"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1",
"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9",
"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9",
"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9",
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"},
},
},
{
@ -50,8 +56,8 @@ package bar
//nolint // this is ok
//nolint:dupl
func foo() {}`,
expected: []string{
"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1",
expected: []issueWithReplacement{
{issue: "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1"},
},
},
{
@ -76,9 +82,9 @@ func foo() {
bad() //nolint
bad() // nolint // because
}`,
expected: []string{
"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9",
"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9",
expected: []issueWithReplacement{
{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"},
},
},
{
@ -91,8 +97,17 @@ func foo() {
bad() // nolint
good() //nolint
}`,
expected: []string{
"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
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: "",
},
},
},
},
},
{
@ -104,8 +119,17 @@ func foo() {
bad() // nolint
good() // nolint
}`,
expected: []string{
"directive `// nolint` should not have more than one leading space at testing.go:5:9",
expected: []issueWithReplacement{
{
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,
NewString: " ",
},
},
},
},
},
{
@ -119,8 +143,8 @@ func foo() {
good() // nolint: linter1,linter2
good() // nolint: linter1, linter2
}`,
expected: []string{
"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", //nolint:lll // this is a string
expected: []issueWithReplacement{
{issue: "directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"}, //nolint:lll // this is a string
},
},
{
@ -133,6 +157,68 @@ func foo() {
// something else
}`,
},
{
desc: "needs unused without specific linter generates replacement",
needs: NeedsUnused,
contents: `
package bar
func foo() {
bad() //nolint
}`,
expected: []issueWithReplacement{
{
issue: "directive `//nolint` is unused at testing.go:5:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 8,
Length: 8,
NewString: "",
},
},
},
},
},
{
desc: "needs unused with one specific linter generates replacement",
needs: NeedsUnused,
contents: `
package bar
func foo() {
bad() //nolint:somelinter
}`,
expected: []issueWithReplacement{
{
issue: "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 8,
Length: 19,
NewString: "",
},
},
},
},
},
{
desc: "needs unused with multiple specific linters does not generate replacements",
needs: NeedsUnused,
contents: `
package bar
func foo() {
bad() //nolint:linter1,linter2
}`,
expected: []issueWithReplacement{
{
issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9",
},
{
issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9",
},
},
},
}
for _, test := range testCases {
@ -149,12 +235,16 @@ func foo() {
actualIssues, err := linter.Run(fset, expr)
require.NoError(t, err)
actualIssueStrs := make([]string, 0, len(actualIssues))
actualIssuesWithReplacements := make([]issueWithReplacement, 0, len(actualIssues))
for _, i := range actualIssues {
actualIssueStrs = append(actualIssueStrs, i.String())
actualIssuesWithReplacements = append(actualIssuesWithReplacements, issueWithReplacement{
issue: i.String(),
replacement: i.Replacement(),
})
}
assert.ElementsMatch(t, test.expected, actualIssueStrs, "expected %s \nbut got %s", test.expected, actualIssues)
assert.ElementsMatch(t, test.expected, actualIssuesWithReplacements,
"expected %s \nbut got %s", test.expected, actualIssuesWithReplacements)
})
}
}

2
pkg/result/issue.go
View File

@ -14,7 +14,7 @@ type Range struct {
type Replacement struct {
NeedOnlyDelete bool // need to delete all lines of the issue without replacement with new lines
NewLines []string // is NeedDelete is false it's the replacement lines
NewLines []string // if NeedDelete is false it's the replacement lines
Inline *InlineFix
}

57
pkg/result/processors/nolint.go
View File

@ -21,7 +21,8 @@ type ignoredRange struct {
linters []string
matchedIssueFromLinter map[string]bool
result.Range
col int
col int
originalRange *ignoredRange // pre-expanded range (used to match nolintlint issues)
}
func (i *ignoredRange) doesMatch(issue *result.Issue) bool {
@ -29,25 +30,29 @@ func (i *ignoredRange) doesMatch(issue *result.Issue) bool {
return false
}
// only allow selective nolinting of nolintlint
nolintFoundForLinter := len(i.linters) == 0 && issue.FromLinter != golinters.NolintlintName
for _, linterName := range i.linters {
if linterName == issue.FromLinter {
nolintFoundForLinter = true
break
}
}
if nolintFoundForLinter {
return true
}
// handle possible unused nolint directives
// nolintlint generates potential issues for every nolint directive and they are filtered out here
if issue.ExpectNoLint {
if issue.FromLinter == golinters.NolintlintName && issue.ExpectNoLint {
if issue.ExpectedNoLintLinter != "" {
return i.matchedIssueFromLinter[issue.ExpectedNoLintLinter]
}
return len(i.matchedIssueFromLinter) > 0
}
if len(i.linters) == 0 {
return true
}
for _, linterName := range i.linters {
if linterName == issue.FromLinter {
return true
}
}
return false
}
@ -141,19 +146,14 @@ func (p *Nolint) buildIgnoredRangesForFile(f *ast.File, fset *token.FileSet, fil
func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
nolintDebugf("got issue: %v", *i)
if i.FromLinter == golinters.NolintlintName {
// always pass nolintlint issues except ones trying find unused nolint directives
if !i.ExpectNoLint {
return true, nil
}
if i.ExpectedNoLintLinter != "" {
// don't expect disabled linters to cover their nolint statements
nolintDebugf("enabled linters: %v", p.enabledLinters)
if p.enabledLinters[i.ExpectedNoLintLinter] == nil {
return false, nil
}
nolintDebugf("checking that lint issue was used for %s: %v", i.ExpectedNoLintLinter, i)
if i.FromLinter == golinters.NolintlintName && i.ExpectNoLint && i.ExpectedNoLintLinter != "" {
// don't expect disabled linters to cover their nolint statements
nolintDebugf("enabled linters: %v", p.enabledLinters)
if p.enabledLinters[i.ExpectedNoLintLinter] == nil {
return false, nil
}
nolintDebugf("checking that lint issue was used for %s: %v", i.ExpectedNoLintLinter, i)
}
fd, err := p.getOrCreateFileData(i)
@ -163,7 +163,11 @@ func (p *Nolint) shouldPassIssue(i *result.Issue) (bool, error) {
for _, ir := range fd.ignoredRanges {
if ir.doesMatch(i) {
nolintDebugf("found ignored range for issue %v: %v", i, ir)
ir.matchedIssueFromLinter[i.FromLinter] = true
if ir.originalRange != nil {
ir.originalRange.matchedIssueFromLinter[i.FromLinter] = true
}
return false, nil
}
}
@ -199,9 +203,14 @@ func (e *rangeExpander) Visit(node ast.Node) ast.Visitor {
}
expandedRange := *foundRange
// store the original unexpanded range for matching nolintlint issues
if expandedRange.originalRange == nil {
expandedRange.originalRange = foundRange
}
if expandedRange.To < nodeEndLine {
expandedRange.To = nodeEndLine
}
nolintDebugf("found range is %v for node %#v [%d;%d], expanded range is %v",
*foundRange, node, nodeStartLine, nodeEndLine, expandedRange)
e.expandedRanges = append(e.expandedRanges, expandedRange)

18
pkg/result/processors/nolint_test.go
View File

@ -291,6 +291,17 @@ func TestNolintUnused(t *testing.T) {
ExpectedNoLintLinter: "varcheck",
}
// the issue below is another nolintlint issue that would be generated for the test file
nolintlintIssueVarcheckUnusedOK := result.Issue{
Pos: token.Position{
Filename: fileName,
Line: 5,
},
FromLinter: golinters.NolintlintName,
ExpectNoLint: true,
ExpectedNoLintLinter: "varcheck",
}
t.Run("when an issue does not occur, it is not removed from the nolintlint issues", func(t *testing.T) {
p := createProcessor(t, log, []string{"nolintlint", "varcheck"})
defer p.Finish()
@ -298,6 +309,13 @@ func TestNolintUnused(t *testing.T) {
processAssertSame(t, p, nolintlintIssueVarcheck)
})
t.Run("when an issue does not occur but nolintlint is nolinted, it is removed from the nolintlint issues", func(t *testing.T) {
p := createProcessor(t, log, []string{"nolintlint", "varcheck"})
defer p.Finish()
processAssertEmpty(t, p, nolintlintIssueVarcheckUnusedOK)
})
t.Run("when an issue occurs, it is removed from the nolintlint issues", func(t *testing.T) {
p := createProcessor(t, log, []string{"nolintlint", "varcheck"})
defer p.Finish()

2
pkg/result/processors/testdata/nolint_unused.go vendored
View File

@ -1,3 +1,5 @@
package testdata
var nolintVarcheck int // nolint:varcheck
var nolintVarcheckUnusedOK int // nolint:varcheck,nolintlint

17
test/testdata/fix/in/nolintlint.go vendored Normal file
View File

@ -0,0 +1,17 @@
//args: -Enolintlint -Elll
//expected_linter: nolintlint
//config: linters-settings.nolintlint.allow-leading-space=false
package p
import "fmt"
func nolintlint() {
fmt.Println() // nolint:bob // leading space should be dropped
fmt.Println() // nolint:bob // leading spaces should be dropped
// note that the next lines will retain trailing whitespace when fixed
fmt.Println() //nolint // nolint should be dropped
fmt.Println() //nolint:lll // nolint should be dropped
fmt.Println() //nolint:alice,lll // we don't drop individual linters from lists
}

17
test/testdata/fix/out/nolintlint.go vendored Normal file
View File

@ -0,0 +1,17 @@
//args: -Enolintlint -Elll
//expected_linter: nolintlint
//config: linters-settings.nolintlint.allow-leading-space=false
package p
import "fmt"
func nolintlint() {
fmt.Println() //nolint:bob // leading space should be dropped
fmt.Println() //nolint:bob // leading spaces should be dropped
// note that the next lines will retain trailing whitespace when fixed
fmt.Println()
fmt.Println()
fmt.Println() //nolint:alice,lll // we don't drop individual linters from lists
}

12
test/testdata/nolintlint.go vendored
View File

@ -1,7 +1,8 @@
//args: -Enolintlint
//args: -Enolintlint -Emisspell
//expected_linter: nolintlint
//config: linters-settings.nolintlint.require-explanation=true
//config: linters-settings.nolintlint.require-specific=true
//config: linters-settings.nolintlint.allowing-leading-space=false
//config: linters-settings.nolintlint.allow-leading-space=false
package testdata
import "fmt"
@ -10,4 +11,11 @@ func Foo() {
fmt.Println("not specific") //nolint // ERROR "directive `.*` should mention specific linter such as `//nolint:my-linter`"
fmt.Println("not machine readable") // nolint // ERROR "directive `.*` should be written as `//nolint`"
fmt.Println("extra spaces") // nolint:deadcode // because // ERROR "directive `.*` should not have more than one leading space"
// test expanded range
//nolint:misspell // deliberate misspelling to trigger nolintlint
func() {
mispell := true
fmt.Println(mispell)
}()
}