fix: honor function visibility in LSP completion

compiler/noirc_frontend/src/hir/resolution/import.rs

@@ -374,9 +374,9 @@ fn resolve_external_dep(
     resolve_path_to_ns(&dep_directive, dep_module.krate, importing_crate, def_maps, path_references)
 }
 
-// Issue an error if the given private function is being called from a non-child module, or
-// if the given pub(crate) function is being called from another crate
-fn can_reference_module_id(
+// Returns false if the given private function is being called from a non-child module, or
+// if the given pub(crate) function is being called from another crate. Otherwise returns true.
+pub fn can_reference_module_id(
     def_maps: &BTreeMap<CrateId, CrateDefMap>,
     importing_crate: CrateId,
     current_module: LocalModuleId,

tooling/lsp/src/requests/completion.rs

@@ -16,16 +16,19 @@
 use noirc_frontend::{
     ast::{
         AsTraitPath, BlockExpression, CallExpression, ConstructorExpression, Expression,
-        ExpressionKind, ForLoopStatement, Ident, IfExpression, LValue, Lambda, LetStatement,
-        MemberAccessExpression, MethodCallExpression, NoirFunction, NoirStruct, NoirTraitImpl,
-        Path, PathKind, PathSegment, Pattern, Statement, StatementKind, TraitItem, TypeImpl,
-        UnresolvedGeneric, UnresolvedGenerics, UnresolvedType, UnresolvedTypeData, UseTree,
-        UseTreeKind,
+        ExpressionKind, ForLoopStatement, Ident, IfExpression, ItemVisibility, LValue, Lambda,
+        LetStatement, MemberAccessExpression, MethodCallExpression, NoirFunction, NoirStruct,
+        NoirTraitImpl, Path, PathKind, PathSegment, Pattern, Statement, StatementKind, TraitItem,
+        TypeImpl, UnresolvedGeneric, UnresolvedGenerics, UnresolvedType, UnresolvedTypeData,
+        UseTree, UseTreeKind,
     },
     graph::{CrateId, Dependency},
     hir::{
         def_map::{CrateDefMap, LocalModuleId, ModuleId},
-        resolution::path_resolver::{PathResolver, StandardPathResolver},
+        resolution::{
+            import::can_reference_module_id,
+            path_resolver::{PathResolver, StandardPathResolver},
+        },
     },
     hir_def::traits::Trait,
     macros_api::{ModuleDefId, NodeInterner},
@@ -40,7 +43,7 @@
 use super::process_request;
 
 mod auto_import;
 mod builtins;
 mod completion_items;
 mod kinds;
 mod sort_text;
@@ -759,7 +762,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;
 
@@ -1238,29 +1241,33 @@
 
             if name_matches(name, prefix) {
                 let per_ns = module_data.find_name(ident);
-                if let Some((module_def_id, _, _)) = per_ns.types {
-                    if let Some(completion_item) = self.module_def_id_completion_item(
-                        module_def_id,
-                        name.clone(),
-                        function_completion_kind,
-                        function_kind,
-                        requested_items,
-                    ) {
-                        self.completion_items.push(completion_item);
-                        self.suggested_module_def_ids.insert(module_def_id);
+                if let Some((module_def_id, visibility, _)) = per_ns.types {
+                    if is_visible(module_id, self.module_id, visibility, self.def_maps) {
+                        if let Some(completion_item) = self.module_def_id_completion_item(
+                            module_def_id,
+                            name.clone(),
+                            function_completion_kind,
+                            function_kind,
+                            requested_items,
+                        ) {
+                            self.completion_items.push(completion_item);
+                            self.suggested_module_def_ids.insert(module_def_id);
+                        }
                     }
                 }
 
-                if let Some((module_def_id, _, _)) = per_ns.values {
-                    if let Some(completion_item) = self.module_def_id_completion_item(
-                        module_def_id,
-                        name.clone(),
-                        function_completion_kind,
-                        function_kind,
-                        requested_items,
-                    ) {
-                        self.completion_items.push(completion_item);
-                        self.suggested_module_def_ids.insert(module_def_id);
+                if let Some((module_def_id, visibility, _)) = per_ns.values {
+                    if is_visible(module_id, self.module_id, visibility, self.def_maps) {
+                        if let Some(completion_item) = self.module_def_id_completion_item(
+                            module_def_id,
+                            name.clone(),
+                            function_completion_kind,
+                            function_kind,
+                            requested_items,
+                        ) {
+                            self.completion_items.push(completion_item);
+                            self.suggested_module_def_ids.insert(module_def_id);
+                        }
                     }
                 }
             }
@@ -1335,8 +1342,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
@@ -1384,6 +1391,21 @@
     }
 }
 
+fn is_visible(
+    target_module_id: ModuleId,
+    current_module_id: ModuleId,
+    visibility: ItemVisibility,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
+) -> bool {
+    can_reference_module_id(
+        def_maps,
+        current_module_id.krate,
+        current_module_id.local_id,
+        target_module_id,
+        visibility,
+    )
+}
+
 #[cfg(test)]
 mod completion_name_matches_tests {
     use crate::requests::completion::name_matches;

tooling/lsp/src/requests/completion/tests.rs

@@ -182,7 +182,8 @@
     async fn test_use_function() {
         let src = r#"
             mod foo {
-                fn bar(x: i32) -> u64 { 0 }
+                pub fn bar(x: i32) -> u64 { 0 }
+                fn bar_is_private(x: i32) -> u64 { 0 }
             }
             use foo::>|<
         "#;
@@ -1537,7 +1538,7 @@
     async fn test_auto_import_suggests_modules_too() {
         let src = r#"
             mod foo {
                 mod barbaz {
                     fn hello_world() {}
                 }
             }
@@ -1550,11 +1551,11 @@
         assert_eq!(items.len(), 1);
 
         let item = &items[0];
         assert_eq!(item.label, "barbaz");
         assert_eq!(
             item.label_details,
             Some(CompletionItemLabelDetails {
                 detail: Some("(use foo::barbaz)".to_string()),
                 description: None
             })
         );
@@ -1661,7 +1662,7 @@
     async fn test_completes_after_first_letter_of_path() {
         let src = r#"
             fn main() {
                 h>|<ello();
             }
 
             fn hello_world() {}
@@ -1703,7 +1704,7 @@
     async fn test_completes_after_colon_in_the_middle_of_an_ident_middle_segment() {
         let src = r#"
             mod foo {
-                fn bar() {}
+                pub fn bar() {}
             }
 
             fn main() {
@@ -1725,7 +1726,7 @@
     async fn test_completes_at_function_call_name() {
         let src = r#"
             mod foo {
-                fn bar() {}
+                pub fn bar() {}
             }
 
             fn main() {