Skip to content

Commit

Permalink
fix: (LSP) check visibility of module that re-exports item, if any (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
asterite authored Oct 28, 2024
1 parent e909dcb commit a4fc6e8
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 5 deletions.
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 @@ -1642,6 +1642,29 @@ fn main() {
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 {
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

0 comments on commit a4fc6e8

Please sign in to comment.