fix: LSP auto-import would import public item inside private module (#6366)

asterite · web-flow · commit 51eb2954e8df · 2024-10-28T11:57:57.000Z
diff --git 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
@@ -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
@@ -56,6 +56,7 @@ pub struct ModuleAttributes {
     pub name: String,
     pub location: Location,
     pub parent: Option<LocalModuleId>,
+    pub visibility: ItemVisibility,
 }
 
 type StructAttributes = Vec<SecondaryAttribute>;
diff --git 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<ModuleId>,
     interner: &NodeInterner,
-    def_maps: &BTreeMap<CrateId, CrateDefMap>,
 ) -> Option<String> {
     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,
             &current_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,
             &current_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
@@ -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>|<InBar) {}"#;
         let expected = r#"use foo::bar::SomeTypeInBar;
 
 mod foo {
-    mod bar {
+    pub mod bar {
         pub struct SomeTypeInBar {}
     }
 }
@@ -184,7 +193,7 @@ fn foo(x: SomeTypeInBar) {}"#;
 
         let src = r#"
         mod foo {
-            mod bar {
+            pub mod bar {
                 pub mod some_module_in_bar {}
             }
         }
@@ -196,7 +205,7 @@ fn foo(x: SomeTypeInBar) {}"#;
 
         let expected = r#"
         mod foo {
-            mod bar {
+            pub mod bar {
                 pub mod some_module_in_bar {}
             }
         }
@@ -214,7 +223,7 @@ fn foo(x: SomeTypeInBar) {}"#;
         let title = "Import foo::bar::some_module_in_bar";
 
         let src = r#"mod foo {
-    mod bar {
+    pub mod bar {
         pub(crate) mod some_module_in_bar {}
     }
 }
@@ -226,7 +235,7 @@ fn main() {
         let expected = r#"use foo::bar::some_module_in_bar;
 
 mod foo {
-    mod bar {
+    pub mod bar {
         pub(crate) mod some_module_in_bar {}
     }
 }
@@ -245,7 +254,7 @@ fn main() {
         let src = r#"use foo::bar::SomeOtherType;
 
 mod foo {
-    mod bar {
+    pub mod bar {
         pub struct SomeTypeInBar {}
     }
 }
@@ -255,7 +264,7 @@ fn foo(x: SomeType>|<InBar) {}"#;
         let expected = r#"use foo::bar::{SomeOtherType, SomeTypeInBar};
 
 mod foo {
-    mod bar {
+    pub mod bar {
         pub struct SomeTypeInBar {}
     }
 }
diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs
@@ -38,7 +38,8 @@ use sort_text::underscore_sort_text;
 
 use crate::{
     requests::to_lsp_location, trait_impl_method_stub_generator::TraitImplMethodStubGenerator,
-    use_segment_positions::UseSegmentPositions, utils, visibility::is_visible, LspState,
+    use_segment_positions::UseSegmentPositions, utils, visibility::item_in_module_is_visible,
+    LspState,
 };
 
 use super::process_request;
@@ -797,7 +798,12 @@ impl<'a> 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
@@ -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
diff --git a/tooling/lsp/src/visibility.rs b/tooling/lsp/src/visibility.rs

Original file line number	Diff line number	Diff line change
`@@ -56,6 +56,7 @@ pub struct ModuleAttributes {`
`56`	`56`	`pub name: String,`
`57`	`57`	`pub location: Location,`
`58`	`58`	`pub parent: Option<LocalModuleId>,`
	`59`	`+ pub visibility: ItemVisibility,`
`59`	`60`	`}`
`60`	`61`
`61`	`62`	`type StructAttributes = Vec<SecondaryAttribute>;`
Original file line number	Diff line number	Diff line change
`@@ -11,6 +11,7 @@ use crate::{`
`11`	`11`	`use_segment_positions::{`
`12`	`12`	`use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest,`
`13`	`13`	`},`
	`14`	`+ visibility::module_def_id_is_visible,`
`14`	`15`	`};`
`15`	`16`
`16`	`17`	`use super::CodeActionFinder;`
`@@ -41,6 +42,16 @@ impl<'a> CodeActionFinder<'a> {`
`41`	`42`	`}`
`42`	`43`
`43`	`44`	`for (module_def_id, visibility, defining_module) in entries {`
	`45`	`+ if !module_def_id_is_visible(`
	`46`	`+ *module_def_id,`
	`47`	`+ self.module_id,`
	`48`	`+ *visibility,`
	`49`	`+ self.interner,`
	`50`	`+ self.def_maps,`
	`51`	`+ ) {`
	`52`	`+ continue;`
	`53`	`+ }`
	`54`	`+`
`44`	`55`	`let module_full_path = if let Some(defining_module) = defining_module {`
`45`	`56`	`relative_module_id_path(`
`46`	`57`	`*defining_module,`
`@@ -51,11 +62,9 @@ impl<'a> CodeActionFinder<'a> {`
`51`	`62`	`} else {`
`52`	`63`	`let Some(module_full_path) = relative_module_full_path(`
`53`	`64`	`*module_def_id,`
`54`		`- *visibility,`
`55`	`65`	`self.module_id,`
`56`	`66`	`current_module_parent_id,`
`57`	`67`	`self.interner,`
`58`		`- self.def_maps,`
`59`	`68`	`) else {`
`60`	`69`	`continue;`
`61`	`70`	`};`
`@@ -132,7 +141,7 @@ mod tests {`
`132`	`141`
`133`	`142`	`let src = r#"`
`134`	`143`	`mod foo {`
`135`		`- mod bar {`
	`144`	`+ pub mod bar {`
`136`	`145`	`pub struct SomeTypeInBar {}`
`137`	`146`	`}`
`138`	`147`	`}`
`@@ -142,7 +151,7 @@ mod tests {`
`142`	`151`
`143`	`152`	`let expected = r#"`
`144`	`153`	`mod foo {`
`145`		`- mod bar {`
	`154`	`+ pub mod bar {`
`146`	`155`	`pub struct SomeTypeInBar {}`
`147`	`156`	`}`
`148`	`157`	`}`
`@@ -158,7 +167,7 @@ mod tests {`
`158`	`167`	`let title = "Import foo::bar::SomeTypeInBar";`
`159`	`168`
`160`	`169`	`let src = r#"mod foo {`
`161`		`- mod bar {`
	`170`	`+ pub mod bar {`
`162`	`171`	`pub struct SomeTypeInBar {}`
`163`	`172`	`}`
`164`	`173`	`}`
`@@ -168,7 +177,7 @@ fn foo(x: SomeType>\|<InBar) {}"#;`
`168`	`177`	`let expected = r#"use foo::bar::SomeTypeInBar;`
`169`	`178`
`170`	`179`	`mod foo {`
`171`		`- mod bar {`
	`180`	`+ pub mod bar {`
`172`	`181`	`pub struct SomeTypeInBar {}`
`173`	`182`	`}`
`174`	`183`	`}`
`@@ -184,7 +193,7 @@ fn foo(x: SomeTypeInBar) {}"#;`
`184`	`193`
`185`	`194`	`let src = r#"`
`186`	`195`	`mod foo {`
`187`		`- mod bar {`
	`196`	`+ pub mod bar {`
`188`	`197`	`pub mod some_module_in_bar {}`
`189`	`198`	`}`
`190`	`199`	`}`
`@@ -196,7 +205,7 @@ fn foo(x: SomeTypeInBar) {}"#;`
`196`	`205`
`197`	`206`	`let expected = r#"`
`198`	`207`	`mod foo {`
`199`		`- mod bar {`
	`208`	`+ pub mod bar {`
`200`	`209`	`pub mod some_module_in_bar {}`
`201`	`210`	`}`
`202`	`211`	`}`
`@@ -214,7 +223,7 @@ fn foo(x: SomeTypeInBar) {}"#;`
`214`	`223`	`let title = "Import foo::bar::some_module_in_bar";`
`215`	`224`
`216`	`225`	`let src = r#"mod foo {`
`217`		`- mod bar {`
	`226`	`+ pub mod bar {`
`218`	`227`	`pub(crate) mod some_module_in_bar {}`
`219`	`228`	`}`
`220`	`229`	`}`
`@@ -226,7 +235,7 @@ fn main() {`
`226`	`235`	`let expected = r#"use foo::bar::some_module_in_bar;`
`227`	`236`
`228`	`237`	`mod foo {`
`229`		`- mod bar {`
	`238`	`+ pub mod bar {`
`230`	`239`	`pub(crate) mod some_module_in_bar {}`
`231`	`240`	`}`
`232`	`241`	`}`
`@@ -245,7 +254,7 @@ fn main() {`
`245`	`254`	`let src = r#"use foo::bar::SomeOtherType;`
`246`	`255`
`247`	`256`	`mod foo {`
`248`		`- mod bar {`
	`257`	`+ pub mod bar {`
`249`	`258`	`pub struct SomeTypeInBar {}`
`250`	`259`	`}`
`251`	`260`	`}`
`@@ -255,7 +264,7 @@ fn foo(x: SomeType>\|<InBar) {}"#;`
`255`	`264`	`let expected = r#"use foo::bar::{SomeOtherType, SomeTypeInBar};`
`256`	`265`
`257`	`266`	`mod foo {`
`258`		`- mod bar {`
	`267`	`+ pub mod bar {`
`259`	`268`	`pub struct SomeTypeInBar {}`
`260`	`269`	`}`
`261`	`270`	`}`