From 3aed172247caa9a9d2adef4f8db79b194eb5f1f9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 10 Oct 2024 14:42:03 -0300 Subject: [PATCH 1/3] feat: show LSP diagnostic related information --- compiler/noirc_errors/src/reporter.rs | 2 +- tooling/lsp/src/notifications/mod.rs | 74 +++++++++++++++++++++------ 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/compiler/noirc_errors/src/reporter.rs b/compiler/noirc_errors/src/reporter.rs index 76e308969b4..f029b4e6de8 100644 --- a/compiler/noirc_errors/src/reporter.rs +++ b/compiler/noirc_errors/src/reporter.rs @@ -10,7 +10,7 @@ use codespan_reporting::term::termcolor::{ColorChoice, StandardStream}; pub struct CustomDiagnostic { pub message: String, pub secondaries: Vec, - notes: Vec, + pub notes: Vec, pub kind: DiagnosticKind, pub deprecated: bool, pub unnecessary: bool, diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 6cddd278e62..4f4c0d080dd 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -8,7 +8,7 @@ use crate::{ use async_lsp::{ErrorCode, LanguageClient, ResponseError}; use fm::{FileManager, FileMap}; use fxhash::FxHashMap as HashMap; -use lsp_types::{DiagnosticTag, Url}; +use lsp_types::{DiagnosticRelatedInformation, DiagnosticTag, Url}; use noirc_driver::check_crate; use noirc_errors::{DiagnosticKind, FileDiagnostic}; @@ -195,12 +195,18 @@ fn publish_diagnostics( 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 Ok(uri) = Url::from_file_path(path) else { + continue; + }; + + let Some(diagnostic) = + file_diagnostic_to_diagnostic(file_diagnostic, files, fm, uri.clone()) + else { + continue; + }; + + diagnostics_per_url.entry(uri).or_default().push(diagnostic); } let new_files_with_errors: HashSet<_> = diagnostics_per_url.keys().cloned().collect(); @@ -228,17 +234,21 @@ fn publish_diagnostics( state.files_with_errors.insert(package_root_dir.clone(), new_files_with_errors); } -fn file_diagnostic_to_diagnostic(file_diagnostic: FileDiagnostic, files: &FileMap) -> Diagnostic { +fn file_diagnostic_to_diagnostic( + file_diagnostic: FileDiagnostic, + files: &FileMap, + fm: &FileManager, + uri: Url, +) -> Option { 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(); + if diagnostic.secondaries.is_empty() { + return None; + } + + let span = diagnostic.secondaries.first().unwrap().span; + let range = byte_span_to_range(files, file_id, span.into())?; let severity = match diagnostic.kind { DiagnosticKind::Error => DiagnosticSeverity::ERROR, @@ -255,13 +265,45 @@ fn file_diagnostic_to_diagnostic(file_diagnostic: FileDiagnostic, files: &FileMa tags.push(DiagnosticTag::DEPRECATED); } - Diagnostic { + let mut related_information = Vec::new(); + + for secondary in diagnostic.secondaries { + let secondary_file = secondary.file.unwrap_or(file_id); + let Some(path) = fm.path(secondary_file) else { + continue; + }; + let Ok(uri) = Url::from_file_path(path) else { + continue; + }; + let Some(range) = byte_span_to_range(files, file_id, secondary.span.into()) else { + continue; + }; + + related_information.push(DiagnosticRelatedInformation { + location: lsp_types::Location { uri, range }, + message: secondary.message, + }); + } + + for note in diagnostic.notes { + related_information.push(DiagnosticRelatedInformation { + location: lsp_types::Location { uri: uri.clone(), range }, + message: note, + }); + } + + Some(Diagnostic { range, severity: Some(severity), message: diagnostic.message, tags: if tags.is_empty() { None } else { Some(tags) }, + related_information: if related_information.is_empty() { + None + } else { + Some(related_information) + }, ..Default::default() - } + }) } pub(super) fn on_exit( From 59e9bf55ca6cad6ab11b5247ac29a9176049077a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 10 Oct 2024 16:52:55 -0300 Subject: [PATCH 2/3] Also include the callstack --- tooling/lsp/src/notifications/mod.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 4f4c0d080dd..8454e66d826 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -292,6 +292,23 @@ fn file_diagnostic_to_diagnostic( }); } + for frame in diagnostic.call_stack.into_iter().rev() { + let Some(path) = fm.path(frame.file) else { + continue; + }; + let Ok(uri) = Url::from_file_path(path) else { + continue; + }; + let Some(range) = byte_span_to_range(files, file_id, frame.span.into()) else { + continue; + }; + + related_information.push(DiagnosticRelatedInformation { + location: lsp_types::Location { uri, range }, + message: "Error originated here".to_string(), + }); + } + Some(Diagnostic { range, severity: Some(severity), From 6c654fac606ce7eb629502d3f96824a21bf74905 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 11 Oct 2024 12:19:50 -0300 Subject: [PATCH 3/3] Avoid doing `else continue` --- tooling/lsp/src/notifications/mod.rs | 107 +++++++++++++-------------- 1 file changed, 51 insertions(+), 56 deletions(-) diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 8454e66d826..d228eb564e6 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -6,11 +6,12 @@ use crate::{ insert_all_files_for_workspace_into_file_manager, PackageCacheData, WorkspaceCacheData, }; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; -use fm::{FileManager, FileMap}; +use fm::{FileId, FileManager, FileMap}; use fxhash::FxHashMap as HashMap; use lsp_types::{DiagnosticRelatedInformation, DiagnosticTag, Url}; use noirc_driver::check_crate; -use noirc_errors::{DiagnosticKind, FileDiagnostic}; +use noirc_errors::reporter::CustomLabel; +use noirc_errors::{DiagnosticKind, FileDiagnostic, Location}; use crate::types::{ notification, Diagnostic, DiagnosticSeverity, DidChangeConfigurationParams, @@ -196,17 +197,13 @@ fn publish_diagnostics( for file_diagnostic in file_diagnostics.into_iter() { let file_id = file_diagnostic.file_id; let path = fm.path(file_id).expect("file must exist to have emitted diagnostic"); - let Ok(uri) = Url::from_file_path(path) else { - continue; - }; - - let Some(diagnostic) = - file_diagnostic_to_diagnostic(file_diagnostic, files, fm, uri.clone()) - else { - continue; - }; - - diagnostics_per_url.entry(uri).or_default().push(diagnostic); + if let Ok(uri) = Url::from_file_path(path) { + if let Some(diagnostic) = + file_diagnostic_to_diagnostic(file_diagnostic, files, fm, uri.clone()) + { + diagnostics_per_url.entry(uri).or_default().push(diagnostic); + } + } } let new_files_with_errors: HashSet<_> = diagnostics_per_url.keys().cloned().collect(); @@ -265,49 +262,19 @@ fn file_diagnostic_to_diagnostic( tags.push(DiagnosticTag::DEPRECATED); } - let mut related_information = Vec::new(); - - for secondary in diagnostic.secondaries { - let secondary_file = secondary.file.unwrap_or(file_id); - let Some(path) = fm.path(secondary_file) else { - continue; - }; - let Ok(uri) = Url::from_file_path(path) else { - continue; - }; - let Some(range) = byte_span_to_range(files, file_id, secondary.span.into()) else { - continue; - }; - - related_information.push(DiagnosticRelatedInformation { - location: lsp_types::Location { uri, range }, - message: secondary.message, - }); - } - - for note in diagnostic.notes { - related_information.push(DiagnosticRelatedInformation { - location: lsp_types::Location { uri: uri.clone(), range }, - message: note, - }); - } - - for frame in diagnostic.call_stack.into_iter().rev() { - let Some(path) = fm.path(frame.file) else { - continue; - }; - let Ok(uri) = Url::from_file_path(path) else { - continue; - }; - let Some(range) = byte_span_to_range(files, file_id, frame.span.into()) else { - continue; - }; - - related_information.push(DiagnosticRelatedInformation { - location: lsp_types::Location { uri, range }, - message: "Error originated here".to_string(), - }); - } + let secondaries = diagnostic + .secondaries + .into_iter() + .filter_map(|secondary| secondary_to_related_information(secondary, file_id, files, fm)); + let notes = diagnostic.notes.into_iter().map(|message| DiagnosticRelatedInformation { + location: lsp_types::Location { uri: uri.clone(), range }, + message, + }); + let call_stack = diagnostic + .call_stack + .into_iter() + .filter_map(|frame| call_stack_frame_to_related_information(frame, files, fm)); + let related_information: Vec<_> = secondaries.chain(notes).chain(call_stack).collect(); Some(Diagnostic { range, @@ -323,6 +290,34 @@ fn file_diagnostic_to_diagnostic( }) } +fn secondary_to_related_information( + secondary: CustomLabel, + file_id: FileId, + files: &FileMap, + fm: &FileManager, +) -> Option { + let secondary_file = secondary.file.unwrap_or(file_id); + let path = fm.path(secondary_file)?; + let uri = Url::from_file_path(path).ok()?; + let range = byte_span_to_range(files, file_id, secondary.span.into())?; + let message = secondary.message; + Some(DiagnosticRelatedInformation { location: lsp_types::Location { uri, range }, message }) +} + +fn call_stack_frame_to_related_information( + frame: Location, + files: &FileMap, + fm: &FileManager, +) -> Option { + let path = fm.path(frame.file)?; + let uri = Url::from_file_path(path).ok()?; + let range = byte_span_to_range(files, frame.file, frame.span.into())?; + Some(DiagnosticRelatedInformation { + location: lsp_types::Location { uri, range }, + message: "Error originated here".to_string(), + }) +} + pub(super) fn on_exit( _state: &mut LspState, _params: (),