Assign diagnostics a `group_id` based on their `related_information`

Antonio Scandurra and Nathan Sobo created

Co-Authored-By: Nathan Sobo <nathan@zed.dev>

Change summary

crates/language/src/lib.rs     |  68 ++++++++-
crates/language/src/proto.rs   |   2 
crates/language/src/tests.rs   | 245 +++++++++++++++++++++++++++--------
crates/project/src/worktree.rs |   3 
crates/rpc/proto/zed.proto     |   1 
5 files changed, 249 insertions(+), 70 deletions(-)

Detailed changes

crates/language/src/lib.rs 🔗

@@ -85,6 +85,7 @@ pub struct Snapshot {
 pub struct Diagnostic {
     pub severity: DiagnosticSeverity,
     pub message: String,
+    pub group_id: usize,
 }
 
 struct LanguageServerState {
@@ -699,6 +700,7 @@ impl Buffer {
         } else {
             self.content()
         };
+        let abs_path = self.file.as_ref().and_then(|f| f.abs_path());
 
         let empty_set = HashSet::new();
         let disk_based_sources = self
@@ -714,17 +716,28 @@ impl Buffer {
                 .peekable();
             let mut last_edit_old_end = PointUtf16::zero();
             let mut last_edit_new_end = PointUtf16::zero();
+            let mut groups = HashMap::new();
+            let mut next_group_id = 0;
 
             content.anchor_range_multimap(
                 Bias::Left,
                 Bias::Right,
-                diagnostics.into_iter().filter_map(|diagnostic| {
-                    let mut start = PointUtf16::new(
-                        diagnostic.range.start.line,
-                        diagnostic.range.start.character,
-                    );
-                    let mut end =
-                        PointUtf16::new(diagnostic.range.end.line, diagnostic.range.end.character);
+                diagnostics.iter().filter_map(|diagnostic| {
+                    let mut start = diagnostic.range.start.to_point_utf16();
+                    let mut end = diagnostic.range.end.to_point_utf16();
+                    let source = diagnostic.source.as_ref();
+                    let code = diagnostic.code.as_ref();
+                    let group_id = diagnostic_ranges(&diagnostic, abs_path.as_deref())
+                        .find_map(|range| groups.get(&(source, code, range)))
+                        .copied()
+                        .unwrap_or_else(|| {
+                            let group_id = post_inc(&mut next_group_id);
+                            for range in diagnostic_ranges(&diagnostic, abs_path.as_deref()) {
+                                groups.insert((source, code, range), group_id);
+                            }
+                            group_id
+                        });
+
                     if diagnostic
                         .source
                         .as_ref()
@@ -760,7 +773,8 @@ impl Buffer {
                         range,
                         Diagnostic {
                             severity: diagnostic.severity.unwrap_or(DiagnosticSeverity::ERROR),
-                            message: diagnostic.message,
+                            message: diagnostic.message.clone(),
+                            group_id,
                         },
                     ))
                 }),
@@ -1888,6 +1902,44 @@ impl ToTreeSitterPoint for Point {
     }
 }
 
+trait ToPointUtf16 {
+    fn to_point_utf16(self) -> PointUtf16;
+}
+
+impl ToPointUtf16 for lsp::Position {
+    fn to_point_utf16(self) -> PointUtf16 {
+        PointUtf16::new(self.line, self.character)
+    }
+}
+
+fn diagnostic_ranges<'a>(
+    diagnostic: &'a lsp::Diagnostic,
+    abs_path: Option<&'a Path>,
+) -> impl 'a + Iterator<Item = Range<PointUtf16>> {
+    diagnostic
+        .related_information
+        .iter()
+        .flatten()
+        .filter_map(move |info| {
+            if info.location.uri.to_file_path().ok()? == abs_path? {
+                let info_start = PointUtf16::new(
+                    info.location.range.start.line,
+                    info.location.range.start.character,
+                );
+                let info_end = PointUtf16::new(
+                    info.location.range.end.line,
+                    info.location.range.end.character,
+                );
+                Some(info_start..info_end)
+            } else {
+                None
+            }
+        })
+        .chain(Some(
+            diagnostic.range.start.to_point_utf16()..diagnostic.range.end.to_point_utf16(),
+        ))
+}
+
 fn contiguous_ranges(
     values: impl IntoIterator<Item = u32>,
     max_len: usize,

crates/language/src/proto.rs 🔗

@@ -141,6 +141,7 @@ pub fn serialize_diagnostics(map: &AnchorRangeMultimap<Diagnostic>) -> proto::Di
                     DiagnosticSeverity::HINT => proto::diagnostic::Severity::Hint,
                     _ => proto::diagnostic::Severity::None,
                 } as i32,
+                group_id: diagnostic.group_id as u64,
             })
             .collect(),
     }
@@ -308,6 +309,7 @@ pub fn deserialize_diagnostics(message: proto::DiagnosticSet) -> AnchorRangeMult
                         proto::diagnostic::Severity::None => return None,
                     },
                     message: diagnostic.message,
+                    group_id: diagnostic.group_id as usize,
                 },
             ))
         }),

crates/language/src/tests.rs 🔗

@@ -482,14 +482,16 @@ async fn test_diagnostics(mut cx: gpui::TestAppContext) {
                     Point::new(3, 9)..Point::new(3, 11),
                     &Diagnostic {
                         severity: DiagnosticSeverity::ERROR,
-                        message: "undefined variable 'BB'".to_string()
+                        message: "undefined variable 'BB'".to_string(),
+                        group_id: 0,
                     },
                 ),
                 (
                     Point::new(4, 9)..Point::new(4, 12),
                     &Diagnostic {
                         severity: DiagnosticSeverity::ERROR,
-                        message: "undefined variable 'CCC'".to_string()
+                        message: "undefined variable 'CCC'".to_string(),
+                        group_id: 0,
                     }
                 )
             ]
@@ -545,14 +547,16 @@ async fn test_diagnostics(mut cx: gpui::TestAppContext) {
                     Point::new(2, 9)..Point::new(2, 12),
                     &Diagnostic {
                         severity: DiagnosticSeverity::WARNING,
-                        message: "unreachable statement".to_string()
+                        message: "unreachable statement".to_string(),
+                        group_id: 0,
                     }
                 ),
                 (
                     Point::new(2, 9)..Point::new(2, 10),
                     &Diagnostic {
                         severity: DiagnosticSeverity::ERROR,
-                        message: "undefined variable 'A'".to_string()
+                        message: "undefined variable 'A'".to_string(),
+                        group_id: 0,
                     },
                 )
             ]
@@ -620,14 +624,16 @@ async fn test_diagnostics(mut cx: gpui::TestAppContext) {
                     Point::new(2, 21)..Point::new(2, 22),
                     &Diagnostic {
                         severity: DiagnosticSeverity::ERROR,
-                        message: "undefined variable 'A'".to_string()
+                        message: "undefined variable 'A'".to_string(),
+                        group_id: 0,
                     }
                 ),
                 (
                     Point::new(3, 9)..Point::new(3, 11),
                     &Diagnostic {
                         severity: DiagnosticSeverity::ERROR,
-                        message: "undefined variable 'BB'".to_string()
+                        message: "undefined variable 'BB'".to_string(),
+                        group_id: 0,
                     },
                 )
             ]
@@ -706,31 +712,30 @@ async fn test_grouped_diagnostics(mut cx: gpui::TestAppContext) {
         "
         .unindent();
 
-        let mut buffer = Buffer::new(0, text, cx);
+        let file = FakeFile::new("/example.rs");
+        let mut buffer = Buffer::from_file(0, text, Box::new(file.clone()), cx);
         buffer.set_language(Some(Arc::new(rust_lang())), None, cx);
         let diagnostics = vec![
             lsp::Diagnostic {
                 range: lsp::Range::new(lsp::Position::new(1, 8), lsp::Position::new(1, 9)),
                 severity: Some(DiagnosticSeverity::WARNING),
-                message: "unused variable: `x`\n`#[warn(unused_variables)]` on by default"
-                    .to_string(),
+                message: "error 1".to_string(),
                 related_information: Some(vec![lsp::DiagnosticRelatedInformation {
                     location: lsp::Location {
-                        uri: lsp::Url::from_file_path("/example.rs").unwrap(),
+                        uri: lsp::Url::from_file_path(&file.abs_path).unwrap(),
                         range: lsp::Range::new(lsp::Position::new(1, 8), lsp::Position::new(1, 9)),
                     },
-                    message: "if this is intentional, prefix it with an underscore: `_x`"
-                        .to_string(),
+                    message: "error 1 hint 1".to_string(),
                 }]),
                 ..Default::default()
             },
             lsp::Diagnostic {
                 range: lsp::Range::new(lsp::Position::new(1, 8), lsp::Position::new(1, 9)),
                 severity: Some(DiagnosticSeverity::HINT),
-                message: "if this is intentional, prefix it with an underscore: `_x`".to_string(),
+                message: "error 1 hint 1".to_string(),
                 related_information: Some(vec![lsp::DiagnosticRelatedInformation {
                     location: lsp::Location {
-                        uri: lsp::Url::from_file_path("/example.rs").unwrap(),
+                        uri: lsp::Url::from_file_path(&file.abs_path).unwrap(),
                         range: lsp::Range::new(lsp::Position::new(1, 8), lsp::Position::new(1, 9)),
                     },
                     message: "original diagnostic".to_string(),
@@ -738,67 +743,108 @@ async fn test_grouped_diagnostics(mut cx: gpui::TestAppContext) {
                 ..Default::default()
             },
             lsp::Diagnostic {
-                range: lsp::Range::new( lsp::Position::new(2, 8), lsp::Position::new(2, 17)),
+                range: lsp::Range::new(lsp::Position::new(2, 8), lsp::Position::new(2, 17)),
                 severity: Some(DiagnosticSeverity::ERROR),
-                message: "cannot borrow `v` as mutable because it is also borrowed as immutable\nmutable borrow occurs here".to_string(),
-                related_information: Some(
-                    vec![
-                        lsp::DiagnosticRelatedInformation {
-                            location: lsp::Location {
-                                uri: lsp::Url::from_file_path("/example.rs").unwrap(),
-                                range: lsp::Range::new(lsp::Position::new( 1, 13, ), lsp::Position::new(1, 15)),
-                            },
-                            message: "immutable borrow occurs here".to_string(),
+                message: "error 2".to_string(),
+                related_information: Some(vec![
+                    lsp::DiagnosticRelatedInformation {
+                        location: lsp::Location {
+                            uri: lsp::Url::from_file_path(&file.abs_path).unwrap(),
+                            range: lsp::Range::new(
+                                lsp::Position::new(1, 13),
+                                lsp::Position::new(1, 15),
+                            ),
                         },
-                        lsp::DiagnosticRelatedInformation {
-                            location: lsp::Location {
-                                uri: lsp::Url::from_file_path("/example.rs").unwrap(),
-                                range: lsp::Range::new(lsp::Position::new( 1, 13, ), lsp::Position::new(1, 15)),
-                            },
-                            message: "immutable borrow later used here".to_string(),
+                        message: "error 2 hint 1".to_string(),
+                    },
+                    lsp::DiagnosticRelatedInformation {
+                        location: lsp::Location {
+                            uri: lsp::Url::from_file_path(&file.abs_path).unwrap(),
+                            range: lsp::Range::new(
+                                lsp::Position::new(1, 13),
+                                lsp::Position::new(1, 15),
+                            ),
                         },
-                    ],
-                ),
+                        message: "error 2 hint 2".to_string(),
+                    },
+                ]),
                 ..Default::default()
             },
             lsp::Diagnostic {
-                range: lsp::Range::new( lsp::Position::new(1, 13), lsp::Position::new(1, 15)),
-                severity: Some( DiagnosticSeverity::HINT),
-                message: "immutable borrow occurs here".to_string(),
-                related_information: Some(
-                    vec![
-                        lsp::DiagnosticRelatedInformation {
-                            location: lsp::Location {
-                                uri: lsp::Url::from_file_path("/example.rs").unwrap(),
-                                range: lsp::Range::new(lsp::Position::new( 2, 8, ), lsp::Position::new(2, 17)),
-                            },
-                            message: "original diagnostic".to_string(),
-                        },
-                    ],
-                ),
+                range: lsp::Range::new(lsp::Position::new(1, 13), lsp::Position::new(1, 15)),
+                severity: Some(DiagnosticSeverity::HINT),
+                message: "error 2 hint 1".to_string(),
+                related_information: Some(vec![lsp::DiagnosticRelatedInformation {
+                    location: lsp::Location {
+                        uri: lsp::Url::from_file_path(&file.abs_path).unwrap(),
+                        range: lsp::Range::new(lsp::Position::new(2, 8), lsp::Position::new(2, 17)),
+                    },
+                    message: "original diagnostic".to_string(),
+                }]),
                 ..Default::default()
             },
             lsp::Diagnostic {
-                range: lsp::Range::new( lsp::Position::new(1, 13), lsp::Position::new(1, 15)),
+                range: lsp::Range::new(lsp::Position::new(1, 13), lsp::Position::new(1, 15)),
                 severity: Some(DiagnosticSeverity::HINT),
-                message: "immutable borrow later used here".to_string(),
-                related_information: Some(
-                    vec![
-                        lsp::DiagnosticRelatedInformation {
-                            location: lsp::Location {
-                                uri: lsp::Url::from_file_path("/example.rs").unwrap(),
-                                range: lsp::Range::new(lsp::Position::new( 2, 8, ), lsp::Position::new(2, 17)),
-                            },
-                            message: "original diagnostic".to_string(),
-                        },
-                    ],
-                ),
+                message: "error 2 hint 2".to_string(),
+                related_information: Some(vec![lsp::DiagnosticRelatedInformation {
+                    location: lsp::Location {
+                        uri: lsp::Url::from_file_path(&file.abs_path).unwrap(),
+                        range: lsp::Range::new(lsp::Position::new(2, 8), lsp::Position::new(2, 17)),
+                    },
+                    message: "original diagnostic".to_string(),
+                }]),
                 ..Default::default()
             },
         ];
         buffer.update_diagnostics(None, diagnostics, cx).unwrap();
-
-        // TODO: Group these diagnostics somehow.
+        assert_eq!(
+            buffer
+                .diagnostics_in_range::<_, Point>(0..buffer.len())
+                .collect::<Vec<_>>(),
+            &[
+                (
+                    Point::new(1, 8)..Point::new(1, 9),
+                    &Diagnostic {
+                        severity: DiagnosticSeverity::WARNING,
+                        message: "error 1".to_string(),
+                        group_id: 0
+                    }
+                ),
+                (
+                    Point::new(1, 8)..Point::new(1, 9),
+                    &Diagnostic {
+                        severity: DiagnosticSeverity::HINT,
+                        message: "error 1 hint 1".to_string(),
+                        group_id: 0
+                    }
+                ),
+                (
+                    Point::new(1, 13)..Point::new(1, 15),
+                    &Diagnostic {
+                        severity: DiagnosticSeverity::HINT,
+                        message: "error 2 hint 1".to_string(),
+                        group_id: 1
+                    }
+                ),
+                (
+                    Point::new(1, 13)..Point::new(1, 15),
+                    &Diagnostic {
+                        severity: DiagnosticSeverity::HINT,
+                        message: "error 2 hint 2".to_string(),
+                        group_id: 1
+                    }
+                ),
+                (
+                    Point::new(2, 8)..Point::new(2, 17),
+                    &Diagnostic {
+                        severity: DiagnosticSeverity::ERROR,
+                        message: "error 2".to_string(),
+                        group_id: 1
+                    }
+                )
+            ]
+        );
 
         buffer
     });
@@ -875,3 +921,80 @@ fn rust_lang() -> Language {
 fn empty(point: Point) -> Range<Point> {
     point..point
 }
+
+#[derive(Clone)]
+struct FakeFile {
+    abs_path: PathBuf,
+}
+
+impl FakeFile {
+    fn new(abs_path: impl Into<PathBuf>) -> Self {
+        Self {
+            abs_path: abs_path.into(),
+        }
+    }
+}
+
+impl File for FakeFile {
+    fn worktree_id(&self) -> usize {
+        todo!()
+    }
+
+    fn entry_id(&self) -> Option<usize> {
+        todo!()
+    }
+
+    fn mtime(&self) -> SystemTime {
+        SystemTime::now()
+    }
+
+    fn path(&self) -> &Arc<Path> {
+        todo!()
+    }
+
+    fn abs_path(&self) -> Option<PathBuf> {
+        Some(self.abs_path.clone())
+    }
+
+    fn full_path(&self) -> PathBuf {
+        todo!()
+    }
+
+    fn file_name(&self) -> Option<OsString> {
+        todo!()
+    }
+
+    fn is_deleted(&self) -> bool {
+        todo!()
+    }
+
+    fn save(
+        &self,
+        _: u64,
+        _: Rope,
+        _: clock::Global,
+        _: &mut MutableAppContext,
+    ) -> Task<Result<(clock::Global, SystemTime)>> {
+        todo!()
+    }
+
+    fn load_local(&self, _: &AppContext) -> Option<Task<Result<String>>> {
+        todo!()
+    }
+
+    fn buffer_updated(&self, _: u64, _: super::Operation, _: &mut MutableAppContext) {
+        todo!()
+    }
+
+    fn buffer_removed(&self, _: u64, _: &mut MutableAppContext) {
+        todo!()
+    }
+
+    fn boxed_clone(&self) -> Box<dyn File> {
+        todo!()
+    }
+
+    fn as_any(&self) -> &dyn Any {
+        todo!()
+    }
+}

crates/project/src/worktree.rs 🔗

@@ -3633,7 +3633,8 @@ mod tests {
                     Point::new(0, 9)..Point::new(0, 10),
                     &Diagnostic {
                         severity: lsp::DiagnosticSeverity::ERROR,
-                        message: "undefined variable 'A'".to_string()
+                        message: "undefined variable 'A'".to_string(),
+                        group_id: 0,
                     }
                 )]
             )

crates/rpc/proto/zed.proto 🔗

@@ -256,6 +256,7 @@ message Diagnostic {
     uint64 end = 2;
     Severity severity = 3;
     string message = 4;
+    uint64 group_id = 5;
     enum Severity {
         None = 0;
         Error = 1;