feat: LSP will now suggest private items if they are visible

compiler/noirc_frontend/src/elaborator/comptime.rs

@@ -269,10 +269,7 @@ impl<'context> Elaborator<'context> {
                 let module = self.module_id();
                 self.interner.push_function(id, &function.def, module, location);
 
-                if self.interner.is_in_lsp_mode()
-                    && !function.def.is_test()
-                    && !function.def.is_private()
-                {
+                if self.interner.is_in_lsp_mode() && !function.def.is_test() {
                     self.interner.register_function(id, &function.def);
                 }
 

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

@@ -239,10 +239,7 @@
             let location = Location::new(function.span(), self.file_id);
             context.def_interner.push_function(func_id, &function.def, module, location);
 
-            if context.def_interner.is_in_lsp_mode()
-                && !function.def.is_test()
-                && !function.def.is_private()
-            {
+            if context.def_interner.is_in_lsp_mode() && !function.def.is_test() {
                 context.def_interner.register_function(func_id, &function.def);
             }
 
@@ -761,7 +758,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(parent, location, is_contract);

tooling/lsp/src/modules.rs

@@ -47,10 +47,11 @@ pub(crate) fn module_full_path(
     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(visibility, current_module_id, module_id) {
+        if !is_visible(module_id, current_module_id, visibility, def_maps) {
             return None;
         }
 
@@ -61,7 +62,7 @@ pub(crate) fn module_full_path(
             return None;
         };
 
-        if !is_visible(visibility, current_module_id, parent_module) {
+        if !is_visible(parent_module, current_module_id, visibility, def_maps) {
             return None;
         }
 

tooling/lsp/src/requests/code_action/import_or_qualify.rs

@@ -52,6 +52,7 @@ impl<'a> CodeActionFinder<'a> {
                         self.module_id,
                         current_module_parent_id,
                         self.interner,
+                        self.def_maps,
                     ) else {
                         continue;
                     };

tooling/lsp/src/requests/completion.rs

@@ -22,10 +22,7 @@
         UnresolvedGenerics, UnresolvedType, UseTree, UseTreeKind, Visitor,
     },
     graph::{CrateId, Dependency},
-    hir::{
-        def_map::{CrateDefMap, LocalModuleId, ModuleId},
-        resolution::import::can_reference_module_id,
-    },
+    hir::def_map::{CrateDefMap, LocalModuleId, ModuleId},
     hir_def::traits::Trait,
     macros_api::{ModuleDefId, NodeInterner},
     node_interner::ReferenceId,
@@ -34,12 +31,12 @@
 };
 use sort_text::underscore_sort_text;
 
-use crate::{requests::to_lsp_location, utils, LspState};
+use crate::{requests::to_lsp_location, utils, visibility::is_visible, LspState};
 
 use super::process_request;
 
 mod auto_import;
 mod builtins;
 mod completion_items;
 mod kinds;
 mod sort_text;
@@ -218,7 +215,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;
 
@@ -1214,8 +1211,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
@@ -1263,21 +1260,6 @@
     }
 }
 
-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/auto_import.rs

@@ -53,6 +53,7 @@ impl<'a> NodeFinder<'a> {
                         self.module_id,
                         current_module_parent_id,
                         self.interner,
+                        self.def_maps,
                     ) else {
                         continue;
                     };

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

@@ -119,14 +119,14 @@
     #[test]
     async fn test_use_first_segment() {
         let src = r#"
             mod foobaz {}
             mod foobar {}
             use foob>|<
         "#;
 
         assert_completion(
             src,
             vec![module_completion_item("foobaz"), module_completion_item("foobar")],
         )
         .await;
     }
@@ -296,7 +296,7 @@
             mod bar {
                 mod something {}
 
                 use super::foob>|<
             }
         "#;
 
@@ -1538,7 +1538,7 @@
     async fn test_auto_import_suggests_modules_too() {
         let src = r#"
             mod foo {
                 mod barbaz {
                     fn hello_world() {}
                 }
             }
@@ -1863,4 +1863,29 @@
             Some("(use bar::foobar)".to_string()),
         );
     }
+
+    #[test]
+    async fn test_auto_import_suggests_private_function_if_visibile() {
+        let src = r#"
+            mod foo {
+                fn qux() {
+                  barba>|<
+                }
+            }
+
+            fn barbaz() {}
+
+            fn main() {}
+        "#;
+
+        let items = get_completions(src).await;
+        assert_eq!(items.len(), 1);
+
+        let item = &items[0];
+        assert_eq!(item.label, "barbaz()");
+        assert_eq!(
+            item.label_details.as_ref().unwrap().detail,
+            Some("(use super::barbaz)".to_string()),
+        );
+    }
 }

tooling/lsp/src/visibility.rs

@@ -1,13 +1,25 @@
-use noirc_frontend::{ast::ItemVisibility, hir::def_map::ModuleId};
+use std::collections::BTreeMap;
+
+use noirc_frontend::{
+    ast::ItemVisibility,
+    graph::CrateId,
+    hir::{
+        def_map::{CrateDefMap, ModuleId},
+        resolution::import::can_reference_module_id,
+    },
+};
 
 pub(super) fn is_visible(
+    target_module_id: ModuleId,
+    current_module_id: ModuleId,
     visibility: ItemVisibility,
-    current_module: ModuleId,
-    target_module: ModuleId,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
 ) -> bool {
-    match visibility {
-        ItemVisibility::Public => true,
-        ItemVisibility::Private => false,
-        ItemVisibility::PublicCrate => current_module.krate == target_module.krate,
-    }
+    can_reference_module_id(
+        def_maps,
+        current_module_id.krate,
+        current_module_id.local_id,
+        target_module_id,
+        visibility,
+    )
 }