Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for SuggestedFixes #5232

Merged
merged 57 commits into from
Dec 17, 2024
Merged

Conversation

ldez
Copy link
Member

@ldez ldez commented Dec 16, 2024

This PR extends the support of the "autofix" to all the linters that support SuggestedFixes.

I did my best to create a reviewable PR, so there are several commits but some are more important than others.

Supported linters
  • canonicalheader (new)
  • dupword (new)
  • err113 (new)
  • errorlint (new)
  • fatcontext (new)
  • gci
  • ginkgolinter (new)
  • gocritic
  • godot
  • gofmt
  • gofumpt
  • goheader
  • goimports
  • gosimple (new)
    • s1001
    • s1002
    • s1003
    • s1004
    • s1005
    • s1010
    • s1011
    • s1012
    • s1016
    • s1018
    • s1021
    • s1024
    • s1025
    • s1028
    • s1030
    • s1033
    • s1034
    • s1035
    • s1036
    • s1037
    • s1039
  • govet (new) (10 passes)
    • assign
    • composites
    • fieldalignment
    • findcall
    • printf
    • sigchanyzer
    • sortslice
    • stringintconv
    • timeformat
    • unreachable
  • iface (new)
  • importas (new)
  • intrange (new)
  • mirror
  • misspell
  • nakedret (new)
  • nlreturn (new)
  • nolintlint
  • perfsprint (new)
  • protogetter
  • revive (new) (2 rules)
    • range
    • errorf
  • staticcheck (new)
    • sa1004
    • sa1006
    • sa1008
    • sa1012
    • sa1013
    • sa1016
    • sa4013
    • sa4026
    • sa4029
    • sa5004
    • sa6005
    • sa9002
    • sa9004
  • stylecheck (new)
    • st1013
    • st1017
    • st1018
  • tagalign
  • testifylint (new)
  • whitespace (new)
  • wsl (new)

Total: 32 linters

Before the PR only 13 linters were able to apply "autofix".

With this PR, every linter that support SuggestedFixes will be able to "autofix" without require a specific implementation inside golangci-lint or inside the linter.
And the plugins can also apply "autofix".

Recommended Usage

I recommend using the following command to apply fixes:

golangci-lint run --enable-only <linter_name> --fix

ex:

golangci-lint run --enable-only wsl --fix

I recommend not using a globally fix: true inside the configuration, or using it globally without --enable-only unless you trust all suggested fixes.

About overlaps

Also overlapping is now managed in a stable way.

The situation:

  • You have 2 linters (gofmt and goimports for example)
  • The 2 linters produce 2 reports/changes on the same line (ex: an import on L4)) -> overlap.
  • 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 formatters 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.

Fixes #1779
Fixes #1490
Fixes #1728
Fixes #2161
Fixes #1726

Fixes #4885
Fixes #1510
Fixes #3819
Fixes #3454
Fixes #1510

Related to #2300
Related to #2609


Notes

Linters that have not been migrated cannot be migrated without creating regressions.

These linters will be processed, but they require significant work so this will be done gradually.


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

@ldez ldez added enhancement New feature or improvement area: auto-fix labels Dec 16, 2024
@ldez ldez added this to the next milestone Dec 16, 2024
@ldez ldez requested a review from bombsimon December 16, 2024 14:27
@ldez ldez force-pushed the feat/suggested-fixes branch from 54579a1 to 6752f12 Compare December 16, 2024 14:49
@ldez ldez force-pushed the feat/suggested-fixes branch from 6752f12 to 94073d2 Compare December 16, 2024 14:53
Copy link
Member

@alexandear alexandear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work.

Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really good, thanks for putting your time and effort into this!

Extra plus for the separate commits and flow making -10k LOC not so scary. Really nice with all the tests as well!

The linters that now have disabled Cgo tests are marked with TODO, should we create a tracking issue for that? Thinking it would be easier to help out with that part.

pkg/golinters/asciicheck/testdata/asasalint_cgo.go Outdated Show resolved Hide resolved
pkg/golinters/whitespace/whitespace.go Show resolved Hide resolved
pkg/golinters/internal/diff.go Outdated Show resolved Hide resolved
pkg/golinters/internal/diff.go Show resolved Hide resolved
pkg/golinters/internal/diff.go Outdated Show resolved Hide resolved
pkg/result/processors/fixer.go Show resolved Hide resolved
pkg/result/processors/fixer.go Show resolved Hide resolved
pkg/result/processors/fixer.go Show resolved Hide resolved
pkg/result/processors/fixer.go Show resolved Hide resolved
pkg/golinters/dupl/dupl.go Show resolved Hide resolved
@ldez
Copy link
Member Author

ldez commented Dec 17, 2024

The linters that now have disabled Cgo tests are marked with TODO, should we create a tracking issue for that? Thinking it would be easier to help out with that part.

#5235

@ldez ldez force-pushed the feat/suggested-fixes branch from db85f32 to 898a5da Compare December 17, 2024 04:08
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment