feat: Prioritize import suggestions based on the expected type #15627

jmintb · 2023-09-18T05:58:47Z

Hi, this is a draft PR to solve #15384. Adt types work and now I have a few questions :)

What other types make sense in this context? Looking at ModuleDef I am thinking everything except Modules.
Is there an existing way of converting between ModeuleDef and hir::Type in the rustanalyzer code base?
Does this approach seem sound to you?

Ups: Upon writing this I just realised that the enum test is invalided as there are no enum variants and this no variant is passed as a function argument.

Veykril · 2023-09-20T13:55:47Z

crates/ide-completion/src/completions/flyimport.rs

    Some(assets_for_path)
 }

+fn is_expected_type(ctx: &CompletionContext<'_>, located_import: &LocatedImport) -> bool {


This is the wrong way to approach it, our CompletionItem structure has a type_match field which is what we need to populate here. According to that, we are later going to sort the completions already. The render_resolution_with_import(RenderContext::new(ctx), path_ctx, import) call already does this for locals being completed in render_resolution_path. Now, the reason this therefore doesn't work here in flyimport already is that we are completin importable things which are never locals! See
https://github.com/Veykril/rust-analyzer/blob/994df3d6a31d39f11600f30a6df0b744b13937c1/crates/ide-completion/src/render.rs#L337-L351

We should instead generalize that part I believe, making it work for more scopedefs, mainly
ScopeDef::ImplSelfType, ScopeDef::AdtSelfType and reklevant ScopeDef::ModuleDef. For ModuleDef, all 3 Adts, Function, Variant, Const, Static and BuiltinType are relevant. (Some of those might not have a function allowing you to easily turn them into a hir version yet. It's fine to skip them in that case (or (ask about how) to implement them).

Awesome that makes a lot more sense haha.

I used UPDATE_EXPECT=1 to see what the new code produces. It looks more or less correct to me. I am not sure why there are two completion items though. Is it because we can choose to either import or use the fully qualified name? Also should type_match: Some(CouldUnify)actually be type_match: Some(Exact)? as this is the exact type we are expecting.

// crates/ide-completion/src/render.rs #[test] fn sort_suggested_structs_by_types() { check( r#" //- /lib.rs crate:dep pub mod test_mod_b { pub struct Struct {} } //- /main.rs crate:main deps:dep fn test(input: dep::test_mod_b::Struct) { } fn main() { test(Struct$0); } "#, SymbolKind::Struct, expect![[r#" [ CompletionItem { label: "Struct (use dep::test_mod_b::Struct)", source_range: 67..73, delete: 67..73, insert: "Struct", kind: SymbolKind( Struct, ), lookup: "Struct", detail: "Struct", relevance: CompletionRelevance { exact_name_match: false, type_match: Some( CouldUnify, ), is_local: false, is_item_from_trait: false, is_name_already_imported: false, requires_import: false, is_op_method: false, is_private_editable: false, postfix_match: None, is_definite: false, }, }, CompletionItem { label: "dep::test_mod_b::Struct {…}", source_range: 67..73, delete: 67..73, insert: "dep::test_mod_b::Struct { }$0", kind: SymbolKind( Struct, ), lookup: "test_mod_b::Struct{}", detail: "dep::test_mod_b::Struct { }", relevance: CompletionRelevance { exact_name_match: false, type_match: Some( CouldUnify, ), is_local: false, is_item_from_trait: false, is_name_already_imported: false, requires_import: false, is_op_method: false, is_private_editable: false, postfix_match: None, is_definite: false, }, trigger_call_info: true, }, ] "#]], ); }

The qualified completion is from another code path, you can ignore that one. Exact doesn't work in this case, since imported things might have generics that we aren't filling out when constructing their type (since we don't know the concrete generics for them).

jmintb · 2023-09-23T06:23:37Z

crates/ide-completion/src/render.rs

@@ -349,6 +349,23 @@ fn render_resolution_path(

        path_ref_match(completion, path_ctx, &ty, &mut item);
    };
+
+    if let ScopeDef::ModuleDef(ModuleDef::Adt(adt)) = resolution {


@Veykril is this the sort of thing you had in mind?

Yes, that should be what I had in mind

jmintb · 2023-10-06T18:03:17Z

crates/ide-completion/src/render.rs

        if !ty.is_unknown() {
            item.detail(ty.display(db).to_string());
        }

        item.set_relevance(CompletionRelevance {
            type_match: compute_type_match(completion, &ty),
            exact_name_match: compute_exact_name_match(completion, &name),
-            is_local: true,
+            is_local,


Should the rest of CompletionRelevance be configured here? I assume options like is_name_already_imported are important.

is_name_already_imported is only used by use item completions, so we can ignore this here as its being set somewhere else already (in this case its overwriting the relevance calculated here)

Also instead of passing is_local as a param to the closre we can put is_local: matches!(resolution, ScopeDef::Local(_)) here. That simplifies things a bit. (I noticed you also set it to true for ImplSelfType but that property is specifically meant for local variables, the doc comment on that isn't clear on that.)

We should set the requires_import field for it here as well if import_to_add (the param of the function we are in right now is Some). The rest we can ignore I think.

jmintb · 2023-10-06T18:04:40Z

crates/ide-completion/src/render.rs

@@ -595,6 +621,206 @@ mod tests {
        }
    }

+    #[test]
+    fn set_struct_type_completion_info() {


What do you think of the tests? I will add similar tests for the remaining cases as well if these look good.

Instead of doing mere check tests here, let's make use of check_relevance instead, as that is what we are mainly interested in checking here (those are used a bit further down in this module)

crates/ide-completion/src/render.rs

jmintb · 2023-11-04T15:48:53Z

crates/ide-completion/src/render/function.rs

    item.set_relevance(CompletionRelevance {
-        type_match: compute_type_match(completion, &ret_type),


Looks like functions were only being type matched based on the return type. I am not sure if there was a reason for this 🤔

This should be done conditionally, by return type if we have a call / complete call, otherwise by signature.

jmintb · 2023-11-04T15:49:58Z

I ended up going down a small rabbit hole getting the function type matching to work, but it should be good now.

Let me know what you think :)

Veykril · 2023-12-08T12:33:20Z

I went ahead and cleaned up the remaining things, I hope you don't mind, I wanna get our PR backlog down and I imagine you might not have the PR in your head at this point anymore.
Thanks a lot for the work on this!
@bors r+

bors · 2023-12-08T12:33:22Z

📌 Commit 1475848 has been approved by Veykril

It is now in the queue for this repository.

Veykril · 2023-12-08T12:34:35Z

I ended up going down a small rabbit hole getting the function type matching to work, but it should be good now.

I removed the signature match function again, as that is the wrong way to approach it (but you are right we need to do this ideally!) I'll open an issue on the matter and fill that out with details later. #16053

bors · 2023-12-08T12:39:26Z

⌛ Testing commit 1475848 with merge b03a0bd...

bors · 2023-12-08T12:49:57Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing b03a0bd to master...

jmintb · 2023-12-08T13:15:25Z

Awesome thanks :)

lnicola · 2023-12-11T09:06:32Z

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2023

jmintb force-pushed the sort_imports branch from 89bc4fe to 5e630fb Compare September 19, 2023 04:42

Veykril reviewed Sep 20, 2023

View reviewed changes

Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 20, 2023

jmintb commented Sep 23, 2023

View reviewed changes

jmintb force-pushed the sort_imports branch from fe48c41 to bf12ce1 Compare October 6, 2023 17:52

jmintb commented Oct 6, 2023

View reviewed changes

crates/ide-completion/src/render.rs Show resolved Hide resolved

Veykril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 9, 2023

jmintb force-pushed the sort_imports branch 5 times, most recently from 636fe55 to b8b1f2c Compare November 4, 2023 15:32

jmintb commented Nov 4, 2023

View reviewed changes

crates/ide-completion/src/render.rs Outdated Show resolved Hide resolved

jmintb commented Nov 4, 2023

View reviewed changes

crates/ide-completion/src/render.rs Outdated Show resolved Hide resolved

jmintb commented Nov 4, 2023

View reviewed changes

jmintb force-pushed the sort_imports branch from b8b1f2c to a216ef9 Compare November 4, 2023 16:02

jmintb added 2 commits December 8, 2023 12:37

WIP - Sort suggested imports by type for data types

14a7a61

Implement function type matching

6abba17

Veykril force-pushed the sort_imports branch from a216ef9 to 96bba33 Compare December 8, 2023 12:19

Cleanup

1475848

Veykril force-pushed the sort_imports branch from 96bba33 to 1475848 Compare December 8, 2023 12:31

Veykril marked this pull request as ready for review December 8, 2023 12:31

Veykril changed the title ~~Draft: Prioritize import suggestions based on the expected type~~ feat: Prioritize import suggestions based on the expected type Dec 8, 2023

bors merged commit b03a0bd into rust-lang:master Dec 8, 2023
10 checks passed

jplatte mentioned this pull request Dec 11, 2023

Feature request: Improve import assist by considering types #10471

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Prioritize import suggestions based on the expected type #15627

feat: Prioritize import suggestions based on the expected type #15627

jmintb commented Sep 18, 2023

Veykril Sep 20, 2023

jmintb Sep 23, 2023

jmintb Sep 24, 2023

Veykril Sep 24, 2023

jmintb Sep 23, 2023

Veykril Sep 23, 2023

jmintb Oct 6, 2023

Veykril Oct 9, 2023

Veykril Oct 9, 2023

Veykril Oct 9, 2023

jmintb Oct 6, 2023

Veykril Oct 9, 2023

jmintb Nov 4, 2023

Veykril Dec 8, 2023

jmintb commented Nov 4, 2023

Veykril commented Dec 8, 2023

bors commented Dec 8, 2023

Veykril commented Dec 8, 2023 •

edited

bors commented Dec 8, 2023

bors commented Dec 8, 2023

jmintb commented Dec 8, 2023

lnicola commented Dec 11, 2023

		item.set_relevance(CompletionRelevance {
		type_match: compute_type_match(completion, &ret_type),

feat: Prioritize import suggestions based on the expected type #15627

feat: Prioritize import suggestions based on the expected type #15627

Conversation

jmintb commented Sep 18, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmintb commented Nov 4, 2023

Veykril commented Dec 8, 2023

bors commented Dec 8, 2023

Veykril commented Dec 8, 2023 • edited

bors commented Dec 8, 2023

bors commented Dec 8, 2023

jmintb commented Dec 8, 2023

lnicola commented Dec 11, 2023

Veykril commented Dec 8, 2023 •

edited