Skip to content

Commit

Permalink
Stop offering private functions in completions
Browse files Browse the repository at this point in the history
Before
Private functions have RawVisibility module, but were
missed because take_types returned None early. After resolve_visibility
returned None, Visibility::Public was set instead and private functions
ended up being offered in autocompletion.

Choosing such a function results in an immediate error diagnostic
about using a private function.

After
Pattern match of take_types that returns None and
query for Module-level visibility from the original_module

Fix #15134 - tested with a unit test and a manual end-to-end
test of building rust-analyzer from my branch and opening
the reproduction repository

REVIEW
Refactor to move scope_def_applicable and check function visibility
from a module

Please let me know what's the best way to add a unit tests to
nameres, which is where the root cause was
  • Loading branch information
petr-tik authored and Veykril committed Dec 8, 2023
1 parent bc9c952 commit 2d879e0
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 14 deletions.
14 changes: 9 additions & 5 deletions crates/hir-def/src/nameres/path_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,15 @@ impl DefMap {
if remaining.is_some() {
return None;
}
let types = result.take_types()?;
match types {
ModuleDefId::ModuleId(m) => Visibility::Module(m),
_ => {
// error: visibility needs to refer to module
let types = result.take_types();

match (types, path.kind) {
(Some(ModuleDefId::ModuleId(m)), _) => Visibility::Module(m),
// resolve_path doesn't find any values for a plan pathkind of a private function
(None, PathKind::Plain | PathKind::Crate) => {
Visibility::Module(self.module_id(original_module))
}
(_, _) => {
return None;
}
}
Expand Down
29 changes: 20 additions & 9 deletions crates/ide-completion/src/completions/expr.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Completion of names from the current scope in expression position.
use hir::ScopeDef;
use hir::{HasVisibility, Module, ScopeDef};
use syntax::ast;

use crate::{
Expand All @@ -9,6 +9,23 @@ use crate::{
CompletionContext, Completions,
};

fn scope_def_applicable(
def: ScopeDef,
ctx: &CompletionContext<'_>,
module: Option<&Module>,
) -> bool {
match (def, module) {
(ScopeDef::GenericParam(hir::GenericParam::LifetimeParam(_)) | ScopeDef::Label(_), _) => {
false
}
(ScopeDef::ModuleDef(hir::ModuleDef::Macro(mac)), _) => mac.is_fn_like(ctx.db),
(ScopeDef::ModuleDef(hir::ModuleDef::Function(f)), Some(m)) => {
f.is_visible_from(ctx.db, *m)
}
_ => true,
}
}

pub(crate) fn complete_expr_path(
acc: &mut Completions,
ctx: &CompletionContext<'_>,
Expand Down Expand Up @@ -37,12 +54,6 @@ pub(crate) fn complete_expr_path(
let wants_mut_token =
ref_expr_parent.as_ref().map(|it| it.mut_token().is_none()).unwrap_or(false);

let scope_def_applicable = |def| match def {
ScopeDef::GenericParam(hir::GenericParam::LifetimeParam(_)) | ScopeDef::Label(_) => false,
ScopeDef::ModuleDef(hir::ModuleDef::Macro(mac)) => mac.is_fn_like(ctx.db),
_ => true,
};

let add_assoc_item = |acc: &mut Completions, item| match item {
hir::AssocItem::Function(func) => acc.add_function(ctx, path_ctx, func, None),
hir::AssocItem::Const(ct) => acc.add_const(ctx, ct),
Expand Down Expand Up @@ -87,7 +98,7 @@ pub(crate) fn complete_expr_path(
hir::PathResolution::Def(hir::ModuleDef::Module(module)) => {
let module_scope = module.scope(ctx.db, Some(ctx.module));
for (name, def) in module_scope {
if scope_def_applicable(def) {
if scope_def_applicable(def, ctx, Some(module)) {
acc.add_path_resolution(
ctx,
path_ctx,
Expand Down Expand Up @@ -233,7 +244,7 @@ pub(crate) fn complete_expr_path(
[..] => acc.add_path_resolution(ctx, path_ctx, name, def, doc_aliases),
}
}
_ if scope_def_applicable(def) => {
_ if scope_def_applicable(def, ctx, None) => {
acc.add_path_resolution(ctx, path_ctx, name, def, doc_aliases)
}
_ => (),
Expand Down
24 changes: 24 additions & 0 deletions crates/ide-completion/src/tests/special.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1286,6 +1286,30 @@ fn here_we_go() {
);
}

#[test]
fn completes_only_public() {
check(
r#"
//- /e.rs
pub(self) fn i_should_be_hidden() {}
pub(in crate::krate) fn i_should_also_be_hidden() {}
pub fn i_am_public () {}
//- /lib.rs crate:krate
pub mod e;
//- /main.rs deps:krate crate:main
use krate::e;
fn main() {
e::$0
}"#,
expect![
"fn i_am_public() fn()
"
],
)
}

#[test]
fn completion_filtering_excludes_non_identifier_doc_aliases() {
check_edit(
Expand Down

0 comments on commit 2d879e0

Please sign in to comment.