From 78bbb83448f73ba2a28e5505f1eaa75d07dbee2c Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 4 Nov 2021 14:52:34 +0100 Subject: [PATCH] Assign diagnostics a `group_id` based on their `related_information` Co-Authored-By: Nathan Sobo --- crates/language/src/lib.rs | 68 +++++++-- crates/language/src/proto.rs | 2 + crates/language/src/tests.rs | 251 ++++++++++++++++++++++++--------- crates/project/src/worktree.rs | 3 +- crates/rpc/proto/zed.proto | 1 + 5 files changed, 252 insertions(+), 73 deletions(-) diff --git a/crates/language/src/lib.rs b/crates/language/src/lib.rs index 58f0ecad64..bae0b0ee5e 100644 --- a/crates/language/src/lib.rs +++ b/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> { + 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, max_len: usize, diff --git a/crates/language/src/proto.rs b/crates/language/src/proto.rs index 4e6a8316c2..bccb965a55 100644 --- a/crates/language/src/proto.rs +++ b/crates/language/src/proto.rs @@ -141,6 +141,7 @@ pub fn serialize_diagnostics(map: &AnchorRangeMultimap) -> 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, }, )) }), diff --git a/crates/language/src/tests.rs b/crates/language/src/tests.rs index 4e71822046..776d4943d9 100644 --- a/crates/language/src/tests.rs +++ b/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(), - }, - ], - ), - ..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 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)), + severity: Some(DiagnosticSeverity::HINT), + 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::>(), + &[ + ( + 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 } + +#[derive(Clone)] +struct FakeFile { + abs_path: PathBuf, +} + +impl FakeFile { + fn new(abs_path: impl Into) -> Self { + Self { + abs_path: abs_path.into(), + } + } +} + +impl File for FakeFile { + fn worktree_id(&self) -> usize { + todo!() + } + + fn entry_id(&self) -> Option { + todo!() + } + + fn mtime(&self) -> SystemTime { + SystemTime::now() + } + + fn path(&self) -> &Arc { + todo!() + } + + fn abs_path(&self) -> Option { + Some(self.abs_path.clone()) + } + + fn full_path(&self) -> PathBuf { + todo!() + } + + fn file_name(&self) -> Option { + todo!() + } + + fn is_deleted(&self) -> bool { + todo!() + } + + fn save( + &self, + _: u64, + _: Rope, + _: clock::Global, + _: &mut MutableAppContext, + ) -> Task> { + todo!() + } + + fn load_local(&self, _: &AppContext) -> Option>> { + todo!() + } + + fn buffer_updated(&self, _: u64, _: super::Operation, _: &mut MutableAppContext) { + todo!() + } + + fn buffer_removed(&self, _: u64, _: &mut MutableAppContext) { + todo!() + } + + fn boxed_clone(&self) -> Box { + todo!() + } + + fn as_any(&self) -> &dyn Any { + todo!() + } +} diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 10025b3054..5a9b7d205e 100644 --- a/crates/project/src/worktree.rs +++ b/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, } )] ) diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 8753f27dba..f826c4b03f 100644 --- a/crates/rpc/proto/zed.proto +++ b/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;