Skip to content

Commit

Permalink
feat: user super:: in LSP autocompletion if possible (#5751)
Browse files Browse the repository at this point in the history
# Description

## Problem

Part of #1577

## Summary

We recently added `super::` to `use` so I thought it would be nice if
the auto-importer used that if possible.


![lsp-use-super](https://github.com/user-attachments/assets/4f8cbf37-f844-4293-832e-bc9685295837)

## Additional Context

This PR also includes a small refactor in the hover code because the
code to format a module path exists there too (but the two are different
enough that I didn't reuse them).

## 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.

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
  • Loading branch information
asterite and TomAFrench authored Aug 20, 2024
1 parent 37af966 commit 5192e53
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 89 deletions.
11 changes: 9 additions & 2 deletions tooling/lsp/src/requests/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1185,9 +1185,15 @@ fn name_matches(name: &str, prefix: &str) -> bool {

let mut last_index: i32 = -1;
for prefix_part in prefix.split('_') {
if let Some(name_part_index) =
name_parts.iter().position(|name_part| name_part.starts_with(prefix_part))
// Look past parts we already matched
let offset = if last_index >= 0 { last_index as usize + 1 } else { 0 };

if let Some(mut name_part_index) =
name_parts.iter().skip(offset).position(|name_part| name_part.starts_with(prefix_part))
{
// Need to adjust the index if we skipped some segments
name_part_index += offset;

if last_index >= name_part_index as i32 {
return false;
}
Expand Down Expand Up @@ -1225,6 +1231,7 @@ mod completion_name_matches_tests {
assert!(name_matches("FooBar", "foo"));
assert!(name_matches("FooBar", "bar"));
assert!(name_matches("FooBar", "foo_bar"));
assert!(name_matches("bar_baz", "bar_b"));

assert!(!name_matches("foo_bar", "o_b"));
}
Expand Down
123 changes: 69 additions & 54 deletions tooling/lsp/src/requests/completion/auto_import.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use std::collections::BTreeMap;

use lsp_types::{Position, Range, TextEdit};
use noirc_frontend::{
ast::ItemVisibility,
graph::{CrateId, Dependency},
hir::def_map::ModuleId,
hir::def_map::{CrateDefMap, ModuleId},
macros_api::{ModuleDefId, NodeInterner},
node_interner::ReferenceId,
};
Expand All @@ -16,6 +18,8 @@ use super::{

impl<'a> NodeFinder<'a> {
pub(super) fn complete_auto_imports(&mut self, prefix: &str, requested_items: RequestedItems) {
let current_module_parent_id = get_parent_module_id(self.def_maps, self.module_id);

for (name, entries) in self.interner.get_auto_import_names() {
if !name_matches(name, prefix) {
continue;
Expand All @@ -39,8 +43,9 @@ impl<'a> NodeFinder<'a> {
let module_full_path;
if let ModuleDefId::ModuleId(module_id) = module_def_id {
module_full_path = module_id_path(
module_id,
*module_id,
&self.module_id,
current_module_parent_id,
self.interner,
self.dependencies,
);
Expand All @@ -67,6 +72,7 @@ impl<'a> NodeFinder<'a> {
module_full_path = module_id_path(
parent_module,
&self.module_id,
current_module_parent_id,
self.interner,
self.dependencies,
);
Expand Down Expand Up @@ -111,9 +117,18 @@ impl<'a> NodeFinder<'a> {
}
}

fn get_parent_module(interner: &NodeInterner, module_def_id: ModuleDefId) -> Option<&ModuleId> {
fn get_parent_module(interner: &NodeInterner, module_def_id: ModuleDefId) -> Option<ModuleId> {
let reference_id = module_def_id_to_reference_id(module_def_id);
interner.reference_module(reference_id)
interner.reference_module(reference_id).copied()
}

fn get_parent_module_id(
def_maps: &BTreeMap<CrateId, CrateDefMap>,
module_id: ModuleId,
) -> Option<ModuleId> {
let crate_def_map = &def_maps[&module_id.krate];
let module_data = &crate_def_map.modules()[module_id.local_id.0];
module_data.parent.map(|parent| ModuleId { krate: module_id.krate, local_id: parent })
}

fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId {
Expand All @@ -127,69 +142,69 @@ fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId {
}
}

/// Computes the path of `module_id` relative to `current_module_id`.
/// If it's not relative, the full path is returned.
/// Returns the path to reach an item inside `target_module_id` from inside `current_module_id`.
/// Returns a relative path if possible.
fn module_id_path(
module_id: &ModuleId,
target_module_id: ModuleId,
current_module_id: &ModuleId,
current_module_parent_id: Option<ModuleId>,
interner: &NodeInterner,
dependencies: &[Dependency],
) -> String {
let mut string = String::new();

let crate_id = module_id.krate;
let crate_name = match crate_id {
CrateId::Root(_) => Some("crate".to_string()),
CrateId::Crate(_) => dependencies
.iter()
.find(|dep| dep.crate_id == crate_id)
.map(|dep| format!("{}", dep.name)),
CrateId::Stdlib(_) => Some("std".to_string()),
CrateId::Dummy => None,
};
if Some(target_module_id) == current_module_parent_id {
return "super".to_string();
}

let wrote_crate = if let Some(crate_name) = crate_name {
string.push_str(&crate_name);
true
} else {
false
};
let mut segments: Vec<&str> = Vec::new();
let mut is_relative = false;

let Some(module_attributes) = interner.try_module_attributes(module_id) else {
return string;
};
if let Some(module_attributes) = interner.try_module_attributes(&target_module_id) {
segments.push(&module_attributes.name);

if wrote_crate {
string.push_str("::");
}
let mut current_attributes = module_attributes;
loop {
let parent_module_id =
&ModuleId { krate: target_module_id.krate, local_id: current_attributes.parent };

let mut segments = Vec::new();
let mut current_attributes = module_attributes;
loop {
let parent_module_id =
&ModuleId { krate: module_id.krate, local_id: current_attributes.parent };

// If the parent module is the current module we stop because we want a relative path to the module
if current_module_id == parent_module_id {
// When the path is relative we don't want the "crate::" prefix anymore
string = string.strip_prefix("crate::").unwrap_or(&string).to_string();
break;
}
if current_module_id == parent_module_id {
is_relative = true;
break;
}

let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else {
break;
};
if current_module_parent_id == Some(*parent_module_id) {
segments.push("super");
is_relative = true;
break;
}

segments.push(&parent_attributes.name);
current_attributes = parent_attributes;
}
let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else {
break;
};

for segment in segments.iter().rev() {
string.push_str(segment);
string.push_str("::");
segments.push(&parent_attributes.name);
current_attributes = parent_attributes;
}
}

string.push_str(&module_attributes.name);
let crate_id = target_module_id.krate;
let crate_name = if is_relative {
None
} else {
match crate_id {
CrateId::Root(_) => Some("crate".to_string()),
CrateId::Stdlib(_) => Some("std".to_string()),
CrateId::Crate(_) => dependencies
.iter()
.find(|dep| dep.crate_id == crate_id)
.map(|dep| dep.name.to_string()),
CrateId::Dummy => unreachable!("ICE: A dummy CrateId should not be accessible"),
}
};

if let Some(crate_name) = &crate_name {
segments.push(crate_name);
};

string
segments.reverse();
segments.join("::")
}
29 changes: 27 additions & 2 deletions tooling/lsp/src/requests/completion/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1446,7 +1446,7 @@ mod completion_tests {
assert_eq!(
item.label_details,
Some(CompletionItemLabelDetails {
detail: Some("(use crate::foo::bar::hello_world)".to_string()),
detail: Some("(use super::bar::hello_world)".to_string()),
description: Some("fn()".to_string())
})
);
Expand All @@ -1458,7 +1458,7 @@ mod completion_tests {
start: Position { line: 7, character: 8 },
end: Position { line: 7, character: 8 },
},
new_text: "use crate::foo::bar::hello_world;\n\n ".to_string(),
new_text: "use super::bar::hello_world;\n\n ".to_string(),
}])
);
}
Expand Down Expand Up @@ -1613,4 +1613,29 @@ mod completion_tests {
)
.await;
}

#[test]
async fn test_auto_import_with_super() {
let src = r#"
pub fn bar_baz() {}
mod tests {
fn foo() {
bar_b>|<
}
}
"#;
let items = get_completions(src).await;
assert_eq!(items.len(), 1);

let item = &items[0];
assert_eq!(item.label, "bar_baz()");
assert_eq!(
item.label_details,
Some(CompletionItemLabelDetails {
detail: Some("(use super::bar_baz)".to_string()),
description: Some("fn()".to_string())
})
);
}
}
54 changes: 23 additions & 31 deletions tooling/lsp/src/requests/hover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,21 @@ fn format_parent_module_from_module_id(
args: &ProcessRequestCallbackArgs,
string: &mut String,
) -> bool {
let mut segments: Vec<&str> = Vec::new();

if let Some(module_attributes) = args.interner.try_module_attributes(module) {
segments.push(&module_attributes.name);

let mut current_attributes = module_attributes;
while let Some(parent_attributes) = args.interner.try_module_attributes(&ModuleId {
krate: module.krate,
local_id: current_attributes.parent,
}) {
segments.push(&parent_attributes.name);
current_attributes = parent_attributes;
}
}

let crate_id = module.krate;
let crate_name = match crate_id {
CrateId::Root(_) => Some(args.crate_name.clone()),
Expand All @@ -328,44 +343,21 @@ fn format_parent_module_from_module_id(
.find(|dep| dep.crate_id == crate_id)
.map(|dep| format!("{}", dep.name)),
CrateId::Stdlib(_) => Some("std".to_string()),
CrateId::Dummy => None,
CrateId::Dummy => unreachable!("ICE: A dummy CrateId should not be accessible"),
};

let wrote_crate = if let Some(crate_name) = crate_name {
string.push_str(" ");
string.push_str(&crate_name);
true
} else {
false
};

let Some(module_attributes) = args.interner.try_module_attributes(module) else {
return wrote_crate;
if let Some(crate_name) = &crate_name {
segments.push(crate_name);
};

if wrote_crate {
string.push_str("::");
} else {
string.push_str(" ");
}

let mut segments = Vec::new();
let mut current_attributes = module_attributes;
while let Some(parent_attributes) = args.interner.try_module_attributes(&ModuleId {
krate: module.krate,
local_id: current_attributes.parent,
}) {
segments.push(&parent_attributes.name);
current_attributes = parent_attributes;
}

for segment in segments.iter().rev() {
string.push_str(segment);
string.push_str("::");
if segments.is_empty() {
return false;
}

string.push_str(&module_attributes.name);
segments.reverse();

string.push_str(" ");
string.push_str(&segments.join("::"));
true
}

Expand Down

0 comments on commit 5192e53

Please sign in to comment.