feat: Code action for variable discards

[WillLillis]

I was interested in addressing ziglang/zig#20584 (comment), so I added a code action to discard variables using the _ = &x; "trick". Happy to close this PR if this isn't a desired feature; I really just wanted a reason to dig around the code action/ quick fix code a bit :)

Quick demo:

[Techatrix]

For a code action to be part of "autofix" it should ideally be reversible but _ = &foo; will never be removed. This means that it can only be a quickfix instead of source.fixAll.

One possible approach to bring the "var -> const" code action back to being part of autofix is to see if Zig's AstGen step can be improved to report a "constant variable is mutated" error. If both of these errors can counter each other then we could consider bringing it back to autofix.

I don't know whether the const to var direction can exactly match var to const or if there are any edge cases but something trivial as this should be detectable:

test {
    const foo = 5;
    foo = 5;
}

[WillLillis]

For a code action to be part of "autofix" it should ideally be reversible but _ = &foo; will never be removed. This means that it can only be a quickfix instead of source.fixAll.

Done👍

One possible approach to bring the "var -> const" code action back to being part of autofix is to see if Zig's AstGen step can be improved to report a "constant variable is mutated" error. If both of these errors can counter each other then we could consider bringing it back to autofix.

Gotcha, it definitely makes sense to have the "var -> const" direction as part of autofix. Do you think it still makes sense to have this _ = &foo; code action available as a non-autofix option?

[mlugg]

The inverse direction cannot be correctly detected due to the existence of pointers. (This is also precisely why the _ = &x trick works in the first place.) If I write &x, it is impossible to know without semantic analysis whether this pointer is used to mutate x. Moreover, it is even harder to know whether any possible comptime code path would mutate x through the pointer.

I know the ZLS authors have the final say here, but I would strongly discourage accepting this PR. The _ = &x trick is very much a dirty secret -- an unfortunate consequence of the check's limitations -- and we don't want it ever appearing in real-world code. _ = x acts as a marker to the reader that the author meant to not use something; however, there is no situation where not mutating a var is ever correct. If you insist on having a fixup for this, please at the very least make it x = x or something similarly silly, because what this fixup is trying to achieve is fundamentally silly and incorrect in all cases. The correct fixup would be to turn var into const; but perhaps the lack of automatic reversibility makes this too unintuitive of a fixup. My strong personal belief is that this error should not have an automatic fixup; the correct solution to this error requires a human to understand the context of the code even more so than "unused variable" errors.

[Techatrix]

The inverse direction cannot be correctly detected due to the existence of pointers.

RIght, I had the feeling that something didn't make this possible. This is why we need &mut foo! /j

The correct fixup would be to turn var into const; but perhaps the lack of automatic reversibility makes this too unintuitive of a fixup.

This is the exact reason why var to const is no longer part of autofix. #1621

Without reversibility this code action could only be a quickfix that need to be manually applied by the user. But as you have pointed out, the correct move is to change var to const and we already have a quickfix for that.

[WillLillis]

The _ = &x trick is very much a dirty secret -- an unfortunate consequence of the check's limitations -- and we don't want it ever appearing in real-world code.

Thanks for the insight @mlugg! I really appreciate the in depth explanation, I was clearly missing a big part of the larger picture. Cheers