noir-lang · TomAFrench · Oct 28, 2024 · Oct 28, 2024
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;
                         };