From 1fe6d768c6f3d040f7049cd3f0fd1bcc9e947bff Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sat, 17 Aug 2024 09:44:26 -0300 Subject: [PATCH 01/15] feat: user super:: in LSP autocompletion if possible --- .../src/requests/completion/auto_import.rs | 60 ++++++++++++++++--- tooling/lsp/src/requests/completion/tests.rs | 29 ++++++++- 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index c5710d30b5c..cf37efd3320 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -1,8 +1,10 @@ +use std::collections::BTreeMap; + use lsp_types::{Position, Range, TextEdit}; use noirc_frontend::{ ast::ItemVisibility, graph::{CrateId, Dependency}, - hir::def_map::ModuleId, + hir::def_map::{CrateDefMap, ModuleId}, macros_api::{ModuleDefId, NodeInterner}, node_interner::ReferenceId, }; @@ -16,6 +18,8 @@ use super::{ impl<'a> NodeFinder<'a> { pub(super) fn complete_auto_imports(&mut self, prefix: &str, requested_items: RequestedItems) { + let current_module_parent_id = get_parent_module_id(&self.def_maps, self.module_id); + for (name, entries) in self.interner.get_auto_import_names() { if !name_matches(name, prefix) { continue; @@ -39,13 +43,15 @@ impl<'a> NodeFinder<'a> { let module_full_path; if let ModuleDefId::ModuleId(module_id) = module_def_id { module_full_path = module_id_path( - module_id, + *module_id, &self.module_id, + current_module_parent_id, self.interner, self.dependencies, ); } else { - let Some(parent_module) = get_parent_module(self.interner, *module_def_id) + let Some(parent_module) = + get_parent_module(self.interner, self.def_maps, *module_def_id) else { continue; }; @@ -67,6 +73,7 @@ impl<'a> NodeFinder<'a> { module_full_path = module_id_path( parent_module, &self.module_id, + current_module_parent_id, self.interner, self.dependencies, ); @@ -111,9 +118,30 @@ impl<'a> NodeFinder<'a> { } } -fn get_parent_module(interner: &NodeInterner, module_def_id: ModuleDefId) -> Option<&ModuleId> { - let reference_id = module_def_id_to_reference_id(module_def_id); - interner.reference_module(reference_id) +fn get_parent_module( + interner: &NodeInterner, + def_maps: &BTreeMap, + module_def_id: ModuleDefId, +) -> Option { + if let ModuleDefId::ModuleId(module_id) = module_def_id { + get_parent_module_id(def_maps, module_id) + } else { + let reference_id = module_def_id_to_reference_id(module_def_id); + interner.reference_module(reference_id).map(|id| *id) + } +} + +fn get_parent_module_id( + def_maps: &BTreeMap, + module_id: ModuleId, +) -> Option { + let crate_def_map = &def_maps[&module_id.krate]; + let module_data = &crate_def_map.modules()[module_id.local_id.0]; + if let Some(parent) = module_data.parent { + Some(ModuleId { krate: module_id.krate, local_id: parent }) + } else { + None + } } fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { @@ -130,8 +158,9 @@ fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { /// Computes the path of `module_id` relative to `current_module_id`. /// If it's not relative, the full path is returned. fn module_id_path( - module_id: &ModuleId, + module_id: ModuleId, current_module_id: &ModuleId, + current_module_parent_id: Option, interner: &NodeInterner, dependencies: &[Dependency], ) -> String { @@ -139,7 +168,14 @@ fn module_id_path( let crate_id = module_id.krate; let crate_name = match crate_id { - CrateId::Root(_) => Some("crate".to_string()), + CrateId::Root(_) => { + if Some(module_id) == current_module_parent_id { + Some("super".to_string()) + } else { + Some("crate".to_string()) + } + } + CrateId::Crate(_) => dependencies .iter() .find(|dep| dep.crate_id == crate_id) @@ -155,7 +191,7 @@ fn module_id_path( false }; - let Some(module_attributes) = interner.try_module_attributes(module_id) else { + let Some(module_attributes) = interner.try_module_attributes(&module_id) else { return string; }; @@ -176,6 +212,12 @@ fn module_id_path( break; } + if current_module_parent_id == Some(*parent_module_id) { + string = string.strip_prefix("crate::").unwrap_or(&string).to_string(); + string.insert_str(0, "super::"); + break; + } + let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else { break; }; diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 825a7dd0081..aa91ef0c279 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1443,7 +1443,7 @@ mod completion_tests { assert_eq!( item.label_details, Some(CompletionItemLabelDetails { - detail: Some("(use crate::foo::bar::hello_world)".to_string()), + detail: Some("(use super::bar::hello_world)".to_string()), description: Some("fn()".to_string()) }) ); @@ -1455,7 +1455,7 @@ mod completion_tests { start: Position { line: 7, character: 8 }, end: Position { line: 7, character: 8 }, }, - new_text: "use crate::foo::bar::hello_world;\n\n ".to_string(), + new_text: "use super::bar::hello_world;\n\n ".to_string(), }]) ); } @@ -1556,4 +1556,29 @@ mod completion_tests { }) ); } + + #[test] + async fn test_auto_import_with_super() { + let src = r#" + pub fn barbaz() {} + + mod tests { + fn foo() { + barb>|< + } + } + "#; + let items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = &items[0]; + assert_eq!(item.label, "barbaz()"); + assert_eq!( + item.label_details, + Some(CompletionItemLabelDetails { + detail: Some("(use super::barbaz)".to_string()), + description: Some("fn()".to_string()) + }) + ); + } } From ae5440d80f99b2117849438311b44a1159410fae Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 18 Aug 2024 09:57:34 -0300 Subject: [PATCH 02/15] Simpler way to create module path --- .../src/requests/completion/auto_import.rs | 107 ++++++++---------- 1 file changed, 48 insertions(+), 59 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index cf37efd3320..076aa6b8fea 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -164,74 +164,63 @@ fn module_id_path( interner: &NodeInterner, dependencies: &[Dependency], ) -> String { - let mut string = String::new(); + let mut segments: Vec<&str> = Vec::new(); + let mut is_relative = false; - let crate_id = module_id.krate; - let crate_name = match crate_id { - CrateId::Root(_) => { - if Some(module_id) == current_module_parent_id { - Some("super".to_string()) - } else { - Some("crate".to_string()) - } - } + if let Some(module_attributes) = interner.try_module_attributes(&module_id) { + segments.push(&module_attributes.name); - CrateId::Crate(_) => dependencies - .iter() - .find(|dep| dep.crate_id == crate_id) - .map(|dep| format!("{}", dep.name)), - CrateId::Stdlib(_) => Some("std".to_string()), - CrateId::Dummy => None, - }; - - let wrote_crate = if let Some(crate_name) = crate_name { - string.push_str(&crate_name); - true - } else { - false - }; + let mut current_attributes = module_attributes; + loop { + let parent_module_id = + &ModuleId { krate: module_id.krate, local_id: current_attributes.parent }; - let Some(module_attributes) = interner.try_module_attributes(&module_id) else { - return string; - }; + if current_module_id == parent_module_id { + is_relative = true; + break; + } - if wrote_crate { - string.push_str("::"); - } + if current_module_parent_id == Some(*parent_module_id) { + segments.push("super"); + is_relative = true; + break; + } - let mut segments = Vec::new(); - let mut current_attributes = module_attributes; - loop { - let parent_module_id = - &ModuleId { krate: module_id.krate, local_id: current_attributes.parent }; - - // If the parent module is the current module we stop because we want a relative path to the module - if current_module_id == parent_module_id { - // When the path is relative we don't want the "crate::" prefix anymore - string = string.strip_prefix("crate::").unwrap_or(&string).to_string(); - break; - } + let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else { + break; + }; - if current_module_parent_id == Some(*parent_module_id) { - string = string.strip_prefix("crate::").unwrap_or(&string).to_string(); - string.insert_str(0, "super::"); - break; + segments.push(&parent_attributes.name); + current_attributes = parent_attributes; } - - let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else { - break; - }; - - segments.push(&parent_attributes.name); - current_attributes = parent_attributes; } - for segment in segments.iter().rev() { - string.push_str(segment); - string.push_str("::"); - } + let crate_id = module_id.krate; + let crate_name = if is_relative { + None + } else { + match crate_id { + CrateId::Root(_) => { + if Some(module_id) == current_module_parent_id { + Some("super".to_string()) + } else { + Some("crate".to_string()) + } + } + + CrateId::Crate(_) => dependencies + .iter() + .find(|dep| dep.crate_id == crate_id) + .map(|dep| format!("{}", dep.name)), + CrateId::Stdlib(_) => Some("std".to_string()), + CrateId::Dummy => None, + } + }; - string.push_str(&module_attributes.name); + if let Some(crate_name) = &crate_name { + segments.push(crate_name); + }; - string + segments.reverse(); + segments.join("::") } From 244e584eb0179aba9b19977f417008f58b8097d2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 18 Aug 2024 10:06:15 -0300 Subject: [PATCH 03/15] Also simplify `format_parent_module_from_module_id` in hover module --- tooling/lsp/src/requests/hover.rs | 52 +++++++++++++------------------ 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index aa97def8274..cd86cb0b00c 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -319,6 +319,21 @@ fn format_parent_module_from_module_id( args: &ProcessRequestCallbackArgs, string: &mut String, ) -> bool { + let mut segments: Vec<&str> = Vec::new(); + + if let Some(module_attributes) = args.interner.try_module_attributes(module) { + segments.push(&module_attributes.name); + + let mut current_attributes = module_attributes; + while let Some(parent_attributes) = args.interner.try_module_attributes(&ModuleId { + krate: module.krate, + local_id: current_attributes.parent, + }) { + segments.push(&parent_attributes.name); + current_attributes = parent_attributes; + } + } + let crate_id = module.krate; let crate_name = match crate_id { CrateId::Root(_) => Some(args.crate_name.clone()), @@ -331,41 +346,18 @@ fn format_parent_module_from_module_id( CrateId::Dummy => None, }; - let wrote_crate = if let Some(crate_name) = crate_name { - string.push_str(" "); - string.push_str(&crate_name); - true - } else { - false - }; - - let Some(module_attributes) = args.interner.try_module_attributes(module) else { - return wrote_crate; + if let Some(crate_name) = &crate_name { + segments.push(crate_name); }; - if wrote_crate { - string.push_str("::"); - } else { - string.push_str(" "); - } - - let mut segments = Vec::new(); - let mut current_attributes = module_attributes; - while let Some(parent_attributes) = args.interner.try_module_attributes(&ModuleId { - krate: module.krate, - local_id: current_attributes.parent, - }) { - segments.push(&parent_attributes.name); - current_attributes = parent_attributes; - } - - for segment in segments.iter().rev() { - string.push_str(segment); - string.push_str("::"); + if segments.is_empty() { + return false; } - string.push_str(&module_attributes.name); + segments.reverse(); + string.push_str(" "); + string.push_str(&segments.join("::")); true } From 10de6f9f0a6f00a8edb110015ae3e1eb43b2836b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 07:00:30 -0300 Subject: [PATCH 04/15] clippy --- tooling/lsp/src/requests/completion/auto_import.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 076aa6b8fea..dec4604dd12 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -18,7 +18,7 @@ use super::{ impl<'a> NodeFinder<'a> { pub(super) fn complete_auto_imports(&mut self, prefix: &str, requested_items: RequestedItems) { - let current_module_parent_id = get_parent_module_id(&self.def_maps, self.module_id); + let current_module_parent_id = get_parent_module_id(self.def_maps, self.module_id); for (name, entries) in self.interner.get_auto_import_names() { if !name_matches(name, prefix) { @@ -127,7 +127,7 @@ fn get_parent_module( get_parent_module_id(def_maps, module_id) } else { let reference_id = module_def_id_to_reference_id(module_def_id); - interner.reference_module(reference_id).map(|id| *id) + interner.reference_module(reference_id).copied() } } @@ -137,11 +137,7 @@ fn get_parent_module_id( ) -> Option { let crate_def_map = &def_maps[&module_id.krate]; let module_data = &crate_def_map.modules()[module_id.local_id.0]; - if let Some(parent) = module_data.parent { - Some(ModuleId { krate: module_id.krate, local_id: parent }) - } else { - None - } + module_data.parent.map(|parent| ModuleId { krate: module_id.krate, local_id: parent }) } fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { From 932c4a35dc23c3ece0f7a21a1c4088d2babc75c3 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 10:38:50 -0300 Subject: [PATCH 05/15] Add comment --- tooling/lsp/src/requests/completion/auto_import.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index dec4604dd12..94b9e5d2687 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -124,6 +124,8 @@ fn get_parent_module( module_def_id: ModuleDefId, ) -> Option { if let ModuleDefId::ModuleId(module_id) = module_def_id { + // ModuleDefId::ModuelId isn't tracked in interner's reference modules, + // so we find the parent in def maps. get_parent_module_id(def_maps, module_id) } else { let reference_id = module_def_id_to_reference_id(module_def_id); From 8ab1afa2b4ffc5dd99026d8b8895b87f20da569a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 12:07:01 -0300 Subject: [PATCH 06/15] Update tooling/lsp/src/requests/completion/tests.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- tooling/lsp/src/requests/completion/tests.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 2bf6f642e96..5b16701396f 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1577,11 +1577,11 @@ mod completion_tests { #[test] async fn test_auto_import_with_super() { let src = r#" - pub fn barbaz() {} + pub fn bar_baz() {} mod tests { fn foo() { - barb>|< + bar_b>|< } } "#; @@ -1589,11 +1589,11 @@ mod completion_tests { assert_eq!(items.len(), 1); let item = &items[0]; - assert_eq!(item.label, "barbaz()"); + assert_eq!(item.label, "bar_baz()"); assert_eq!( item.label_details, Some(CompletionItemLabelDetails { - detail: Some("(use super::barbaz)".to_string()), + detail: Some("(use super::bar_baz)".to_string()), description: Some("fn()".to_string()) }) ); From 3719b4dbc25a8e0aecc3f038ef98d4fc26b92e7e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 12:44:15 -0300 Subject: [PATCH 07/15] Fix a bug in name_matches --- tooling/lsp/src/requests/completion.rs | 29 ++++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 387006f7dae..71ef3849933 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -1185,9 +1185,19 @@ fn name_matches(name: &str, prefix: &str) -> bool { let mut last_index: i32 = -1; for prefix_part in prefix.split('_') { - if let Some(name_part_index) = - name_parts.iter().position(|name_part| name_part.starts_with(prefix_part)) + if let Some(mut name_part_index) = name_parts + .iter() + .skip( + // Look past parts we already matched + if last_index >= 0 { last_index as usize + 1 } else { 0 }, + ) + .position(|name_part| name_part.starts_with(prefix_part)) { + if last_index >= 0 { + // Need to adjust the index if we skipped some segments + name_part_index += last_index as usize + 1; + } + if last_index >= name_part_index as i32 { return false; } @@ -1220,12 +1230,13 @@ mod completion_name_matches_tests { #[test] fn test_name_matches() { - assert!(name_matches("foo", "foo")); - assert!(name_matches("foo_bar", "bar")); - assert!(name_matches("FooBar", "foo")); - assert!(name_matches("FooBar", "bar")); - assert!(name_matches("FooBar", "foo_bar")); - - assert!(!name_matches("foo_bar", "o_b")); + // assert!(name_matches("foo", "foo")); + // assert!(name_matches("foo_bar", "bar")); + // assert!(name_matches("FooBar", "foo")); + // assert!(name_matches("FooBar", "bar")); + // assert!(name_matches("FooBar", "foo_bar")); + assert!(name_matches("bar_baz", "bar_b")); + + // assert!(!name_matches("foo_bar", "o_b")); } } From cca667a46688765cf5d9652e9d6488af8d260e66 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 12:46:25 -0300 Subject: [PATCH 08/15] Add some comments --- tooling/lsp/src/requests/completion/auto_import.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 94b9e5d2687..a95c0fe8fbc 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -153,8 +153,8 @@ fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { } } -/// Computes the path of `module_id` relative to `current_module_id`. -/// If it's not relative, the full path is returned. +/// Returns the path to reach an item inside `module_id` from inside `current_module_id`. +/// Returns a relative path if possible. fn module_id_path( module_id: ModuleId, current_module_id: &ModuleId, @@ -200,6 +200,7 @@ fn module_id_path( match crate_id { CrateId::Root(_) => { if Some(module_id) == current_module_parent_id { + // This can happen if the item to import is inside the parent module Some("super".to_string()) } else { Some("crate".to_string()) From 59d4845a7aec52e33dcdb3c595164ec8f809391a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 13:18:51 -0300 Subject: [PATCH 09/15] Remove unreachable case --- .../src/requests/completion/auto_import.rs | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index a95c0fe8fbc..65eab064e1d 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -50,8 +50,7 @@ impl<'a> NodeFinder<'a> { self.dependencies, ); } else { - let Some(parent_module) = - get_parent_module(self.interner, self.def_maps, *module_def_id) + let Some(parent_module) = get_parent_module(self.interner, *module_def_id) else { continue; }; @@ -118,19 +117,9 @@ impl<'a> NodeFinder<'a> { } } -fn get_parent_module( - interner: &NodeInterner, - def_maps: &BTreeMap, - module_def_id: ModuleDefId, -) -> Option { - if let ModuleDefId::ModuleId(module_id) = module_def_id { - // ModuleDefId::ModuelId isn't tracked in interner's reference modules, - // so we find the parent in def maps. - get_parent_module_id(def_maps, module_id) - } else { - let reference_id = module_def_id_to_reference_id(module_def_id); - interner.reference_module(reference_id).copied() - } +fn get_parent_module(interner: &NodeInterner, module_def_id: ModuleDefId) -> Option { + let reference_id = module_def_id_to_reference_id(module_def_id); + interner.reference_module(reference_id).copied() } fn get_parent_module_id( From 81d6d00f32202aa21255806952b76e83c2a4bfb5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 13:53:54 -0300 Subject: [PATCH 10/15] module_id -> target_module_id --- tooling/lsp/src/requests/completion/auto_import.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 65eab064e1d..bc560bb2881 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -142,10 +142,10 @@ fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { } } -/// Returns the path to reach an item inside `module_id` from inside `current_module_id`. +/// Returns the path to reach an item inside `target_module_id` from inside `current_module_id`. /// Returns a relative path if possible. fn module_id_path( - module_id: ModuleId, + target_module_id: ModuleId, current_module_id: &ModuleId, current_module_parent_id: Option, interner: &NodeInterner, @@ -154,13 +154,13 @@ fn module_id_path( let mut segments: Vec<&str> = Vec::new(); let mut is_relative = false; - if let Some(module_attributes) = interner.try_module_attributes(&module_id) { + if let Some(module_attributes) = interner.try_module_attributes(&target_module_id) { segments.push(&module_attributes.name); let mut current_attributes = module_attributes; loop { let parent_module_id = - &ModuleId { krate: module_id.krate, local_id: current_attributes.parent }; + &ModuleId { krate: target_module_id.krate, local_id: current_attributes.parent }; if current_module_id == parent_module_id { is_relative = true; @@ -182,13 +182,13 @@ fn module_id_path( } } - let crate_id = module_id.krate; + let crate_id = target_module_id.krate; let crate_name = if is_relative { None } else { match crate_id { CrateId::Root(_) => { - if Some(module_id) == current_module_parent_id { + if Some(target_module_id) == current_module_parent_id { // This can happen if the item to import is inside the parent module Some("super".to_string()) } else { From f878ed3a2901209d068823a7d3a6f179f0a1cdee Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 16:51:44 -0300 Subject: [PATCH 11/15] Move condition upwards --- .../lsp/src/requests/completion/auto_import.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index bc560bb2881..babe03bc2ff 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -151,6 +151,10 @@ fn module_id_path( interner: &NodeInterner, dependencies: &[Dependency], ) -> String { + if Some(target_module_id) == current_module_parent_id { + return "super".to_string(); + } + let mut segments: Vec<&str> = Vec::new(); let mut is_relative = false; @@ -187,20 +191,12 @@ fn module_id_path( None } else { match crate_id { - CrateId::Root(_) => { - if Some(target_module_id) == current_module_parent_id { - // This can happen if the item to import is inside the parent module - Some("super".to_string()) - } else { - Some("crate".to_string()) - } - } - + CrateId::Root(_) => Some("crate".to_string()), + CrateId::Stdlib(_) => Some("std".to_string()), CrateId::Crate(_) => dependencies .iter() .find(|dep| dep.crate_id == crate_id) .map(|dep| format!("{}", dep.name)), - CrateId::Stdlib(_) => Some("std".to_string()), CrateId::Dummy => None, } }; From 8a169a4529dac8d22eff7fb66951a3e2af807317 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 19 Aug 2024 20:39:07 -0300 Subject: [PATCH 12/15] Extract offset variable --- tooling/lsp/src/requests/completion.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 71ef3849933..458fa82480c 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -1185,18 +1185,14 @@ fn name_matches(name: &str, prefix: &str) -> bool { let mut last_index: i32 = -1; for prefix_part in prefix.split('_') { - if let Some(mut name_part_index) = name_parts - .iter() - .skip( - // Look past parts we already matched - if last_index >= 0 { last_index as usize + 1 } else { 0 }, - ) - .position(|name_part| name_part.starts_with(prefix_part)) + // Look past parts we already matched + let offset = if last_index >= 0 { last_index as usize + 1 } else { 0 }; + + if let Some(mut name_part_index) = + name_parts.iter().skip(offset).position(|name_part| name_part.starts_with(prefix_part)) { - if last_index >= 0 { - // Need to adjust the index if we skipped some segments - name_part_index += last_index as usize + 1; - } + // Need to adjust the index if we skipped some segments + name_part_index += offset; if last_index >= name_part_index as i32 { return false; From f8ca6472ba5a8d2f077c3cff93ebd4e3d3fe98cb Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 20 Aug 2024 08:23:44 -0300 Subject: [PATCH 13/15] Re-enable assertions --- tooling/lsp/src/requests/completion.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 458fa82480c..302152f0829 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -1226,13 +1226,13 @@ mod completion_name_matches_tests { #[test] fn test_name_matches() { - // assert!(name_matches("foo", "foo")); - // assert!(name_matches("foo_bar", "bar")); - // assert!(name_matches("FooBar", "foo")); - // assert!(name_matches("FooBar", "bar")); - // assert!(name_matches("FooBar", "foo_bar")); + assert!(name_matches("foo", "foo")); + assert!(name_matches("foo_bar", "bar")); + assert!(name_matches("FooBar", "foo")); + assert!(name_matches("FooBar", "bar")); + assert!(name_matches("FooBar", "foo_bar")); assert!(name_matches("bar_baz", "bar_b")); - // assert!(!name_matches("foo_bar", "o_b")); + assert!(!name_matches("foo_bar", "o_b")); } } From 88bdb7a86776043a14070be9c0e758305fd149a4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 20 Aug 2024 08:25:00 -0300 Subject: [PATCH 14/15] Dummy crates should be unreachable --- tooling/lsp/src/requests/completion/auto_import.rs | 2 +- tooling/lsp/src/requests/hover.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index babe03bc2ff..c2a0917321c 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -197,7 +197,7 @@ fn module_id_path( .iter() .find(|dep| dep.crate_id == crate_id) .map(|dep| format!("{}", dep.name)), - CrateId::Dummy => None, + CrateId::Dummy => unreachable!("ICE: A dummy CrateId should not be accessible"), } }; diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index cd86cb0b00c..95d8b82f84f 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -343,7 +343,7 @@ fn format_parent_module_from_module_id( .find(|dep| dep.crate_id == crate_id) .map(|dep| format!("{}", dep.name)), CrateId::Stdlib(_) => Some("std".to_string()), - CrateId::Dummy => None, + CrateId::Dummy => unreachable!("ICE: A dummy CrateId should not be accessible"), }; if let Some(crate_name) = &crate_name { From 4c98660f43c9d8f07bc9a58ddee31205354e215c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 20 Aug 2024 08:25:32 -0300 Subject: [PATCH 15/15] Use to_string instead of format! --- tooling/lsp/src/requests/completion/auto_import.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index c2a0917321c..8d7824502c1 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -196,7 +196,7 @@ fn module_id_path( CrateId::Crate(_) => dependencies .iter() .find(|dep| dep.crate_id == crate_id) - .map(|dep| format!("{}", dep.name)), + .map(|dep| dep.name.to_string()), CrateId::Dummy => unreachable!("ICE: A dummy CrateId should not be accessible"), } };