Support trait auto import #3120

SomeoneToIgnore · 2020-02-12T13:22:43Z

Unfortunately, for real cases it does not work as spectacular as in the tests.

The main reason for that is type inference:

The callee type here is unknown for many cases
The trait solution here is also often ambiguous

That results in trait candidates being rejected, and some real cases not supported.
Example: no imports for String::from_str("test")

matklad · 2020-02-12T14:33:40Z

Also not that I had to expose AssocContainerId in hir since I need to get traits from the

Yeah, this is unfortunate. I feel that we should more actively try to cut a nice API at the ra_hir boundary. Would something like this #3122 work for you?

SomeoneToIgnore · 2020-02-12T14:36:11Z

Maybe as_assoc_item will do the trick, have to check.

SomeoneToIgnore · 2020-02-12T14:56:15Z

Had tried, almost works for me, but needs one more tweak, commented the PR.

SomeoneToIgnore · 2020-02-12T15:20:10Z

No more AssocContainerId exposition 👍

matklad · 2020-02-12T15:27:13Z

crates/ra_hir/src/source_analyzer.rs

+        match self.resolve_path(db, &path_to_method)? {
+            PathResolution::Def(ModuleDef::Adt(adt)) => {
+                let ty = adt.ty(db);
+                iterate_method_candidates(


Why do we need to do this here?

I would expect that we reoslve assoc functions during type inferece, and record the result in the appropriate map in InferenceResult. Perhaps assoc_resolutions are not properly filled in?

That actually comes from a very confusing (to me) SourceAnalyzer behavior: when constructed for a module (as it had been done before in auto_import assist code: https://github.com/rust-analyzer/rust-analyzer/pull/3120/files#diff-9f55419f762ae3317cf277aab2d8e37aL36), it does not resolve anything related to the functions' types.

If constructed for a very small path under the coursor (as it is done now), it magically starts to resolve those types.

I've removed the module syntax search completely since it was redundant, but did not realize that this code is not needed anymore.

This behavior depends on what the source_binder::find_container function returns and I find it a bit strange, I would expect to be able to resolve all types, including the function-related ones when the whole module code is given to the analyzer.

Yeah, this aspect of SourceAnalyzer is kind of a mess, you need to use the right source analyzer. We really should do something like lifting all sa methods to sb, such that they just work.

Not sure how to make that ergonomic though, because the caller would have to pass in InFile<ast::Foo> things instead of just ast::Foo.

Yeah, this is something that has annoyed me as well. It would be nice if working with InFile<>s would be more ergonomic in general, and then we could maybe put more API directly on them 🤔

Thanks for the explanation.

bwaaaa, why dealing with macros in IDE is so annoying :-(

If I just go with the most obvious API, I get this for, for example, goto type definition:

That's way to many layers of Functors :c

Isn't and_then a monad?

We could at least e.g. add a cast<A: AstNode>(&self: InFile<SyntaxNode>) -> Option<InFile<A>>...

Oh, right, we do need full monadic power here :-(

And yeah, for full generality we need ReaderT monad transformer ;(

I wish we could do a simple Roslyn style api: https://github.com/dotnet/roslyn/wiki/Getting-Started-C%23-Semantic-Analysis

crates/ra_assists/src/handlers/auto_import.rs

flodiebold · 2020-02-12T20:45:03Z

I checked String::from_str and it seems that there's an inherent from_str method on String that is defined for cfg(test), I guess we pick that up (wrongly) and so there seems to be no need for an import.

SomeoneToIgnore · 2020-02-12T20:49:28Z

Thanks for checking. Funny, intellij-rust does not provide an import for that function also.

matklad · 2020-02-13T10:53:42Z

there's an inherent from_str method on String that is defined for cfg(test)

Lol, the person who wrote this sits in front of me :D

I wonder if that 's a problem for us though -- we should handle #[cfg(test)]?

flodiebold · 2020-02-13T10:58:02Z

we should handle #[cfg(test)]?

Yeah, we should. I assume we don't do it correctly for libraries? (I.e. we want cfg(test) to be active for crates in the workspace, but not for libraries.) I haven't looked into it, but the call clearly resolves to that.

matklad · 2020-02-13T10:55:56Z

crates/ra_assists/src/handlers/auto_import.rs

    if proposed_imports.is_empty() {
        return None;
    }

-    let mut group = ctx.add_assist_group(format!("Import {}", name_to_import));
+    let assist_group_name = if proposed_imports.len() == 1 {


i wonder if we should just automatically flatten singleton grups at the level of AssistCtx or even the code_actions handler?

We could, we can do all those things on finish() I think.

crates/ra_assists/src/handlers/auto_import.rs

matklad · 2020-02-13T11:12:02Z

Yeah, we should. I assume we don't do it correctly for libraries?

I though so as well, but unfortunately the fix is not this simple. We just don't take cfg into account when loweting impls here.

We only use cfgs in CrateDefMap, which I think uses a different, non-ast based infra

matklad · 2020-02-13T11:37:16Z

bors r+

3120: Support trait auto import r=matklad a=SomeoneToIgnore Unfortunately, for real cases it does not work as spectacular as in the tests. The main reason for that is type inference: * The callee type [here](https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir_ty/src/method_resolution.rs#L369) is unknown for many cases * The trait solution [here](https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir_ty/src/method_resolution.rs#L399) is also often ambiguous That results in trait candidates being rejected, and some real cases not supported. Example: no imports for `String::from_str("test")` Co-authored-by: Kirill Bulatov <[email protected]>

bors · 2020-02-13T11:44:22Z

Build succeeded

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

SomeoneToIgnore added 9 commits February 12, 2020 17:18

Resolve methods and functions better

24ab3e8

Add method tests

2b9b16c

Refactor path for imports extraction

9b6db7b

Trait location draft

8f959f2

Support trait method call autoimports

d5c3808

Add profiling

24f7028

Adjust the assist group name

07058cb

Refactor the code

acf5f43

Fix post-rebase issues

afc1d18

SomeoneToIgnore force-pushed the trait-auto-import branch from d3ef529 to afc1d18 Compare February 12, 2020 15:18

matklad reviewed Feb 12, 2020

View reviewed changes

Revert source_analyzer changes

f65daf2

flodiebold reviewed Feb 12, 2020

View reviewed changes

crates/ra_assists/src/handlers/auto_import.rs Outdated Show resolved Hide resolved

Also consider associated constants

3ccf8b7

flodiebold reviewed Feb 12, 2020

View reviewed changes

Support associated consts

e008b08

Do not collect all traits

e4f4cd7

matklad reviewed Feb 13, 2020

View reviewed changes

bors bot merged commit e4f4cd7 into rust-lang:master Feb 13, 2020

SomeoneToIgnore deleted the trait-auto-import branch February 13, 2020 11:44

matklad mentioned this pull request Feb 13, 2020

Design: syntax trees vs identity #3129

Closed

SomeoneToIgnore mentioned this pull request Jun 29, 2020

Trait auto import not working for some cases (e.g. tokio AsyncWriteExt in async method) #5115

Closed

Support trait auto import #3120

Support trait auto import #3120

Uh oh!

Conversation

SomeoneToIgnore commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Feb 12, 2020

Uh oh!

SomeoneToIgnore commented Feb 12, 2020

Uh oh!

SomeoneToIgnore commented Feb 12, 2020

Uh oh!

SomeoneToIgnore commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomeoneToIgnore Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomeoneToIgnore Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flodiebold commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SomeoneToIgnore commented Feb 12, 2020

Uh oh!

matklad commented Feb 13, 2020

Uh oh!

flodiebold commented Feb 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matklad commented Feb 13, 2020

Uh oh!

matklad commented Feb 13, 2020

Uh oh!

bors bot commented Feb 13, 2020

Build succeeded

Uh oh!

Uh oh!

SomeoneToIgnore commented Feb 12, 2020 •

edited

Loading

SomeoneToIgnore Feb 12, 2020 •

edited

Loading

SomeoneToIgnore Feb 12, 2020 •

edited

Loading

flodiebold commented Feb 12, 2020 •

edited

Loading