From ebcc6cdb782f36e4655523e57dc8d6bc3abfee0f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 28 Oct 2024 06:58:07 -0300 Subject: [PATCH] fix: LSP auto-import would import public item inside private module --- .../src/hir/def_collector/dc_crate.rs | 7 +- .../src/hir/def_collector/dc_mod.rs | 1 + compiler/noirc_frontend/src/node_interner.rs | 1 + tooling/lsp/src/modules.rs | 17 +--- .../requests/code_action/import_or_qualify.rs | 33 +++++--- tooling/lsp/src/requests/completion.rs | 17 +++- .../src/requests/completion/auto_import.rs | 13 ++- tooling/lsp/src/requests/completion/tests.rs | 79 +++++++++++-------- tooling/lsp/src/visibility.rs | 53 ++++++++++++- 9 files changed, 154 insertions(+), 67 deletions(-) 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 16fd43ba2a2..658812be324 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -303,7 +303,12 @@ impl DefCollector { def_map.extern_prelude.insert(dep.as_name(), module_id); let location = dep_def_map[dep_def_root].location; - let attributes = ModuleAttributes { name: dep.as_name(), location, parent: None }; + let attributes = ModuleAttributes { + name: dep.as_name(), + location, + parent: None, + visibility: ItemVisibility::Public, + }; context.def_interner.add_module_attributes(module_id, attributes); } 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 b9ce8f361f7..825a8414fe0 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -885,6 +885,7 @@ fn push_child_module( name: mod_name.0.contents.clone(), location: mod_location, parent: Some(parent), + visibility, }, ); diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 2183cfba0ef..9e0d8179076 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -56,6 +56,7 @@ pub struct ModuleAttributes { pub name: String, pub location: Location, pub parent: Option, + pub visibility: ItemVisibility, } type StructAttributes = Vec; diff --git a/tooling/lsp/src/modules.rs b/tooling/lsp/src/modules.rs index 9f9a826d6ca..cadf71b2eec 100644 --- a/tooling/lsp/src/modules.rs +++ b/tooling/lsp/src/modules.rs @@ -1,14 +1,9 @@ -use std::collections::BTreeMap; - use noirc_frontend::{ - ast::ItemVisibility, graph::{CrateId, Dependency}, - hir::def_map::{CrateDefMap, ModuleDefId, ModuleId}, + hir::def_map::{ModuleDefId, ModuleId}, node_interner::{NodeInterner, ReferenceId}, }; -use crate::visibility::is_visible; - pub(crate) fn get_parent_module( interner: &NodeInterner, module_def_id: ModuleDefId, @@ -33,18 +28,12 @@ pub(crate) fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> Refer /// - Otherwise, that item's parent module's path is returned pub(crate) fn relative_module_full_path( module_def_id: ModuleDefId, - visibility: ItemVisibility, current_module_id: ModuleId, current_module_parent_id: Option, interner: &NodeInterner, - def_maps: &BTreeMap, ) -> Option { let full_path; if let ModuleDefId::ModuleId(module_id) = module_def_id { - if !is_visible(module_id, current_module_id, visibility, def_maps) { - return None; - } - full_path = relative_module_id_path( module_id, ¤t_module_id, @@ -56,10 +45,6 @@ pub(crate) fn relative_module_full_path( return None; }; - if !is_visible(parent_module, current_module_id, visibility, def_maps) { - return None; - } - full_path = relative_module_id_path( parent_module, ¤t_module_id, 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 a3053f8f304..2e051890544 100644 --- a/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -11,6 +11,7 @@ use crate::{ use_segment_positions::{ use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, }, + visibility::module_def_id_is_visible, }; use super::CodeActionFinder; @@ -41,6 +42,16 @@ impl<'a> CodeActionFinder<'a> { } for (module_def_id, visibility, defining_module) in entries { + if !module_def_id_is_visible( + *module_def_id, + self.module_id, + *visibility, + self.interner, + self.def_maps, + ) { + continue; + } + let module_full_path = if let Some(defining_module) = defining_module { relative_module_id_path( *defining_module, @@ -51,11 +62,9 @@ impl<'a> CodeActionFinder<'a> { } else { let Some(module_full_path) = relative_module_full_path( *module_def_id, - *visibility, self.module_id, current_module_parent_id, self.interner, - self.def_maps, ) else { continue; }; @@ -132,7 +141,7 @@ mod tests { let src = r#" mod foo { - mod bar { + pub mod bar { pub struct SomeTypeInBar {} } } @@ -142,7 +151,7 @@ mod tests { let expected = r#" mod foo { - mod bar { + pub mod bar { pub struct SomeTypeInBar {} } } @@ -158,7 +167,7 @@ mod tests { let title = "Import foo::bar::SomeTypeInBar"; let src = r#"mod foo { - mod bar { + pub mod bar { pub struct SomeTypeInBar {} } } @@ -168,7 +177,7 @@ fn foo(x: SomeType>|| NodeFinder<'a> { if name_matches(name, prefix) { let per_ns = module_data.find_name(ident); if let Some((module_def_id, visibility, _)) = per_ns.types { - if is_visible(module_id, self.module_id, visibility, self.def_maps) { + if item_in_module_is_visible( + module_id, + self.module_id, + visibility, + self.def_maps, + ) { let completion_items = self.module_def_id_completion_items( module_def_id, name.clone(), @@ -813,7 +819,12 @@ impl<'a> NodeFinder<'a> { } if let Some((module_def_id, visibility, _)) = per_ns.values { - if is_visible(module_id, self.module_id, visibility, self.def_maps) { + if item_in_module_is_visible( + module_id, + self.module_id, + visibility, + self.def_maps, + ) { let completion_items = self.module_def_id_completion_items( module_def_id, name.clone(), diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 9ed633289c1..3b12d941c98 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -5,6 +5,7 @@ use crate::{ use_segment_positions::{ use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, }, + visibility::module_def_id_is_visible, }; use super::{ @@ -33,6 +34,16 @@ impl<'a> NodeFinder<'a> { continue; } + if !module_def_id_is_visible( + *module_def_id, + self.module_id, + *visibility, + self.interner, + self.def_maps, + ) { + continue; + } + let completion_items = self.module_def_id_completion_items( *module_def_id, name.clone(), @@ -58,11 +69,9 @@ impl<'a> NodeFinder<'a> { } else { let Some(module_full_path) = relative_module_full_path( *module_def_id, - *visibility, self.module_id, current_module_parent_id, self.interner, - self.def_maps, ) else { continue; }; diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index be3a75f72c8..b399088d05b 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1394,7 +1394,7 @@ mod completion_tests { #[test] async fn test_auto_imports() { let src = r#"mod foo { - mod bar { + pub mod bar { pub fn hello_world() {} struct Foo { @@ -1415,7 +1415,7 @@ fn main() { let expected = r#"use foo::bar::hello_world; mod foo { - mod bar { + pub mod bar { pub fn hello_world() {} struct Foo { @@ -1456,7 +1456,7 @@ fn main() { async fn test_auto_imports_when_in_nested_module_and_item_is_further_nested() { let src = r#"#[something] mod foo { - mod bar { + pub mod bar { pub fn hello_world() {} } @@ -1470,7 +1470,7 @@ mod foo { mod foo { use bar::hello_world; - mod bar { + pub mod bar { pub fn hello_world() {} } @@ -1500,11 +1500,11 @@ mod foo { #[test] async fn test_auto_imports_when_in_nested_module_and_item_is_not_further_nested() { let src = r#"mod foo { - mod bar { + pub mod bar { pub fn hello_world() {} } - mod baz { + pub mod baz { fn foo() { hel>|< } @@ -1512,11 +1512,11 @@ mod foo { }"#; let expected = r#"mod foo { - mod bar { + pub mod bar { pub fn hello_world() {} } - mod baz { + pub mod baz { use super::bar::hello_world; fn foo() { @@ -1545,7 +1545,7 @@ mod foo { #[test] async fn test_auto_import_inserts_after_last_use() { let src = r#"mod foo { - mod bar { + pub mod bar { pub fn hello_world() {} } } @@ -1561,7 +1561,7 @@ fn main() { }"#; let expected = r#"mod foo { - mod bar { + pub mod bar { pub fn hello_world() {} } } @@ -1625,6 +1625,23 @@ fn main() { assert!(items.is_empty()); } + #[test] + async fn test_does_not_auto_import_public_function_in_private_module() { + let src = r#" + mod foo { + mod bar { + pub fn hello_world() {} + } + } + + fn main() { + hel>|< + } + "#; + let items = get_completions(src).await; + assert!(items.is_empty()); + } + #[test] async fn test_auto_import_suggests_modules_too() { let src = r#"mod foo { @@ -1675,7 +1692,7 @@ mod foo { let src = r#"use foo::bar::one_hello_world; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} } @@ -1688,7 +1705,7 @@ fn main() { let expected = r#"use foo::bar::{one_hello_world, two_hello_world}; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} } @@ -1713,7 +1730,7 @@ fn main() { let src = r#"use foo::bar::one_hello_world; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} } pub fn two_hello_world() {} @@ -1726,7 +1743,7 @@ fn main() { let expected = r#"use foo::{bar::one_hello_world, two_hello_world}; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} } pub fn two_hello_world() {} @@ -1751,11 +1768,11 @@ fn main() { let src = r#"use foo::{bar::one_hello_world, baz}; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} } - mod baz {} + pub mod baz {} } fn main() { @@ -1765,11 +1782,11 @@ fn main() { let expected = r#"use foo::{bar::{one_hello_world, two_hello_world}, baz}; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} } - mod baz {} + pub mod baz {} } fn main() { @@ -1791,11 +1808,11 @@ fn main() { let src = r#"use foo::bar::baz; mod foo { - mod bar { - mod baz { + pub mod bar { + pub mod baz { pub fn one_hello_world() {} } - mod qux { + pub mod qux { pub fn two_hello_world() {} } } @@ -1808,11 +1825,11 @@ fn main() { let expected = r#"use foo::bar::{baz, qux::two_hello_world}; mod foo { - mod bar { - mod baz { + pub mod bar { + pub mod baz { pub fn one_hello_world() {} } - mod qux { + pub mod qux { pub fn two_hello_world() {} } } @@ -1837,7 +1854,7 @@ fn main() { let src = r#"use foo::bar; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} } @@ -1850,7 +1867,7 @@ fn main() { let expected = r#"use foo::bar::{self, two_hello_world}; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} } @@ -1875,7 +1892,7 @@ fn main() { let src = r#"use foo::bar::{one_hello_world, three_hello_world}; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} pub fn three_hello_world() {} @@ -1889,7 +1906,7 @@ fn main() { let expected = r#"use foo::bar::{two_hello_world, one_hello_world, three_hello_world}; mod foo { - mod bar { + pub mod bar { pub fn one_hello_world() {} pub fn two_hello_world() {} pub fn three_hello_world() {} @@ -1915,7 +1932,7 @@ fn main() { let src = r#"use foo::bar::{}; mod foo { - mod bar { + pub mod bar { pub fn two_hello_world() {} } } @@ -1927,7 +1944,7 @@ fn main() { let expected = r#"use foo::bar::{two_hello_world}; mod foo { - mod bar { + pub mod bar { pub fn two_hello_world() {} } } @@ -2233,7 +2250,7 @@ fn main() { async fn test_auto_import_suggests_pub_use_for_function() { let src = r#" mod bar { - mod baz { + pub mod baz { pub fn coco() {} } diff --git a/tooling/lsp/src/visibility.rs b/tooling/lsp/src/visibility.rs index 207302f327e..f5b807055d5 100644 --- a/tooling/lsp/src/visibility.rs +++ b/tooling/lsp/src/visibility.rs @@ -4,12 +4,23 @@ use noirc_frontend::{ ast::ItemVisibility, graph::CrateId, hir::{ - def_map::{CrateDefMap, ModuleId}, + def_map::{CrateDefMap, ModuleDefId, ModuleId}, resolution::visibility::can_reference_module_id, }, + node_interner::NodeInterner, }; -pub(super) fn is_visible( +use crate::modules::get_parent_module; + +/// Returns true if an item with the given visibility in the target module +/// is visible from the current module. For example: +/// +/// mod foo { +/// ^^^ <-- target module +/// pub(crate) fn bar() {} +/// ^^^^^^^^^^ <- visibility +/// } +pub(super) fn item_in_module_is_visible( target_module_id: ModuleId, current_module_id: ModuleId, visibility: ItemVisibility, @@ -23,3 +34,41 @@ pub(super) fn is_visible( visibility, ) } + +/// Returns true if the given ModuleDefId is visible from the current module, given its visibility. +/// This will in turn check if the ModuleDefId parent modules are visible from the current module. +pub(super) fn module_def_id_is_visible( + module_def_id: ModuleDefId, + current_module_id: ModuleId, + mut visibility: ItemVisibility, + interner: &NodeInterner, + def_maps: &BTreeMap, +) -> bool { + // First find out which module we need to check. + // If a module is trying to be referenced, it's that module. Otherwise it's the module that contains the item. + let mut target_module_id = if let ModuleDefId::ModuleId(module_id) = module_def_id { + Some(module_id) + } else { + get_parent_module(interner, module_def_id) + }; + + // Then check if it's visible, and upwards + while let Some(module_id) = target_module_id { + if !item_in_module_is_visible(module_id, current_module_id, visibility, def_maps) { + return false; + } + + let module_data = &def_maps[&module_id.krate].modules()[module_id.local_id.0]; + let parent_local_id = module_data.parent; + target_module_id = + parent_local_id.map(|local_id| ModuleId { krate: module_id.krate, local_id }); + + // This is a bit strange, but the visibility is always that of the item inside another module, + // so the visibility we update here is for the next loop check. + visibility = interner + .try_module_attributes(&module_id) + .map_or(ItemVisibility::Public, |attributes| attributes.visibility); + } + + true +}