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

feat: user super:: in LSP autocompletion if possible #5751

Merged
merged 17 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from 14 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
25 changes: 16 additions & 9 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 @@ -1220,12 +1226,13 @@ mod completion_name_matches_tests {

#[test]
fn test_name_matches() {
assert!(name_matches("foo", "foo"));
assert!(name_matches("foo_bar", "bar"));
assert!(name_matches("FooBar", "foo"));
assert!(name_matches("FooBar", "bar"));
assert!(name_matches("FooBar", "foo_bar"));

assert!(!name_matches("foo_bar", "o_b"));
// assert!(name_matches("foo", "foo"));
// assert!(name_matches("foo_bar", "bar"));
// 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"));
asterite marked this conversation as resolved.
Show resolved Hide resolved
}
}
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| format!("{}", dep.name)),
asterite marked this conversation as resolved.
Show resolved Hide resolved
CrateId::Dummy => None,
asterite marked this conversation as resolved.
Show resolved Hide resolved
}
};

if let Some(crate_name) = &crate_name {
segments.push(crate_name);
};
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved

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())
})
);
}
}
52 changes: 22 additions & 30 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 @@ -331,41 +346,18 @@ fn format_parent_module_from_module_id(
CrateId::Dummy => None,
};

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
Loading