From 9ed684e7bfa27e21cceceb53cedb247ae88fac52 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 13:24:58 -0300 Subject: [PATCH] feat: LSP will now suggest private items if they are visible --- .../noirc_frontend/src/elaborator/comptime.rs | 5 +--- .../src/hir/def_collector/dc_mod.rs | 5 +--- tooling/lsp/src/modules.rs | 5 ++-- .../requests/code_action/import_or_qualify.rs | 1 + tooling/lsp/src/requests/completion.rs | 22 ++------------- .../src/requests/completion/auto_import.rs | 1 + tooling/lsp/src/requests/completion/tests.rs | 25 +++++++++++++++++ tooling/lsp/src/visibility.rs | 28 +++++++++++++------ 8 files changed, 54 insertions(+), 38 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index baa9c0ab371..5678fa8ddee 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/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); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 590cdc541ce..1dbd5a1383b 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -239,10 +239,7 @@ impl<'a> ModCollector<'a> { 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); } diff --git a/tooling/lsp/src/modules.rs b/tooling/lsp/src/modules.rs index 54074dbd94c..d78da15a8ff 100644 --- a/tooling/lsp/src/modules.rs +++ b/tooling/lsp/src/modules.rs @@ -47,10 +47,11 @@ pub(crate) fn module_full_path( current_module_id: ModuleId, current_module_parent_id: Option, interner: &NodeInterner, + def_maps: &BTreeMap, ) -> Option { 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; } diff --git a/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/tooling/lsp/src/requests/code_action/import_or_qualify.rs index d07d117a317..25accc8a008 100644 --- a/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/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; }; diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 987746d37ce..59758f4b972 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -22,10 +22,7 @@ use noirc_frontend::{ 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,7 +31,7 @@ use noirc_frontend::{ }; 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; @@ -1263,21 +1260,6 @@ fn module_def_id_from_reference_id(reference_id: ReferenceId) -> Option, -) -> 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; diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index bbd471dfea1..d8823794999 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/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; }; diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index d621ca21bb8..ca959f5d5ca 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1863,4 +1863,29 @@ mod completion_tests { 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()), + ); + } } diff --git a/tooling/lsp/src/visibility.rs b/tooling/lsp/src/visibility.rs index aad8b47fbbe..d6e26f7bc48 100644 --- a/tooling/lsp/src/visibility.rs +++ b/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, ) -> 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, + ) }