fix: LSP auto-import would import public item inside private module

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);
         }
 

compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -840,7 +840,7 @@
     //   if it's an inline module, or the first char of a the file if it's an external module.
     // - `location` will always point to the token "foo" in `mod foo` regardless of whether
     //   it's inline or external.
     // Eventually the location put in `ModuleData` is used for codelenses about `contract`s,
     // so we keep using `location` so that it continues to work as usual.
     let location = Location::new(mod_name.span(), mod_location.file);
     let new_module = ModuleData::new(
@@ -885,6 +885,7 @@
                 name: mod_name.0.contents.clone(),
                 location: mod_location,
                 parent: Some(parent),
+                visibility,
             },
         );
 

compiler/noirc_frontend/src/node_interner.rs

@@ -56,6 +56,7 @@
     pub name: String,
     pub location: Location,
     pub parent: Option<LocalModuleId>,
+    pub visibility: ItemVisibility,
 }
 
 type StructAttributes = Vec<SecondaryAttribute>;
@@ -219,12 +220,12 @@
     interned_statement_kinds: noirc_arena::Arena<StatementKind>,
 
     // Interned `UnresolvedTypeData`s during comptime code.
     interned_unresolved_type_datas: noirc_arena::Arena<UnresolvedTypeData>,
 
     // Interned `Pattern`s during comptime code.
     interned_patterns: noirc_arena::Arena<Pattern>,
 
     /// Determins whether to run in LSP mode. In LSP mode references are tracked.
     pub(crate) lsp_mode: bool,
 
     /// Store the location of the references in the graph.
@@ -669,7 +670,7 @@
             quoted_types: Default::default(),
             interned_expression_kinds: Default::default(),
             interned_statement_kinds: Default::default(),
             interned_unresolved_type_datas: Default::default(),
             interned_patterns: Default::default(),
             lsp_mode: false,
             location_indices: LocationIndices::default(),
@@ -2180,11 +2181,11 @@
         &mut self,
         typ: UnresolvedTypeData,
     ) -> InternedUnresolvedTypeData {
         InternedUnresolvedTypeData(self.interned_unresolved_type_datas.insert(typ))
     }
 
     pub fn get_unresolved_type_data(&self, id: InternedUnresolvedTypeData) -> &UnresolvedTypeData {
         &self.interned_unresolved_type_datas[id.0]
     }
 
     /// Returns the type of an operator (which is always a function), along with its return type.

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,

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 {}
     }
 }

tooling/lsp/src/requests/completion.rs

@@ -38,13 +38,14 @@
 
 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;
 
 mod auto_import;
 mod builtins;
 mod completion_items;
 mod kinds;
 mod sort_text;
@@ -240,7 +241,7 @@
         let mut idents: Vec<Ident> = Vec::new();
 
         // Find in which ident we are in, and in which part of it
         // (it could be that we are completting in the middle of an ident)
         for segment in &path.segments {
             let ident = &segment.ident;
 
@@ -797,7 +798,12 @@
             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 @@
                 }
 
                 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(),
@@ -1755,8 +1766,8 @@
 ///
 /// For example:
 ///
 /// // "merk" and "ro" match "merkle" and "root" and are in order
 /// name_matches("compute_merkle_root", "merk_ro") == true
 ///
 /// // "ro" matches "root", but "merkle" comes before it, so no match
 /// name_matches("compute_merkle_root", "ro_mer") == false

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;
                         };