From 951e821a585fe7e0697291cadd4d3c3aa49fd8e4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 15 Jul 2024 19:57:31 -0300 Subject: [PATCH] fix: lsp hover wasn't always working (#5515) # Description ## Problem Resolves #5516 ## Summary It seems hover broke in 70dabfa454516b839b57554fb2e2574a84748d66 . However, our tests still pass. I still don't understand why it only breaks when the request comes from an actual VS Code instance and not in tests... but for now I'd rather revert the commit that broke it and eventually try to do the refactor again, because having hover is pretty nice :-) ## Additional Context None. ## 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> --- .../src/hir/def_collector/dc_mod.rs | 6 ++- compiler/noirc_frontend/src/node_interner.rs | 5 +++ tooling/lsp/src/requests/hover.rs | 37 +++++++------------ tooling/lsp/src/requests/mod.rs | 13 +------ 4 files changed, 26 insertions(+), 35 deletions(-) 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 12f4aafb304..1f30b4388b6 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -732,7 +732,11 @@ impl<'a> ModCollector<'a> { context.def_interner.add_module_attributes( mod_id, - ModuleAttributes { name: mod_name.0.contents.clone(), location: mod_location }, + ModuleAttributes { + name: mod_name.0.contents.clone(), + location: mod_location, + parent: self.module_id, + }, ); } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 6520dc6aa1f..5ed4ad85a58 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -48,6 +48,7 @@ const IMPL_SEARCH_RECURSION_LIMIT: u32 = 10; pub struct ModuleAttributes { pub name: String, pub location: Location, + pub parent: LocalModuleId, } type StructAttributes = Vec; @@ -1016,6 +1017,10 @@ impl NodeInterner { self.module_attributes.get(module_id) } + pub fn try_module_parent(&self, module_id: &ModuleId) -> Option { + self.try_module_attributes(module_id).map(|attrs| attrs.parent) + } + pub fn global_attributes(&self, global_id: &GlobalId) -> &[SecondaryAttribute] { &self.global_attributes[global_id] } diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index 729fe85e70e..161fd20f555 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -56,17 +56,14 @@ fn format_reference(reference: ReferenceId, args: &ProcessRequestCallbackArgs) - } fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> String { let module_attributes = args.interner.module_attributes(&id); - let parent_module_id = args.def_maps[&id.krate].modules()[id.local_id.0].parent; let mut string = String::new(); - if let Some(parent_module_id) = parent_module_id { - if format_parent_module_from_module_id( - &ModuleId { krate: id.krate, local_id: parent_module_id }, - args, - &mut string, - ) { - string.push('\n'); - } + if format_parent_module_from_module_id( + &ModuleId { krate: id.krate, local_id: module_attributes.parent }, + args, + &mut string, + ) { + string.push('\n'); } string.push_str(" "); string.push_str("mod "); @@ -319,6 +316,7 @@ fn format_parent_module_from_module_id( CrateId::Stdlib(_) => Some("std".to_string()), CrateId::Dummy => None, }; + let wrote_crate = if let Some(crate_name) = crate_name { string.push_str(" "); string.push_str(&crate_name); @@ -331,9 +329,6 @@ fn format_parent_module_from_module_id( return wrote_crate; }; - let modules = args.def_maps[&module.krate].modules(); - let module_data = &modules[module.local_id.0]; - if wrote_crate { string.push_str("::"); } else { @@ -341,17 +336,13 @@ fn format_parent_module_from_module_id( } let mut segments = Vec::new(); - let mut current_parent = module_data.parent; - while let Some(parent) = current_parent { - let parent_attributes = args - .interner - .try_module_attributes(&ModuleId { krate: module.krate, local_id: parent }); - if let Some(parent_attributes) = parent_attributes { - segments.push(&parent_attributes.name); - current_parent = modules[module.local_id.0].parent; - } else { - break; - } + 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() { diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index fbdfbc1cb1a..e029750e589 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -1,7 +1,4 @@ -use std::{ - collections::{BTreeMap, HashMap}, - future::Future, -}; +use std::{collections::HashMap, future::Future}; use crate::{ parse_diff, resolve_workspace_for_source_path, @@ -17,11 +14,7 @@ use lsp_types::{ use nargo::insert_all_files_for_workspace_into_file_manager; use nargo_fmt::Config; use noirc_driver::file_manager_with_stdlib; -use noirc_frontend::{ - graph::{CrateId, Dependency}, - hir::def_map::CrateDefMap, - macros_api::NodeInterner, -}; +use noirc_frontend::{graph::Dependency, macros_api::NodeInterner}; use serde::{Deserialize, Serialize}; use crate::{ @@ -285,7 +278,6 @@ pub(crate) struct ProcessRequestCallbackArgs<'a> { interners: &'a HashMap, root_crate_name: String, root_crate_dependencies: &'a Vec, - def_maps: &'a BTreeMap, } pub(crate) fn process_request( @@ -340,7 +332,6 @@ where interners: &state.cached_definitions, root_crate_name: package.name.to_string(), root_crate_dependencies: &context.crate_graph[context.root_crate_id()].dependencies, - def_maps: &context.def_maps, })) } pub(crate) fn find_all_references_in_workspace(