Skip to content

Commit

Permalink
Add fix option for nolintlint
Browse files Browse the repository at this point in the history 
Repair whitespace issues and remove nolint statements that aren't being triggered
  • Loading branch information
@ashanbrown
ashanbrown committed Feb 20, 2021
1 parent eace6a1 commit c69dde5
Show file tree
Hide file tree
Showing 10 changed files with 271 additions and 64 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
82 changes: 64 additions & 18 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 (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
issues = append(issues, NotMachine{BaseIssue: base})
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)
Expand All @@ -187,14 +208,22 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {

lintersText, explanation := fullMatches[1], fullMatches[2]
var linters []string
var linterRange []result.Range
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)
if ll != "" {
linters = append(linters, ll)
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
}
}

Expand All @@ -206,11 +235,28 @@ 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})
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)
}
}
}
Expand Down
132 changes: 112 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: 10,
Length: 1,
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: 10,
Length: 2,
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,70 @@ 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 linters does not generate replacements",
needs: NeedsUnused,
contents: `
package bar
func foo() {
bad() //nolint:linter1,linter2
}`,
expected: []issueWithReplacement{
{
"directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9",
nil,
},
{
"directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9",
nil,
},
},
},
}

for _, test := range testCases {
Expand All @@ -149,12 +237,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 c69dde5

Please sign in to comment.