scalameta / metals-feature-requests

Issue tracker for Metals feature requests
37 stars 4 forks source link

More intelligent auto-import positions #221

Open dos65 opened 3 years ago

dos65 commented 3 years ago

Currently, auto-imports are placed at the end of import block that isn't connected to organize imports rule. It would be more convenient if auto-import changes would produce well ordered import block that doesn't require running organize imports after it.

olafurpg commented 3 years ago

I would caution against this feature because

dos65 commented 3 years ago

@olafurpg I wouldn't say that it's tricky if there would be an option to call organize-import rule directly by passing some fake document / trees only. That would allow metals to call it with import-section part of source only and turn it's output into a single TextEdit.

I agree that it isn't a huge improvement but I would like to have it because I often forget to run scalafix after making minor changes.

olafurpg commented 3 years ago

@dos65 that approach won’t work if the source file doesn’t parse/compile. What do you think about running organize imports on file save instead when the compilation is up-to-date? Alternatively, you could have a setting to automatically organize imports after compilation succeeds and Metals would apply the text edit whenever it can (could be triggered on a debounce so it doesn’t happen while the user is actively typing).

olafurpg commented 3 years ago

One alternative would be report a diagnostic with severity "hint" suggesting that imports can be organized and always suggest a code action when that diagnostic is active.

dos65 commented 3 years ago

What do you think about running organize imports on file save instead when the compilation is up-to-date

It sounds like a good idea and won't require significant changes. The only problem that it might be non-intuitive as it will be performed with a delay after the source will be compiled as it requires semanticdb file.

that approach won’t work if the source file doesn’t parse/compile.

It think it would be enough if it parses. In the most cases when auto-import happens source is parsed. To specify what I mean.

Organize-imports rule has the following function:

private def organizeGlobalImports(imports: Seq[Import])(implicit doc: SemanticDocument): Patch = ...

The most interesting part of it doesn't use SemanticDocument. It's needed only for producing delete patch. So, technically it should be possible to turn it into a public function that might be called from Metals without semanticdb.

def organizeGlobalImports(config: OrganizeImportConfig, imports: Seq[Import]): Patch = ...

That would allow Metals to use it for auto imports + perform organize-imports on files that aren't compiled.

olafurpg commented 3 years ago

OrganizeImports has a setting groupExplicitlyImportedImplicitsSeparately to place implicits separately from other imports, which requires access to SemanticDocument.

Regardless, it would still be an improvement over the current behavior if Metals did a better job guessing where to place the import (even if it’s not perfect). Thank you @dos65 for pushing on this idea, it’s worth a try!