diagnostics: Prefer activating diagnostic under cursor (#52957)

Update the behavior of both `editor: go to diagnostic` and `editor: go
to previous diagnostic` in order to ensure that, if there's a diagnostic
under the user's cursor that isn't active, it is first activated, with a
subsequent call jumping to the next or previous diagnostic,
respectively.

These changes also update how diagnostic activation handles the
situation when the global diagnostic renderer is not registered, as we
used to not update the active diagnostic group in that situation.
However, we now rely on it to determine whether the user's cursor is
already in the active diagnostic, with some tests now failing, so we now
default to an empty set of blocks for the active diagnostic group when
no global renderer is registered.

Release Notes:

- Update both `editor: go to diagnostic` and `editor: go to previous
diagnostic` to prefer activating the diagnostic under the cursor before
jumping to the next or previous diagnostic, respectively

---------

Co-authored-by: dino <dinojoaocosta@gmail.com>

crates/diagnostics/src/diagnostics_tests.rs

@@ -1550,6 +1550,8 @@ async fn go_to_diagnostic_with_severity(cx: &mut TestAppContext) {
 
     // Default, should cycle through all diagnostics
     go!(GoToDiagnosticSeverityFilter::default());
+    cx.assert_editor_state(indoc! {"error warning info ˇhint"});
+    go!(GoToDiagnosticSeverityFilter::default());
     cx.assert_editor_state(indoc! {"ˇerror warning info hint"});
     go!(GoToDiagnosticSeverityFilter::default());
     cx.assert_editor_state(indoc! {"error ˇwarning info hint"});

crates/diagnostics/src/items.rs

@@ -64,13 +64,19 @@ impl Render for DiagnosticIndicator {
                 .message
                 .split_once('\n')
                 .map_or(&*diagnostic.message, |(first, _)| first);
+            let diagnostics_already_active = self.any_active_diagnostics(cx);
+            let tooltip = if !diagnostics_already_active {
+                "Expand Diagnostics"
+            } else {
+                "Next Diagnostic"
+            };
             Some(
                 Button::new("diagnostic_message", SharedString::new(message))
                     .label_size(LabelSize::Small)
                     .truncate(true)
-                    .tooltip(|_window, cx| {
+                    .tooltip(move |_window, cx| {
                         Tooltip::for_action(
-                            "Next Diagnostic",
+                            tooltip,
                             &editor::actions::GoToDiagnostic::default(),
                             cx,
                         )
@@ -154,10 +160,18 @@ impl DiagnosticIndicator {
         }
     }
 
+    fn any_active_diagnostics(&self, cx: &mut Context<Self>) -> bool {
+        if let Some(editor) = self.active_editor.as_ref().and_then(|e| e.upgrade()) {
+            editor.read(cx).any_active_diagnostics()
+        } else {
+            false
+        }
+    }
+
     fn go_to_next_diagnostic(&mut self, window: &mut Window, cx: &mut Context<Self>) {
         if let Some(editor) = self.active_editor.as_ref().and_then(|e| e.upgrade()) {
             editor.update(cx, |editor, cx| {
-                editor.go_to_diagnostic_impl(
+                editor.go_to_diagnostic_at_cursor(
                     editor::Direction::Next,
                     GoToDiagnosticSeverityFilter::default(),
                     window,

crates/editor/src/actions.rs

@@ -323,7 +323,8 @@ pub struct SplitSelectionIntoLines {
     pub keep_selections: bool,
 }
 
-/// Goes to the next diagnostic in the file.
+/// Expands the diagnostic under the cursor, if any, in case diagnostics are not
+/// yet active. Otherwise, goes to the next diagnostic in the file.
 #[derive(PartialEq, Clone, Default, Debug, Deserialize, JsonSchema, Action)]
 #[action(namespace = editor)]
 #[serde(deny_unknown_fields)]
@@ -332,7 +333,8 @@ pub struct GoToDiagnostic {
     pub severity: GoToDiagnosticSeverityFilter,
 }
 
-/// Goes to the previous diagnostic in the file.
+/// Expands the diagnostic under the cursor, if any, in case diagnostics are not
+/// yet active. Otherwise, goes to the previous diagnostic in the file.
 #[derive(PartialEq, Clone, Default, Debug, Deserialize, JsonSchema, Action)]
 #[action(namespace = editor)]
 #[serde(deny_unknown_fields)]

crates/editor/src/diagnostics.rs

@@ -77,7 +77,8 @@ impl Editor {
         if !self.diagnostics_enabled() {
             return;
         }
-        self.go_to_diagnostic_impl(Direction::Next, action.severity, window, cx)
+
+        self.go_to_diagnostic_at_cursor(Direction::Next, action.severity, window, cx);
     }
 
     pub fn go_to_prev_diagnostic(
@@ -89,10 +90,43 @@ impl Editor {
         if !self.diagnostics_enabled() {
             return;
         }
-        self.go_to_diagnostic_impl(Direction::Prev, action.severity, window, cx)
+
+        self.go_to_diagnostic_at_cursor(Direction::Prev, action.severity, window, cx);
     }
 
-    pub fn go_to_diagnostic_impl(
+    fn diagnostics_before_cursor<'a>(
+        buffer: &'a MultiBufferSnapshot,
+        cursor: MultiBufferOffset,
+        severity: GoToDiagnosticSeverityFilter,
+    ) -> impl Iterator<Item = DiagnosticEntryRef<'a, MultiBufferOffset>> {
+        buffer
+            .diagnostics_in_range(MultiBufferOffset(0)..cursor)
+            .filter(move |entry| entry.range.start <= cursor)
+            .filter(move |entry| severity.matches(entry.diagnostic.severity))
+            .filter(|entry| entry.range.start != entry.range.end)
+            .filter(|entry| !entry.diagnostic.is_unnecessary)
+    }
+
+    fn diagnostics_after_cursor<'a>(
+        buffer: &'a MultiBufferSnapshot,
+        cursor: MultiBufferOffset,
+        severity: GoToDiagnosticSeverityFilter,
+    ) -> impl Iterator<Item = DiagnosticEntryRef<'a, MultiBufferOffset>> {
+        buffer
+            .diagnostics_in_range(cursor..buffer.len())
+            .filter(move |entry| entry.range.start >= cursor)
+            .filter(move |entry| severity.matches(entry.diagnostic.severity))
+            .filter(|entry| entry.range.start != entry.range.end)
+            .filter(|entry| !entry.diagnostic.is_unnecessary)
+    }
+
+    /// Attempts to expand the diagnostic at the current cursor position,
+    /// updating the cursor position to the diagnostic's start point.
+    ///
+    /// In case there's no diagnostic at the current cursor position, this will
+    /// fallback to finding the next or previous diagnostic instead, depending
+    /// on the provided `direction`.
+    pub fn go_to_diagnostic_at_cursor(
         &mut self,
         direction: Direction,
         severity: GoToDiagnosticSeverityFilter,
@@ -104,6 +138,71 @@ impl Editor {
             .selections
             .newest::<MultiBufferOffset>(&self.display_snapshot(cx));
 
+        let before = Self::diagnostics_before_cursor(&buffer, selection.start, severity);
+        let after = Self::diagnostics_after_cursor(&buffer, selection.start, severity);
+        let active_group_id = match &self.active_diagnostics {
+            ActiveDiagnostic::Group(group) => Some(group.group_id),
+            _ => None,
+        };
+
+        let mut cursor_on_active = false;
+        let mut target = None;
+
+        for diagnostic in after.chain(before) {
+            let contains_cursor = diagnostic.range.contains(&selection.start)
+                || diagnostic.range.end == selection.head();
+
+            if !contains_cursor {
+                continue;
+            }
+
+            if active_group_id == Some(diagnostic.diagnostic.group_id) {
+                cursor_on_active = true;
+            } else if target.is_none() {
+                target = Some(diagnostic);
+            }
+        }
+
+        match (target, cursor_on_active) {
+            (Some(diagnostic), false) => self.activate_diagnostic(&buffer, diagnostic, window, cx),
+            _ => self.go_to_diagnostic_in_direction(
+                &buffer, &selection, direction, severity, window, cx,
+            ),
+        }
+    }
+
+    fn activate_diagnostic(
+        &mut self,
+        buffer: &MultiBufferSnapshot,
+        diagnostic: DiagnosticEntryRef<MultiBufferOffset>,
+        window: &mut Window,
+        cx: &mut Context<Self>,
+    ) {
+        let diagnostic_start = buffer.anchor_after(diagnostic.range.start);
+        let Some((buffer_anchor, _)) = buffer.anchor_to_buffer_anchor(diagnostic_start) else {
+            return;
+        };
+        let buffer_id = buffer_anchor.buffer_id;
+        let snapshot = self.snapshot(window, cx);
+        if snapshot.intersects_fold(diagnostic.range.start) {
+            self.unfold_ranges(std::slice::from_ref(&diagnostic.range), true, false, cx);
+        }
+        self.change_selections(Default::default(), window, cx, |s| {
+            s.select_ranges(vec![diagnostic.range.start..diagnostic.range.start])
+        });
+        self.activate_diagnostics(buffer_id, diagnostic, window, cx);
+        self.refresh_edit_prediction(false, true, window, cx);
+    }
+
+    pub fn go_to_diagnostic_in_direction(
+        &mut self,
+        buffer: &MultiBufferSnapshot,
+        selection: &Selection<MultiBufferOffset>,
+        direction: Direction,
+        severity: GoToDiagnosticSeverityFilter,
+        window: &mut Window,
+        cx: &mut Context<Self>,
+    ) {
         let mut active_group_id = None;
         if let ActiveDiagnostic::Group(active_group) = &self.active_diagnostics
             && active_group.active_range.start.to_offset(&buffer) == selection.start
@@ -111,28 +210,8 @@ impl Editor {
             active_group_id = Some(active_group.group_id);
         }
 
-        fn filtered<'a>(
-            severity: GoToDiagnosticSeverityFilter,
-            diagnostics: impl Iterator<Item = DiagnosticEntryRef<'a, MultiBufferOffset>>,
-        ) -> impl Iterator<Item = DiagnosticEntryRef<'a, MultiBufferOffset>> {
-            diagnostics
-                .filter(move |entry| severity.matches(entry.diagnostic.severity))
-                .filter(|entry| entry.range.start != entry.range.end)
-                .filter(|entry| !entry.diagnostic.is_unnecessary)
-        }
-
-        let before = filtered(
-            severity,
-            buffer
-                .diagnostics_in_range(MultiBufferOffset(0)..selection.start)
-                .filter(|entry| entry.range.start <= selection.start),
-        );
-        let after = filtered(
-            severity,
-            buffer
-                .diagnostics_in_range(selection.start..buffer.len())
-                .filter(|entry| entry.range.start >= selection.start),
-        );
+        let before = Self::diagnostics_before_cursor(&buffer, selection.start, severity);
+        let after = Self::diagnostics_after_cursor(&buffer, selection.start, severity);
 
         let mut found: Option<DiagnosticEntryRef<MultiBufferOffset>> = None;
         if direction == Direction::Prev {
@@ -158,31 +237,12 @@ impl Editor {
                 }
             }
         }
+
         let Some(next_diagnostic) = found else {
             return;
         };
 
-        let next_diagnostic_start = buffer.anchor_after(next_diagnostic.range.start);
-        let Some((buffer_anchor, _)) = buffer.anchor_to_buffer_anchor(next_diagnostic_start) else {
-            return;
-        };
-        let buffer_id = buffer_anchor.buffer_id;
-        let snapshot = self.snapshot(window, cx);
-        if snapshot.intersects_fold(next_diagnostic.range.start) {
-            self.unfold_ranges(
-                std::slice::from_ref(&next_diagnostic.range),
-                true,
-                false,
-                cx,
-            );
-        }
-        self.change_selections(Default::default(), window, cx, |s| {
-            s.select_ranges(vec![
-                next_diagnostic.range.start..next_diagnostic.range.start,
-            ])
-        });
-        self.activate_diagnostics(buffer_id, next_diagnostic, window, cx);
-        self.refresh_edit_prediction(false, true, window, cx);
+        self.activate_diagnostic(&buffer, next_diagnostic, window, cx);
     }
 
     #[cfg(any(test, feature = "test-support"))]
@@ -324,32 +384,37 @@ impl Editor {
             return;
         }
         self.dismiss_diagnostics(cx);
-        let snapshot = self.snapshot(window, cx);
         let buffer = self.buffer.read(cx).snapshot(cx);
-        let Some(renderer) = GlobalDiagnosticRenderer::global(cx) else {
-            return;
+
+        let blocks = if let Some(renderer) = GlobalDiagnosticRenderer::global(cx) {
+            let snapshot = self.snapshot(window, cx);
+            let diagnostic_group = buffer
+                .diagnostic_group(buffer_id, diagnostic.diagnostic.group_id)
+                .collect::<Vec<_>>();
+
+            let language_registry = self
+                .project()
+                .map(|project| project.read(cx).languages().clone());
+
+            let blocks = renderer.render_group(
+                diagnostic_group,
+                buffer_id,
+                snapshot,
+                cx.weak_entity(),
+                language_registry,
+                cx,
+            );
+
+            self.display_map.update(cx, |display_map, cx| {
+                display_map.insert_blocks(blocks, cx).into_iter().collect()
+            })
+        } else {
+            // Ensure that, even if there's no global renderer set, we still use
+            // an empty set of blocks, such that we can record the active group
+            // below instead of bailing out.
+            HashSet::default()
         };
 
-        let diagnostic_group = buffer
-            .diagnostic_group(buffer_id, diagnostic.diagnostic.group_id)
-            .collect::<Vec<_>>();
-
-        let language_registry = self
-            .project()
-            .map(|project| project.read(cx).languages().clone());
-
-        let blocks = renderer.render_group(
-            diagnostic_group,
-            buffer_id,
-            snapshot,
-            cx.weak_entity(),
-            language_registry,
-            cx,
-        );
-
-        let blocks = self.display_map.update(cx, |display_map, cx| {
-            display_map.insert_blocks(blocks, cx).into_iter().collect()
-        });
         self.active_diagnostics = ActiveDiagnostic::Group(ActiveDiagnosticGroup {
             active_range: buffer.anchor_before(diagnostic.range.start)
                 ..buffer.anchor_after(diagnostic.range.end),
@@ -516,4 +581,12 @@ impl Editor {
         self.scrollbar_marker_state.dirty = true;
         cx.notify();
     }
+
+    pub fn any_active_diagnostics(&self) -> bool {
+        match &self.active_diagnostics {
+            ActiveDiagnostic::None => false,
+            ActiveDiagnostic::All => true,
+            ActiveDiagnostic::Group(_) => true,
+        }
+    }
 }

crates/editor/src/editor_tests.rs

@@ -20506,6 +20506,130 @@ async fn go_to_prev_overlapping_diagnostic(executor: BackgroundExecutor, cx: &mu
     "});
 }
 
+#[gpui::test]
+async fn go_to_diagnostic(executor: BackgroundExecutor, cx: &mut TestAppContext) {
+    init_test(cx, |_| {});
+
+    let mut cx = EditorTestContext::new(cx).await;
+    let lsp_store =
+        cx.update_editor(|editor, _, cx| editor.project().unwrap().read(cx).lsp_store());
+
+    // Place the cursor inside the `def` diagnostic (`[12, 15)`) before any
+    // diagnostic is active so we can later confirm that running `editor: go to
+    // diagnostic` will activate this diagnostic instead of advancing to the
+    // next one.
+    cx.set_state(indoc! {"
+        fn func(abc dˇef: i32) -> u32 {
+        }
+    "});
+
+    // Set up the diagnostics:
+    //
+    // * `[11, 12)` (the space before `def`),
+    // * `[12, 15)` (`def`),
+    // * `[25, 28)` (`u32`).
+    cx.update(|_, cx| {
+        lsp_store.update(cx, |lsp_store, cx| {
+            lsp_store
+                .update_diagnostics(
+                    LanguageServerId(0),
+                    lsp::PublishDiagnosticsParams {
+                        uri: lsp::Uri::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, 25),
+                                    lsp::Position::new(0, 28),
+                                ),
+                                severity: Some(lsp::DiagnosticSeverity::ERROR),
+                                ..Default::default()
+                            },
+                        ],
+                    },
+                    None,
+                    DiagnosticSourceKind::Pushed,
+                    &[],
+                    cx,
+                )
+                .unwrap()
+        });
+    });
+
+    executor.run_until_parked();
+
+    // When the cursor is at an inactive diagnostic, cursor should be moved to
+    // the start of that same diagnostic and activate it.
+    cx.update_editor(|editor, window, cx| {
+        editor.go_to_diagnostic(&GoToDiagnostic::default(), window, cx);
+    });
+    cx.assert_editor_state(indoc! {"
+        fn func(abc ˇdef: i32) -> u32 {
+        }
+    "});
+
+    cx.update_editor(|editor, window, cx| {
+        editor.go_to_diagnostic(&GoToDiagnostic::default(), window, cx);
+    });
+    cx.assert_editor_state(indoc! {"
+        fn func(abc def: i32) -> ˇu32 {
+        }
+    "});
+
+    cx.update_editor(|editor, window, cx| {
+        editor.go_to_diagnostic(&GoToDiagnostic::default(), window, cx);
+    });
+    cx.assert_editor_state(indoc! {"
+        fn func(abcˇ def: i32) -> u32 {
+        }
+    "});
+
+    // Manually move the cursor to a different, not yet active diagnostic to
+    // confirm that using `editor: go to diagnostic` will now activate this one.
+    cx.update_editor(|editor, window, cx| {
+        editor.change_selections(Default::default(), window, cx, |s| {
+            s.select_ranges([Point::new(0, 26)..Point::new(0, 26)])
+        });
+    });
+
+    cx.update_editor(|editor, window, cx| {
+        editor.go_to_diagnostic(&GoToDiagnostic::default(), window, cx);
+    });
+    cx.assert_editor_state(indoc! {"
+        fn func(abc def: i32) -> ˇu32 {
+        }
+    "});
+
+    cx.update_editor(|editor, window, cx| {
+        editor.change_selections(Default::default(), window, cx, |s| {
+            s.select_ranges([Point::new(0, 0)..Point::new(0, 0)])
+        });
+    });
+    cx.update_editor(|editor, window, cx| {
+        editor.go_to_diagnostic(&GoToDiagnostic::default(), window, cx);
+    });
+    cx.assert_editor_state(indoc! {"
+        fn func(abcˇ def: i32) -> u32 {
+        }
+    "});
+}
+
 #[gpui::test]
 async fn test_go_to_hunk(executor: BackgroundExecutor, cx: &mut TestAppContext) {
     init_test(cx, |_| {});

docs/src/reference/all-settings.md

@@ -2098,6 +2098,7 @@ To interpret all `.c` files as C++, files called `MyLockFile` as TOML and files
 ```json [settings]
 {
   "diagnostics": {
+    "button": true,
     "include_warnings": true,
     "inline": {
       "enabled": false
@@ -2106,6 +2107,11 @@ To interpret all `.c` files as C++, files called `MyLockFile` as TOML and files
 }
 ```
 
+**Options**
+
+- `button`: Whether to show the project diagnostics button in the status bar
+- `include_warnings`: Whether to show warnings or not by default
+
 ### Inline Diagnostics
 
 - Description: Whether or not to show diagnostics information inline.