From b91a34a8fba6cfb15490d360e771e4eb59cc0d3e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 27 Jul 2025 11:56:40 -0300 Subject: [PATCH 1/6] chore(LSP): avoid unnecessary compilations --- tooling/lsp/src/lib.rs | 4 ++ tooling/lsp/src/notifications/mod.rs | 94 ++++++++++++++++++-------- tooling/lsp/src/requests/mod.rs | 6 ++ tooling/lsp/src/requests/references.rs | 2 +- 4 files changed, 78 insertions(+), 28 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 522a9d3d175..a1c67b08a87 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -29,6 +29,7 @@ use nargo::{ }; use nargo_toml::{PackageSelection, find_file_manifest, resolve_workspace_from_toml}; use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; +use noirc_errors::CustomDiagnostic; use noirc_frontend::{ ParsedModule, graph::{CrateGraph, CrateId, CrateName}, @@ -96,6 +97,7 @@ pub struct LspState { workspace_cache: HashMap, package_cache: HashMap, workspace_symbol_cache: WorkspaceSymbolCache, + work_queue: HashSet, options: LspInitializationOptions, // Tracks files that currently have errors, by package root. @@ -112,6 +114,7 @@ struct PackageCacheData { node_interner: NodeInterner, def_maps: BTreeMap, usage_tracker: UsageTracker, + diagnostics: Vec, } impl LspState { @@ -129,6 +132,7 @@ impl LspState { package_cache: HashMap::new(), workspace_symbol_cache: WorkspaceSymbolCache::default(), open_documents_count: 0, + work_queue: HashSet::new(), options: Default::default(), files_with_errors: HashMap::new(), } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index bcdcd9eb954..5c3aa8b1b62 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -1,6 +1,6 @@ -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::ops::ControlFlow; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use crate::{ PackageCacheData, WorkspaceCacheData, insert_all_files_for_workspace_into_file_manager, @@ -9,7 +9,7 @@ use async_lsp::lsp_types; use async_lsp::lsp_types::{DiagnosticRelatedInformation, DiagnosticTag, Url}; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; use fm::{FileManager, FileMap}; -use fxhash::FxHashMap as HashMap; +use nargo::workspace::Workspace; use noirc_driver::check_crate; use noirc_errors::reporter::CustomLabel; use noirc_errors::{CustomDiagnostic, DiagnosticKind, Location}; @@ -44,15 +44,12 @@ pub(crate) fn on_did_open_text_document( params: DidOpenTextDocumentParams, ) -> ControlFlow> { state.input_files.insert(params.text_document.uri.to_string(), params.text_document.text); + state.open_documents_count += 1; let document_uri = params.text_document.uri; - let output_diagnostics = true; - match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { - Ok(_) => { - state.open_documents_count += 1; - ControlFlow::Continue(()) - } + match handle_text_document_notification(state, document_uri) { + Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } } @@ -66,9 +63,8 @@ pub(super) fn on_did_change_text_document( state.workspace_symbol_cache.reprocess_uri(¶ms.text_document.uri); let document_uri = params.text_document.uri; - let output_diagnostics = false; - match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { + match handle_text_document_notification(state, document_uri) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -89,9 +85,8 @@ pub(super) fn on_did_close_text_document( } let document_uri = params.text_document.uri; - let output_diagnostics = false; - match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { + match handle_text_document_notification(state, document_uri) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -101,23 +96,53 @@ pub(super) fn on_did_save_text_document( state: &mut LspState, params: DidSaveTextDocumentParams, ) -> ControlFlow> { - let document_uri = params.text_document.uri; - let output_diagnostics = true; + let workspace = match workspace_from_document_uri(params.text_document.uri) { + Ok(workspace) => workspace, + Err(err) => return ControlFlow::Break(Err(err)), + }; - match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { - Ok(_) => ControlFlow::Continue(()), - Err(err) => ControlFlow::Break(Err(err)), - } + let Some(package_cache) = state.package_cache.get(&workspace.root_dir) else { + let output_diagnostics = true; + return match process_workspace_for_noir_document( + state, + &workspace.root_dir, + output_diagnostics, + ) { + Ok(_) => ControlFlow::Continue(()), + Err(err) => return ControlFlow::Break(Err(err)), + }; + }; + + let mut workspace_file_manager = workspace.new_file_manager(); + + insert_all_files_for_workspace_into_file_manager( + state, + &workspace, + &mut workspace_file_manager, + ); + + publish_diagnostics( + state, + &workspace.root_dir, + &workspace_file_manager, + package_cache.diagnostics.clone(), + ); + + ControlFlow::Continue(()) } -// Given a Noir document, find the workspace it's contained in (an assumed workspace is created if -// it's only contained in a package), then type-checks the workspace's packages, -// caching code lenses and type definitions, and notifying about compilation errors. -pub(crate) fn process_workspace_for_noir_document( +fn handle_text_document_notification( state: &mut LspState, document_uri: Url, - output_diagnostics: bool, ) -> Result<(), async_lsp::Error> { + let workspace = workspace_from_document_uri(document_uri.clone())?; + + state.work_queue.insert(workspace.root_dir.clone()); + + Ok(()) +} + +fn workspace_from_document_uri(document_uri: Url) -> Result { let file_path = document_uri.to_file_path().map_err(|_| { ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") })?; @@ -126,6 +151,21 @@ pub(crate) fn process_workspace_for_noir_document( ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string()) })?; + Ok(workspace) +} + +// Given a Noir document, find the workspace it's contained in (an assumed workspace is created if +// it's only contained in a package), then type-checks the workspace's packages, +// caching code lenses and type definitions, and notifying about compilation errors. +pub(crate) fn process_workspace_for_noir_document( + state: &mut LspState, + file_path: &Path, + output_diagnostics: bool, +) -> Result<(), async_lsp::Error> { + 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 = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager( @@ -161,14 +201,14 @@ pub(crate) fn process_workspace_for_noir_document( node_interner: context.def_interner, def_maps: context.def_maps, usage_tracker: context.usage_tracker, + diagnostics: file_diagnostics.clone(), }, ); let fm = &context.file_manager; - let files = fm.as_file_map(); if output_diagnostics { - publish_diagnostics(state, &package.root_dir, files, fm, file_diagnostics); + publish_diagnostics(state, &package.root_dir, fm, file_diagnostics); } } @@ -183,10 +223,10 @@ pub(crate) fn process_workspace_for_noir_document( fn publish_diagnostics( state: &mut LspState, package_root_dir: &PathBuf, - files: &FileMap, fm: &FileManager, custom_diagnostics: Vec, ) { + let files = fm.as_file_map(); let mut diagnostics_per_url: HashMap> = HashMap::default(); for custom_diagnostic in custom_diagnostics.into_iter() { diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 2e522912d26..462d16f05a6 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -2,6 +2,7 @@ use std::collections::BTreeMap; use std::path::{Path, PathBuf}; use std::{collections::HashMap, future::Future}; +use crate::notifications::process_workspace_for_noir_document; use crate::{PackageCacheData, insert_all_files_for_workspace_into_file_manager, parse_diff}; use crate::{ resolve_workspace_for_source_path, @@ -488,6 +489,11 @@ where })?; let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); + + if state.work_queue.remove(&workspace.root_dir) { + let _ = process_workspace_for_noir_document(state, &workspace.root_dir, false); + } + let package = crate::workspace_package_for_file(&workspace, &file_path).ok_or_else(|| { ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not find package for file") })?; diff --git a/tooling/lsp/src/requests/references.rs b/tooling/lsp/src/requests/references.rs index 66edf3cbd63..2d1a8166172 100644 --- a/tooling/lsp/src/requests/references.rs +++ b/tooling/lsp/src/requests/references.rs @@ -116,7 +116,7 @@ mod references_tests { notifications::process_workspace_for_noir_document( &mut state, - one_lib.clone(), + &one_lib.to_file_path().unwrap(), output_diagnostics, ) .unwrap(); From 98abdbbbf3b06aa2e1d68905bbca52264cc2f74f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 27 Jul 2025 12:12:34 -0300 Subject: [PATCH 2/6] Type-check on save if we just published diagnostics --- tooling/lsp/src/lib.rs | 1 + tooling/lsp/src/notifications/mod.rs | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index a1c67b08a87..41ca361fcae 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -115,6 +115,7 @@ struct PackageCacheData { def_maps: BTreeMap, usage_tracker: UsageTracker, diagnostics: Vec, + diagnostics_just_published: bool, } impl LspState { diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 5c3aa8b1b62..898e5dd14dd 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -101,6 +101,10 @@ pub(super) fn on_did_save_text_document( Err(err) => return ControlFlow::Break(Err(err)), }; + if state.work_queue.remove(&workspace.root_dir) { + let _ = process_workspace_for_noir_document(state, &workspace.root_dir, false); + } + let Some(package_cache) = state.package_cache.get(&workspace.root_dir) else { let output_diagnostics = true; return match process_workspace_for_noir_document( @@ -113,6 +117,18 @@ pub(super) fn on_did_save_text_document( }; }; + if package_cache.diagnostics_just_published { + let output_diagnostics = true; + return match process_workspace_for_noir_document( + state, + &workspace.root_dir, + output_diagnostics, + ) { + Ok(_) => ControlFlow::Continue(()), + Err(err) => return ControlFlow::Break(Err(err)), + }; + } + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager( @@ -128,6 +144,10 @@ pub(super) fn on_did_save_text_document( package_cache.diagnostics.clone(), ); + if let Some(package_cache) = state.package_cache.get_mut(&workspace.root_dir) { + package_cache.diagnostics_just_published = true; + } + ControlFlow::Continue(()) } @@ -202,6 +222,7 @@ pub(crate) fn process_workspace_for_noir_document( def_maps: context.def_maps, usage_tracker: context.usage_tracker, diagnostics: file_diagnostics.clone(), + diagnostics_just_published: output_diagnostics, }, ); From 4aea74f93a0a427fbb9916780bac9ede886f6428 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 27 Jul 2025 12:21:17 -0300 Subject: [PATCH 3/6] Comments and renames --- tooling/lsp/src/lib.rs | 7 +++++-- tooling/lsp/src/notifications/mod.rs | 10 ++++++++-- tooling/lsp/src/requests/mod.rs | 3 ++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 41ca361fcae..5a528843f4d 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -97,7 +97,10 @@ pub struct LspState { workspace_cache: HashMap, package_cache: HashMap, workspace_symbol_cache: WorkspaceSymbolCache, - work_queue: HashSet, + + /// Whenever a file in a workspace is changed we'll add it to this set. + workspaces_to_process: HashSet, + options: LspInitializationOptions, // Tracks files that currently have errors, by package root. @@ -133,7 +136,7 @@ impl LspState { package_cache: HashMap::new(), workspace_symbol_cache: WorkspaceSymbolCache::default(), open_documents_count: 0, - work_queue: HashSet::new(), + workspaces_to_process: HashSet::new(), options: Default::default(), files_with_errors: HashMap::new(), } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 898e5dd14dd..d2805aa6a5e 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -101,10 +101,12 @@ pub(super) fn on_did_save_text_document( Err(err) => return ControlFlow::Break(Err(err)), }; - if state.work_queue.remove(&workspace.root_dir) { + // Process any pending change + if state.workspaces_to_process.remove(&workspace.root_dir) { let _ = process_workspace_for_noir_document(state, &workspace.root_dir, false); } + // A package cache should be here but, if it doesn't, we'll just type-check and output diagnostics let Some(package_cache) = state.package_cache.get(&workspace.root_dir) else { let output_diagnostics = true; return match process_workspace_for_noir_document( @@ -117,6 +119,9 @@ pub(super) fn on_did_save_text_document( }; }; + // If the last thing the user did was to save a file in the workspace, it could be that + // the underlying files in the filesystem have changed (for example a `git checkout`), + // so here we force a type-check just in case. if package_cache.diagnostics_just_published { let output_diagnostics = true; return match process_workspace_for_noir_document( @@ -129,6 +134,7 @@ pub(super) fn on_did_save_text_document( }; } + // Otherwise, we can publish the diagnostics we computed in the last type-check let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager( @@ -157,7 +163,7 @@ fn handle_text_document_notification( ) -> Result<(), async_lsp::Error> { let workspace = workspace_from_document_uri(document_uri.clone())?; - state.work_queue.insert(workspace.root_dir.clone()); + state.workspaces_to_process.insert(workspace.root_dir.clone()); Ok(()) } diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 462d16f05a6..32398847c1f 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -490,7 +490,8 @@ where let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); - if state.work_queue.remove(&workspace.root_dir) { + // First type-check the workspace if needed + if state.workspaces_to_process.remove(&workspace.root_dir) { let _ = process_workspace_for_noir_document(state, &workspace.root_dir, false); } From 31ddfc1a10b3aa627e5566b3e076f7307a58e772 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 27 Jul 2025 12:37:44 -0300 Subject: [PATCH 4/6] Don't keep track of open document count --- tooling/lsp/src/lib.rs | 2 -- tooling/lsp/src/notifications/mod.rs | 8 -------- 2 files changed, 10 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 5a528843f4d..e1c7bd364fb 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -91,7 +91,6 @@ pub struct LspState { root_path: Option, client: ClientSocket, solver: WrapperSolver, - open_documents_count: usize, input_files: HashMap, cached_parsed_files: HashMap))>, workspace_cache: HashMap, @@ -135,7 +134,6 @@ impl LspState { workspace_cache: HashMap::new(), package_cache: HashMap::new(), workspace_symbol_cache: WorkspaceSymbolCache::default(), - open_documents_count: 0, workspaces_to_process: HashSet::new(), options: Default::default(), files_with_errors: HashMap::new(), diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index d2805aa6a5e..adacf010914 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -44,7 +44,6 @@ pub(crate) fn on_did_open_text_document( params: DidOpenTextDocumentParams, ) -> ControlFlow> { state.input_files.insert(params.text_document.uri.to_string(), params.text_document.text); - state.open_documents_count += 1; let document_uri = params.text_document.uri; @@ -77,13 +76,6 @@ pub(super) fn on_did_close_text_document( state.input_files.remove(¶ms.text_document.uri.to_string()); state.workspace_symbol_cache.reprocess_uri(¶ms.text_document.uri); - state.open_documents_count -= 1; - - if state.open_documents_count == 0 { - state.package_cache.clear(); - state.workspace_cache.clear(); - } - let document_uri = params.text_document.uri; match handle_text_document_notification(state, document_uri) { From f92dc3655d90793ac6a0cb9465f74fd69ab3c067 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 27 Jul 2025 12:52:22 -0300 Subject: [PATCH 5/6] Better type-checking logic when opening/closing files --- tooling/lsp/src/notifications/mod.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index adacf010914..36b7475ae06 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -46,8 +46,9 @@ pub(crate) fn on_did_open_text_document( state.input_files.insert(params.text_document.uri.to_string(), params.text_document.text); let document_uri = params.text_document.uri; + let change = false; - match handle_text_document_notification(state, document_uri) { + match handle_text_document_notification(state, document_uri, change) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -62,8 +63,9 @@ pub(super) fn on_did_change_text_document( state.workspace_symbol_cache.reprocess_uri(¶ms.text_document.uri); let document_uri = params.text_document.uri; + let change = true; - match handle_text_document_notification(state, document_uri) { + match handle_text_document_notification(state, document_uri, change) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -77,8 +79,9 @@ pub(super) fn on_did_close_text_document( state.workspace_symbol_cache.reprocess_uri(¶ms.text_document.uri); let document_uri = params.text_document.uri; + let change = false; - match handle_text_document_notification(state, document_uri) { + match handle_text_document_notification(state, document_uri, change) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -152,12 +155,22 @@ pub(super) fn on_did_save_text_document( fn handle_text_document_notification( state: &mut LspState, document_uri: Url, + change: bool, ) -> Result<(), async_lsp::Error> { let workspace = workspace_from_document_uri(document_uri.clone())?; - state.workspaces_to_process.insert(workspace.root_dir.clone()); - - Ok(()) + if state.package_cache.contains_key(&workspace.root_dir) { + // If we have cached data but the file didn't change there's nothing to do + if change { + state.workspaces_to_process.insert(workspace.root_dir.clone()); + } + Ok(()) + } else { + // If it's the first time we see this package, show diagnostics. + // This can happen for example when a user opens a Noir file in a package for the first time. + let output_diagnostics = true; + process_workspace_for_noir_document(state, &workspace.root_dir, output_diagnostics) + } } fn workspace_from_document_uri(document_uri: Url) -> Result { From a1df3f936630d97ca40f203d3b1761c7e186d701 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 27 Jul 2025 15:09:12 -0300 Subject: [PATCH 6/6] Reuse existing file manager when saving --- tooling/lsp/src/notifications/mod.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 36b7475ae06..45ca05a8154 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -96,13 +96,16 @@ pub(super) fn on_did_save_text_document( Err(err) => return ControlFlow::Break(Err(err)), }; - // Process any pending change + // Process any pending changes if state.workspaces_to_process.remove(&workspace.root_dir) { let _ = process_workspace_for_noir_document(state, &workspace.root_dir, false); } - // A package cache should be here but, if it doesn't, we'll just type-check and output diagnostics - let Some(package_cache) = state.package_cache.get(&workspace.root_dir) else { + // Cached data should be here but, if it doesn't, we'll just type-check and output diagnostics + let (Some(workspace_cache), Some(package_cache)) = ( + state.workspace_cache.get(&workspace.root_dir), + state.package_cache.get(&workspace.root_dir), + ) else { let output_diagnostics = true; return match process_workspace_for_noir_document( state, @@ -130,18 +133,10 @@ pub(super) fn on_did_save_text_document( } // Otherwise, we can publish the diagnostics we computed in the last type-check - let mut workspace_file_manager = workspace.new_file_manager(); - - insert_all_files_for_workspace_into_file_manager( - state, - &workspace, - &mut workspace_file_manager, - ); - publish_diagnostics( state, &workspace.root_dir, - &workspace_file_manager, + &workspace_cache.file_manager.clone(), package_cache.diagnostics.clone(), );