Add support for SuggestedFixes

[ldez]

Is your feature request related to a problem? Please describe.

Add support for SuggestedFixes.

https://github.com/golang/tools/blob/b4639ccb830b1c9602f1c96eba624bbbe20650ea/go/analysis/doc/suggested_fixes.md

Describe the solution you'd like

Be able to apply automatically the fixes via --fix.

Describe alternatives you've considered

Display the suggestion as extra information.

Additional context

type Diagnostic struct {
	// ...

	// SuggestedFixes contains suggested fixes for a diagnostic which can be used to perform
	// edits to a file that address the diagnostic.
	// TODO(matloob): Should multiple SuggestedFixes be allowed for a diagnostic?
	// Diagnostics should not contain SuggestedFixes that overlap.
	// Experimental: This API is experimental and may change in the future.
	SuggestedFixes []SuggestedFix // optional


	// ...
}

https://github.com/golang/tools/blob/b4639ccb830b1c9602f1c96eba624bbbe20650ea/go/analysis/diagnostic.go#L23-L28

Current linters that support SuggestedFixes:

• goerr113
• ruleguard
• staticcheck
• nlreturn
• exportloopref
• exhaustive
• govet

Related to #1728

If you want to help me:

• https://bento.me/golangci-lint
• https://bento.me/ldez

Latest update: #1779 (comment)

[ldez]

I'm working on and for now, I'm able to apply suggestions from goerr113 because the suggestions concern only one line.

For the other, the problem is a bit more complex because the fix must be applied on multiple lines.

[SVilgelm]

I think we need to print the suggested fixes if they are not applied (without --fix)

[ldez]

In some cases, it will be very weird to display the suggestions, because a suggestion can be a \n or the removal of a line, and TextEdits is a slice.

[SVilgelm]

agree

[SVilgelm]

we can print a diff, anyway you are going to create a diff, so we can print replacement, maybe in verbose output

[SVilgelm]

@ldez
I think we need to replace the Replacement with SuggestedFixes in golangci-lint, to avoid a transformation.

[ldez]

I think we can support both as the first step, but I'm thinking about it, I need more time.

[SVilgelm]

Here is the code of how to apply SuggestedFixes: https://github.com/golang/tools/blob/54dc8c5edb561003330ec7e649333181bbd0a9bd/go/analysis/internal/checker/checker.go#L316

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nightlyone]

@ldez I guess this will be the next major feature after the generics issues are fixed? The developer productivity and happiness enabled by just clicking Apply Suggested fixes is quite significant on a Go ecosystem scale.

[ldez]

This issue is not related to GitHub review suggestions.
It's related to the CLI flag --fix of the command line.

[pohly]

Is there anything that can be done about this on a per-linter basis? In nunnatsa/ginkgolinter#63 (comment) it was pointed out that for some linters, fixing already works.

[ldez]

I plan to handle the topic by a breaking change, I don't want to introduce several ways to handle fixes.

[JoelSpeed]

I'm currently coming up against this issue. I'm building a custom linter and have SuggestedFixes implemented. They work fine when I use the binary with multichecker directly, but I would like to be able to apply the fixes via golangci-lint as well.

Was any work ever started here? Is there a rough outline of what needs to be done to implement this? I'm new to this codebase but may be able to help implement it if I can get a high level overview of what's needed

[ldez]

I will handle this issue.

This will be my work for this week.

https://bsky.app/profile/golangcilint.bsky.social/post/3lcfpfu5lbc24

[ldez]

UPDATE

I will give some details of my current work on the topic.

My plan is to replace the current implementation (based on Replacement, HunkPos, etc.) with an implementation based on analysis.SuggestedFix.
But this is very complex because the current implementation "leaks" everywhere, and some linter implementations (gofmt, goimport, etc.) are heavily linked to this implementation.

FYI, the current implementation is not "friendly", and is completely different than something based analysis.SuggestedFix.
It requires rewriting the entire implementation from scratch.

It's a long job, so it will take me more than one week.

Maybe at the end, I will just create a kind of "compatibility layer".
This solution will be a type of failure to me because it will increase complexity instead of decreasing it, and so the goal of simplifying the maintenance will not be achieved.

FYI, currently, my only revenue comes from community sponsors, I work full-time on OSS, and I'm fully transparent with my revenues (everything is public), so if you want to help me:

• https://bento.me/golangci-lint
• https://bento.me/ldez

[ldez]

Update

I have a working PoC (it's a draft), I keep it locally for now because it's a bit too raw.

Currently, golangci-lint is able to "autofix" reports from 13 linters:

• gci
• gocritic
• godot
• gofmt
• gofumpt
• goheader
• goimports
• mirror
• misspell
• nolintlint
• protogetter
• tagalign
• whitespace

I converted 8 linters of those currently supported linters to SuggestedFix.
So, 5 of the 13 are still not working (gofmt, gofumpt, goheader, goimports, nolintlint)

About the other linters, I'm able to use "autofix" for the totality of linters that support SuggestedFix:

• canonicalheader
• dupword
• fatcontext
• ginkgolinter
• err113
• errorlint
• iface
• importas
• intrange
• nakedret
• nlreturn
• perfsprint
• testifylint
• wsl
• govet

Also, the new "autofix" system is less powerful: there are some limitations on multiple fixes on the same line. For now, I don't know if I will be able to improve that.

This is still under development. Until it is fully working, I do not consider my current implementation as a working solution.

If you want to help me:

• https://bento.me/golangci-lint
• https://bento.me/ldez

[JoelSpeed]

Also, the "autofix" system is less powerful: there are some limitations on multiple fixes on the same line.

Could you expand on this point? My understanding of SuggestedFixes was that it could apply fixes as long as they didn't overlap, so you have to specify starts and ends if you have multiple fixes on the same line. Was that not true of the previous implementation? How did it handle overlapping fixes?

[ldez]

My sentence was not clear: the new system is less powerful. (I edited my previous post)

Remember, this is a rewrite from scratch (#1779 (comment)) and this is a draft.

[bombsimon]

Was that not true of the previous implementation? How did it handle overlapping fixes?

The previous (current) implementation does not support this, e.g #1490, #4885, #1510, #3819, #3454.

I don't know what the new implementation looks like but the idea of using SuggestedFixes sounds like it would use the data from pass.Report/pass.Diagnostic which holds the SuggestedFixes slice. Given this it might be possible at some point to apply overlapping fixes or at least fixes on the same line with different pos if they share the pass.

I think for now, just using SuggestedFixes is a huge thing and probably not worth focusing on overlapping fixes just yet.

[ldez]

The previous (current) implementation does not support this

This is more subtle: the current implementation is able, in some cases, to support that (and some overlap cases).

For example, run golangci-lint run --enable-only misspell --fix on this:

package sandbox

import "log"

// langauge langauge

func langaugeMisspell() {
	var langauge, langaugeAnd string
	log.Printf("it's becouse of them: %s, %s", langauge, langaugeAnd)
}

This example contains several problems on the same line.

FYI the problems from the issues about imports are also more subtle than overlap or same line, the problem is that each linter reports "identical" changes but not on the same line.

[pohly]

Perhaps overlapping fixes can be handled incrementally, as in "apply some fixes that don't overlap, rerun analysis, apply again, until nothing pops up anymore"? Just a thought.

[ldez]

Update

I think I win the fight against the most complex part: the "formaters" (gofmt, gofumpt, goimports).
This led me to deeper thoughts about how "formaters" are integrated, but that's another topic.

So the 12 currently supported linters, and 15 linters that support SuggestedFix are working:

• canonicalheader (new)
• dupword (new)
• err113 (new)
• errorlint (new)
• fatcontext (new)
• gci
• ginkgolinter (new)
• gocritic
• godot
• gofmt
• gofumpt
• goheader
• goimports
• govet (new)
• iface (new)
• importas (new)
• intrange (new)
• mirror
• misspell
• nakedret (new)
• nlreturn (new)
• perfsprint (new)
• protogetter
• tagalign
• testifylint (new)
• whitespace
• wsl (new)

Now, I have to handle nolintlint.

Reports on the same line (not overlapping) are still not supported.
There are several other problems that I should solve too.

This is still in WIP.
Until it is fully working, I do not consider my current implementation as a working solution.

If you want to help me:

• https://bento.me/golangci-lint
• https://bento.me/ldez

[ldez]

Update

The problem of changes on the same line is fixed: the fun fact is it was just a missing condition inside an if.

---

Also overlapping is now managed in a stable way.

The situation:

1. You have 2 linters (gofmt and goimports for example)
2. The 2 linters produce 2 reports/changes on the same line (ex: an import on L4)) -> overlap.
3. The 2 same linters have also produced reports/changes but without overlap (ex: changes one import on L5 and another on L6)

This situation is 99% related to formaters because they want to apply changes to the same sections but not in the same way, so not in the same positions.

Before/Currently:

The overlap was managed by picking "randomly" one change for one of both linters (L4).
And all the other changes (without overlap) from the 2 linters were applied (L5, L6).

The result was a mix of non-consistent changes: deletion of imports, duplication of imports, etc.

The problem here is not the overlaps by themselves (because they are "managed") but the other changes.

With my implementation:

If there are overlaps, only one of the 2 linters is "allowed" to apply changes on the targeted file (ex: L4, L5, not L6).
The other linters that don't overlap are also "allowed" to apply changes to this file.

I tested my implementation with the examples from the issues, and it works as expected: no unexpected deletions, and no duplications.

---

I still have to handle nolintlint, it is not the easiest to migrate.
(maybe my next work will be to deprecate it and implement a new directive system, but this is another topic)

I'm increasingly confident in my current implementation.
Now, IMHO, this is a working solution.

---

If you benefit from this change, and you want to help me:

• https://bento.me/golangci-lint
• https://bento.me/ldez

[ldez]

Update

nolintlint was a pain to migrate but this is done.

The "autofix" is now working for the 28 linters 🎉

Now, I should clean the implementation, add new linter tests, create a reviewable PR, and maybe add some bonuses ✨.

It will take one or two days so I will create a PR this weekend.

The go1.24.0-rc1 release will happen very soon, I already prepared a PR but this could take some time too.

This will be the last update in this issue as the next step is to create a PR.

---

I already worked 10 days on this topic, and my only revenue is donations.
During this period golangci-lint received $5 ( ❤️ ) and I received $0.
It's a bit sad, but not surprising, I need to think about how to improve that.

If you enjoy this change, and you or your company want to help golangci-lint or me:

• https://bento.me/golangci-lint
• https://bento.me/ldez

[mitar]

We use thanks.dev for donation distribution and I just added https://github.com/golangci/golangci-lint as a manual inclusion. You can quickly sign up and then it connects with your GitHub sponsors, if that would work for you. Thank you for your great work!

[ldez]

I already have a thanks.dev account but I didn't find how to add golangci-lint.
I need to dig into this 🤔

[StevenACoffman]

It's not much I know, but I just put in $20 for you for this, and am still waiting to hear if my employer will sponsor ongoing.

[ldez]

Update

Sadly, I have an "extra" update 😢

I'm fighting against CGO and friends, so I must add tests everywhere and adjust the linter implementations when needed.
This takes a lot of time.

I do my best but I will not be able to open a PR today.

edit1: When I fix CGO, it breaks yacc directives...
edit2: after more analysis, some linters were broken with CGO and yacc before my changes. So instead of just adding tests for modified linters, I must add tests for all the linters.
edit3: Adding tests (200) for all the existing linters took me 8 hours, I'm a bit tired.

[Crocmagnon]

Just threw 20€ in the hat, thank you for your continued work!
Let me know if anything is needed on fatcontext.

[ldez]

Update

After a very very long tunnel to add tests, run tests, and analyze the results, I have some conclusions:

• yacc directives have never worked for 95% of the linters
• cgo is not supported by 13 linters. With my changes (and some PR I opened on linters) I fixed 2 linters but broke 3 other linters (I will revert some changes on those linters and this will be "fixed").

I evaluate this topic as independent: I postpone the problems with cgo and yacc, I will handle this later.

I will remove all the yacc tests (but keep the cgo tests), clean everything, and open a PR.

It was a difficult and exciting tunnel, a bit extreme, and now I need some rest.

[ldez]

🎉 The PR ready: #5232

[Crocmagnon]

Awesome! Can't wait to try this in a new release :D