From 76549dfda84ba2fa966134bbf8185c0540461d5f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 09:07:18 -0300 Subject: [PATCH 1/4] LSP: report diagnostics for all files in a package --- tooling/lsp/src/lib.rs | 7 +- tooling/lsp/src/notifications/mod.rs | 200 +++++++++++++++------------ 2 files changed, 120 insertions(+), 87 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 4a764f4268b..d8476736d5a 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -4,7 +4,7 @@ #![cfg_attr(not(test), warn(unused_crate_dependencies, unused_extern_crates))] use std::{ - collections::{BTreeMap, HashMap}, + collections::{BTreeMap, HashMap, HashSet}, future::Future, ops::{self, ControlFlow}, path::{Path, PathBuf}, @@ -95,6 +95,9 @@ pub struct LspState { cached_parsed_files: HashMap))>, cached_def_maps: HashMap>, options: LspInitializationOptions, + + // Tracks files that currently have errors, by package root. + files_with_errors: HashMap>, } impl LspState { @@ -113,6 +116,8 @@ impl LspState { cached_parsed_files: HashMap::new(), cached_def_maps: HashMap::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 d1ffdb55066..b25255f457c 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -1,8 +1,10 @@ +use std::collections::HashSet; use std::ops::ControlFlow; use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; -use lsp_types::DiagnosticTag; +use fxhash::FxHashMap as HashMap; +use lsp_types::{DiagnosticTag, Url}; use noirc_driver::{check_crate, file_manager_with_stdlib}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; @@ -105,7 +107,7 @@ pub(super) fn on_did_save_text_document( // caching code lenses and type definitions, and notifying about compilation errors. pub(crate) fn process_workspace_for_noir_document( state: &mut LspState, - document_uri: lsp_types::Url, + document_uri: Url, output_diagnostics: bool, ) -> Result<(), async_lsp::Error> { let file_path = document_uri.to_file_path().map_err(|_| { @@ -125,97 +127,108 @@ pub(crate) fn process_workspace_for_noir_document( let parsed_files = parse_diff(&workspace_file_manager, state); - let diagnostics: Vec<_> = workspace - .into_iter() - .flat_map(|package| -> Vec { - let package_root_dir: String = package.root_dir.as_os_str().to_string_lossy().into(); + for package in workspace.into_iter() { + let package_root_dir: String = package.root_dir.as_os_str().to_string_lossy().into(); - let (mut context, crate_id) = - crate::prepare_package(&workspace_file_manager, &parsed_files, package); + let (mut context, crate_id) = + crate::prepare_package(&workspace_file_manager, &parsed_files, package); - let file_diagnostics = match check_crate(&mut context, crate_id, &Default::default()) { - Ok(((), warnings)) => warnings, - Err(errors_and_warnings) => errors_and_warnings, + let file_diagnostics = match check_crate(&mut context, crate_id, &Default::default()) { + Ok(((), warnings)) => warnings, + Err(errors_and_warnings) => errors_and_warnings, + }; + + // 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 { + package: package.name.to_string(), + tests, + }); + } + + let collected_lenses = crate::requests::collect_lenses_for_package( + &context, + crate_id, + &workspace, + package, + Some(&file_path), + ); + state.cached_lenses.insert(document_uri.to_string(), collected_lenses); + state.cached_definitions.insert(package_root_dir.clone(), context.def_interner); + state.cached_def_maps.insert(package_root_dir.clone(), context.def_maps); + + let fm = &context.file_manager; + let files = fm.as_file_map(); + + if !output_diagnostics { + continue; + } + + let mut diagnostics_per_url: HashMap> = HashMap::default(); + + for FileDiagnostic { file_id, diagnostic, call_stack: _ } in file_diagnostics.into_iter() { + // TODO: Should this be the first item in secondaries? Should we bail when we find a range? + let range = diagnostic + .secondaries + .into_iter() + .filter_map(|sec| byte_span_to_range(files, file_id, sec.span.into())) + .last() + .unwrap_or_default(); + + let severity = match diagnostic.kind { + DiagnosticKind::Error => DiagnosticSeverity::ERROR, + DiagnosticKind::Warning => DiagnosticSeverity::WARNING, + DiagnosticKind::Info => DiagnosticSeverity::INFORMATION, + DiagnosticKind::Bug => DiagnosticSeverity::WARNING, }; - // 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 { - package: package.name.to_string(), - tests, - }); + let mut tags = Vec::new(); + if diagnostic.unnecessary { + tags.push(DiagnosticTag::UNNECESSARY); + } + if diagnostic.deprecated { + tags.push(DiagnosticTag::DEPRECATED); } - let collected_lenses = crate::requests::collect_lenses_for_package( - &context, - crate_id, - &workspace, - package, - Some(&file_path), - ); - state.cached_lenses.insert(document_uri.to_string(), collected_lenses); - state.cached_definitions.insert(package_root_dir.clone(), context.def_interner); - state.cached_def_maps.insert(package_root_dir.clone(), context.def_maps); - - let fm = &context.file_manager; - let files = fm.as_file_map(); - - if output_diagnostics { - file_diagnostics - .into_iter() - .filter_map(|FileDiagnostic { file_id, diagnostic, call_stack: _ }| { - // Ignore diagnostics for any file that wasn't the file we saved - // TODO: In the future, we could create "related" diagnostics for these files - if fm.path(file_id).expect("file must exist to have emitted diagnostic") - != file_path - { - return None; - } - - // TODO: Should this be the first item in secondaries? Should we bail when we find a range? - let range = diagnostic - .secondaries - .into_iter() - .filter_map(|sec| byte_span_to_range(files, file_id, sec.span.into())) - .last() - .unwrap_or_default(); - - let severity = match diagnostic.kind { - DiagnosticKind::Error => DiagnosticSeverity::ERROR, - DiagnosticKind::Warning => DiagnosticSeverity::WARNING, - DiagnosticKind::Info => DiagnosticSeverity::INFORMATION, - DiagnosticKind::Bug => DiagnosticSeverity::WARNING, - }; - - let mut tags = Vec::new(); - if diagnostic.unnecessary { - tags.push(DiagnosticTag::UNNECESSARY); - } - if diagnostic.deprecated { - tags.push(DiagnosticTag::DEPRECATED); - } - - Some(Diagnostic { - range, - severity: Some(severity), - message: diagnostic.message, - tags: if tags.is_empty() { None } else { Some(tags) }, - ..Default::default() - }) - }) - .collect() - } else { - vec![] + let diagnostic = Diagnostic { + range, + severity: Some(severity), + message: diagnostic.message, + tags: if tags.is_empty() { None } else { Some(tags) }, + ..Default::default() + }; + + let path = fm.path(file_id).expect("file must exist to have emitted diagnostic"); + log(&format!("{:?}: {}", path, diagnostic.message)); + if let Ok(uri) = Url::from_file_path(path) { + log(&format!("{:?}: {}", uri, diagnostic.message)); + diagnostics_per_url.entry(uri).or_default().push(diagnostic); } - }) - .collect(); - - if output_diagnostics { - let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { - uri: document_uri, - version: None, - diagnostics, - }); + } + + let new_files_with_errors: HashSet<_> = diagnostics_per_url.keys().cloned().collect(); + + for (uri, diagnostics) in diagnostics_per_url { + let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { + uri, + version: None, + diagnostics, + }); + } + + // For files that previously had errors but no longer have errors we still need to publish empty diagnostics + if let Some(old_files_with_errors) = state.files_with_errors.get(&package_root_dir) { + for uri in old_files_with_errors.difference(&new_files_with_errors) { + let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { + uri: uri.clone(), + version: None, + diagnostics: vec![], + }); + } + } + + // Remember which files currently have errors, for next time + state.files_with_errors.insert(package_root_dir, new_files_with_errors); } Ok(()) @@ -306,3 +319,18 @@ mod notification_tests { } } } + +fn log(contents: &str) { + use std::fs::OpenOptions; + use std::io::prelude::*; + + let mut file = OpenOptions::new() + .write(true) + .append(true) + .open("/Users/asterite/Sandbox/output/lsp.txt") + .unwrap(); + + if let Err(e) = writeln!(file, "{}", contents) { + eprintln!("Couldn't write to file: {}", e); + } +} From 01dcc0e6f3ac67c2e95a723b8c1cfec3ec97bc6d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 09:16:08 -0300 Subject: [PATCH 2/4] Refactor --- tooling/lsp/src/notifications/mod.rs | 148 +++++++++++++-------------- 1 file changed, 74 insertions(+), 74 deletions(-) diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index b25255f457c..9818e423e25 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -3,6 +3,7 @@ use std::ops::ControlFlow; use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; +use fm::{FileManager, FileMap}; use fxhash::FxHashMap as HashMap; use lsp_types::{DiagnosticTag, Url}; use noirc_driver::{check_crate, file_manager_with_stdlib}; @@ -160,78 +161,92 @@ pub(crate) fn process_workspace_for_noir_document( let fm = &context.file_manager; let files = fm.as_file_map(); - if !output_diagnostics { - continue; + if output_diagnostics { + publish_diagnostics(state, package_root_dir, files, fm, file_diagnostics); } + } + + Ok(()) +} - let mut diagnostics_per_url: HashMap> = HashMap::default(); - - for FileDiagnostic { file_id, diagnostic, call_stack: _ } in file_diagnostics.into_iter() { - // TODO: Should this be the first item in secondaries? Should we bail when we find a range? - let range = diagnostic - .secondaries - .into_iter() - .filter_map(|sec| byte_span_to_range(files, file_id, sec.span.into())) - .last() - .unwrap_or_default(); - - let severity = match diagnostic.kind { - DiagnosticKind::Error => DiagnosticSeverity::ERROR, - DiagnosticKind::Warning => DiagnosticSeverity::WARNING, - DiagnosticKind::Info => DiagnosticSeverity::INFORMATION, - DiagnosticKind::Bug => DiagnosticSeverity::WARNING, - }; - - let mut tags = Vec::new(); - if diagnostic.unnecessary { - tags.push(DiagnosticTag::UNNECESSARY); - } - if diagnostic.deprecated { - tags.push(DiagnosticTag::DEPRECATED); - } - - let diagnostic = Diagnostic { - range, - severity: Some(severity), - message: diagnostic.message, - tags: if tags.is_empty() { None } else { Some(tags) }, - ..Default::default() - }; - - let path = fm.path(file_id).expect("file must exist to have emitted diagnostic"); - log(&format!("{:?}: {}", path, diagnostic.message)); - if let Ok(uri) = Url::from_file_path(path) { - log(&format!("{:?}: {}", uri, diagnostic.message)); - diagnostics_per_url.entry(uri).or_default().push(diagnostic); - } +fn publish_diagnostics( + state: &mut LspState, + package_root_dir: String, + files: &FileMap, + fm: &FileManager, + file_diagnostics: Vec, +) { + let mut diagnostics_per_url: HashMap> = HashMap::default(); + + for file_diagnostic in file_diagnostics.into_iter() { + let file_id = file_diagnostic.file_id; + let diagnostic = file_diagnostic_to_diagnostic(file_diagnostic, files); + + let path = fm.path(file_id).expect("file must exist to have emitted diagnostic"); + if let Ok(uri) = Url::from_file_path(path) { + diagnostics_per_url.entry(uri).or_default().push(diagnostic); } + } - let new_files_with_errors: HashSet<_> = diagnostics_per_url.keys().cloned().collect(); + let new_files_with_errors: HashSet<_> = diagnostics_per_url.keys().cloned().collect(); - for (uri, diagnostics) in diagnostics_per_url { + for (uri, diagnostics) in diagnostics_per_url { + let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { + uri, + version: None, + diagnostics, + }); + } + + // For files that previously had errors but no longer have errors we still need to publish empty diagnostics + if let Some(old_files_with_errors) = state.files_with_errors.get(&package_root_dir) { + for uri in old_files_with_errors.difference(&new_files_with_errors) { let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { - uri, + uri: uri.clone(), version: None, - diagnostics, + diagnostics: vec![], }); } + } - // For files that previously had errors but no longer have errors we still need to publish empty diagnostics - if let Some(old_files_with_errors) = state.files_with_errors.get(&package_root_dir) { - for uri in old_files_with_errors.difference(&new_files_with_errors) { - let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { - uri: uri.clone(), - version: None, - diagnostics: vec![], - }); - } - } + // Remember which files currently have errors, for next time + state.files_with_errors.insert(package_root_dir, new_files_with_errors); +} - // Remember which files currently have errors, for next time - state.files_with_errors.insert(package_root_dir, new_files_with_errors); +fn file_diagnostic_to_diagnostic(file_diagnostic: FileDiagnostic, files: &FileMap) -> Diagnostic { + let file_id = file_diagnostic.file_id; + let diagnostic = file_diagnostic.diagnostic; + + // TODO: Should this be the first item in secondaries? Should we bail when we find a range? + let range = diagnostic + .secondaries + .into_iter() + .filter_map(|sec| byte_span_to_range(files, file_id, sec.span.into())) + .last() + .unwrap_or_default(); + + let severity = match diagnostic.kind { + DiagnosticKind::Error => DiagnosticSeverity::ERROR, + DiagnosticKind::Warning => DiagnosticSeverity::WARNING, + DiagnosticKind::Info => DiagnosticSeverity::INFORMATION, + DiagnosticKind::Bug => DiagnosticSeverity::WARNING, + }; + + let mut tags = Vec::new(); + if diagnostic.unnecessary { + tags.push(DiagnosticTag::UNNECESSARY); + } + if diagnostic.deprecated { + tags.push(DiagnosticTag::DEPRECATED); } - Ok(()) + Diagnostic { + range, + severity: Some(severity), + message: diagnostic.message, + tags: if tags.is_empty() { None } else { Some(tags) }, + ..Default::default() + } } pub(super) fn on_exit( @@ -319,18 +334,3 @@ mod notification_tests { } } } - -fn log(contents: &str) { - use std::fs::OpenOptions; - use std::io::prelude::*; - - let mut file = OpenOptions::new() - .write(true) - .append(true) - .open("/Users/asterite/Sandbox/output/lsp.txt") - .unwrap(); - - if let Err(e) = writeln!(file, "{}", contents) { - eprintln!("Couldn't write to file: {}", e); - } -} From 2ea8337910875087926f866ff12557794bef0996 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 13:52:45 -0300 Subject: [PATCH 3/4] Use PathBuf as HashMap key --- tooling/lsp/src/lib.rs | 2 +- tooling/lsp/src/notifications/mod.rs | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index d8476736d5a..3073f46d8e2 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -97,7 +97,7 @@ pub struct LspState { options: LspInitializationOptions, // Tracks files that currently have errors, by package root. - files_with_errors: HashMap>, + files_with_errors: HashMap>, } impl LspState { diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 9818e423e25..2567dd12438 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -1,5 +1,6 @@ use std::collections::HashSet; use std::ops::ControlFlow; +use std::path::PathBuf; use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; @@ -162,7 +163,7 @@ pub(crate) fn process_workspace_for_noir_document( 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, files, fm, file_diagnostics); } } @@ -171,7 +172,7 @@ pub(crate) fn process_workspace_for_noir_document( fn publish_diagnostics( state: &mut LspState, - package_root_dir: String, + package_root_dir: &PathBuf, files: &FileMap, fm: &FileManager, file_diagnostics: Vec, @@ -199,7 +200,7 @@ fn publish_diagnostics( } // For files that previously had errors but no longer have errors we still need to publish empty diagnostics - if let Some(old_files_with_errors) = state.files_with_errors.get(&package_root_dir) { + if let Some(old_files_with_errors) = state.files_with_errors.get(package_root_dir) { for uri in old_files_with_errors.difference(&new_files_with_errors) { let _ = state.client.publish_diagnostics(PublishDiagnosticsParams { uri: uri.clone(), @@ -210,7 +211,7 @@ fn publish_diagnostics( } // Remember which files currently have errors, for next time - state.files_with_errors.insert(package_root_dir, new_files_with_errors); + state.files_with_errors.insert(package_root_dir.clone(), new_files_with_errors); } fn file_diagnostic_to_diagnostic(file_diagnostic: FileDiagnostic, files: &FileMap) -> Diagnostic { From 2c243497fcfee85258bf271301f58ea10e36a4b0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 3 Sep 2024 13:54:45 -0300 Subject: [PATCH 4/4] Also use PathBuf in a couple more places --- tooling/lsp/src/lib.rs | 4 ++-- tooling/lsp/src/notifications/mod.rs | 6 ++---- tooling/lsp/src/requests/mod.rs | 10 ++++------ 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 3073f46d8e2..6557975743c 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -91,9 +91,9 @@ pub struct LspState { open_documents_count: usize, input_files: HashMap, cached_lenses: HashMap>, - cached_definitions: HashMap, + cached_definitions: HashMap, cached_parsed_files: HashMap))>, - cached_def_maps: HashMap>, + cached_def_maps: HashMap>, options: LspInitializationOptions, // Tracks files that currently have errors, by package root. diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 2567dd12438..87e7bea8c3b 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -130,8 +130,6 @@ pub(crate) fn process_workspace_for_noir_document( let parsed_files = parse_diff(&workspace_file_manager, state); for package in workspace.into_iter() { - let package_root_dir: String = package.root_dir.as_os_str().to_string_lossy().into(); - let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); @@ -156,8 +154,8 @@ pub(crate) fn process_workspace_for_noir_document( Some(&file_path), ); state.cached_lenses.insert(document_uri.to_string(), collected_lenses); - state.cached_definitions.insert(package_root_dir.clone(), context.def_interner); - state.cached_def_maps.insert(package_root_dir.clone(), context.def_maps); + state.cached_definitions.insert(package.root_dir.clone(), context.def_interner); + state.cached_def_maps.insert(package.root_dir.clone(), context.def_maps); let fm = &context.file_manager; let files = fm.as_file_map(); diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 5bd9959fd63..af58396550d 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -407,7 +407,7 @@ pub(crate) struct ProcessRequestCallbackArgs<'a> { location: noirc_errors::Location, files: &'a FileMap, interner: &'a NodeInterner, - interners: &'a HashMap, + interners: &'a HashMap, crate_id: CrateId, crate_name: String, dependencies: &'a Vec, @@ -432,8 +432,6 @@ where ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not find package for file") })?; - 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( state, @@ -447,9 +445,9 @@ where let interner; let def_maps; - if let Some(def_interner) = state.cached_definitions.get(&package_root_path) { + if let Some(def_interner) = state.cached_definitions.get(&package.root_dir) { interner = def_interner; - def_maps = state.cached_def_maps.get(&package_root_path).unwrap(); + def_maps = state.cached_def_maps.get(&package.root_dir).unwrap(); } else { // We ignore the warnings and errors produced by compilation while resolving the definition let _ = noirc_driver::check_crate(&mut context, crate_id, &Default::default()); @@ -479,7 +477,7 @@ where pub(crate) fn find_all_references_in_workspace( location: noirc_errors::Location, interner: &NodeInterner, - cached_interners: &HashMap, + cached_interners: &HashMap, files: &FileMap, include_declaration: bool, include_self_type_name: bool,