feat(lsp): remove unused and duplicated imports in organize @imports code action

[WillLillis]

This PR augments the existing organize @imports code action to do the following:

• Remove duplicate imports (e.g. if const foo = @import("foo.zig"); is repeated)
• Remove unused imports
  • I did this by constructing a references request on the import decl. If no references are found, the import is marked to be removed. This rule is ignored if the import is marked with pub.

This required modifications to most of the existing tests for the code action, as nearly all imports were unused and thus removed. I avoided this by adding marking all imports in the existing tests with pub. Another option was to create a function in which the import is discard (e.g. fn foo() void { _ = foo; }). I used this for most of the new tests, but it seemed fairly invasive to apply to all of the existing tests.

Closes #2240

[xdBronch]

this might work fine most of the time but theres a reason zig itself doesnt have an error for unused globals, comptime makes it impossible to properly detect. im not necessarily against this being an option but i dont think i like it being part of an existing code action since its possible for it to break compilation

[WillLillis]

this might work fine most of the time but theres a reason zig itself doesnt have an error for unused globals, comptime makes it impossible to properly detect. im not necessarily against this being an option but i dont think i like it being part of an existing code action since its possible for it to break compilation

Ah that makes sense, thanks! I'll look into moving the unused removals into a separate code action.

[Techatrix]

The implementation of symbol references is incredibly inefficient which is especially problematic when used in a request like code actions which is frequently requested by editors without user input. Just running a debug build of ZLS in src/analysis.zig shows that the time to handle the request goes from around 15ms to 2200ms which is not acceptable.

Also a source.organizeImports code action may be executed on save where a false positive unused import would give the user no other choice than to disable this feature.

As mentioned by xdBronch, it is not possible to correctly detect unused imports in all cases but we should at least try to find a method where we can make sure that it has no false positives at least. If we do decide to allow false positives it needs to be fully opt-in and make it possible to disable the error per import.

• Remove duplicate imports (e.g. if const foo = @import("foo.zig"); is repeated)

Doesn't this already cause an error by zig ast-check because the decl name would be defined twice?

[xdBronch]

Doesn't this already cause an error by zig ast-check because the decl name would be defined twice?

could maybe just check if the imported file/module is duplicated? e.g.

const std = @import("std");
const std2 = @import("std");

not sure how often such a thing would ever happen though...

[WillLillis]

The implementation of symbol references is incredibly inefficient which is especially problematic when used in a request like code actions which is frequently requested by editors without user input. Just running a debug build of ZLS in src/analysis.zig shows that the time to handle the request goes from around 15ms to 2200ms which is not acceptable.

My apologies there, do you have any ideas for alternate routes that doesn't use symbol references?

Also a source.organizeImports code action may be executed on save where a false positive unused import would give the user no other choice than to disable this feature.

I thought this would be avoided by marking the code action with isPreferred = false, but I must have been mistaken.

As mentioned by xdBronch, it is not possible to correctly detect unused imports in all cases but we should at least try to find a method where we can make sure that it has no false positives at least. If we do decide to allow false positives it needs to be fully opt-in and make it possible to disable the error per import.

That seems reasonable to me, I'll try to work on some ways to prevent false positives. Thinking on this more, it's probably best to not allow any false positives. Just for my own curiosity, how would this be made fully opt-in? Categorizing it under a different code action kind?

• Remove duplicate imports (e.g. if const foo = @import("foo.zig"); is repeated)

Doesn't this already cause an error by zig ast-check because the decl name would be defined twice?

I hadn't personally run into this issue, but I added it in because it was mentioned in #2240. I could alter it to remove duplicates if the file path/module is repeated as xdBronch mentioned, but that could also leave users with broken file as there could be references to both of the duplicate imports. Since this is a bit messy and such an edge case in the first place, it sounds like it's better to just remove this part of the PR?

[Techatrix]

My apologies there, do you have any ideas for alternate routes that doesn't use symbol references?

Not really, sorry. My guess is that this will require a solution that is implemented from to ground up instead of being able to use existing utilities provided by ZLS. But I haven't put to much thought into this so I may be wrong here. If you do decide to continue working on this, I would suggest to discuss possible solutions to #2240 in the issue itself before trying to implement them.

I thought this would be avoided by marking the code action with isPreferred = false, but I must have been mistaken.

AFAIK the isPreferred field has a different meaning. Code actions on save is configured on the client side where users specify the code action kinds that should be executed.

Just for my own curiosity, how would this be made fully opt-in? Categorizing it under a different code action kind?

For code actions, it would probably have to be a different code action kind. It gets harder when we would like diagnostic being reported for unused imports as well.

Since this is a bit messy and such an edge case in the first place, it sounds like it's better to just remove this part of the PR?

Yes. Since the symbol references approach for unused imports doesn't really work out, it may be reasonable to close this PR until we agree on a better solution.

[WillLillis]

Apologies for the rushed implementation. Thanks so much for the feedback and helpful tips!