zls icon indicating copy to clipboard operation
zls copied to clipboard

Code action to organize imports

Open Sekky61 opened this issue 4 months ago • 6 comments

Organize Imports Diagnostic and Code Action

This PR adds a diagnostic and a code action to organize imports in the current Zig file. The code action is available in the command palette as organize @import.

It is designed to be fairly customizable, so that we can arrive at a acceptable sorting style, like different grouping or case sensitivity. All decisions, especially those around diagnostics are very much open to critique as I do not have experience with the LSP.

Closes #1637, continues work from #881.

Design

An import is a top level declaration of a variable that uses the @import builtin. A top level declaration that uses import variables (like const print = std.debug.print;) is not considered an import and stays in place.

The sorting splits imports into three separated groups: builtin packages, packages and project files. The builtin packages are sorted in the order: std, builtin, build_options. The sorting inside other groups is done by comparing the declaration names (the x in const x = @import("file.zig");).

The feature is tested both in unit tests and in neovim. The unit tests required a new helper function.

Diagnostic

The diagnostic is enabled under the warn_style zls config option, which is disabled by default. The diagnostic's range is the entire line of the import which would be moved by organizing imports. That means one diagnostic per wrongly placed import line.

The diagnostic does not consider the correctness of separators between imports.

Code Action

The code action is available based on the diagnostic. This means that users with the warn_style option disabled will not be offered the code action.

The code action always sorts. The previous PR had optimization using isSorted to avoid sorting when it was not necessary. This, however, did not work well in some corner cases.

The documentation comments (///) are moved with the import they are attached to. It also respects Top-Level Doc Comments (//!), which must appear before any expression. This is handled using the commments token location.

This feature required running generateCodeAction for each diagnostic provided in request. Previously, it ran the function only on diagnostics from getAstCheckDiagnostics.

Considerations and notes

  • Top-level declarations (that includes imports) are order-independent (Source and tested)
  • zig fmt and textDocument/formatting does not interfere with the sorting.
  • Only root declarations are considered for sorting (no import inside functions).
  • Will not work with @import(variable); (we don't do comptime)
  • Resolving one diagnostic will resolve all diagnostics in the file (since all imports are moved).
  • I removed the part dealing with intersection with action's range (builder.range field which no longer exists). If it was relevant, I can study it further.
  • The code action works without warn_style as seen in tests. It is just that user cannot call it without the diagnostic.
  • std.sort.sort does not exist anymore. I picked std.sort.heap as a replacement. My understanding is that we do not need a stable sort here, but feel free to correct me and suggest a different sort from the std library.

Unresolved questions

  • [ ] Allow for const print = std.debug.print; to be sorted as well in some way?
  • [ ] How to treat @import("root")? (Currently a package, which does not seem right)
  • [ ] The getSortSlice function ported from previous PR seems weird. I would just do return self.name and not look in the import path. Any reasoning behind this?
  • [ ] What about grouping by directory for the file imports? Perhaps if there are > 1 file imports from the same directory, they could be in a group of their own.
  • [ ] I think DiagnosticKind could be parsed from the diagnostic code, which would be cleaner and would easily allow more versions of the diagnostic message. Or use the data field of the diagnostic message.
  • [ ] What about @embedFile?
  • [ ] How to implement ignoring the diagnostic? Ast.zig, for example, imports extra stuff near end of the file for tests.

Sekky61 avatar Oct 09 '24 19:10 Sekky61