Skip to content

Commit aeb9830

Browse files
ashanbrownashanbrown
authored
Update nolintlint to fix nolint formatting and remove unused nolint statements (#1573)
Also allow multiple ranges to satisfy a nolint statement as having been used.
1 parent df9278e commit aeb9830

File tree

8 files changed

+240
-38
lines changed

8 files changed

+240
-38
lines changed

‎pkg/golinters/nolintlint.go

+1
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ func NewNoLintLint() *goanalysis.Linter {
7272
Pos: i.Position(),
7373
ExpectNoLint: expectNoLint,
7474
ExpectedNoLintLinter: expectedNolintLinter,
75+
Replacement: i.Replacement(),
7576
}
7677
res = append(res, goanalysis.NewIssue(issue, pass))
7778
}

‎pkg/golinters/nolintlint/nolintlint.go

+58-14
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,21 @@ import (
88
"regexp"
99
"strings"
1010
"unicode"
11+
12+
"github.com/golangci/golangci-lint/pkg/result"
1113
)
1214

1315
type BaseIssue struct {
1416
fullDirective string
1517
directiveWithOptionalLeadingSpace string
1618
position token.Position
19+
replacement *result.Replacement
1720
}
1821

19-
func (b BaseIssue) Position() token.Position {
20-
return b.position
22+
func (b BaseIssue) Position() token.Position { return b.position }
23+
24+
func (b BaseIssue) Replacement() *result.Replacement {
25+
return b.replacement
2126
}
2227

2328
type ExtraLeadingSpace struct {
@@ -85,7 +90,7 @@ type UnusedCandidate struct {
8590
func (i UnusedCandidate) Details() string {
8691
details := fmt.Sprintf("directive `%s` is unused", i.fullDirective)
8792
if i.ExpectedLinter != "" {
88-
details += fmt.Sprintf(" for linter %s", i.ExpectedLinter)
93+
details += fmt.Sprintf(" for linter %q", i.ExpectedLinter)
8994
}
9095
return details
9196
}
@@ -100,6 +105,7 @@ type Issue interface {
100105
Details() string
101106
Position() token.Position
102107
String() string
108+
Replacement() *result.Replacement
103109
}
104110

105111
type Needs uint
@@ -115,7 +121,7 @@ const (
115121
var commentPattern = regexp.MustCompile(`^//\s*(nolint)(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\b`)
116122

117123
// matches a complete nolint directive
118-
var fullDirectivePattern = regexp.MustCompile(`^//\s*nolint(:\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*)?\s*(//.*)?\s*\n?$`)
124+
var fullDirectivePattern = regexp.MustCompile(`^//\s*nolint(?::(\s*[\w-]+\s*(?:,\s*[\w-]+\s*)*))?\s*(//.*)?\s*\n?$`)
119125

120126
type Linter struct {
121127
excludes []string // lists individual linters that don't require explanations
@@ -164,19 +170,34 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
164170
directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1])
165171
}
166172

173+
pos := fset.Position(comment.Pos())
174+
end := fset.Position(comment.End())
175+
167176
base := BaseIssue{
168177
fullDirective: comment.Text,
169178
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
170-
position: fset.Position(comment.Pos()),
179+
position: pos,
171180
}
172181

173182
// check for, report and eliminate leading spaces so we can check for other issues
174-
if len(leadingSpace) > 1 {
175-
issues = append(issues, ExtraLeadingSpace{BaseIssue: base})
176-
}
183+
if len(leadingSpace) > 0 {
184+
machineReadableReplacement := &result.Replacement{
185+
Inline: &result.InlineFix{
186+
StartCol: pos.Column - 1,
187+
Length: len(leadingSpace) + 2,
188+
NewString: "//",
189+
},
190+
}
177191

178-
if (l.needs&NeedsMachineOnly) != 0 && len(leadingSpace) > 0 {
179-
issues = append(issues, NotMachine{BaseIssue: base})
192+
if (l.needs & NeedsMachineOnly) != 0 {
193+
issue := NotMachine{BaseIssue: base}
194+
issue.BaseIssue.replacement = machineReadableReplacement
195+
issues = append(issues, issue)
196+
} else if len(leadingSpace) > 1 {
197+
issue := ExtraLeadingSpace{BaseIssue: base}
198+
issue.BaseIssue.replacement = machineReadableReplacement
199+
issues = append(issues, issue)
200+
}
180201
}
181202

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

207228
// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
208229
if (l.needs & NeedsUnused) != 0 {
230+
removeNolintCompletely := &result.Replacement{
231+
Inline: &result.InlineFix{
232+
StartCol: pos.Column - 1,
233+
Length: end.Column - pos.Column,
234+
NewString: "",
235+
},
236+
}
237+
209238
if len(linters) == 0 {
210-
issues = append(issues, UnusedCandidate{BaseIssue: base})
239+
issue := UnusedCandidate{BaseIssue: base}
240+
issue.replacement = removeNolintCompletely
241+
issues = append(issues, issue)
211242
} else {
212-
for _, linter := range linters {
213-
issues = append(issues, UnusedCandidate{BaseIssue: base, ExpectedLinter: linter})
243+
for i, linter := range linters {
244+
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
245+
replacement := removeNolintCompletely
246+
if len(linters) > 1 {
247+
otherLinters := append(append([]string(nil), linters[0:i]...), linters[i+1:]...)
248+
replacement = &result.Replacement{
249+
Inline: &result.InlineFix{
250+
StartCol: (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:"),
251+
Length: len(lintersText) - 1,
252+
NewString: strings.Join(otherLinters, ","),
253+
},
254+
}
255+
}
256+
issue.replacement = replacement
257+
issues = append(issues, issue)
214258
}
215259
}
216260
}

‎pkg/golinters/nolintlint/nolintlint_test.go

+134-20
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,22 @@ import (
77

88
"github.com/stretchr/testify/assert"
99
"github.com/stretchr/testify/require"
10+
11+
"github.com/golangci/golangci-lint/pkg/result"
1012
)
1113

1214
//nolint:funlen
1315
func TestNoLintLint(t *testing.T) {
16+
type issueWithReplacement struct {
17+
issue string
18+
replacement *result.Replacement
19+
}
1420
testCases := []struct {
1521
desc string
1622
needs Needs
1723
excludes []string
1824
contents string
19-
expected []string
25+
expected []issueWithReplacement
2026
}{
2127
{
2228
desc: "when no explanation is provided",
@@ -33,11 +39,11 @@ func foo() {
3339
good() //nolint // this is ok
3440
other() //nolintother
3541
}`,
36-
expected: []string{
37-
"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1",
38-
"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9",
39-
"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9",
40-
"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9",
42+
expected: []issueWithReplacement{
43+
{"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", nil},
44+
{"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", nil},
45+
{"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", nil},
46+
{"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", nil},
4147
},
4248
},
4349
{
@@ -50,8 +56,8 @@ package bar
5056
//nolint // this is ok
5157
//nolint:dupl
5258
func foo() {}`,
53-
expected: []string{
54-
"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1",
59+
expected: []issueWithReplacement{
60+
{"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil},
5561
},
5662
},
5763
{
@@ -76,9 +82,9 @@ func foo() {
7682
bad() //nolint
7783
bad() // nolint // because
7884
}`,
79-
expected: []string{
80-
"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9",
81-
"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9",
85+
expected: []issueWithReplacement{
86+
{"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9", nil},
87+
{"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9", nil},
8288
},
8389
},
8490
{
@@ -91,8 +97,17 @@ func foo() {
9197
bad() // nolint
9298
good() //nolint
9399
}`,
94-
expected: []string{
95-
"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
100+
expected: []issueWithReplacement{
101+
{
102+
"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
103+
&result.Replacement{
104+
Inline: &result.InlineFix{
105+
StartCol: 8,
106+
Length: 3,
107+
NewString: "//",
108+
},
109+
},
110+
},
96111
},
97112
},
98113
{
@@ -104,8 +119,17 @@ func foo() {
104119
bad() // nolint
105120
good() // nolint
106121
}`,
107-
expected: []string{
108-
"directive `// nolint` should not have more than one leading space at testing.go:5:9",
122+
expected: []issueWithReplacement{
123+
{
124+
"directive `// nolint` should not have more than one leading space at testing.go:5:9",
125+
&result.Replacement{
126+
Inline: &result.InlineFix{
127+
StartCol: 8,
128+
Length: 4,
129+
NewString: "//",
130+
},
131+
},
132+
},
109133
},
110134
},
111135
{
@@ -119,8 +143,8 @@ func foo() {
119143
good() // nolint: linter1,linter2
120144
good() // nolint: linter1, linter2
121145
}`,
122-
expected: []string{
123-
"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", //nolint:lll // this is a string
146+
expected: []issueWithReplacement{
147+
{"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", nil}, //nolint:lll // this is a string
124148
},
125149
},
126150
{
@@ -133,6 +157,92 @@ func foo() {
133157
// something else
134158
}`,
135159
},
160+
{
161+
desc: "needs unused without specific linter generates replacement",
162+
needs: NeedsUnused,
163+
contents: `
164+
package bar
165+
166+
func foo() {
167+
bad() //nolint
168+
}`,
169+
expected: []issueWithReplacement{
170+
{
171+
"directive `//nolint` is unused at testing.go:5:9",
172+
&result.Replacement{
173+
Inline: &result.InlineFix{
174+
StartCol: 8,
175+
Length: 8,
176+
NewString: "",
177+
},
178+
},
179+
},
180+
},
181+
},
182+
{
183+
desc: "needs unused with one specific linter generates replacement",
184+
needs: NeedsUnused,
185+
contents: `
186+
package bar
187+
188+
func foo() {
189+
bad() //nolint:somelinter
190+
}`,
191+
expected: []issueWithReplacement{
192+
{
193+
"directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
194+
&result.Replacement{
195+
Inline: &result.InlineFix{
196+
StartCol: 8,
197+
Length: 19,
198+
NewString: "",
199+
},
200+
},
201+
},
202+
},
203+
},
204+
{
205+
desc: "needs unused with multiple specific linter generates replacement for each linter",
206+
needs: NeedsUnused,
207+
contents: `
208+
package bar
209+
210+
func foo() {
211+
bad() //nolint:linter1,linter2,linter3
212+
}`,
213+
expected: []issueWithReplacement{
214+
{
215+
"directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter1\" at testing.go:5:9",
216+
&result.Replacement{
217+
Inline: &result.InlineFix{
218+
StartCol: 17,
219+
Length: 22,
220+
NewString: "linter2,linter3",
221+
},
222+
},
223+
},
224+
{
225+
"directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter2\" at testing.go:5:9",
226+
&result.Replacement{
227+
Inline: &result.InlineFix{
228+
StartCol: 17,
229+
Length: 22,
230+
NewString: "linter1,linter3",
231+
},
232+
},
233+
},
234+
{
235+
"directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter3\" at testing.go:5:9",
236+
&result.Replacement{
237+
Inline: &result.InlineFix{
238+
StartCol: 17,
239+
Length: 22,
240+
NewString: "linter1,linter2",
241+
},
242+
},
243+
},
244+
},
245+
},
136246
}
137247

138248
for _, test := range testCases {
@@ -149,12 +259,16 @@ func foo() {
149259
actualIssues, err := linter.Run(fset, expr)
150260
require.NoError(t, err)
151261

152-
actualIssueStrs := make([]string, 0, len(actualIssues))
262+
actualIssuesWithReplacements := make([]issueWithReplacement, 0, len(actualIssues))
153263
for _, i := range actualIssues {
154-
actualIssueStrs = append(actualIssueStrs, i.String())
264+
actualIssuesWithReplacements = append(actualIssuesWithReplacements, issueWithReplacement{
265+
issue: i.String(),
266+
replacement: i.Replacement(),
267+
})
155268
}
156269

157-
assert.ElementsMatch(t, test.expected, actualIssueStrs, "expected %s \nbut got %s", test.expected, actualIssues)
270+
assert.ElementsMatch(t, test.expected, actualIssuesWithReplacements,
271+
"expected %s \nbut got %s", test.expected, actualIssuesWithReplacements)
158272
})
159273
}
160274
}

‎pkg/result/issue.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ type Range struct {
1414

1515
type Replacement struct {
1616
NeedOnlyDelete bool // need to delete all lines of the issue without replacement with new lines
17-
NewLines []string // is NeedDelete is false it's the replacement lines
17+
NewLines []string // if NeedDelete is false it's the replacement lines
1818
Inline *InlineFix
1919
}
2020

0 commit comments

Comments
 (0)