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 5 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
135 changes: 81 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,13 +43,15 @@ 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,
);
} else {
let Some(parent_module) = get_parent_module(self.interner, *module_def_id)
let Some(parent_module) =
get_parent_module(self.interner, self.def_maps, *module_def_id)
else {
continue;
};
Expand All @@ -67,6 +73,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 +118,26 @@ impl<'a> NodeFinder<'a> {
}
}

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)
fn get_parent_module(
asterite marked this conversation as resolved.
Show resolved Hide resolved
interner: &NodeInterner,
def_maps: &BTreeMap<CrateId, CrateDefMap>,
module_def_id: ModuleDefId,
) -> Option<ModuleId> {
if let ModuleDefId::ModuleId(module_id) = module_def_id {
get_parent_module_id(def_maps, module_id)
} else {
let reference_id = module_def_id_to_reference_id(module_def_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 @@ -130,66 +154,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.
fn module_id_path(
module_id: &ModuleId,
module_id: ModuleId,
asterite marked this conversation as resolved.
Show resolved Hide resolved
current_module_id: &ModuleId,
current_module_parent_id: Option<ModuleId>,
interner: &NodeInterner,
dependencies: &[Dependency],
) -> String {
let mut string = String::new();
let mut segments: Vec<&str> = Vec::new();
let mut is_relative = false;

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 let Some(module_attributes) = interner.try_module_attributes(&module_id) {
segments.push(&module_attributes.name);

let wrote_crate = if let Some(crate_name) = crate_name {
string.push_str(&crate_name);
true
} else {
false
};
let mut current_attributes = module_attributes;
loop {
let parent_module_id =
&ModuleId { krate: module_id.krate, local_id: current_attributes.parent };

let Some(module_attributes) = interner.try_module_attributes(module_id) else {
return string;
};

if wrote_crate {
string.push_str("::");
}
if current_module_id == parent_module_id {
is_relative = true;
break;
}

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_parent_id == Some(*parent_module_id) {
segments.push("super");
is_relative = true;
break;
}

let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else {
break;
};
let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else {
break;
};

segments.push(&parent_attributes.name);
current_attributes = parent_attributes;
segments.push(&parent_attributes.name);
current_attributes = parent_attributes;
}
}

for segment in segments.iter().rev() {
string.push_str(segment);
string.push_str("::");
}
let crate_id = module_id.krate;
let crate_name = if is_relative {
None
} else {
match crate_id {
CrateId::Root(_) => {
if Some(module_id) == current_module_parent_id {
Some("super".to_string())
} else {
Some("crate".to_string())
}
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
}

string.push_str(&module_attributes.name);
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::Stdlib(_) => Some("std".to_string()),
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 @@
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 @@
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 @@ -1537,7 +1537,7 @@
async fn test_auto_import_suggests_modules_too() {
let src = r#"
mod foo {
mod barbaz {

Check warning on line 1540 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 @@ -1550,11 +1550,11 @@
assert_eq!(items.len(), 1);

let item = &items[0];
assert_eq!(item.label, "barbaz");

Check warning on line 1553 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 1557 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)
description: None
})
);
Expand All @@ -1573,4 +1573,29 @@
"#;
assert_completion(src, vec![field_completion_item("some_property", "i32")]).await;
}

#[test]
async fn test_auto_import_with_super() {
let src = r#"
pub fn barbaz() {}

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

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)

mod tests {
fn foo() {
barb>|<
}
}
"#;
let items = get_completions(src).await;
assert_eq!(items.len(), 1);

let item = &items[0];
assert_eq!(item.label, "barbaz()");

Check warning on line 1592 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 super::barbaz)".to_string()),
description: Some("fn()".to_string())
})
asterite marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
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 @@
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 @@
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 Expand Up @@ -558,7 +550,7 @@
"two/src/lib.nr",
Position { line: 6, character: 9 },
r#" one
mod subone"#,

Check warning on line 553 in tooling/lsp/src/requests/hover.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (subone)
)
.await;
}
Expand All @@ -569,7 +561,7 @@
"workspace",
"two/src/lib.nr",
Position { line: 9, character: 20 },
r#" one::subone

Check warning on line 564 in tooling/lsp/src/requests/hover.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (subone)
struct SubOneStruct {
some_field: i32,
some_other_field: Field,
Expand Down
Loading