Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: (LSP) check visibility of module that re-exports item, if any #6371

Merged
merged 1 commit into from
Oct 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tooling/lsp/src/requests/code_action/import_or_qualify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ impl<'a> CodeActionFinder<'a> {
*module_def_id,
self.module_id,
*visibility,
*defining_module,
self.interner,
self.def_maps,
) {
Expand Down
1 change: 1 addition & 0 deletions tooling/lsp/src/requests/completion/auto_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ impl<'a> NodeFinder<'a> {
*module_def_id,
self.module_id,
*visibility,
*defining_module,
self.interner,
self.def_maps,
) {
Expand Down
23 changes: 23 additions & 0 deletions tooling/lsp/src/requests/completion/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,14 @@
#[test]
async fn test_use_first_segment() {
let src = r#"
mod foobaz {}

Check warning on line 137 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foobaz)
mod foobar {}
use foob>|<

Check warning on line 139 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foob)
"#;

assert_completion(
src,
vec![module_completion_item("foobaz"), module_completion_item("foobar")],

Check warning on line 144 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foobaz)
)
.await;
}
Expand Down Expand Up @@ -304,7 +304,7 @@
mod bar {
mod something {}

use super::foob>|<

Check warning on line 307 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foob)
}
"#;

Expand Down Expand Up @@ -1642,10 +1642,33 @@
assert!(items.is_empty());
}

#[test]
async fn checks_visibility_of_module_that_exports_item_if_any() {
let src = r#"
mod foo {
mod bar {
pub fn hello_world() {}
}

pub use bar::hello_world;
}

fn main() {
hello_w>|<
}
"#;
let mut items = get_completions(src).await;
assert_eq!(items.len(), 1);

let item = items.remove(0);
assert_eq!(item.label, "hello_world()");
assert_eq!(item.label_details.unwrap().detail.unwrap(), "(use foo::hello_world)");
}

#[test]
async fn test_auto_import_suggests_modules_too() {
let src = r#"mod foo {
pub mod barbaz {

Check warning on line 1671 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)
fn hello_world() {}
}
}
Expand All @@ -1655,10 +1678,10 @@
}
}"#;

let expected = r#"use foo::barbaz;

Check warning on line 1681 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)

mod foo {
pub mod barbaz {

Check warning on line 1684 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)
fn hello_world() {}
}
}
Expand All @@ -1672,12 +1695,12 @@
assert_eq!(items.len(), 1);

let item = items.remove(0);
assert_eq!(item.label, "barbaz");

Check warning on line 1698 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)

assert_eq!(
item.label_details,
Some(CompletionItemLabelDetails {
detail: Some("(use foo::barbaz)".to_string()),

Check warning on line 1703 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)
description: None
})
);
Expand Down Expand Up @@ -2064,7 +2087,7 @@
async fn test_completes_after_first_letter_of_path() {
let src = r#"
fn main() {
h>|<ello();

Check warning on line 2090 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (ello)
}

fn hello_world() {}
Expand Down
14 changes: 9 additions & 5 deletions tooling/lsp/src/visibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,13 @@ pub(super) fn item_in_module_is_visible(

/// Returns true if the given ModuleDefId is visible from the current module, given its visibility.
/// This will in turn check if the ModuleDefId parent modules are visible from the current module.
/// If `defining_module` is Some, it will be considered as the parent of the item to check
/// (this is the case when the item is re-exported with `pub use` or similar).
pub(super) fn module_def_id_is_visible(
module_def_id: ModuleDefId,
current_module_id: ModuleId,
mut visibility: ItemVisibility,
mut defining_module: Option<ModuleId>,
interner: &NodeInterner,
def_maps: &BTreeMap<CrateId, CrateDefMap>,
) -> bool {
Expand All @@ -49,7 +52,7 @@ pub(super) fn module_def_id_is_visible(
let mut target_module_id = if let ModuleDefId::ModuleId(module_id) = module_def_id {
Some(module_id)
} else {
get_parent_module(interner, module_def_id)
std::mem::take(&mut defining_module).or_else(|| get_parent_module(interner, module_def_id))
};

// Then check if it's visible, and upwards
Expand All @@ -58,10 +61,11 @@ pub(super) fn module_def_id_is_visible(
return false;
}

let module_data = &def_maps[&module_id.krate].modules()[module_id.local_id.0];
let parent_local_id = module_data.parent;
target_module_id =
parent_local_id.map(|local_id| ModuleId { krate: module_id.krate, local_id });
target_module_id = std::mem::take(&mut defining_module).or_else(|| {
let module_data = &def_maps[&module_id.krate].modules()[module_id.local_id.0];
let parent_local_id = module_data.parent;
parent_local_id.map(|local_id| ModuleId { krate: module_id.krate, local_id })
});

// This is a bit strange, but the visibility is always that of the item inside another module,
// so the visibility we update here is for the next loop check.
Expand Down
Loading