Skip to content

Commit

Permalink
fix: honor function visibility in LSP completion (#5809)
Browse files Browse the repository at this point in the history
# 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.
  • Loading branch information
asterite authored Aug 23, 2024
1 parent 1c84038 commit 335de05
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 32 deletions.
6 changes: 3 additions & 3 deletions compiler/noirc_frontend/src/hir/resolution/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
74 changes: 48 additions & 26 deletions tooling/lsp/src/requests/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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);
}
}
}
}
Expand Down Expand Up @@ -1384,6 +1391,21 @@ fn module_def_id_from_reference_id(reference_id: ReferenceId) -> Option<ModuleDe
}
}

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;
Expand Down
7 changes: 4 additions & 3 deletions tooling/lsp/src/requests/completion/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::>|<
"#;
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand Down

0 comments on commit 335de05

Please sign in to comment.