Initial auto import action implementation #2887

SomeoneToIgnore · 2020-01-21T12:24:03Z

Closes #2180

Adds an auto import action implementation.

This implementation is not ideal and has a few limitations:

The import search functionality should be moved into a separate crate accessible from ra_assists.
This requires a lot of changes and a preliminary design.
Currently the functionality is provided as a trait impl, more on that here: Auto import #2180 (comment)
Due to the design desicion from the previous item, no doctests are run for the new aciton (look for a new FIXME in the PR)
For the same reason, I have to create the mock trait implementaion to test the assist
Ideally, I think we should have this feature as a diagnostics (that detects an absense of an import) that has a corresponding quickfix action that gets evaluated on demand.
Curretly we perform the import search every time we resolve the import which looks suboptimal.
This requires classify_name_ref to be moved from ra_ide, so not done currently.

A few improvements to the imports mechanism to be considered later:

Constants like ra_syntax::SyntaxKind::NAME are not imported, because they are not present in the database
Method usages are not imported, they are found in the database, but find_use_path does not return any import paths for them
Some import paths returned by the find_use_path method end up in core:: or alloc:: instead of std:, for example: core::fmt::Debug instead of std::fmt::Debug.
This is not an error techically, but still looks weird.
No detection of cases where a trait should be imported in order to be able to call a method
Improve auto_import_text_edit functionality: refactor it and move away from the place it is now, add better logic for merging the new import with already existing imports

crates/ra_ide/src/imports_locator.rs

flodiebold

Method usages are not imported, they are found in the database, but find_use_path does not return any import paths for them

You can't import methods, so that seems fine?

Some import paths returned by the find_use_path method end up in core:: or alloc:: instead of std:, for example: core::fmt::Debug instead of std::fmt::Debug.
This is not an error techically, but still looks weird.

I think the main problem here is that we unconditionally add core and alloc to the extern prelude; I think rustc only allows using them if you use #[no_std]? Otherwise, we might need special handling for this in the path finder.

flodiebold · 2020-01-23T18:08:53Z

crates/ra_ide/src/imports_locator.rs

+            .filter_map(|import_candidate| self.get_name_definition(db, &import_candidate))
+            .filter_map(|name_definition_to_import| {
+                if let NameKind::Def(module_def) = name_definition_to_import.kind {
+                    module_with_name_to_import.find_use_path(db, module_def)


note that the use path may actually end in a different segment than the original name, if it's using a reexport, so we should probably filter that out, or change the actual path in the assist, or maybe later add a parameter to find_use_path that disregards such paths

I'm afraid I cannot imagine this myself, do you have any code snippet I can try out and fix it?

flodiebold · 2020-01-23T18:13:50Z

crates/ra_assists/src/lib.rs

+        &mut self,
+        name_to_import: InFile<&NameRef>,
+        module_with_name_to_import: Module,
+    ) -> Option<Vec<ModPath>>;


obviously it would be nicest if we didn't need this at all, but if we need it, maybe we can make the interface smaller? i.e. just directly wrapping the index lookup, a function that takes a Name and returns a vector of ModuleDefs, instead of also putting the path generation in there?

I think this should be a very temporary trait before we decide how to properly extract the functionality we need into a separate crate.
But I like the idea in general, although I have a few questions:

classify_name_ref requires InFile<&ast::NameRef>, classify_name requires InFile<&ast::Name>, so not sure how I can pass Name only there.
I'm also not sure how to get the Name in the first place, since for the case

fn main() { let zz = HasMap<|>::new(); }

there's no NAME around the coursor, only NAME_REF.
So it looks like I should require InFile<&ast::NameRef> as an input here.

The biggest issue here are the tests: since I have to keep them in the ra_assists, I have to create a mock trait implementation. I can return something as a valid ModPath there, but how can I create the ModuleDef in the ra_assists?
Moveover, I cannot get any random ModuleDef, I need only those that the find_use_path method will consider as impotable.
Is there a way to get one with those properties?

If not, maybe we should focus on the code extraction instead to remove these hacks.

I think classify_name_ref should be called outside that function (I just now realize that that's where we check whether the name resolves); that's part of making it smaller. classify_name gets called on the import candidate, not the name we're searching for. I actually meant a hir::Name, not an ast::Name, though probably a String would be just fine (it's getting converted to one anyway). Conceptually, since this is just an index lookup, it shouldn't need a file location or AST node.

For tests, I think we could write a small implementation that just does a linear search through all defs in the crate by name? The advantage for tests would be that we'd be testing a bigger part of the functionality.

As for classify_name_ref, I think you actually don't need 90% of what it does; since you already are on an ast::Path, analyzer.resolve_path should be enough.

Have implemented all the suggestions, the interface looks way better now, thanks!

One thing that bothers me though: I had to reexport a ModuleDefId in since I could not find a better way to extract the LocalModuleId that I needed to implement the linear search through all defs in the crate for the test impl ImportsLocator for TestImportsLocator.

crates/ra_assists/src/assists/auto_import.rs

crates/ra_assists/src/lib.rs

SomeoneToIgnore · 2020-01-23T19:09:51Z

You can't import methods, so that seems fine?

My bad, I've meant "functions" of course.
For instance, in this PR, in auto_import.rs there's a auto_import_text_edit function imported, if you remove the import and try to call the assist, you get nothing.

If you put enough debug prints in imports_locator.rs, you can notice that we find the right item with the symbol_index::world_symbols method, but fail to get anything out of the find_use_path method later.

flodiebold · 2020-01-23T19:18:07Z

Ah, the problem there is this FIXME:
https://github.com/rust-analyzer/rust-analyzer/blob/8a4c248c48ad7bb9ad556717ee013129c190dbfa/crates/ra_hir/src/code_model.rs#L240
We would need to detect the namespace in which we actually want to import (either from the def we're trying to import, or even better from the location of the name), and choose the types / values / macros namespace correspondingly.

SomeoneToIgnore · 2020-01-23T19:40:34Z

I think rustc only allows using them if you use #[no_std]?

It looks like we can use core without issues: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dcc7e06d43de6e6c3ba08f35e982357b

But when I try to import something from alloc, I get the compile time error despite the docs (https://doc.rust-lang.org/alloc/index.html) stating the following:

This library, like libcore, normally doesn’t need to be used directly since its contents are re-exported in the std crate. Crates that use the #![no_std] attribute however will typically not depend on std, so they’d use this crate instead.

bjorn3 · 2020-01-23T19:45:53Z

You need to explictely use extern crate alloc; in all cases. For core this is only when #![no_std] is not used.

SomeoneToIgnore · 2020-01-23T20:04:12Z

TIL. Thanks for the info :)
I start to doubt that I've seen the alloc import returned by this assist now, so let's consider core only now.

flodiebold · 2020-01-23T20:52:59Z

We do add the alloc dependency unconditionally currently, so it's completely possible.

SomeoneToIgnore · 2020-01-26T22:13:16Z

Can confirm now: Arc gets imported from alloc.

SomeoneToIgnore · 2020-01-26T23:20:34Z

Interesting, I've written a test that tries to import a struct that is available in the three modules under the same name:

PubStruct<|>

pub mod PubMod1 {
    pub struct PubStruct;
}
pub mod PubMod2 {
    pub struct PubStruct;
}
pub mod PubMod3 {
    pub struct PubStruct;
}

The test picks the firts result assist proposes and it looks like all three platforms had gotten a different import.
Looks a bit weird to me since nothing platform-specific is in the code.

matklad · 2020-01-27T11:31:35Z

It looks like we can use core without issues:

Whaaa, looks like we can use core as in <T: core::fmt::Debug>, but we can't use core as in use core. At least locally, the following fails to compile for me:

use core::fmt;

The following does work though:

trait T: core::fmt::Debug {}

I won't use playground to test these kinds of things, it might be doing something funny with sysroot to make extern crates magically available.

matklad

Finally took a look at this!

I really love how this looks like! I especialy like how all various components like indexes, import and name classification just magically work together. it's beautiful!

r=me with AsName re-export removed

matklad · 2020-01-27T11:59:30Z

crates/ra_hir/src/lib.rs

 };
 pub use hir_expand::{
-    name::Name, HirFileId, InFile, MacroCallId, MacroCallLoc, MacroDefId, MacroFile, Origin,
+    name::{AsName, Name},


AsName should be private to hir. I think we basically go ast::NameRef -> hir::Name -> String anyway, so we should be able to short-circuit it and go directly to -> String.

Indeed, that's a brain fart, .syntax().to_string() provides the same output and should be used instead, thanks.

On a related topic: is it safe to reexport hir_def::ModuleDefId? I have to use it to implement the mock trait test implementation suggested here: #2887 (comment)

It is not, but I guess we can live with it for test, and should fix once we do that ide_db thing. But it is a good idea to add a FIXME there.

Sure, will add.

Maybe there's a better way to do the things, I'm still realtively bad with the APIs the codebase provides.
I use it here: https://github.com/rust-analyzer/rust-analyzer/pull/2887/files#diff-001b3b1412c33fee802c22b0f848dcd7R270 to continue searching the nested modules for the definitions.

I'm still realtively bad with the APIs the codebase provides.

It's rather that everything above ra_syntax and bellow ra_ide is a mess, API wise. I like our syntax tree API, and I love AnalysisHost / Analysis plain data based APIs, but everything in the middle is questionable...

to continue searching the nested modules for the definitions.

I feel we just need, shrug, a module visitor, because many tests repeat this recursive collection pattern

crates/ra_assists/src/assists/auto_import.rs

crates/ra_ide/src/imports_locator.rs

matklad · 2020-01-27T12:20:22Z

crates/ra_ide/src/imports_locator.rs

+        };
+        let lib_results = {
+            let mut query = Query::new(name_to_import.to_string());
+            query.libs();


One day we should fix this to look only at the direct deps

Right, another point to improve to not to forget about.
I'll gather all those and mention in the related ticket.

I think there's some more discrepancies between the current symbol index and what we'd ideally want for autoimport: We want macro-generated defs, and for any def reexported under a new name we'd also want to index it under that name. (Also, if we do restrict it to direct dependencies, we need to include anything that's reexported in one of them.)

Yeah, ideally we should include macro-generated stuff into the index. I think that one day™ we'll just throw-away the current syntax-based indexing and replace it with crate_def_map based one.

matklad · 2020-01-27T13:04:36Z

bors r+

⭐ 🚀 ⭐

2887: Initial auto import action implementation r=matklad a=SomeoneToIgnore Closes #2180 Adds an auto import action implementation. This implementation is not ideal and has a few limitations: * The import search functionality should be moved into a separate crate accessible from ra_assists. This requires a lot of changes and a preliminary design. Currently the functionality is provided as a trait impl, more on that here: #2180 (comment) * Due to the design desicion from the previous item, no doctests are run for the new aciton (look for a new FIXME in the PR) * For the same reason, I have to create the mock trait implementaion to test the assist * Ideally, I think we should have this feature as a diagnostics (that detects an absense of an import) that has a corresponding quickfix action that gets evaluated on demand. Curretly we perform the import search every time we resolve the import which looks suboptimal. This requires `classify_name_ref` to be moved from ra_ide, so not done currently. A few improvements to the imports mechanism to be considered later: * Constants like `ra_syntax::SyntaxKind::NAME` are not imported, because they are not present in the database * Method usages are not imported, they are found in the database, but `find_use_path` does not return any import paths for them * Some import paths returned by the `find_use_path` method end up in `core::` or `alloc::` instead of `std:`, for example: `core::fmt::Debug` instead of `std::fmt::Debug`. This is not an error techically, but still looks weird. * No detection of cases where a trait should be imported in order to be able to call a method * Improve `auto_import_text_edit` functionality: refactor it and move away from the place it is now, add better logic for merging the new import with already existing imports Co-authored-by: Kirill Bulatov <mail4score@gmail.com>

bors · 2020-01-27T13:12:51Z

Build succeeded

Rust (macos-latest)
Rust (ubuntu-latest)
Rust (windows-latest)
TypeScript

2978: Auto import functions r=flodiebold a=SomeoneToIgnore A follow up for #2887 (comment) I've used the logic for conversion from the https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir_def/src/item_scope.rs#L169 method. I'm not fond of how the conversion is implemented and for my needs, I can simply replace the `hir_def::item_scope::ItemInNs::Types(item.into())` with `hir_def::item_scope::ItemInNs::Values(item.into())` and it will work, so I can use this approach instead, if you find it a better one. Co-authored-by: Kirill Bulatov <mail4score@gmail.com>

SomeoneToIgnore commented Jan 21, 2020

View reviewed changes

crates/ra_ide/src/imports_locator.rs Outdated Show resolved Hide resolved

SomeoneToIgnore force-pushed the auto-import branch from e28307f to 396f352 Compare January 22, 2020 20:48

flodiebold reviewed Jan 23, 2020

View reviewed changes

SomeoneToIgnore added 5 commits January 27, 2020 00:16

Initial auto import action implementation

316795e

Remove unnecessary lifetime parameter

f572397

Raise the import search query cap

bef5cf0

Have a better trait interface

d0a782e

Adjust the tests

1a78991

SomeoneToIgnore force-pushed the auto-import branch from fec493c to 1a78991 Compare January 26, 2020 23:07

Enforce alphabetical import sorting

9a6b5c6

matklad reviewed Jan 27, 2020

View reviewed changes

Code review fixes

9be1ab7

bors bot merged commit 9be1ab7 into rust-lang:master Jan 27, 2020

SomeoneToIgnore deleted the auto-import branch January 27, 2020 13:14

dodomorandi mentioned this pull request Jan 27, 2020

Support for auto import autozimu/LanguageClient-neovim#963

Closed

SomeoneToIgnore mentioned this pull request Feb 1, 2020

Auto import functions #2978

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial auto import action implementation #2887

Initial auto import action implementation #2887

SomeoneToIgnore commented Jan 21, 2020 •

edited

Loading

flodiebold left a comment

flodiebold Jan 23, 2020

SomeoneToIgnore Jan 26, 2020

flodiebold Jan 23, 2020

SomeoneToIgnore Jan 23, 2020

flodiebold Jan 23, 2020

flodiebold Jan 23, 2020

SomeoneToIgnore Jan 26, 2020

SomeoneToIgnore commented Jan 23, 2020 •

edited

Loading

flodiebold commented Jan 23, 2020

SomeoneToIgnore commented Jan 23, 2020

bjorn3 commented Jan 23, 2020

SomeoneToIgnore commented Jan 23, 2020

flodiebold commented Jan 23, 2020

SomeoneToIgnore commented Jan 26, 2020

SomeoneToIgnore commented Jan 26, 2020 •

edited

Loading

matklad commented Jan 27, 2020

matklad left a comment

matklad Jan 27, 2020

SomeoneToIgnore Jan 27, 2020

matklad Jan 27, 2020

SomeoneToIgnore Jan 27, 2020

matklad Jan 27, 2020

matklad Jan 27, 2020

matklad Jan 27, 2020

SomeoneToIgnore Jan 27, 2020

flodiebold Jan 27, 2020

matklad Jan 27, 2020

matklad commented Jan 27, 2020

bors bot commented Jan 27, 2020

Initial auto import action implementation #2887

Initial auto import action implementation #2887

Conversation

SomeoneToIgnore commented Jan 21, 2020 • edited Loading

flodiebold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomeoneToIgnore commented Jan 23, 2020 • edited Loading

flodiebold commented Jan 23, 2020

SomeoneToIgnore commented Jan 23, 2020

bjorn3 commented Jan 23, 2020

SomeoneToIgnore commented Jan 23, 2020

flodiebold commented Jan 23, 2020

SomeoneToIgnore commented Jan 26, 2020

SomeoneToIgnore commented Jan 26, 2020 • edited Loading

matklad commented Jan 27, 2020

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Jan 27, 2020

bors bot commented Jan 27, 2020

Build succeeded

SomeoneToIgnore commented Jan 21, 2020 •

edited

Loading

SomeoneToIgnore commented Jan 23, 2020 •

edited

Loading

SomeoneToIgnore commented Jan 26, 2020 •

edited

Loading