Auto import

[matklad]

I think we've reached the state where we can have a useful auto-import functionality!

UI

We don't have 100% reliable resolve yet, so we can't implement auto-import as a fix for "unresovled reference" diagnostic. Instead, we should implement it as an assist, available when the cursor is on an identifier, which doesn't resolve to anything

General Flow

The assist should work like this:

• we check that the cursor is on an ast::NameRef
• using hir, we verify that identifier does not resolve to anything (classify_name_ref should help here)
• using symbol index, we find a symbol with a matching name
• symbol index gives us only rough textual match. We should convert this textual match into a hir object (classify_name should work here)
• using this hir type, we should construct a canonical path to this item from it's crate root (crate::foo::bar::Thing)
• we should transform this canonical path to the one that we should insert at the use-site. For simplicity, this should just replace crate:: with the correct crate name for the start.
• we should compute the edit to insert this path into the target file. This is already done in add_import

Details

One interesting thing here is that we use text-based index for finding auto-import candidates. I think this is essential to make this feature scale to gigantic code-bases. At the moment, this won't work perfectly because symbol index misses items declared by macros, but this should be fixed by improving the index. Note that symbol index implementation already has a mode for precise, non-fuzzy queries (though I don't think that it is actually used anywhere, so it might have bit-rotten). Note that symbol_index is strictly and IDE part, not a compiler part. For this reason, the code for auto-import should live in the ra_ide_api crate. That is, for the time being, this should be the only assist that lives outside of ra_assist crate.

classify_name / classify_name_ref I think could be used for switching between semantic hir and semantic-less AST. I am not 100% sure here, maybe it's better to write some custom code.

construction of a canonical path for an item is a well-isolated task that can be done separately. It's even useful indepndently: for example. currently "run test" functionality uses only a function name, which might run more than one test. Ideally, we should use full path and --exact flag.

The code for inserting an import path into a file already exists. I am not sure if it has the right interface to be extracted, some refactoring might be required to make it really work for this use-case.

[natanfudge]

Why not add missing imports as an auto completion option, and then auto-import when the completion option is chosen, like in Intellij-Rust and most other editors?

[matklad]

That would require 100% correct resolve to not be annoying. We'll get there one day.

[SomeoneToIgnore]

Is there any infrastructure in RA related to reexports? (pub use some_other_module)

I'm going through the flow suggested and currently at this step

using this hir type, we should construct a canonical path to this item from it's crate root (crate::foo::bar::Thing)

I have a Module struct as a hir type returned by the classify_name method, but the module path that we can build from that object may contain private modules that are reexported in different modules.
HashMap is a good example of this case.

[matklad]

No, there’s nothing for reexports yet. It might be ok to ignore them at first: adding them later shouldn’t be harder.

A simple rule might be to just check if an item is visible in an ancestor path with the same visibility.

[SomeoneToIgnore]

For this reason, the code for auto-import should live in the ra_ide_api crate. That is, for the time being, this should be the only assist that lives outside of ra_assist crate.

I've considered options, here are a few observations:

• In order to define an assist in the ra_ide crate, I need the following ra_assists API to be pub instead of pub(crate) as it is now:

use ra_assists::{
    assist_ctx::{ActionBuilder, Assist, AssistCtx},
    auto_import_text_edit, AssistId,
};

Additionally, I need a few fields and methods from AssistCtx (db, frange, add_assist_group, find_node_at_offset, covering_element) and ActionBuilder (text_edit_builder, label)

• We need to create the assist somehow, for that, I think an extra method in the ra_assists/lib.rs should be added, something like
  pub fn assist<'a, H>(db: &H, range: FileRange) -> ResolvedAssist

The issue here is to understand how to fit it into sort that assists method does.
Alternatively, we can add another parameter to the assists method.

• Some infra for writing an assist tests should be extracted also, I expect that might require quite a lot of imports either.

The only alternative I can imagine currently is to pack the functionality needed into some kind of a struct/closure/trait impl in ra_ide crate and pass it to the ra_assists in a method that will create us an action.

This way we do not need to change that many imports (we only need AssistCtx and its source_analyzer method to be public and this is also avoidable, I think), have less test issues (we still have to mock that struct/closure/trait impl, but no import craziness at least), but need to construct and pass that object into ra_assists conssidering the same issue from the previous approach:

The issue here is to understand how to fit it into sort that assists method does.
Alternatively, we can add another parameter to the assists method.

Here's a branch with an alternative way: https://github.com/rust-analyzer/rust-analyzer/compare/master...SomeoneToIgnore:auto-import?expand=1

Which way do you like more? Maybe there's more opportunities to make it proper?

[matklad]

I think its important to tackle "implement new feature" and "refactor to the good design" separately. Specifically, first we should add new stuff in maybe questionable ways (as long as those are relatively isolated), and then we should refactor. So, I'd go with the simplest possible way, which is probably just making a bunch of stuff public (and maybe even moving the assist registry itself to ra_ide).

[SomeoneToIgnore]

I think its important to tackle "implement new feature" and "refactor to the good design" separately.

Well, somehow I did not feel that approach worked well for me in #2716 hence asking upfront :)

Ok, I'll go with the current changes in my branch then, with a new trait in ra_assist and its implementation passed from ra_ide into assists method as a new parameter.
This way it's much easier to write the assist tests IMO.

[matklad]

#2716 failed the "relatively isolated" rule, as the initial formed touched every assist and broke the public-ish LSP API. But yeah, it's also true that I, as a maintainer, sometimes try too hard to push the upfront cleanup :(

[ivnsch]

Auto import here doesn't include importing automatically with text autocompletion or e.g. shortcut to auto import all missing imports in the file, right?

[Veykril]

Auto import here is meant as the explicit assist to import an unresolved item via user action, what you refer to sounds more like the auto import completion #1062