From 73b32a20e25cefe7ec495a28d7e7a21cf0f1190d Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 12 Feb 2025 00:41:23 +0200 Subject: [PATCH] Fix `editor::GoToDiagnostics` cycle (#24697) Re-lands https://github.com/zed-industries/zed/pull/24446 with a more appropriate fix https://github.com/user-attachments/assets/45f665f0-473a-49bd-b013-b9d1bdb902bd After activating 2nd diagnostics group, `find_map` code for next diagnostics did not skip the previous group for the same place. This time, instead of fiddling with the diagnostics group comparison, the code splits the diagnostics by search place, looks up the active group (if any) in both split parts, and selects the entries after the group elements. Release Notes: - Fixed `editor::GoToDiagnostics` action stuck when multiple diagnostics groups belong to the same place --- crates/editor/src/editor.rs | 160 +++++++++++++++------------- crates/editor/src/editor_tests.rs | 170 ++++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+), 74 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 5d78c656f544cf..e356cf8f762e98 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -10582,13 +10582,17 @@ impl Editor { } } - let mut active_primary_range = self.active_diagnostics.as_ref().map(|active_diagnostics| { + let active_group_id = self + .active_diagnostics + .as_ref() + .map(|active_group| active_group.group_id); + let active_primary_range = self.active_diagnostics.as_ref().map(|active_diagnostics| { active_diagnostics .primary_range .to_offset(&buffer) .to_inclusive() }); - let mut search_start = if let Some(active_primary_range) = active_primary_range.as_ref() { + let search_start = if let Some(active_primary_range) = active_primary_range.as_ref() { if active_primary_range.contains(&selection.head()) { *active_primary_range.start() } else { @@ -10597,83 +10601,91 @@ impl Editor { } else { selection.head() }; + let snapshot = self.snapshot(window, cx); - loop { - let mut diagnostics; - if direction == Direction::Prev { - diagnostics = buffer - .diagnostics_in_range::(0..search_start) - .collect::>(); - diagnostics.reverse(); - } else { - diagnostics = buffer - .diagnostics_in_range::(search_start..buffer.len()) - .collect::>(); - }; - let group = diagnostics - .into_iter() - .filter(|diagnostic| !snapshot.intersects_fold(diagnostic.range.start)) - // relies on diagnostics_in_range to return diagnostics with the same starting range to - // be sorted in a stable way - // skip until we are at current active diagnostic, if it exists - .skip_while(|entry| { - let is_in_range = match direction { - Direction::Prev => entry.range.end > search_start, - Direction::Next => entry.range.start < search_start, - }; - is_in_range - && self - .active_diagnostics - .as_ref() - .is_some_and(|a| a.group_id != entry.diagnostic.group_id) - }) - .find_map(|entry| { - if entry.diagnostic.is_primary - && entry.diagnostic.severity <= DiagnosticSeverity::WARNING - && entry.range.start != entry.range.end - // if we match with the active diagnostic, skip it - && Some(entry.diagnostic.group_id) - != self.active_diagnostics.as_ref().map(|d| d.group_id) - { - Some((entry.range, entry.diagnostic.group_id)) + let primary_diagnostics_before = buffer + .diagnostics_in_range::(0..search_start) + .filter(|entry| entry.diagnostic.is_primary) + .filter(|entry| entry.range.start != entry.range.end) + .filter(|entry| entry.diagnostic.severity <= DiagnosticSeverity::WARNING) + .filter(|entry| !snapshot.intersects_fold(entry.range.start)) + .collect::>(); + let last_same_group_diagnostic_before = active_group_id.and_then(|active_group_id| { + primary_diagnostics_before + .iter() + .position(|entry| entry.diagnostic.group_id == active_group_id) + }); + + let primary_diagnostics_after = buffer + .diagnostics_in_range::(search_start..buffer.len()) + .filter(|entry| entry.diagnostic.is_primary) + .filter(|entry| entry.range.start != entry.range.end) + .filter(|entry| entry.diagnostic.severity <= DiagnosticSeverity::WARNING) + .filter(|diagnostic| !snapshot.intersects_fold(diagnostic.range.start)) + .collect::>(); + let last_same_group_diagnostic_after = active_group_id.and_then(|active_group_id| { + primary_diagnostics_after + .iter() + .enumerate() + .rev() + .find_map(|(i, entry)| { + if entry.diagnostic.group_id == active_group_id { + Some(i) } else { None } - }); + }) + }); - if let Some((primary_range, group_id)) = group { - let Some(buffer_id) = buffer.anchor_after(primary_range.start).buffer_id else { - return; - }; - self.activate_diagnostics(buffer_id, group_id, window, cx); - if self.active_diagnostics.is_some() { - self.change_selections(Some(Autoscroll::fit()), window, cx, |s| { - s.select(vec![Selection { - id: selection.id, - start: primary_range.start, - end: primary_range.start, - reversed: false, - goal: SelectionGoal::None, - }]); - }); - self.refresh_inline_completion(false, true, window, cx); - } - break; - } else { - // Cycle around to the start of the buffer, potentially moving back to the start of - // the currently active diagnostic. - active_primary_range.take(); - if direction == Direction::Prev { - if search_start == buffer.len() { - break; - } else { - search_start = buffer.len(); - } - } else if search_start == 0 { - break; - } else { - search_start = 0; - } + let next_primary_diagnostic = match direction { + Direction::Prev => primary_diagnostics_before + .iter() + .take(last_same_group_diagnostic_before.unwrap_or(usize::MAX)) + .rev() + .next(), + Direction::Next => primary_diagnostics_after + .iter() + .skip( + last_same_group_diagnostic_after + .map(|index| index + 1) + .unwrap_or(0), + ) + .next(), + }; + + // Cycle around to the start of the buffer, potentially moving back to the start of + // the currently active diagnostic. + let cycle_around = || match direction { + Direction::Prev => primary_diagnostics_after + .iter() + .rev() + .chain(primary_diagnostics_before.iter().rev()) + .next(), + Direction::Next => primary_diagnostics_before + .iter() + .chain(primary_diagnostics_after.iter()) + .next(), + }; + + if let Some((primary_range, group_id)) = next_primary_diagnostic + .or_else(cycle_around) + .map(|entry| (&entry.range, entry.diagnostic.group_id)) + { + let Some(buffer_id) = buffer.anchor_after(primary_range.start).buffer_id else { + return; + }; + self.activate_diagnostics(buffer_id, group_id, window, cx); + if self.active_diagnostics.is_some() { + self.change_selections(Some(Autoscroll::fit()), window, cx, |s| { + s.select(vec![Selection { + id: selection.id, + start: primary_range.start, + end: primary_range.start, + reversed: false, + goal: SelectionGoal::None, + }]); + }); + self.refresh_inline_completion(false, true, window, cx); } } } diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 0cc4ac46fa9904..8b89a9f0faa60c 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -10668,6 +10668,176 @@ async fn go_to_prev_overlapping_diagnostic( "}); } +#[gpui::test] +async fn cycle_through_same_place_diagnostics( + executor: BackgroundExecutor, + cx: &mut gpui::TestAppContext, +) { + init_test(cx, |_| {}); + + let mut cx = EditorTestContext::new(cx).await; + let lsp_store = + cx.update_editor(|editor, _, cx| editor.project.as_ref().unwrap().read(cx).lsp_store()); + + cx.set_state(indoc! {" + ˇfn func(abc def: i32) -> u32 { + } + "}); + + cx.update(|_, cx| { + lsp_store.update(cx, |lsp_store, cx| { + lsp_store + .update_diagnostics( + LanguageServerId(0), + lsp::PublishDiagnosticsParams { + uri: lsp::Url::from_file_path(path!("/root/file")).unwrap(), + version: None, + diagnostics: vec![ + lsp::Diagnostic { + range: lsp::Range::new( + lsp::Position::new(0, 11), + lsp::Position::new(0, 12), + ), + severity: Some(lsp::DiagnosticSeverity::ERROR), + ..Default::default() + }, + lsp::Diagnostic { + range: lsp::Range::new( + lsp::Position::new(0, 12), + lsp::Position::new(0, 15), + ), + severity: Some(lsp::DiagnosticSeverity::ERROR), + ..Default::default() + }, + lsp::Diagnostic { + range: lsp::Range::new( + lsp::Position::new(0, 12), + lsp::Position::new(0, 15), + ), + severity: Some(lsp::DiagnosticSeverity::ERROR), + ..Default::default() + }, + lsp::Diagnostic { + range: lsp::Range::new( + lsp::Position::new(0, 25), + lsp::Position::new(0, 28), + ), + severity: Some(lsp::DiagnosticSeverity::ERROR), + ..Default::default() + }, + ], + }, + &[], + cx, + ) + .unwrap() + }); + }); + executor.run_until_parked(); + + //// Backward + + // Fourth diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_prev_diagnostic(&GoToPrevDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abc def: i32) -> ˇu32 { + } + "}); + + // Third diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_prev_diagnostic(&GoToPrevDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abc ˇdef: i32) -> u32 { + } + "}); + + // Second diagnostic, same place + cx.update_editor(|editor, window, cx| { + editor.go_to_prev_diagnostic(&GoToPrevDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abc ˇdef: i32) -> u32 { + } + "}); + + // First diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_prev_diagnostic(&GoToPrevDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abcˇ def: i32) -> u32 { + } + "}); + + // Wrapped over, fourth diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_prev_diagnostic(&GoToPrevDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abc def: i32) -> ˇu32 { + } + "}); + + cx.update_editor(|editor, window, cx| { + editor.move_to_beginning(&MoveToBeginning, window, cx); + }); + cx.assert_editor_state(indoc! {" + ˇfn func(abc def: i32) -> u32 { + } + "}); + + //// Forward + + // First diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_diagnostic(&GoToDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abcˇ def: i32) -> u32 { + } + "}); + + // Second diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_diagnostic(&GoToDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abc ˇdef: i32) -> u32 { + } + "}); + + // Third diagnostic, same place + cx.update_editor(|editor, window, cx| { + editor.go_to_diagnostic(&GoToDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abc ˇdef: i32) -> u32 { + } + "}); + + // Fourth diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_diagnostic(&GoToDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abc def: i32) -> ˇu32 { + } + "}); + + // Wrapped around, first diagnostic + cx.update_editor(|editor, window, cx| { + editor.go_to_diagnostic(&GoToDiagnostic, window, cx); + }); + cx.assert_editor_state(indoc! {" + fn func(abcˇ def: i32) -> u32 { + } + "}); +} + #[gpui::test] async fn test_diagnostics_with_links(cx: &mut TestAppContext) { init_test(cx, |_| {});