Repository: tweag/nickel (rust)

Issue:
NLS reports same diagnostic message multiple times
**Describe the bug**

NLS reports same diagnostic message multiple times when the recursive definitions is present. Related to #1875.

https://github.com/tweag/nickel/assets/13243167/dca92462-5493-4891-aa1a-83765bb27518



**To Reproduce**

```nickel
{
  applications = {
    apps | not_exported = applications,
    message = "message",
    blah | String = 5,
  }
} 
```

**Expected behavior**

The diagnostic message will be reported only once per case.

**Environment**
 - OS name + version:

```
NAME="Fedora Linux"
VERSION="39.20240327.0 (Sericea)"
```

 - Version of the code: [262ac8fa36df3268414750094ddaa358344decf2](https://github.com/tweag/nickel/commit/262ac8fa36df3268414750094ddaa358344decf2)


Pull request:
Dedup diagnostics
Unfortunately the `lsp_types` crate doesn't provide a lot of `Ord` impls, so I had to write some wrappers. Is there a better way? (Of course we could use hashmaps to dedup, but `lsp_types` also doesn't provide `Hash` impls)

Fixes #1882 

Patch:
diff --git a/lsp/nls/src/background.rs b/lsp/nls/src/background.rs
index 405569b6..c9cb95ae 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 9a6d0bd4..5d387060 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 <https://github.com/serde-rs/serde/issues/1732>
-#[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<lsp_types::DiagnosticSeverity>,
-    pub code: Option<NumberOrString>,
+    pub code: Option<String>,
     pub message: String,
-    pub related_information: Option<Vec<lsp_types::DiagnosticRelatedInformation>>,
+    pub related_information: Option<Vec<OrdDiagnosticRelatedInformation>>,
+}
+
+#[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<std::cmp::Ordering> {
+        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<std::cmp::Ordering> {
+        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<SerializableDiagnostic> 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(),


Test patch:
diff --git a/lsp/nls/tests/inputs/diagnostics-recursion.ncl b/lsp/nls/tests/inputs/diagnostics-recursion.ncl
index 084d24b8..9b3cf335 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 ec0fbf9a..4da41bf1 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)