From b65c7db11783ad28bc65c36759f940db28304948 Mon Sep 17 00:00:00 2001 From: Koby Date: Wed, 27 Dec 2023 14:00:14 +0100 Subject: [PATCH 1/3] chore: parse and collect definitions on doc open --- tooling/lsp/src/notifications/mod.rs | 53 +++++++++++++++------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 6cefaa134ce..723243c3368 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -36,7 +36,13 @@ pub(super) fn on_did_open_text_document( params: DidOpenTextDocumentParams, ) -> ControlFlow> { state.input_files.insert(params.text_document.uri.to_string(), params.text_document.text); - ControlFlow::Continue(()) + + let document_uri = params.text_document.uri; + + match process_noir_document(document_uri, state) { + Ok(_) => ControlFlow::Continue(()), + Err(err) => ControlFlow::Break(Err(err)), + } } pub(super) fn on_did_change_text_document( @@ -92,27 +98,25 @@ pub(super) fn on_did_save_text_document( state: &mut LspState, params: DidSaveTextDocumentParams, ) -> ControlFlow> { - let file_path = match params.text_document.uri.to_file_path() { - Ok(file_path) => file_path, - Err(()) => { - return ControlFlow::Break(Err(ResponseError::new( - ErrorCode::REQUEST_FAILED, - "URI is not a valid file path", - ) - .into())) - } - }; + let document_uri = params.text_document.uri; - let workspace = match resolve_workspace_for_source_path(&file_path) { - Ok(value) => value, - Err(lsp_error) => { - return ControlFlow::Break(Err(ResponseError::new( - ErrorCode::REQUEST_FAILED, - lsp_error.to_string(), - ) - .into())) - } - }; + match process_noir_document(document_uri, state) { + Ok(_) => ControlFlow::Continue(()), + Err(err) => ControlFlow::Break(Err(err)), + } +} + +fn process_noir_document( + document_uri: lsp_types::Url, + state: &mut LspState, +) -> Result<(), async_lsp::Error> { + let file_path = document_uri.to_file_path().map_err(|_| { + ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") + })?; + + let workspace = resolve_workspace_for_source_path(&file_path).map_err(|lsp_error| { + ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string()) + })?; let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); @@ -142,7 +146,7 @@ pub(super) fn on_did_save_text_document( package, Some(&file_path), ); - state.cached_lenses.insert(params.text_document.uri.to_string(), collected_lenses); + state.cached_lenses.insert(document_uri.to_string(), collected_lenses); let fm = &context.file_manager; let files = fm.as_file_map(); @@ -178,14 +182,13 @@ pub(super) fn on_did_save_text_document( .collect() }) .collect(); - let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { - uri: params.text_document.uri, + uri: document_uri, version: None, diagnostics, }); - ControlFlow::Continue(()) + Ok(()) } pub(super) fn on_exit( From dfe2403ca4e8df3b450be648f769f1d9491a0e15 Mon Sep 17 00:00:00 2001 From: Koby Date: Tue, 2 Jan 2024 19:47:37 +0100 Subject: [PATCH 2/3] feat(lsp): cache definitions after parse --- compiler/noirc_frontend/src/hir_def/traits.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 4 ++-- tooling/lsp/src/lib.rs | 5 +++++ tooling/lsp/src/notifications/mod.rs | 16 +++++++++++++++- tooling/lsp/src/requests/goto_definition.rs | 10 ++++++++-- 5 files changed, 31 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index ea9c2e2928c..056d5343a08 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -35,7 +35,7 @@ pub struct TraitType { /// Represents a trait in the type system. Each instance of this struct /// will be shared across all Type::Trait variants that represent /// the same trait. -#[derive(Debug, Eq)] +#[derive(Clone, Debug, Eq)] pub struct Trait { /// A unique id representing this trait type. Used to check if two /// struct traits are equal. diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 236f1e0b513..68b0cd37b89 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -37,7 +37,7 @@ type StructAttributes = Vec; /// each definition or struct, etc. Because it is used on the Hir, the NodeInterner is /// useful in passes where the Hir is used - name resolution, type checking, and /// monomorphization - and it is not useful afterward. -#[derive(Debug)] +#[derive(Clone, Debug)] pub struct NodeInterner { nodes: Arena, func_meta: HashMap, @@ -158,7 +158,7 @@ pub enum TraitImplKind { /// /// Additionally, types can define specialized impls with methods of the same name /// as long as these specialized impls do not overlap. E.g. `impl Struct` and `impl Struct` -#[derive(Default, Debug)] +#[derive(Clone, Default, Debug)] pub struct Methods { direct: Vec, trait_impl_methods: Vec, diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 9887e5b8e96..cc541aaa033 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -25,6 +25,7 @@ use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; use noirc_frontend::{ graph::{CrateId, CrateName}, hir::{Context, FunctionNameMatch}, + node_interner::NodeInterner, }; use fm::FileManager; @@ -61,8 +62,10 @@ pub struct LspState { root_path: Option, client: ClientSocket, solver: WrapperSolver, + open_documents_count: usize, input_files: HashMap, cached_lenses: HashMap>, + cached_definitions: HashMap, } impl LspState { @@ -73,6 +76,8 @@ impl LspState { solver: WrapperSolver(Box::new(solver)), input_files: HashMap::new(), cached_lenses: HashMap::new(), + cached_definitions: HashMap::new(), + open_documents_count: 0, } } } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 723243c3368..e37a78df704 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -40,7 +40,10 @@ pub(super) fn on_did_open_text_document( let document_uri = params.text_document.uri; match process_noir_document(document_uri, state) { - Ok(_) => ControlFlow::Continue(()), + Ok(_) => { + state.open_documents_count += 1; + ControlFlow::Continue(()) + } Err(err) => ControlFlow::Break(Err(err)), } } @@ -91,6 +94,13 @@ pub(super) fn on_did_close_text_document( ) -> ControlFlow> { state.input_files.remove(¶ms.text_document.uri.to_string()); state.cached_lenses.remove(¶ms.text_document.uri.to_string()); + + state.open_documents_count -= 1; + + if state.open_documents_count == 0 { + state.cached_definitions.clear(); + } + ControlFlow::Continue(()) } @@ -131,6 +141,10 @@ fn process_noir_document( Err(errors_and_warnings) => errors_and_warnings, }; + let package_root_dir: String = package.root_dir.as_os_str().to_string_lossy().into(); + + state.cached_definitions.insert(package_root_dir, context.def_interner.clone()); + // We don't add test headings for a package if it contains no `#[test]` functions if let Some(tests) = get_package_tests_in_crate(&context, &crate_id, &package.name) { let _ = state.client.notify::(NargoPackageTests { diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index d2b66682d89..ded5cb727af 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -29,13 +29,19 @@ fn on_goto_definition_inner( let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); let package = workspace.members.first().unwrap(); + let package_root_path: String = package.root_dir.as_os_str().to_string_lossy().into(); + let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package); - // We ignore the warnings and errors produced by compilation while resolving the definition - let _ = noirc_driver::check_crate(&mut context, crate_id, false, false); + if let Some(def_interner) = _state.cached_definitions.get(&package_root_path) { + context.def_interner = def_interner.clone(); + } else { + // We ignore the warnings and errors produced by compilation while resolving the definition + let _ = noirc_driver::check_crate(&mut context, crate_id, false, false); + } let files = context.file_manager.as_file_map(); let file_id = context.file_manager.name_to_id(file_path.clone()).ok_or(ResponseError::new( From f821f0671095a87fc08146014fe25d9e7b126ece Mon Sep 17 00:00:00 2001 From: Koby Date: Thu, 4 Jan 2024 12:57:35 +0100 Subject: [PATCH 3/3] chore: drop clone on node_interner --- compiler/noirc_frontend/src/hir/mod.rs | 8 -------- compiler/noirc_frontend/src/hir_def/traits.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 12 +++++++++--- tooling/lsp/src/notifications/mod.rs | 4 ++-- tooling/lsp/src/requests/goto_definition.rs | 7 ++++--- 5 files changed, 16 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index 25875385fd9..3683b17a27c 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -194,14 +194,6 @@ impl Context<'_> { .collect() } - /// Returns the [Location] of the definition of the given Ident found at [Span] of the given [FileId]. - /// Returns [None] when definition is not found. - pub fn get_definition_location_from(&self, location: Location) -> Option { - let interner = &self.def_interner; - - interner.find_location_index(location).and_then(|index| interner.resolve_location(index)) - } - /// Return a Vec of all `contract` declarations in the source code and the functions they contain pub fn get_all_contracts(&self, crate_id: &CrateId) -> Vec { self.def_map(crate_id) diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index 056d5343a08..ea9c2e2928c 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -35,7 +35,7 @@ pub struct TraitType { /// Represents a trait in the type system. Each instance of this struct /// will be shared across all Type::Trait variants that represent /// the same trait. -#[derive(Clone, Debug, Eq)] +#[derive(Debug, Eq)] pub struct Trait { /// A unique id representing this trait type. Used to check if two /// struct traits are equal. diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 68b0cd37b89..2a07c41aa50 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -37,7 +37,7 @@ type StructAttributes = Vec; /// each definition or struct, etc. Because it is used on the Hir, the NodeInterner is /// useful in passes where the Hir is used - name resolution, type checking, and /// monomorphization - and it is not useful afterward. -#[derive(Clone, Debug)] +#[derive(Debug)] pub struct NodeInterner { nodes: Arena, func_meta: HashMap, @@ -158,7 +158,7 @@ pub enum TraitImplKind { /// /// Additionally, types can define specialized impls with methods of the same name /// as long as these specialized impls do not overlap. E.g. `impl Struct` and `impl Struct` -#[derive(Clone, Default, Debug)] +#[derive(Default, Debug)] pub struct Methods { direct: Vec, trait_impl_methods: Vec, @@ -1264,10 +1264,16 @@ impl NodeInterner { self.selected_trait_implementations.get(&ident_id).cloned() } + /// Returns the [Location] of the definition of the given Ident found at [Span] of the given [FileId]. + /// Returns [None] when definition is not found. + pub fn get_definition_location_from(&self, location: Location) -> Option { + self.find_location_index(location).and_then(|index| self.resolve_location(index)) + } + /// For a given [Index] we return [Location] to which we resolved to /// We currently return None for features not yet implemented /// TODO(#3659): LSP goto def should error when Ident at Location could not resolve - pub(crate) fn resolve_location(&self, index: impl Into) -> Option { + fn resolve_location(&self, index: impl Into) -> Option { let node = self.nodes.get(index.into())?; match node { diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index e37a78df704..aec3bf61f4e 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -143,8 +143,6 @@ fn process_noir_document( let package_root_dir: String = package.root_dir.as_os_str().to_string_lossy().into(); - state.cached_definitions.insert(package_root_dir, context.def_interner.clone()); - // We don't add test headings for a package if it contains no `#[test]` functions if let Some(tests) = get_package_tests_in_crate(&context, &crate_id, &package.name) { let _ = state.client.notify::(NargoPackageTests { @@ -162,6 +160,8 @@ fn process_noir_document( ); state.cached_lenses.insert(document_uri.to_string(), collected_lenses); + state.cached_definitions.insert(package_root_dir, context.def_interner); + let fm = &context.file_manager; let files = fm.as_file_map(); diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index ded5cb727af..2ff5901ff9c 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -36,11 +36,13 @@ fn on_goto_definition_inner( let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package); + let interner; if let Some(def_interner) = _state.cached_definitions.get(&package_root_path) { - context.def_interner = def_interner.clone(); + interner = def_interner; } else { // We ignore the warnings and errors produced by compilation while resolving the definition let _ = noirc_driver::check_crate(&mut context, crate_id, false, false); + interner = &context.def_interner; } let files = context.file_manager.as_file_map(); @@ -48,7 +50,6 @@ fn on_goto_definition_inner( ErrorCode::REQUEST_FAILED, format!("Could not find file in file manager. File path: {:?}", file_path), ))?; - let byte_index = position_to_byte_index(files, file_id, ¶ms.text_document_position_params.position) .map_err(|err| { @@ -64,7 +65,7 @@ fn on_goto_definition_inner( }; let goto_definition_response = - context.get_definition_location_from(search_for_location).and_then(|found_location| { + interner.get_definition_location_from(search_for_location).and_then(|found_location| { let file_id = found_location.file; let definition_position = to_lsp_location(files, file_id, found_location.span)?; let response: GotoDefinitionResponse =