wsl Add fixer

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

[x] Add newline when linter says to cuddled
[x] Add newline when linter says to cuddled - support comments
[x] Don't add newline if a previous fix resolves multi cuddle¹
[x] Add newline in end of case block when enforced
[ ] Remove newline in beginning of block
[ ] Remove newline in beginning of block - support comments
[ ] Remove newline in end of block
[ ] Remove newline in end of block - support comments
[ ] Remove newline when enforce to cuddle error checking
[ ] Advanced formatting, e.g. group var

¹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
}

Feb 24 '20 22:02 bombsimon

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

Feb 24 '20 22:02 coveralls

this would be a great feature

Mar 21 '20 13:03 dcu

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.

Apr 04 '20 15:04 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.

Jun 05 '20 13:06 bombsimon

What's the current state of this?

Nov 30 '22 21:11 Davincible

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...

Dec 01 '22 19:12 bombsimon

@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.

Feb 13 '23 05:02 dgocoder

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

Feb 13 '23 14:02 mgreg90

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:

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.
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
Be naive and just add a whitespace. Instead of changing

from to

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))`

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

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)`

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 https://github.com/bombsimon/wsl/issues/90#issuecomment-684436609 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.

Feb 13 '23 20:02 bombsimon

I think most things are sorted now. I have rebased on master, ported diff, we can nowreformat most things and we have a way to test it. The formatting for var blocks isn't the best but if one would want to convert to a group I would suggest setting allow-cuddle-declarations to true and relying on gofumpt.

What's left to sort are removal of newlines when we have comments. I added a test case for that which currently do not pass but we know the goal. I just need to add more states of the blocks so I can do proper formatting

For leading whitespace:

If no comments - remove newline between blockStart and firstStatementStart
If leading comment has newline after blockStart - set range to blockStartPos - firstCommentStart
If allow-separated-leading-comment is false
- If leading comment(s) - keep track of last comment group and remove newline between lastCommentEnd and firstCommentStart
- Keep track of all comment groups and if space between any add diagnostic between comment groups (end/start)

And then similar logic for trailing whitespace but we only need to keep track of the first and last comment group and remove whitespace if needed.

I'll be busy the coming days but I'll try to make some progress during the weekend.

Feb 16 '23 00:02 bombsimon

I realised when working on the leading and trailing comments and whitespaces that forbidding trailing comments will be hard or impossible to fix since it's not just adding or removing a newline. I was considering moving the comment above the last statement but that only makes sense if it's related to that statement and no other comment is already there.

I wonder if it's better to relax the rule or just don't support fixes for them. Pondering if it will be weird to run the fix flag but still have to fix that manually. I guess not since most linters require manual attention and I think for this pass it's better to not change any rules or diagnostics.

Feb 19 '23 10:02 bombsimon

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

Feb 19 '23 21:02 Davincible

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.

Feb 19 '23 22:02 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.

Feb 20 '23 22:02 bombsimon

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

Feb 22 '23 22:02 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.

Feb 26 '23 11:02 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.

Mar 01 '23 22:03 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.

Mar 02 '23 00:03 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.

Mar 05 '23 16:03 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 https://github.com/golangci/golangci-lint/issues/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)

Mar 15 '23 20:03 bombsimon

wsl wsl copied to clipboard

Add fixer

Whitespace parsing in beginning and end of block

Advanced formatting

Testing

wsl
wsl copied to clipboard