From c3cb38a7c4de6fc321b367eda3fca6d06e76b77a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 3 Oct 2024 14:50:26 -0300 Subject: [PATCH] fix: add missing visibility for auto-import names (#6205) # Description ## Problem Even though we started tracking visibility for many items, this visibility wasn't registered for auto-import. ## Summary ## Additional Context I was thinking that maybe registering things for auto-import isn't necessary. We could traverse all types and all functions in all modules and check if they match... but maybe that'll be much slower (right now we group all items by name so maybe iterating that is faster). In any case we could explore that other option later on. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- .../src/hir/def_collector/dc_crate.rs | 1 + .../src/hir/def_collector/dc_mod.rs | 18 +++++-- compiler/noirc_frontend/src/locations.rs | 24 +++++---- .../requests/code_action/import_or_qualify.rs | 8 +-- tooling/lsp/src/requests/completion/tests.rs | 54 ++++++++++++++++++- 6 files changed, 88 insertions(+), 19 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index b4ba80a307c..bd78febc1d0 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1385,7 +1385,7 @@ impl<'context> Elaborator<'context> { } if let Some(name) = name { - self.interner.register_global(global_id, name, self.module_id()); + self.interner.register_global(global_id, name, global.visibility, self.module_id()); } self.local_module = old_module; diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 9805e04a0b1..16fd43ba2a2 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -111,6 +111,7 @@ pub struct UnresolvedGlobal { pub module_id: LocalModuleId, pub global_id: GlobalId, pub stmt_def: LetStatement, + pub visibility: ItemVisibility, } pub struct ModuleAttribute { diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index df2ada94ce0..ba1075dfb30 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -357,7 +357,12 @@ impl<'a> ModCollector<'a> { if context.def_interner.is_in_lsp_mode() { let parent_module_id = ModuleId { krate, local_id: self.module_id }; let name = name.to_string(); - context.def_interner.register_type_alias(type_alias_id, name, parent_module_id); + context.def_interner.register_type_alias( + type_alias_id, + name, + visibility, + parent_module_id, + ); } } errors @@ -591,7 +596,12 @@ impl<'a> ModCollector<'a> { if context.def_interner.is_in_lsp_mode() { let parent_module_id = ModuleId { krate, local_id: self.module_id }; - context.def_interner.register_trait(trait_id, name.to_string(), parent_module_id); + context.def_interner.register_trait( + trait_id, + name.to_string(), + visibility, + parent_module_id, + ); } self.def_collector.items.traits.insert(trait_id, unresolved); @@ -879,7 +889,7 @@ fn push_child_module( ); if interner.is_in_lsp_mode() { - interner.register_module(mod_id, mod_name.0.contents.clone()); + interner.register_module(mod_id, visibility, mod_name.0.contents.clone()); } } @@ -1221,7 +1231,7 @@ pub(crate) fn collect_global( interner.set_doc_comments(ReferenceId::Global(global_id), doc_comments); - let global = UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global }; + let global = UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global, visibility }; (global, error) } diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 65adf4ca9c4..f3439316538 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -278,8 +278,12 @@ impl NodeInterner { .next() } - pub(crate) fn register_module(&mut self, id: ModuleId, name: String) { - let visibility = ItemVisibility::Public; + pub(crate) fn register_module( + &mut self, + id: ModuleId, + visibility: ItemVisibility, + name: String, + ) { self.register_name_for_auto_import(name, ModuleDefId::ModuleId(id), visibility, None); } @@ -287,11 +291,10 @@ impl NodeInterner { &mut self, id: GlobalId, name: String, + visibility: ItemVisibility, parent_module_id: ModuleId, ) { self.add_definition_location(ReferenceId::Global(id), Some(parent_module_id)); - - let visibility = ItemVisibility::Public; self.register_name_for_auto_import(name, ModuleDefId::GlobalId(id), visibility, None); } @@ -306,10 +309,14 @@ impl NodeInterner { self.register_name_for_auto_import(name, ModuleDefId::TypeId(id), visibility, None); } - pub(crate) fn register_trait(&mut self, id: TraitId, name: String, parent_module_id: ModuleId) { + pub(crate) fn register_trait( + &mut self, + id: TraitId, + name: String, + visibility: ItemVisibility, + parent_module_id: ModuleId, + ) { self.add_definition_location(ReferenceId::Trait(id), Some(parent_module_id)); - - let visibility = ItemVisibility::Public; self.register_name_for_auto_import(name, ModuleDefId::TraitId(id), visibility, None); } @@ -317,11 +324,10 @@ impl NodeInterner { &mut self, id: TypeAliasId, name: String, + visibility: ItemVisibility, parent_module_id: ModuleId, ) { self.add_definition_location(ReferenceId::Alias(id), Some(parent_module_id)); - - let visibility = ItemVisibility::Public; self.register_name_for_auto_import(name, ModuleDefId::TypeAliasId(id), visibility, None); } diff --git a/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/tooling/lsp/src/requests/code_action/import_or_qualify.rs index 769f801a6f1..bf1fe906be3 100644 --- a/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -186,7 +186,7 @@ fn foo(x: SomeTypeInBar) {}"#; let src = r#" mod foo { mod bar { - mod some_module_in_bar {} + pub mod some_module_in_bar {} } } @@ -198,7 +198,7 @@ fn foo(x: SomeTypeInBar) {}"#; let expected = r#" mod foo { mod bar { - mod some_module_in_bar {} + pub mod some_module_in_bar {} } } @@ -216,7 +216,7 @@ fn foo(x: SomeTypeInBar) {}"#; let src = r#"mod foo { mod bar { - mod some_module_in_bar {} + pub(crate) mod some_module_in_bar {} } } @@ -228,7 +228,7 @@ fn main() { mod foo { mod bar { - mod some_module_in_bar {} + pub(crate) mod some_module_in_bar {} } } diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 147fb5be4f3..e4beea48064 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1575,7 +1575,7 @@ mod completion_tests { async fn test_auto_import_suggests_modules_too() { let src = r#" mod foo { - mod barbaz { + pub mod barbaz { fn hello_world() {} } } @@ -2271,4 +2271,56 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_does_not_auto_import_private_global() { + let src = r#"mod moo { + global foobar = 1; + } + + fn main() { + fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } + + #[test] + async fn test_does_not_auto_import_private_type_alias() { + let src = r#"mod moo { + type foobar = i32; + } + + fn main() { + fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } + + #[test] + async fn test_does_not_auto_import_private_trait() { + let src = r#"mod moo { + trait Foobar {} + } + + fn main() { + Fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } + + #[test] + async fn test_does_not_auto_import_private_module() { + let src = r#"mod moo { + mod foobar {} + } + + fn main() { + fooba>|< + }"#; + + assert_completion(src, Vec::new()).await; + } }