Update nolintlint to fix nolint formatting and remove unused nolint statements #1573

ashanbrown · 2020-12-23T05:00:44Z

This PR adds a fixing capability to nolintlint. It doesn't try to fix everything.

What it does (when --fix is enabled):

Removes non-specific //nolint directives that are not used.
Removes a linter from a linter-dispecific //nolint directive if that linter is not used for that line.
Removes all leading whitespace from //. nolint directives for any violations on the number of spaces.

What it does not do:

Offer explanations when explanations for nolint are required but not provided. I suppose it could add a placeholder that the user could replace but that doesn't seem really in the spirit of how the fix option is supposed to work (since the user might check in that placeholder).
Make non-specific linter directives specific. This could be done but there is a risk that it cannot divine the intent of the user in setting the nolint directive and I'd argue it's best for the user to make this determination on their own.

Note that fies may leave trailing spaces or empty lines behind. Presumably, this would be cleaned up by gofmt in most cases which could also fix these cases. I'm not sure whether it's really possible or necessary to try to remove such spaces. It would be nice to at least remove empty lines if there is a way to identify if a comment is the only thing on a line, but I'm not sure how to do that just given the AST. One possibility might be to give the fixer a hint to clean up trailing whitespace or remove blank lines. That could potentially be useful for other linters as well.

~~This PR also runs the fix on golangci-lint itself to enforce machine-readable nolint styling. (I can back this out if anyone things this isn't a good idea but I just wanted to see it in action).~~

…tatements

ashanbrown · 2020-12-23T05:54:09Z

pkg/result/processors/nolint.go

 			ir.matchedIssueFromLinter[i.FromLinter] = true
-			return false, nil
+			matchesAtLeastOneRange = true


I don't have test support for this. It seemed like it would be pretty complicated to construct but I'm open to suggestions.

This was needed because when I converted golangci lint to use machine-readable style directives (no leading space), the interfacer linter created a problem at https://github.com/golangci/golangci-lint/blob/master/pkg/golinters/scopelint.go#L166 (although this line itself did not change).

Running with env GL_DEBUG=nolint ./golangci-lint run -E interfacer --disable-all, you can see the nolint range getting expanded to include the entire function:

DEBU [nolint] file pkg/golinters/scopelint.go: inline nolint ranges are [{linters:[gocyclo gocritic] matchedIssueFromLinter:map[] Range:{From:73 To:76} col:1} {linters:[interfacer] matchedIssueFromLinter:map[] Range:{From:163 To:166} col:1}] DEBU [nolint] found range is {[gocyclo gocritic] map[] {73 76} 1} for node &ast.FuncDecl{Doc:(*ast.CommentGroup)(0xc002b978c0), Recv:(*ast.FieldList)(0xc002b87ef0), Name:(*ast.Ident)(0xc002b97960), Type:(*ast.FuncType)(0xc0007776e0), Body:(*ast.BlockStmt)(0xc001e18b10)} [77;161], expanded range is {[gocyclo gocritic] map[] {73 161} 1} DEBU [nolint] found range is {[gocyclo gocritic] map[] {73 76} 1} for node &ast.FuncType{Func:1966, Params:(*ast.FieldList)(0xc002b87f20), Results:(*ast.FieldList)(0xc002b87f50)} [77;77], expanded range is {[gocyclo gocritic] map[] {73 77} 1} DEBU [nolint] found range is {[interfacer] map[] {163 166} 1} for node &ast.FuncDecl{Doc:(*ast.CommentGroup)(0xc0007776c0), Recv:(*ast.FieldList)(0xc001e18ba0), Name:(*ast.Ident)(0xc000777760), Type:(*ast.FuncType)(0xc000777ae0), Body:(*ast.BlockStmt)(0xc001e18c60)} [167;170], expanded range is {[interfacer] map[] {163 170} 1} DEBU [nolint] found range is {[interfacer] map[] {163 166} 1} for node &ast.FuncType{Func:4213, Params:(*ast.FieldList)(0xc001e18c00), Results:(*ast.FieldList)(nil)} [167;167], expanded range is {[interfacer] map[] {163 167} 1} DEBU [nolint] file pkg/golinters/scopelint.go: built nolint ranges are [{linters:[gocyclo gocritic] matchedIssueFromLinter:map[] Range:{From:73 To:76} col:1} {linters:[interfacer] matchedIssueFromLinter:map[] Range:{From:163 To:166} col:1} {linters:[gocyclo gocritic] matchedIssueFromLinter:map[] Range:{From:73 To:161} col:1} {linters:[gocyclo gocritic] matchedIssueFromLinter:map[] Range:{From:73 To:77} col:1} {linters:[interfacer] matchedIssueFromLinter:map[] Range:{From:163 To:170} col:1} {linters:[interfacer] matchedIssueFromLinter:map[] Range:{From:163 To:167} col:1}]

I'm still not completely clear on what changed but I think it makes sense that the expanded ranges would be treated as a group for the purpose of determining unused nolint directives.

ashanbrown · 2020-12-23T05:54:31Z

test/testdata/nolintlint.go

@@ -1,7 +1,7 @@
 //args: -Enolintlint
 //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


noticed this typo although it didn't affect this test

ernado · 2020-12-24T18:33:20Z

IMO it is better to extract changing // nolint to //nolint to another PR.

I'm even not sure that latter is better, because I'm not familiar with reasoning behind using non-spaced annotation.

ernado

LGTM

ldez · 2020-12-27T14:30:06Z

I was testing and reviewing this PR, but it was merged before I finished.
I had added myself as a reviewer to notify that I was on the subject.
I didn't want to assign myself to it because I didn't want to discourage others from reviewing it.
Maybe my process is not good.

ashanbrown · 2020-12-27T14:37:51Z

@ldez Totally my fault. I failed to notice the additional reviewer listed. Please do let me know if you find anything and I'm more than happy to make additional changes. As mentioned, I'd still like better coverage for the situation where the nolintlint issue somehow doesn't overlap the range matched to a linter, as appears to happen with the //nolint:interfacer call in golinters/scopelint.go. I'll be looking more into that myself.

ldez · 2020-12-27T14:50:28Z

before the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret, gocyclo
func wantedErrors(file, short string) (errs []wantedError) {

after the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:nakedreto
func wantedErrors(file, short string) (errs []wantedError) {

KO

before the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret,gocyclo
func wantedErrors(file, short string) (errs []wantedError) {

after the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:nakedreto
func wantedErrors(file, short string) (errs []wantedError) {

KO

nakedreto is not a linter 😉

ldez · 2020-12-27T14:56:47Z

before the fix:

// wantedErrors parses expected errors from comments in a file.
// nolint:nakedret
func wantedErrors(file, short string) (errs []wantedError) {

after the fix:

// wantedErrors parses expected errors from comments in a file.
// nolint:nakedret
func wantedErrors(file, short string) (errs []wantedError) {

OK

before the fix:

// wantedErrors parses expected errors from comments in a file.
//    nolint:nakedret
func wantedErrors(file, short string) (errs []wantedError) {

after the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:nakedret
func wantedErrors(file, short string) (errs []wantedError) {

KO

The fix removes all whitespaces, the choice to remove all the whitespaces, in this case, seems not right.

ldez · 2020-12-27T15:05:08Z

before the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:gocyclo, nakedret
func wantedErrors(file, short string) (errs []wantedError) {

after the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:nakedrett
func wantedErrors(file, short string) (errs []wantedError) {

KO

before the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:gocyclo, nakedret
func wantedErrors(file, short string) (errs []wantedError) {

after the fix:

// wantedErrors parses expected errors from comments in a file.
//nolint:nakedrett
func wantedErrors(file, short string) (errs []wantedError) {

KO

nakedrett is not a linter 😉

ldez · 2020-12-27T15:09:01Z

before the fix:

// wantedErrors parses expected errors from comments in a file.
// nolint:gocyclo,nakedret,whitespace
func wantedErrors(file, short string) (errs []wantedError) {

or

// wantedErrors parses expected errors from comments in a file.
// nolint:nakedret,gocyclo,whitespace
func wantedErrors(file, short string) (errs []wantedError) {

or

// wantedErrors parses expected errors from comments in a file.
// nolint:gocyclo,whitespace,nakedret
func wantedErrors(file, short string) (errs []wantedError) {

results:

WARN Line 163 has multiple intersecting issues: []result.Issue{result.Issue{FromLinter:"nolintlint", Text:"directive `// nolint:gocyclo,nakedret,whitespace` is unused for linter \"gocyclo\"", Severity:"", SourceLines:[]string{"// nolint:gocyclo,nakedret,whitespace"}, Replacement:(*result.Replacement)(0xc0074e3ec0), Pkg:(*packages.Package)(0xc0012b3b00), LineRange:(*result.Range)(nil), Pos:token.Position{Filename:"test/errchk.go", Offset:4489, Line:163, Column:1}, HunkPos:0, ExpectNoLint:true, ExpectedNoLintLinter:"gocyclo"}, result.Issue{FromLinter:"nolintlint", Text:"directive `// nolint:gocyclo,nakedret,whitespace` is unused for linter \"whitespace\"", Severity:"", SourceLines:[]string{"// nolint:gocyclo,nakedret,whitespace"}, Replacement:(*result.Replacement)(0xc0074e3f20), Pkg:(*packages.Package)(0xc0012b3b00), LineRange:(*result.Range)(nil), Pos:token.Position{Filename:"test/errchk.go", Offset:4489, Line:163, Column:1}, HunkPos:0, ExpectNoLint:true, ExpectedNoLintLinter:"whitespace"}}

no fix applied.

KO

ldez · 2020-12-27T15:13:23Z

I will open an issue with all of these elements, I think that will be easier to track.

ashanbrown · 2020-12-27T15:14:18Z

@ldez I'll look into the mangled linter names. That's clearly a bug.

Regarding multiple linters, I didn't attempt to reconcile multiple extra linters in this change. Does that seem like what's happening?

Regarding leading whitespace, we could add try to preserve the user's original whitespace if we are not enforcing re-formatting.

ldez · 2020-12-27T15:30:50Z

@ashanbrown I think that the PR has been merged too quickly, it's not the end of the world, but in our context, that means that it's impossible to create a release. Since anyone can create a release, this can be problematic.

I think the best way to handle that is to revert the commit and create a new PR. WDYT?

ldez · 2020-12-27T15:40:49Z

FYI I tested the PR with golangci-lint code:

golangci-lint/test/errchk.go

Lines 162 to 164 in aeb9830

    
           // wantedErrors parses expected errors from comments in a file. 
        
           //nolint:nakedret 
        
           func wantedErrors(file, short string) (errs []wantedError) {

go run ./cmd/golangci-lint/ run test

ernado · 2020-12-27T15:52:11Z

Oh, sorry, missed that on review.

Probably it is OK to revert it so @ashanbrown can fix issues without time pressure.

…nolint statements (golangci#1573)" This reverts commit aeb9830.

…tatements (golangci#1573) Also allow multiple ranges to satisfy a nolint statement as having been used.

…nolint statements (#1573)" (#1584) This reverts commit aeb9830. There are some cases that nolinter fixer wasn't handling properly or expectedly (#1579, #1580, #1581) so we'll fix those in a new attempt.

…tatements (golangci#1573) Also allow multiple ranges to satisfy a nolint statement as having been used.

Update nolintlint to fix nolint formatting and remove unused nolint s…

908f431

…tatements

ashanbrown force-pushed the asb/add-fix-for-nolintlint branch from e593eef to 7489c5a Compare December 23, 2020 05:44

ashanbrown commented Dec 23, 2020

View reviewed changes

ashanbrown marked this pull request as ready for review December 23, 2020 05:54

ashanbrown force-pushed the asb/add-fix-for-nolintlint branch from 7489c5a to 6ee5247 Compare December 24, 2020 14:56

ldez added the linter: update version Update version of linter label Dec 24, 2020

ldez self-requested a review December 24, 2020 15:31

ernado approved these changes Dec 26, 2020

View reviewed changes

Allow a single issue to fulfill multiple ignore ranges

e9e8f38

ashanbrown force-pushed the asb/add-fix-for-nolintlint branch from eb467e1 to e9e8f38 Compare December 27, 2020 13:46

ashanbrown merged commit aeb9830 into golangci:master Dec 27, 2020

ashanbrown deleted the asb/add-fix-for-nolintlint branch December 27, 2020 14:18

This was referenced Dec 27, 2020

nolintlint: invalid linter name after autofix #1579

Closed

nolintlint: too many whitespaces removed after autofix #1580

Closed

nolintlint: the autofix is skipped when several unsed linters #1581

Closed

ldez mentioned this pull request Dec 27, 2020

nolintlint: allow to fix //nolint lines #1583

Merged

ashanbrown added a commit to ashanbrown/golangci-lint that referenced this pull request Dec 27, 2020

Revert "Update nolintlint to fix nolint formatting and remove unused …

11d6231

…nolint statements (golangci#1573)" This reverts commit aeb9830.

ashanbrown added a commit to ashanbrown/golangci-lint that referenced this pull request Dec 27, 2020

Update nolintlint to fix nolint formatting and remove unused nolint s…

fdd1cac

…tatements (golangci#1573) Also allow multiple ranges to satisfy a nolint statement as having been used.

ashanbrown added a commit to ashanbrown/golangci-lint that referenced this pull request Dec 27, 2020

Update nolintlint to fix nolint formatting and remove unused nolint s…

80eb2ff

…tatements (golangci#1573) Also allow multiple ranges to satisfy a nolint statement as having been used.

ldez added this to the v1.34 milestone Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update nolintlint to fix nolint formatting and remove unused nolint statements #1573

Update nolintlint to fix nolint formatting and remove unused nolint statements #1573

ashanbrown commented Dec 23, 2020 •

edited by ldez

Loading

ashanbrown Dec 23, 2020

ashanbrown Dec 23, 2020

ernado commented Dec 24, 2020

ernado left a comment

ldez commented Dec 27, 2020

ashanbrown commented Dec 27, 2020

ldez commented Dec 27, 2020 •

edited

Loading

ldez commented Dec 27, 2020

ldez commented Dec 27, 2020

ldez commented Dec 27, 2020 •

edited

Loading

ldez commented Dec 27, 2020 •

edited

Loading

ashanbrown commented Dec 27, 2020

ldez commented Dec 27, 2020

ldez commented Dec 27, 2020

ernado commented Dec 27, 2020

Update nolintlint to fix nolint formatting and remove unused nolint statements #1573

Update nolintlint to fix nolint formatting and remove unused nolint statements #1573

Conversation

ashanbrown commented Dec 23, 2020 • edited by ldez Loading

ashanbrown Dec 23, 2020

Choose a reason for hiding this comment

ashanbrown Dec 23, 2020

Choose a reason for hiding this comment

ernado commented Dec 24, 2020

ernado left a comment

Choose a reason for hiding this comment

ldez commented Dec 27, 2020

ashanbrown commented Dec 27, 2020

ldez commented Dec 27, 2020 • edited Loading

ldez commented Dec 27, 2020

ldez commented Dec 27, 2020

ldez commented Dec 27, 2020 • edited Loading

ldez commented Dec 27, 2020 • edited Loading

ashanbrown commented Dec 27, 2020

ldez commented Dec 27, 2020

ldez commented Dec 27, 2020

ernado commented Dec 27, 2020

ashanbrown commented Dec 23, 2020 •

edited by ldez

Loading

ldez commented Dec 27, 2020 •

edited

Loading

ldez commented Dec 27, 2020 •

edited

Loading

ldez commented Dec 27, 2020 •

edited

Loading