Skip to content

Commit

Permalink
feat: show LSP diagnostic related information (#6277)
Browse files Browse the repository at this point in the history
# Description

## Problem

Errors shown in LSP didn't show the "secondaries" we capture, nor the
notes.

Resolves #6272

## Summary

Now secondaries and notes are shown as details to the primary error,
like in Rust Analyzer:


![lsp-related-information-1](https://github.com/user-attachments/assets/bc307cb0-e314-4e40-97a2-a5a57abed717)


![lsp-related-information-2](https://github.com/user-attachments/assets/a79e379e-dc06-4f74-ab51-8c4b4f868899)

## Additional Context

## 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.
  • Loading branch information
asterite authored Oct 11, 2024
1 parent d3301a4 commit c8a91a5
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 17 deletions.
2 changes: 1 addition & 1 deletion compiler/noirc_errors/src/reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use codespan_reporting::term::termcolor::{ColorChoice, StandardStream};
pub struct CustomDiagnostic {
pub message: String,
pub secondaries: Vec<CustomLabel>,
notes: Vec<String>,
pub notes: Vec<String>,
pub kind: DiagnosticKind,
pub deprecated: bool,
pub unnecessary: bool,
Expand Down
86 changes: 70 additions & 16 deletions tooling/lsp/src/notifications/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{DiagnosticTag, Url};
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,
Expand Down Expand Up @@ -195,11 +196,13 @@ 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);
if let Some(diagnostic) =
file_diagnostic_to_diagnostic(file_diagnostic, files, fm, uri.clone())
{
diagnostics_per_url.entry(uri).or_default().push(diagnostic);
}
}
}

Expand Down Expand Up @@ -228,17 +231,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<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();
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,
Expand All @@ -255,13 +262,60 @@ fn file_diagnostic_to_diagnostic(file_diagnostic: FileDiagnostic, files: &FileMa
tags.push(DiagnosticTag::DEPRECATED);
}

Diagnostic {
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,
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()
}
})
}

fn secondary_to_related_information(
secondary: CustomLabel,
file_id: FileId,
files: &FileMap,
fm: &FileManager,
) -> Option<DiagnosticRelatedInformation> {
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<DiagnosticRelatedInformation> {
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(
Expand Down

0 comments on commit c8a91a5

Please sign in to comment.