zigtools
Should autofix be disabled by default?
I've been seeing a larger number of people disliking autofix being enabled by default as it mutates their code unexpectedly.
I'm undecided.
My reasoning why I wanted to enable autofix by default in #877 was to show ZLS users that such a feature exists instead of hoping that they stumble on it when looking through config options. Those who do not like it can then disable it. Anyway, the dumpster fire is still going.
My opinion might not be worth a lot, but since nobody else has chimed in; I got here via discovering https://github.com/ziglang/zig/issues/335.
I don't have strong opinions as many in that issue ticket partly because it's not been a big deal for me because zls handles this automatically for me in neovim. I would probably not have discovered zls being able to this if it had not been enabled by default, and it's made my zig journey a lot more pleasant as a result.
I think it's easier for people to search for how to disable a behaviour they don't like compared to searching for how to enable something they might not know is possible.
I turned it off (removed zls at first until I found the option) when it unexpectedly mutated my code.
The language server should not make choices on it's own concerning the structure of code outside of reporting issues, documentation, and suggesting possible fixes which are only executed upon request. It should certainly not mutate code by default unless the user opts-in as that may be destructive / create a mess when making opinionated choices regarding features such as discards.
Thanks for the feedback. Talked to some other people, and as much as I absolutely hate this decision, it is clear that people generally prefer autofix off. I'll disable it by default this afternoon.
I likely wouldn't have found this feature if it wasn't enabled by default.
The other possible option is to inject in a formatted comment along with the discard so it's documented (could even add an editor warning for unused variable wherever the comment is found). E.g., generated discard like this now:
_ = a;
becomes
// ZLS: auto discard unused variable see: {doc link} for more information
_ = a;
_ = a;
This is one of the reasons why autofix is problematic. It's not actually fixing anything by inserting this line. The variable remains effectively unused. It's only hiding the problem without addressing it. A language server should reveal problems to the user, not hide them.
// ZLS: auto discard unused variable see: {doc link} for more information _ = a;
This would address my concern, assuming the comment is highlighted by the editor as a warning. That way, you're not obscuring anything. The problem is still visible, but at least the code compiles.
Ideally, both of these would happen when there are unused variables:
- code compiles successfully
- editor or compiler lets you know that there are unused variables
Unfortunately, we can't have both right now. Zig chooses 2, and ZLS autofix chooses 1. If I have to pick only one option, my choice is 2 (which means disabling autofix), but ideally we could have both.
I always wanted to have comment that showed that an edit was performed by autofix but never got around to implementing it.
could even add an editor warning for unused variable wherever the comment is found
I've never though about also highlighting the autofix as a warning. This would be a great addition in my opinion.
@Techatrix mentioned improving UX by removing the autofix option entirely and instead relying on the codeActionKind capability and source.fixAll (LSP's name for autofix) being present or not.
@Techatrix mentioned improving UX by removing the autofix option entirely and instead relying on the
codeActionKindcapability andsource.fixAll(LSP's name for autofix) being present or not.
I've worked on implementing that but I still need to figure out how to get a source.fixAll.zls options working as described in #1093.