From 335de054dfcda366df50cc215900910ebdc8be63 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 23 Aug 2024 11:20:00 -0300 Subject: [PATCH] fix: honor function visibility in LSP completion (#5809) # Description ## Problem Resolves #5808 ## Summary Reuses the same logic done when resolving a `use` or `Path` in the compiler. ## Additional Context It's debatable whether this functionality is desired, because sometimes you add a function, want to use it and autocompletion doesn't suggest it, and it takes a while for you to realize that you forgot to add `pub`. However, a lot of the suggested functions come from the std or external crates, and it doesn't make sense to reveal these private functions. This is also how Rust Analyzer works too. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/hir/resolution/import.rs | 6 +- tooling/lsp/src/requests/completion.rs | 74 ++++++++++++------- tooling/lsp/src/requests/completion/tests.rs | 7 +- 3 files changed, 55 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 761da6c361d..b820e4664e3 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/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, importing_crate: CrateId, current_module: LocalModuleId, diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 15b5be1690c..c61f92795ad 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -16,16 +16,19 @@ use noirc_errors::{Location, Span}; 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}, @@ -1238,29 +1241,33 @@ impl<'a> NodeFinder<'a> { 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); + } } } } @@ -1384,6 +1391,21 @@ 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/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 07ac8885bd5..59e007c5a70 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -182,7 +182,8 @@ mod completion_tests { 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::>|< "#; @@ -1703,7 +1704,7 @@ mod completion_tests { 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 @@ mod completion_tests { async fn test_completes_at_function_call_name() { let src = r#" mod foo { - fn bar() {} + pub fn bar() {} } fn main() {