Skip to content

Commit

Permalink
Update nolintlint to fix nolint formatting and remove unused nolint s…
Browse files Browse the repository at this point in the history 
…tatements (#1573)

Also allow multiple ranges to satisfy a nolint statement as having been used.
  • Loading branch information
@ashanbrown
ashanbrown committed Dec 27, 2020
1 parent df9278e commit aeb9830
Show file tree
Hide file tree
Showing 8 changed files with 240 additions and 38 deletions.
1 change: 1 addition & 0 deletions pkg/golinters/nolintlint.go
View file
Expand Up @@ -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))
}
Expand Down
72 changes: 58 additions & 14 deletions pkg/golinters/nolintlint/nolintlint.go
View file
Expand Up @@ -8,16 +8,21 @@ 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) Position() token.Position { return b.position }

func (b BaseIssue) Replacement() *result.Replacement {
return b.replacement
}

type ExtraLeadingSpace struct {
Expand Down Expand Up @@ -85,7 +90,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
}
Expand All @@ -100,6 +105,7 @@ type Issue interface {
Details() string
Position() token.Position
String() string
Replacement() *result.Replacement
}

type Needs uint
Expand All @@ -115,7 +121,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
Expand Down Expand Up @@ -164,19 +170,34 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1])
}

pos := fset.Position(comment.Pos())
end := fset.Position(comment.End())

base := BaseIssue{
fullDirective: comment.Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: fset.Position(comment.Pos()),
position: 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 len(leadingSpace) > 0 {
machineReadableReplacement := &result.Replacement{
Inline: &result.InlineFix{
StartCol: pos.Column - 1,
Length: len(leadingSpace) + 2,
NewString: "//",
},
}

if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
issues = append(issues, NotMachine{BaseIssue: base})
if (l.needs & NeedsMachineOnly) != 0 {
issue := NotMachine{BaseIssue: base}
issue.BaseIssue.replacement = machineReadableReplacement
issues = append(issues, issue)
} else if len(leadingSpace) > 1 {
issue := ExtraLeadingSpace{BaseIssue: base}
issue.BaseIssue.replacement = machineReadableReplacement
issues = append(issues, issue)
}
}

fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
Expand All @@ -188,7 +209,7 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
lintersText, explanation := fullMatches[1], fullMatches[2]
var linters []string
if len(lintersText) > 0 {
lls := strings.Split(lintersText[1:], ",")
lls := strings.Split(lintersText, ",")
linters = make([]string, 0, len(lls))
for _, ll := range lls {
ll = strings.TrimSpace(ll)
Expand All @@ -206,11 +227,34 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {

// 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 {
issues = append(issues, UnusedCandidate{BaseIssue: base})
issue := UnusedCandidate{BaseIssue: base}
issue.replacement = removeNolintCompletely
issues = append(issues, issue)
} else {
for _, linter := range linters {
issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
for i, linter := range linters {
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
replacement := removeNolintCompletely
if len(linters) > 1 {
otherLinters := append(append([]string(nil), linters[0:i]...), linters[i+1:]...)
replacement = &result.Replacement{
Inline: &result.InlineFix{
StartCol: (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:"),
Length: len(lintersText) - 1,
NewString: strings.Join(otherLinters, ","),
},
}
}
issue.replacement = replacement
issues = append(issues, issue)
}
}
}
Expand Down
154 changes: 134 additions & 20 deletions pkg/golinters/nolintlint/nolintlint_test.go
View file
Expand Up @@ -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",
Expand All @@ -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{
{"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},
},
},
{
Expand All @@ -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{
{"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil},
},
},
{
Expand All @@ -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{
{"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},
},
},
{
Expand All @@ -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{
{
"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
&result.Replacement{
Inline: &result.InlineFix{
StartCol: 8,
Length: 3,
NewString: "//",
},
},
},
},
},
{
Expand All @@ -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{
{
"directive `// nolint` should not have more than one leading space at testing.go:5:9",
&result.Replacement{
Inline: &result.InlineFix{
StartCol: 8,
Length: 4,
NewString: "//",
},
},
},
},
},
{
Expand All @@ -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{
{"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", nil}, //nolint:lll // this is a string
},
},
{
Expand All @@ -133,6 +157,92 @@ func foo() {
// something else
}`,
},
{
desc: "needs unused without specific linter generates replacement",
needs: NeedsUnused,
contents: `
package bar
func foo() {
bad() //nolint
}`,
expected: []issueWithReplacement{
{
"directive `//nolint` is unused at testing.go:5:9",
&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{
{
"directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
&result.Replacement{
Inline: &result.InlineFix{
StartCol: 8,
Length: 19,
NewString: "",
},
},
},
},
},
{
desc: "needs unused with multiple specific linter generates replacement for each linter",
needs: NeedsUnused,
contents: `
package bar
func foo() {
bad() //nolint:linter1,linter2,linter3
}`,
expected: []issueWithReplacement{
{
"directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter1\" at testing.go:5:9",
&result.Replacement{
Inline: &result.InlineFix{
StartCol: 17,
Length: 22,
NewString: "linter2,linter3",
},
},
},
{
"directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter2\" at testing.go:5:9",
&result.Replacement{
Inline: &result.InlineFix{
StartCol: 17,
Length: 22,
NewString: "linter1,linter3",
},
},
},
{
"directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter3\" at testing.go:5:9",
&result.Replacement{
Inline: &result.InlineFix{
StartCol: 17,
Length: 22,
NewString: "linter1,linter2",
},
},
},
},
},
}

for _, test := range testCases {
Expand All @@ -149,12 +259,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 changes: 1 addition & 1 deletion pkg/result/issue.go
View file
Expand Up @@ -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
}

Expand Down

0 comments on commit aeb9830

Please sign in to comment.