feat: Implement extern crate completion #15374

jmintb · 2023-08-02T05:20:33Z

Hi, this is a draft PR for #13002.

I have basic completion working as well as a filter for existing extern crate imports in the same file. This is based on the tests, I have not actually tried this in an editor. Before going further I think this is a good point to stop and get feedback on the
structure and approach I have taken so far. Let me know what you think :)

I will make sure to add more tests, rebase commits and align with the code style guidelines before submitting a final version.

A few specific questions :

Is there a better way to check for matching suggestions? right now I just test if an extern crate name starts with the current
user input.
Am I creating the CompletionItem correctly? I noticed that use_.rs invokes a builder where as I do not.
When checking for existing extern crate imports the current implementation only looks at the current source file, is that sufficient?

Veykril · 2023-08-02T07:29:03Z

crates/hir-def/src/resolver.rs

+    pub fn extern_crates_in_scope(&self) -> Vec<Name> {
+        self.module_scope.def_map.extern_prelude().map(|(name, _)| name.clone()).collect()


Should be no need to collect here and we are also interested in the module here

Suggested change

pub fn extern_crates_in_scope(&self) -> Vec<Name> {

self.module_scope.def_map.extern_prelude().map(|(name, _)| name.clone()).collect()

pub fn extern_crates_in_scope(&self) -> impl Iterator<Item = (Name, ModuleId)> {

self.module_scope.def_map.extern_prelude().map(|(name, mid)| (name.clone(), mid))

Veykril · 2023-08-02T07:31:23Z

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

+
+use crate::{context::CompletionContext, CompletionItem, CompletionItemKind};
+
+use super::Completions;


nit, we prefer not to use super imports, I assume this was copied from another file (we have a few rogue super imports lying around still)

Veykril · 2023-08-02T07:34:37Z

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

+    let current_txt =
+        name_ref.as_ref().map(|name_ref| name_ref.to_string()).unwrap_or(String::new());


We don't need to filter the names ourselves, that's something the clients (text editors) are doing for us.

Ah, that makes a lot of sense.

Veykril · 2023-08-02T07:41:39Z

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

+    let imported_extern_crates: FxHashSet<String> = ctx
+        .token
+        .parent_ancestors()
+        .find_map(ast::SourceFile::cast)
+        .map(|src_file| {
+            src_file
+                .syntax()
+                .children()
+                .into_iter()
+                .filter_map(ast::ExternCrate::cast)
+                .filter_map(|node| node.name_ref())
+                .map(|name| name.to_string())
+                .collect()
+        })
+        .unwrap_or_default();


Going via syntax is pretty messy and error prone (given macros could create these as well), but we can't go the HIR route here right now as we don't record extern_crate decls in the HIR yet. Can you put a FIXME above this something like FIXME: Collect extern crate names via HIR when possible?

Actually with #15377 we can make this work on the HIR

Nice it felt like a hack haha.

I made a quick implementation using the resolver to perform external crate decl queries here. Am I on the right track or should I be finding the information somewhere else? I suspect a bit of adjustment somewhere might remove the need for querying extern crate decl data 🤔

Yup, using the query is the correct approach

Veykril · 2023-08-02T07:46:00Z

crates/hir/src/semantics.rs

+    pub fn extern_crates(&self) -> Vec<Name> {
+        self.resolver.extern_crates_in_scope()
+    }


Similar stuff here, but map from ModuleId to Module

Suggested change

pub fn extern_crates(&self) -> Vec<Name> {

self.resolver.extern_crates_in_scope()

}

pub fn extern_crates(&self) -> impl Iterator<Item = (Name, Module)> {

self.resolver.extern_crates_in_scope().map(...)

}

Veykril · 2023-08-02T07:50:47Z

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

+        CompletionItem::new(
+            CompletionItemKind::SymbolKind(SymbolKind::Module),
+            ctx.source_range(),
+            name.to_smol_str(),
+        )
+        .add_to(acc, ctx.db);


With the scope.extern_crates() changes above giving us the module as well we can add the module docs via .set_documentation(module.docs(db)

Veykril · 2023-08-02T07:52:27Z

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

+#[cfg(test)]
+mod test {
+    use crate::tests::completion_list_no_kw;
+
+    #[test]
+    fn can_complete_extern_crate() {
+        let case = r#"
+//- /lib.rs crate:other_crate_a
+// nothing here
+//- /other_crate_b.rs crate:other_crate_b
+pub mod good_mod{}
+//- /lib.rs crate:crate_c
+// nothing here
+//- /lib.rs crate:lib deps:other_crate_a,other_crate_b,crate_c extern-prelude:other_crate_a,crate_c
+extern crate crate_c;
+extern crate oth$0
+mod other_mod {}
+"#;
+
+        let completion_list = completion_list_no_kw(case);
+
+        assert_eq!("md other_crate_a\n".to_string(), completion_list);
+    }
+
+    #[test]
+    fn will_not_complete_existing_import() {
+        let case = r#"
+//- /lib.rs crate:other_crate_a
+// nothing here
+//- /lib.rs crate:crate_c
+// nothing here
+//- /lib.rs crate:other_crate_b
+//
+//- /lib.rs crate:lib deps:other_crate_a,other_crate_b,crate_c extern-prelude:other_crate_a,other_crate_b,crate_c
+extern crate other_crate_b;
+extern crate oth$0
+mod other_mod {}
+"#;
+
+        let completion_list = completion_list_no_kw(case);
+
+        assert_eq!("md other_crate_a\n".to_string(), completion_list);
+    }
+}


Let's create a new module in the ide-completion/src/tests folder instead, you can also check how a completion test is structured for this usually by taking a look at the use_tree module in there for example.

Veykril · 2023-08-08T13:11:17Z

Thanks!
@bors r+

bors · 2023-08-08T13:11:19Z

📌 Commit 4764447 has been approved by Veykril

It is now in the queue for this repository.

jmintb · 2023-08-08T13:32:00Z

Thanks! @bors r+

I wasn't quite finished but I guess it was good enough haha. Thanks for the insightful feedback!

Veykril · 2023-08-08T13:52:53Z

Oh were you not? It looked fine to me 😅

bors · 2023-08-08T13:58:18Z

⌛ Testing commit 4764447 with merge b4759fd...

feat: Implement extern crate completion Hi, this is a draft PR for #13002. I have basic completion working as well as a filter for existing extern crate imports in the same file. This is based on the tests, I have not actually tried this in an editor. Before going further I think this is a good point to stop and get feedback on the structure and approach I have taken so far. Let me know what you think :) I will make sure to add more tests, rebase commits and align with the code style guidelines before submitting a final version. A few specific questions : 1. Is there a better way to check for matching suggestions? right now I just test if an extern crate name starts with the current user input. 2. Am I creating the `CompletionItem` correctly? I noticed that `use_.rs` invokes a builder where as I do not. 3. When checking for existing extern crate imports the current implementation only looks at the current source file, is that sufficient?

jmintb · 2023-08-08T13:58:43Z

Oh were you not? It looked fine to me 😅

I had not addressed this:

Let's create a new module in the ide-completion/src/tests folder instead, you can also check how a completion test is structured for this usually by taking a look at the use_tree module in there for example.

But if you think it looks fine then that is good enough for me 👍

bors · 2023-08-08T14:13:19Z

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

bors · 2023-08-08T14:13:23Z

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

jmintb · 2023-08-08T14:18:27Z

Should be fast forward-able now.

Veykril · 2023-08-15T06:22:00Z

@bors r+

bors · 2023-08-15T06:22:02Z

📌 Commit d070aea has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-08-15T06:22:09Z

⌛ Testing commit d070aea with merge ebbcf58...

feat: Implement extern crate completion Hi, this is a draft PR for #13002. I have basic completion working as well as a filter for existing extern crate imports in the same file. This is based on the tests, I have not actually tried this in an editor. Before going further I think this is a good point to stop and get feedback on the structure and approach I have taken so far. Let me know what you think :) I will make sure to add more tests, rebase commits and align with the code style guidelines before submitting a final version. A few specific questions : 1. Is there a better way to check for matching suggestions? right now I just test if an extern crate name starts with the current user input. 2. Am I creating the `CompletionItem` correctly? I noticed that `use_.rs` invokes a builder where as I do not. 3. When checking for existing extern crate imports the current implementation only looks at the current source file, is that sufficient?

bors · 2023-08-15T06:25:10Z

💔 Test failed - checks-actions

Veykril · 2023-08-15T09:42:28Z

Ah, there is a semantic conflict now when merging onto master. Could you rebase?

@bors delegate+

bors · 2023-08-15T09:42:30Z

✌️ @jmintb, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

jmintb · 2023-08-21T07:38:24Z

@bors r=Veykril

bors · 2023-08-21T07:38:25Z

📌 Commit 37e0e8a has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-08-21T07:38:31Z

⌛ Testing commit 37e0e8a with merge a3892f0...

bors · 2023-08-21T07:53:04Z

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

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

jmintb force-pushed the extern_crate branch 2 times, most recently from 38f2010 to b56c5a2 Compare August 2, 2023 07:16

Veykril reviewed Aug 2, 2023

View reviewed changes

jmintb force-pushed the extern_crate branch 2 times, most recently from e4ace6e to 4764447 Compare August 7, 2023 07:20

Veykril approved these changes Aug 8, 2023

View reviewed changes

Veykril changed the title ~~Draft: Implement extern crate completion~~ feat: Implement extern crate completion Aug 8, 2023

jmintb force-pushed the extern_crate branch from 4764447 to d070aea Compare August 8, 2023 14:18

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 Aug 15, 2023

Implement extern crate completion

37e0e8a

jmintb force-pushed the extern_crate branch from d070aea to 37e0e8a Compare August 20, 2023 16:38

bors merged commit a3892f0 into rust-lang:master Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement extern crate completion #15374

feat: Implement extern crate completion #15374

jmintb commented Aug 2, 2023

Veykril Aug 2, 2023

Veykril Aug 2, 2023

Veykril Aug 2, 2023

jmintb Aug 4, 2023

Veykril Aug 2, 2023

Veykril Aug 2, 2023

jmintb Aug 4, 2023

Veykril Aug 4, 2023

Veykril Aug 2, 2023

Veykril Aug 2, 2023

Veykril Aug 2, 2023

Veykril commented Aug 8, 2023

bors commented Aug 8, 2023

jmintb commented Aug 8, 2023

Veykril commented Aug 8, 2023

bors commented Aug 8, 2023

jmintb commented Aug 8, 2023

bors commented Aug 8, 2023

bors commented Aug 8, 2023

jmintb commented Aug 8, 2023

Veykril commented Aug 15, 2023

bors commented Aug 15, 2023

bors commented Aug 15, 2023

bors commented Aug 15, 2023

Veykril commented Aug 15, 2023

bors commented Aug 15, 2023

jmintb commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023

		pub fn extern_crates_in_scope(&self) -> Vec<Name> {
		self.module_scope.def_map.extern_prelude().map(\|(name, _)\| name.clone()).collect()


		use crate::{context::CompletionContext, CompletionItem, CompletionItemKind};

		use super::Completions;

		let current_txt =
		name_ref.as_ref().map(\|name_ref\| name_ref.to_string()).unwrap_or(String::new());

feat: Implement extern crate completion #15374

feat: Implement extern crate completion #15374

Conversation

jmintb commented Aug 2, 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

Veykril commented Aug 8, 2023

bors commented Aug 8, 2023

jmintb commented Aug 8, 2023

Veykril commented Aug 8, 2023

bors commented Aug 8, 2023

jmintb commented Aug 8, 2023

bors commented Aug 8, 2023

bors commented Aug 8, 2023

jmintb commented Aug 8, 2023

Veykril commented Aug 15, 2023

bors commented Aug 15, 2023

bors commented Aug 15, 2023

bors commented Aug 15, 2023

Veykril commented Aug 15, 2023

bors commented Aug 15, 2023

jmintb commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023

bors commented Aug 21, 2023