From 86d884044ee5bac72af820d623e00e1375271845 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 21 Aug 2024 22:27:59 -0300 Subject: [PATCH] feat: LSP hover and go-to-definition for crates (#5786) # Description ## Problem Resolves #5787 ## Summary This is a minor thing, but hover and go-to-definition didn't work in path segments that referred to crates. For completeness, and for features we could build later on (for example making auto-import insert new stuff inside existing paths if possible) I thought it would be nice to have this working. ![lsp-crate](https://github.com/user-attachments/assets/be6d4308-6387-45c4-aac8-66459d15b746) ## Additional Context While doing it some code got simplified: the part of a path that referred to a crate didn't refer to anything (None) and that was the only case where an Option was needed. Now that we capture all references that None is gone. I know we are kind of capturing module attributes twice, one in the def maps and another in NodeInterner, but otherwise getting a Location from a ReferenceId would also need def maps to be passed to NodeInterner, which is a bit less convenient. In any case this information was tracked like that already, it's just that it was missing for modules that were crate roots. Note that the information is still missing for the root module of the root crate, so that case is handled separately in a couple of places. Capturing this is doable, but I didn't know where to get the root crate name from in `CrateDefMap::collect_defs`. ## 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. --- .../noirc_frontend/src/elaborator/scope.rs | 3 - .../src/hir/def_collector/dc_crate.rs | 18 +++-- .../src/hir/def_collector/dc_mod.rs | 2 +- .../src/hir/resolution/import.rs | 19 +++--- .../src/hir/resolution/path_resolver.rs | 6 +- compiler/noirc_frontend/src/node_interner.rs | 4 +- .../src/requests/completion/auto_import.rs | 34 ++++------ tooling/lsp/src/requests/completion/tests.rs | 18 +++++ tooling/lsp/src/requests/goto_definition.rs | 14 ++++ tooling/lsp/src/requests/hover.rs | 66 ++++++++++++------- .../test_programs/go_to_definition/Nargo.toml | 1 + .../go_to_definition/dependency/Nargo.toml | 6 ++ .../go_to_definition/dependency/src/lib.nr | 1 + .../go_to_definition/src/main.nr | 1 + 14 files changed, 121 insertions(+), 72 deletions(-) create mode 100644 tooling/lsp/test_programs/go_to_definition/dependency/Nargo.toml create mode 100644 tooling/lsp/test_programs/go_to_definition/dependency/src/lib.nr diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index b2367e0cf0e..3288d10b62e 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -83,9 +83,6 @@ impl<'context> Elaborator<'context> { resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?; for (referenced, segment) in references.iter().zip(path.segments) { - let Some(referenced) = referenced else { - continue; - }; self.interner.add_reference( *referenced, Location::new(segment.ident.span(), self.file), diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index fabd76a2818..8c62eb431b5 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -14,7 +14,8 @@ use crate::hir::Context; use crate::macros_api::{MacroError, MacroProcessor}; use crate::node_interner::{ - FuncId, GlobalId, NodeInterner, ReferenceId, StructId, TraitId, TraitImplId, TypeAliasId, + FuncId, GlobalId, ModuleAttributes, NodeInterner, ReferenceId, StructId, TraitId, TraitImplId, + TypeAliasId, }; use crate::ast::{ @@ -269,11 +270,17 @@ impl DefCollector { macro_processors, )); - let dep_def_root = - context.def_map(&dep.crate_id).expect("ice: def map was just created").root; + let dep_def_map = + context.def_map(&dep.crate_id).expect("ice: def map was just created"); + + let dep_def_root = dep_def_map.root; let module_id = ModuleId { krate: dep.crate_id, local_id: dep_def_root }; // Add this crate as a dependency by linking it's root module def_map.extern_prelude.insert(dep.as_name(), module_id); + + let location = dep_def_map[dep_def_root].location; + let attriutes = ModuleAttributes { name: dep.as_name(), location, parent: None }; + context.def_interner.add_module_attributes(module_id, attriutes); } // At this point, all dependencies are resolved and type checked. @@ -309,7 +316,7 @@ impl DefCollector { for collected_import in std::mem::take(&mut def_collector.imports) { let module_id = collected_import.module_id; let resolved_import = if context.def_interner.lsp_mode { - let mut references: Vec> = Vec::new(); + let mut references: Vec = Vec::new(); let resolved_import = resolve_import( crate_id, &collected_import, @@ -322,9 +329,6 @@ impl DefCollector { for (referenced, segment) in references.iter().zip(&collected_import.path.segments) { - let Some(referenced) = referenced else { - continue; - }; context.def_interner.add_reference( *referenced, Location::new(segment.ident.span(), file_id), diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 6436c2562bc..03ab9fa3a75 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -771,7 +771,7 @@ impl<'a> ModCollector<'a> { ModuleAttributes { name: mod_name.0.contents.clone(), location: mod_location, - parent: self.module_id, + parent: Some(self.module_id), }, ); diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 4693d3826a8..761da6c361d 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -86,7 +86,7 @@ pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; let NamespaceResolution { @@ -131,7 +131,7 @@ fn resolve_path_to_ns( crate_id: CrateId, importing_crate: CrateId, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; let def_map = &def_maps[&crate_id]; @@ -221,7 +221,7 @@ fn resolve_path_from_crate_root( import_path: &[PathSegment], def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { resolve_name_in_module( crate_id, @@ -239,7 +239,7 @@ fn resolve_name_in_module( import_path: &[PathSegment], starting_mod: LocalModuleId, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let def_map = &def_maps[&krate]; let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; @@ -275,7 +275,7 @@ fn resolve_name_in_module( current_mod_id = match typ { ModuleDefId::ModuleId(id) => { if let Some(path_references) = path_references { - path_references.push(Some(ReferenceId::Module(id))); + path_references.push(ReferenceId::Module(id)); } id } @@ -283,14 +283,14 @@ fn resolve_name_in_module( // TODO: If impls are ever implemented, types can be used in a path ModuleDefId::TypeId(id) => { if let Some(path_references) = path_references { - path_references.push(Some(ReferenceId::Struct(id))); + path_references.push(ReferenceId::Struct(id)); } id.module_id() } ModuleDefId::TypeAliasId(_) => panic!("type aliases cannot be used in type namespace"), ModuleDefId::TraitId(id) => { if let Some(path_references) = path_references { - path_references.push(Some(ReferenceId::Trait(id))); + path_references.push(ReferenceId::Trait(id)); } id.0 } @@ -337,7 +337,7 @@ fn resolve_external_dep( current_def_map: &CrateDefMap, directive: &ImportDirective, def_maps: &BTreeMap, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, importing_crate: CrateId, ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep @@ -355,9 +355,8 @@ fn resolve_external_dep( // See `singleton_import.nr` test case for a check that such cases are handled elsewhere. let path_without_crate_name = &path[1..]; - // Given that we skipped the first segment, record that it doesn't refer to any module or type. if let Some(path_references) = path_references { - path_references.push(None); + path_references.push(ReferenceId::Module(*dep_module)); } let path = Path { diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index 7cd44a84018..712951ad6cb 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -15,7 +15,7 @@ pub trait PathResolver { &self, def_maps: &BTreeMap, path: Path, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult; fn local_module_id(&self) -> LocalModuleId; @@ -39,7 +39,7 @@ impl PathResolver for StandardPathResolver { &self, def_maps: &BTreeMap, path: Path, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { resolve_path(def_maps, self.module_id, path, path_references) } @@ -59,7 +59,7 @@ pub fn resolve_path( def_maps: &BTreeMap, module_id: ModuleId, path: Path, - path_references: &mut Option<&mut Vec>>, + path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that let import = diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 46ec2676930..f9c6921ac8c 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -49,7 +49,7 @@ const IMPL_SEARCH_RECURSION_LIMIT: u32 = 10; pub struct ModuleAttributes { pub name: String, pub location: Location, - pub parent: LocalModuleId, + pub parent: Option, } type StructAttributes = Vec; @@ -1035,7 +1035,7 @@ impl NodeInterner { } pub fn try_module_parent(&self, module_id: &ModuleId) -> Option { - self.try_module_attributes(module_id).map(|attrs| attrs.parent) + self.try_module_attributes(module_id).and_then(|attrs| attrs.parent) } pub fn global_attributes(&self, global_id: &GlobalId) -> &[SecondaryAttribute] { diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 8d7824502c1..fa39c7a60d2 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -3,7 +3,7 @@ use std::collections::BTreeMap; use lsp_types::{Position, Range, TextEdit}; use noirc_frontend::{ ast::ItemVisibility, - graph::{CrateId, Dependency}, + graph::CrateId, hir::def_map::{CrateDefMap, ModuleId}, macros_api::{ModuleDefId, NodeInterner}, node_interner::ReferenceId, @@ -47,7 +47,6 @@ impl<'a> NodeFinder<'a> { &self.module_id, current_module_parent_id, self.interner, - self.dependencies, ); } else { let Some(parent_module) = get_parent_module(self.interner, *module_def_id) @@ -74,7 +73,6 @@ impl<'a> NodeFinder<'a> { &self.module_id, current_module_parent_id, self.interner, - self.dependencies, ); } @@ -149,7 +147,6 @@ fn module_id_path( current_module_id: &ModuleId, current_module_parent_id: Option, interner: &NodeInterner, - dependencies: &[Dependency], ) -> String { if Some(target_module_id) == current_module_parent_id { return "super".to_string(); @@ -163,8 +160,12 @@ fn module_id_path( let mut current_attributes = module_attributes; loop { + let Some(parent_local_id) = current_attributes.parent else { + break; + }; + let parent_module_id = - &ModuleId { krate: target_module_id.krate, local_id: current_attributes.parent }; + &ModuleId { krate: target_module_id.krate, local_id: parent_local_id }; if current_module_id == parent_module_id { is_relative = true; @@ -186,24 +187,13 @@ fn module_id_path( } } - 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 !is_relative { + // We don't record module attriubtes for the root module, + // so we handle that case separately + if let CrateId::Root(_) = target_module_id.krate { + segments.push("crate"); } - }; - - if let Some(crate_name) = &crate_name { - segments.push(crate_name); - }; + } segments.reverse(); segments.join("::") diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index 8cf06adc027..68d9910f5f9 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1638,4 +1638,22 @@ mod completion_tests { }) ); } + + #[test] + async fn test_auto_import_from_std() { + let src = r#" + fn main() { + compute_merkle_roo>|< + } + "#; + let items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = &items[0]; + assert_eq!(item.label, "compute_merkle_root(…)"); + assert_eq!( + item.label_details.as_ref().unwrap().detail, + Some("(use std::merkle::compute_merkle_root)".to_string()), + ); + } } diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index 5e655766024..6538e64dc90 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -235,4 +235,18 @@ mod goto_definition_tests { ) .await; } + + #[test] + async fn goto_crate() { + expect_goto( + "go_to_definition", + Position { line: 29, character: 6 }, // "dependency" in "use dependency::something" + "dependency/src/lib.nr", + Range { + start: Position { line: 0, character: 0 }, + end: Position { line: 0, character: 0 }, + }, + ) + .await; + } } diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index 95d8b82f84f..52171ca9b23 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -58,6 +58,13 @@ fn format_reference(reference: ReferenceId, args: &ProcessRequestCallbackArgs) - } } fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> Option { + let crate_root = args.def_maps[&id.krate].root(); + + if id.local_id == crate_root { + let dep = args.dependencies.iter().find(|dep| dep.crate_id == id.krate); + return dep.map(|dep| format!(" crate {}", dep.name)); + } + // Note: it's not clear why `try_module_attributes` might return None here, but it happens. // This is a workaround to avoid panicking in that case (which brings the LSP server down). // Cases where this happens are related to generated code, so once that stops happening @@ -65,12 +72,14 @@ fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> Option Some(args.crate_name.clone()), - CrateId::Crate(_) => args - .dependencies - .iter() - .find(|dep| dep.crate_id == crate_id) - .map(|dep| format!("{}", dep.name)), - CrateId::Stdlib(_) => Some("std".to_string()), - CrateId::Dummy => unreachable!("ICE: A dummy CrateId should not be accessible"), - }; - - if let Some(crate_name) = &crate_name { - segments.push(crate_name); + // We don't record module attriubtes for the root module, + // so we handle that case separately + if let CrateId::Root(_) = module.krate { + segments.push(&args.crate_name); }; if segments.is_empty() { @@ -804,4 +811,15 @@ mod hover_tests { ) .await; } + + #[test] + async fn hover_on_crate_segment() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 0, character: 5 }, + " crate one", + ) + .await; + } } diff --git a/tooling/lsp/test_programs/go_to_definition/Nargo.toml b/tooling/lsp/test_programs/go_to_definition/Nargo.toml index c894a050c40..96fc9cab39a 100644 --- a/tooling/lsp/test_programs/go_to_definition/Nargo.toml +++ b/tooling/lsp/test_programs/go_to_definition/Nargo.toml @@ -4,3 +4,4 @@ type = "bin" authors = [""] [dependencies] +dependency = { path = "dependency" } \ No newline at end of file diff --git a/tooling/lsp/test_programs/go_to_definition/dependency/Nargo.toml b/tooling/lsp/test_programs/go_to_definition/dependency/Nargo.toml new file mode 100644 index 00000000000..2e471678a44 --- /dev/null +++ b/tooling/lsp/test_programs/go_to_definition/dependency/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "dependency" +type = "lib" +authors = [""] + +[dependencies] \ No newline at end of file diff --git a/tooling/lsp/test_programs/go_to_definition/dependency/src/lib.nr b/tooling/lsp/test_programs/go_to_definition/dependency/src/lib.nr new file mode 100644 index 00000000000..6833f391999 --- /dev/null +++ b/tooling/lsp/test_programs/go_to_definition/dependency/src/lib.nr @@ -0,0 +1 @@ +pub fn something() {} diff --git a/tooling/lsp/test_programs/go_to_definition/src/main.nr b/tooling/lsp/test_programs/go_to_definition/src/main.nr index 9223fdc0bd3..4550324c182 100644 --- a/tooling/lsp/test_programs/go_to_definition/src/main.nr +++ b/tooling/lsp/test_programs/go_to_definition/src/main.nr @@ -27,3 +27,4 @@ trait Trait { } +use dependency::something;