From 1f40fcfb0f2c1120aba5dda4101c1e5e8c9c10f6 Mon Sep 17 00:00:00 2001 From: jneem Date: Fri, 5 Apr 2024 14:19:00 -0500 Subject: [PATCH] Dedup diagnostics (#1883) * Deduplicate diagnostics * Test the diagnostics deduplication --- lsp/nls/src/background.rs | 2 + lsp/nls/src/diagnostic.rs | 82 +++++++++++++++---- .../tests/inputs/diagnostics-recursion.ncl | 2 +- ...ts__inputs__diagnostics-recursion.ncl.snap | 5 +- 4 files changed, 73 insertions(+), 18 deletions(-) diff --git a/lsp/nls/src/background.rs b/lsp/nls/src/background.rs index 405569b653..c9cb95ae7d 100644 --- a/lsp/nls/src/background.rs +++ b/lsp/nls/src/background.rs @@ -112,6 +112,8 @@ pub fn worker_main() -> anyhow::Result<()> { ); } + diagnostics.sort(); + diagnostics.dedup(); let diagnostics = Diagnostics { path, diagnostics }; // If this fails, the main process has already exited. No need for a loud error in that case. diff --git a/lsp/nls/src/diagnostic.rs b/lsp/nls/src/diagnostic.rs index 9a6d0bd4f4..5d3870604e 100644 --- a/lsp/nls/src/diagnostic.rs +++ b/lsp/nls/src/diagnostic.rs @@ -13,22 +13,70 @@ use crate::codespan_lsp::byte_span_to_range; /// lsp_types::Diagnostic is not serializable to bincode (and therefore not /// sendable across an ipc-channel channel) because it has optional fields that /// get skipped serializing if empty. See -#[derive(Debug, Serialize, Deserialize)] +/// +/// We also support `PartialOrd` and `Ord` through various wrappers. Not because +/// there's any semantically meaningful ordering, but because it lets us deduplicate +/// the output. +#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] pub struct SerializableDiagnostic { - pub range: lsp_types::Range, + pub range: OrdRange, pub severity: Option, - pub code: Option, + pub code: Option, pub message: String, - pub related_information: Option>, + pub related_information: Option>, +} + +#[derive(Debug, Eq, PartialEq, Copy, Clone, Default, Deserialize, Serialize)] +pub struct OrdRange(pub lsp_types::Range); + +impl PartialOrd for OrdRange { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for OrdRange { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + (&self.0.start, &self.0.end).cmp(&(&other.0.start, &other.0.end)) + } +} + +#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)] +pub struct OrdDiagnosticRelatedInformation(pub lsp_types::DiagnosticRelatedInformation); + +impl PartialOrd for OrdDiagnosticRelatedInformation { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for OrdDiagnosticRelatedInformation { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + ( + &self.0.location.uri, + &self.0.location.range.start, + &self.0.location.range.end, + &self.0.message, + ) + .cmp(&( + &other.0.location.uri, + &other.0.location.range.start, + &other.0.location.range.end, + &other.0.message, + )) + } } impl From for lsp_types::Diagnostic { fn from(d: SerializableDiagnostic) -> Self { Self { - range: d.range, + range: d.range.0, severity: d.severity, - code: d.code, + code: d.code.map(NumberOrString::String), message: d.message, + related_information: d + .related_information + .map(|xs| xs.into_iter().map(|x| x.0).collect()), ..Default::default() } } @@ -98,7 +146,7 @@ impl DiagnosticCompat for SerializableDiagnostic { diagnostic::Severity::Help => lsp_types::DiagnosticSeverity::HINT, }); - let code = diagnostic.code.clone().map(NumberOrString::String); + let code = diagnostic.code.clone(); let mut diagnostics = Vec::new(); @@ -132,19 +180,21 @@ impl DiagnosticCompat for SerializableDiagnostic { format!("{}\n{}", diagnostic.message, diagnostic.notes.join("\n")) }; diagnostics.push(SerializableDiagnostic { - range, + range: OrdRange(range), severity, code: code.clone(), message, related_information: Some( cross_file_labels - .map(|label| DiagnosticRelatedInformation { - location: lsp_types::Location::from_codespan( - &label.file_id, - &label.range, - files, - ), - message: label.message.clone(), + .map(|label| { + OrdDiagnosticRelatedInformation(DiagnosticRelatedInformation { + location: lsp_types::Location::from_codespan( + &label.file_id, + &label.range, + files, + ), + message: label.message.clone(), + }) }) .collect(), ), @@ -156,7 +206,7 @@ impl DiagnosticCompat for SerializableDiagnostic { let range = lsp_types::Range::from_codespan(&label.file_id, &label.range, files); SerializableDiagnostic { - range, + range: OrdRange(range), message: label.message.clone(), severity: Some(lsp_types::DiagnosticSeverity::HINT), code: code.clone(), diff --git a/lsp/nls/tests/inputs/diagnostics-recursion.ncl b/lsp/nls/tests/inputs/diagnostics-recursion.ncl index 084d24b802..9b3cf33528 100644 --- a/lsp/nls/tests/inputs/diagnostics-recursion.ncl +++ b/lsp/nls/tests/inputs/diagnostics-recursion.ncl @@ -1,5 +1,5 @@ ### /diagnostics-recursion.ncl -let rec foo = { bar = foo } in +let rec foo = { bar = foo, quux | String = 1 } in [ foo, foo.bar.bar.bar.bar.bar.baz diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__diagnostics-recursion.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__diagnostics-recursion.ncl.snap index ec0fbf9a8f..4da41bf173 100644 --- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__diagnostics-recursion.ncl.snap +++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__diagnostics-recursion.ncl.snap @@ -2,7 +2,10 @@ source: lsp/nls/tests/main.rs expression: output --- -(file:///diagnostics-recursion.ncl, 0:14-0:27: this record lacks the field `baz`) +(file:///diagnostics-recursion.ncl, 0:14-0:46: this record lacks the field `baz`) +(file:///diagnostics-recursion.ncl, 0:34-0:40: expected type) +(file:///diagnostics-recursion.ncl, 0:43-0:44: applied to this expression) +(file:///diagnostics-recursion.ncl, 0:43-0:44: contract broken by the value of `quux`) (file:///diagnostics-recursion.ncl, 3:2-3:29: missing field `baz` Did you mean `bar`?) (file:///diagnostics-recursion.ncl, 3:2-3:29: this requires the field `baz` to exist)