Add fixer

[bombsimon]

This is a tracking pull request for the implementation of a fixer. This PR aims to solve #4

• Add newline when linter says to cuddled
• Add newline when linter says to cuddled - support comments
• Don't add newline if a previous fix resolves multi cuddle¹
• Add newline in end of case block when enforced
• Add newline at proper place when considering errors (see example below)
• Remove newline in beginning of block
• Remove newline in beginning of block (body) - support comments
• Remove newline in end of block
• Remove newline in end of block (body) - support comments
• Remove newline when enforce to cuddle error checking
• Advanced formatting, e.g. group var
• Remove newline in beginning of block (case) - support comments
• Remove newline in beginning of block (case) - support comments

¹Since only one assignment/statement is allowed before if checks the linter will warn if blocks are cuddled. The fixer will resolve the block cuddling and thus the fixer should not add proposals to fix the check after. Example:

This

if true {
    return
}
_, err := doSomething()
if err != nil {
    return
}

Should be resolved to this

if true {
    return
}

_, err := doSomething()
if err != nil {
    return
}

But is currently resolved to this

if true {
    return
}

_, err := doSomething()

if err != nil {
    return
}

[coveralls]

Coverage: 94.249% (+2.2%) from 92.077% when pulling 02bf217 on fixer into 70fb523 on master.

[dcu]

this would be a great feature

[bombsimon]

I'm not looking forward to support removal of lines and not mess up comments in the current state. I'm not sure if whitespace will resolve errors 1:1 with how this linter want's it but at least it will fix obvious empty lines. I think > 99% are using this linter with golangci-lint and if so using --fix will run both fixers. Based on this I'm considering to not implement any whitespace removal in the current state, at least not in the beginning or end of block.

[bombsimon]

Just found out about https://github.com/mvdan/gofumpt which overlaps a lot of features here, many removing newlines. Might be some help figuring out a way forward for this. Although this has been stale manly because I haven't had the time to work on it.

[Davincible]

What's the current state of this?

[bombsimon]

What's the current state of this?

Sadly unchanged from last time and the note in #90 and I consider the project mostly in a maintenance state. I haven't really gotten much feedback or request so motivation has been low. I actually started some rework in a v4.0.0 branch this year but I've been too focused on other things to give this the attention needed. The idea with the v4 branch was to rebuild a lot of things from scratch to be more confident around the fixer but I never kept working on it.

Any help or PR in the right direction would be appreciated but I think my biggest problem is that I don't want to release a fixer that doesn't fix 100% of the cases and until I have that in place there will be no fixer in wsl. I do however think about it every now and then so I can't say it will never happen, although the fact that I haven't gotten to it in 2,5 yeas says otherwise...

[dgocoder]

@bombsimon we love this linter at work but are getting tired of fixing the issues manually specifically (cuddling) which seems you have working based off the list above. Wondering if any progress has been made since you last wrote? If not we would love to help if possible.

[mgreg90]

This feature would be great. How can we help this progress?

[bombsimon]

Hey, glad to hear you're finding this linter useful and want to contribute! And sorry for not pushing this issue more forward.

So first off, not much has changed and the reasons are the same; a combination of time and motivation. The motivation is mostly based on the assumption not many people use this linter and that I also no longer use this at my current workplace so there's no natural maintenance happening. Also since 2018 there's only 18 👍 on #4.

A second reason for little progress is probably Second-system effect. I created the v4.0.0 branch back in 2020 and have been tinkering on and off trying to address all open issues but every time I instead get exhausted by starting over. There's also the discussion (oh well, an issue with just a single reaction) in #110 about dropping support to make it easier to land this. After some thinking this would mostly be things that's not really related to whitespaces and mentioned under Advanced formatting in this issue, f.ex. declarations should never be cuddled.

Hearing you offering to help is very appreciated and makes me think about what would need to happen to land this. I think for now the roadmap should start with:

• Give up every idea around v4.0.0 and fixing existing issues for now
• Rebasing this PR on master, fix all conflicts and port all missing features to ensure all the current rules work with the analysis.Analyzer
• Add support to unit-test expected output

These three (or at least first two) things is probably something I can try to address fairly soon so we have a better starting point. Let me give this a go and feel free to ping me next week if no progress is made in this PR. If you know any good way or framework to test expected output (f.ex. a golden file) please let me know! Seems like we just need to run RunWithSuggestedFixes and create golden files to have this done automatically, will look into this.

If we end up there there's a few things to figure out:

Whitespace parsing in beginning and end of block

This has become more complex than intended given all the hard requirements of wsl. F.ex. wsl supports allow-trailing-comment and allow-separated-leading-comment. On top of that there's special handling of Example functions. Basically findLeadingAndTrailingWhitespaces is a mess to say the least. What's needed here is to ensure we have all the comments and groups related to their nodes (comments is a bit messy in the ast) and then just write all the logic. PR to improve handling of this would probably help.

Advanced formatting

This is things like grouping multiple var blocks or shuffling around append calls when possible. I think there's two or three options for this one:

1. Don't provide a fixer. I don't know if this is reasonable but a fixer could be a best effort. I said before I only want to add a fixer if it covers 100% of the cases but I'm open to re-evaluate that. We would still fix probably a good >90% of the cases.
2. Drop support for these kind of things at the same time as the fixer is implemented. To be fair the whole var block rule feels a bit misplaced in this linter and is also already supported (with fixer) in gofumpt
3. Be naive and just add a whitespace. Instead of changing

from	to
var foo = 1 var bar = 2 bar biz x := []string{} x = append(x, "literal") notUsed := "just assigning, don't mind me" x = append(x, "not z..") useMe := "right away" alsoNotUsed := ":(" x = append(x, "just noise" x = append(x, useMe)	var ( foo = 1 bar = 2 biz ) notUsed := "just assigning, don't mind me" alsoNotUsed := ":(" x := []string{} x = append(x, "literal") x = append(x, "not z..") x = append(x, "just noise" useMe := "to be used" x = append(x, useMe))

We would just add whitespaces:

from	to
var foo = 1 var bar = 2 var biz x := []string{} x = append(x, "literal") notUsed := "just assigning, don't mind me" x = append(x, "not z..") useMe := "right away" alsoNotUsed := ":(" x = append(x, "just noise" x = append(x, useMe)	var foo = 1 var bar = 2 var biz x := []string{} x = append(x, "literal") notUsed := "just assigning, don't mind me" x = append(x, "not z..") useMe := "right away" alsoNotUsed := ":(" x = append(x, "just noise" x = append(x, useMe)

Testing

Lastly as mentioned in #90 (comment) I think it's scary to modify other's source code and I can't guarantee everyone has backups or use revision control. I think just help with testing this and finding edge cases would help a lot.

[Davincible]

Regarding the trailing comments, I have found myself commenting out the last line of a function/ block sometimes and as a result wsl complained. Moving the comments wouldn't make sense in this case as it's just normal code and I might uncomment it later on. So vouching for relaxing around trailing comments

[bombsimon]

Soo, I think this might be getting closer. This is from the kubernetes repository:

› time wsl --fix ./...
[...snip]
wsl --fix ./...  50.21s user 5.55s system 434% cpu 12.840 total
› git status | wc -l
    2840
› git diff | wc -l
  329355

I'm running git diff --ignore-blank-lines -w and I only see diff when a leading or trailing whitespace is replaced by a now moved statement, f.ex.

        }
-       return nil

+       return nil
 }

Running the linter again after the first fix only shows the unfixable block should not end with a whitespace (or comment):

› time wsl --fix ./... 2>&1 | grep -v "block should not end with a whitespace"
wsl --fix ./... 2>&1  47.04s user 5.23s system 585% cpu 8.928 total

I think this is fine since we already support --allow-trailing-comment:

› time wsl --allow-trailing-comment --fix ./... && echo "Zero violations"
wsl --allow-trailing-comment --fix ./...  45.87s user 5.14s system 582% cpu 8.757 total
Zero violations

I've also tested this on the prometheus repository and it doesn't break any code or comments. By doing a quick inspection it looks like all the changes are intentional and correct.

[bombsimon]

I've discovered an intermittent bug where if I run it on kubernetes I sometimes get this error:

wsl: analysis "wsl" suggests invalid fix: pos (204243988) > end (204243907)

I think it's related to not allowing trailing comments and case blocks. Last example I reproduced was pkg/apis/core/validation/validation.go:7073.

What's weird is that it's not consistent. Running wsl on ./pkg/apis/core/validation can error 4 times in a row and suddenly starts working on the fifth attempt.

I need to troubleshoot this further but I'm travelling tomorrow and won't be back until Sunday evening so I might not pick this up until next week.

[bombsimon]

Found this which I somehow didn't know so probably worth looking into before merging this PR, maybe I can convert it somehow... golangci/golangci-lint#1779

[bombsimon]

Found a way to reproduce the intermittent bug, fixed it and ran the test a couple of 100 times to ensure it's fixed.

[bombsimon]

I'm trying to figure out how I can convert my issues to fixable via golangci-lint. No matter how (if) I do that I need to expose some public interface instead of just the analyzer which would be nice not to have since every change requires a new major release.

According to primary maintainer of golangci-lint we do not want to introduce linter specific implementation that converts between issues so I think I'll just settle on this fixer with the analyer api.

[bombsimon]

I found a way to work with DeclStmt and grouping var. The only issue now is that if a statement spans over multiple lines and contains comments the comments will be lost so I need to look into that. Added a failing test to show the behaviour.

[bombsimon]

I've made some iterations on converting multiple ast.DeclStmt to a single one with multiple ast.Spec but it's just so many edge cases when considering comments that it's very hard to make work properly. Comment's can be in a lot of places and even after bringing in dst and getting access to the decorated ast it's a bit tricky to move things around.

I think I'm going to revert the last changes for this and put that in a separate PR/issue. Either I go with #110 and just remove the rule or I spend some more time with an improved fixer.

[bombsimon]

I feel I'm losing pace on this PR so I'm going to leave the fixer as is and create a followup ticket to improve the fixer. A summary of what this means:

• This will add a fixer to remove every report except trailing comments (we don't know where to move them)
• The fixer will only add or remove whitespaces, not do any rewrite (f.ex. for DeclStmt)
• All types are private since we need to bump major version and this will avoid breaking changes in the near future
• I will not initially bump the version in golangci-lint because this adds no new features and the fixer will sadly not work, see Add support for SuggestedFixes golangci/golangci-lint#1779
• I prepared the flags to work with how golangci-lint overwrites the flags and I have a working branch in golangci-lint if we need or want to bump it (will probably happen if I address more issues)