Fall back to FindAllReferences if cmd-click did not preform GoToDefinition elsewhere (#9243)

crates/editor/src/editor.rs

@@ -7610,36 +7610,52 @@ impl Editor {
         }
     }
 
-    pub fn go_to_definition(&mut self, _: &GoToDefinition, cx: &mut ViewContext<Self>) {
+    pub fn go_to_definition(
-        self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, false, cx);
+        &mut self,
+        _: &GoToDefinition,
+        cx: &mut ViewContext<Self>,
+    ) -> Task<Result<bool>> {
+        self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, false, cx)
     }
 
-    pub fn go_to_implementation(&mut self, _: &GoToImplementation, cx: &mut ViewContext<Self>) {
+    pub fn go_to_implementation(
-        self.go_to_definition_of_kind(GotoDefinitionKind::Implementation, false, cx);
+        &mut self,
+        _: &GoToImplementation,
+        cx: &mut ViewContext<Self>,
+    ) -> Task<Result<bool>> {
+        self.go_to_definition_of_kind(GotoDefinitionKind::Implementation, false, cx)
     }
 
     pub fn go_to_implementation_split(
         &mut self,
         _: &GoToImplementationSplit,
         cx: &mut ViewContext<Self>,
-    ) {
+    ) -> Task<Result<bool>> {
-        self.go_to_definition_of_kind(GotoDefinitionKind::Implementation, true, cx);
+        self.go_to_definition_of_kind(GotoDefinitionKind::Implementation, true, cx)
     }
 
-    pub fn go_to_type_definition(&mut self, _: &GoToTypeDefinition, cx: &mut ViewContext<Self>) {
+    pub fn go_to_type_definition(
-        self.go_to_definition_of_kind(GotoDefinitionKind::Type, false, cx);
+        &mut self,
+        _: &GoToTypeDefinition,
+        cx: &mut ViewContext<Self>,
+    ) -> Task<Result<bool>> {
+        self.go_to_definition_of_kind(GotoDefinitionKind::Type, false, cx)
     }
 
-    pub fn go_to_definition_split(&mut self, _: &GoToDefinitionSplit, cx: &mut ViewContext<Self>) {
+    pub fn go_to_definition_split(
-        self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, true, cx);
+        &mut self,
+        _: &GoToDefinitionSplit,
+        cx: &mut ViewContext<Self>,
+    ) -> Task<Result<bool>> {
+        self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, true, cx)
     }
 
     pub fn go_to_type_definition_split(
         &mut self,
         _: &GoToTypeDefinitionSplit,
         cx: &mut ViewContext<Self>,
-    ) {
+    ) -> Task<Result<bool>> {
-        self.go_to_definition_of_kind(GotoDefinitionKind::Type, true, cx);
+        self.go_to_definition_of_kind(GotoDefinitionKind::Type, true, cx)
     }
 
     fn go_to_definition_of_kind(
@@ -7647,16 +7663,16 @@ impl Editor {
         kind: GotoDefinitionKind,
         split: bool,
         cx: &mut ViewContext<Self>,
-    ) {
+    ) -> Task<Result<bool>> {
         let Some(workspace) = self.workspace() else {
-            return;
+            return Task::ready(Ok(false));
         };
         let buffer = self.buffer.read(cx);
         let head = self.selections.newest::<usize>(cx).head();
         let (buffer, head) = if let Some(text_anchor) = buffer.text_anchor_for_position(head, cx) {
             text_anchor
         } else {
-            return;
+            return Task::ready(Ok(false));
         };
 
         let project = workspace.read(cx).project().clone();
@@ -7668,17 +7684,18 @@ impl Editor {
 
         cx.spawn(|editor, mut cx| async move {
             let definitions = definitions.await?;
-            editor.update(&mut cx, |editor, cx| {
+            let navigated = editor
-                editor.navigate_to_hover_links(
+                .update(&mut cx, |editor, cx| {
-                    Some(kind),
+                    editor.navigate_to_hover_links(
-                    definitions.into_iter().map(HoverLink::Text).collect(),
+                        Some(kind),
-                    split,
+                        definitions.into_iter().map(HoverLink::Text).collect(),
-                    cx,
+                        split,
-                );
+                        cx,
-            })?;
+                    )
-            Ok::<(), anyhow::Error>(())
+                })?
+                .await?;
+            anyhow::Ok(navigated)
         })
-        .detach_and_log_err(cx);
     }
 
     pub fn open_url(&mut self, _: &OpenUrl, cx: &mut ViewContext<Self>) {
@@ -7707,7 +7724,7 @@ impl Editor {
         mut definitions: Vec<HoverLink>,
         split: bool,
         cx: &mut ViewContext<Editor>,
-    ) {
+    ) -> Task<Result<bool>> {
         // If there is one definition, just open it directly
         if definitions.len() == 1 {
             let definition = definitions.pop().unwrap();
@@ -7726,7 +7743,7 @@ impl Editor {
                 if let Some(target) = target {
                     editor.update(&mut cx, |editor, cx| {
                         let Some(workspace) = editor.workspace() else {
-                            return;
+                            return false;
                         };
                         let pane = workspace.read(cx).active_pane().clone();
 
@@ -7763,12 +7780,12 @@ impl Editor {
                                 });
                             });
                         }
+                        true
                     })
                 } else {
-                    Ok(())
+                    Ok(false)
                 }
             })
-            .detach_and_log_err(cx);
         } else if !definitions.is_empty() {
             let replica_id = self.replica_id(cx);
             cx.spawn(|editor, mut cx| async move {
@@ -7817,9 +7834,9 @@ impl Editor {
                     .context("location tasks")?;
 
                 let Some(workspace) = workspace else {
-                    return Ok(());
+                    return Ok(false);
                 };
-                workspace
+                let opened = workspace
                     .update(&mut cx, |workspace, cx| {
                         Self::open_locations_in_multibuffer(
                             workspace, locations, replica_id, title, split, cx,
@@ -7827,9 +7844,10 @@ impl Editor {
                     })
                     .ok();
 
-                anyhow::Ok(())
+                anyhow::Ok(opened.is_some())
             })
-            .detach_and_log_err(cx);
+        } else {
+            Task::ready(Ok(false))
         }
     }
 

crates/editor/src/element.rs

@@ -258,12 +258,28 @@ impl EditorElement {
         register_action(view, cx, Editor::go_to_prev_diagnostic);
         register_action(view, cx, Editor::go_to_hunk);
         register_action(view, cx, Editor::go_to_prev_hunk);
-        register_action(view, cx, Editor::go_to_definition);
+        register_action(view, cx, |editor, a, cx| {
-        register_action(view, cx, Editor::go_to_definition_split);
+            editor.go_to_definition(a, cx).detach_and_log_err(cx);
-        register_action(view, cx, Editor::go_to_implementation);
+        });
-        register_action(view, cx, Editor::go_to_implementation_split);
+        register_action(view, cx, |editor, a, cx| {
-        register_action(view, cx, Editor::go_to_type_definition);
+            editor.go_to_definition_split(a, cx).detach_and_log_err(cx);
-        register_action(view, cx, Editor::go_to_type_definition_split);
+        });
+        register_action(view, cx, |editor, a, cx| {
+            editor.go_to_implementation(a, cx).detach_and_log_err(cx);
+        });
+        register_action(view, cx, |editor, a, cx| {
+            editor
+                .go_to_implementation_split(a, cx)
+                .detach_and_log_err(cx);
+        });
+        register_action(view, cx, |editor, a, cx| {
+            editor.go_to_type_definition(a, cx).detach_and_log_err(cx);
+        });
+        register_action(view, cx, |editor, a, cx| {
+            editor
+                .go_to_type_definition_split(a, cx)
+                .detach_and_log_err(cx);
+        });
         register_action(view, cx, Editor::open_url);
         register_action(view, cx, Editor::fold);
         register_action(view, cx, Editor::fold_at);

crates/editor/src/hover_links.rs

@@ -1,7 +1,7 @@
 use crate::{
     hover_popover::{self, InlayHover},
-    Anchor, Editor, EditorSnapshot, GoToDefinition, GoToTypeDefinition, InlayId, PointForPosition,
+    Anchor, Editor, EditorSnapshot, FindAllReferences, GoToDefinition, GoToTypeDefinition, InlayId,
-    SelectPhase,
+    PointForPosition, SelectPhase,
 };
 use gpui::{px, AsyncWindowContext, Model, Modifiers, Task, ViewContext};
 use language::{Bias, ToOffset};
@@ -11,9 +11,10 @@ use project::{
     HoverBlock, HoverBlockKind, InlayHintLabelPartTooltip, InlayHintTooltip, LocationLink,
     ResolveState,
 };
-use std::ops::Range;
+use std::{cmp, ops::Range};
+use text::Point;
 use theme::ActiveTheme as _;
-use util::{maybe, TryFutureExt};
+use util::{maybe, ResultExt, TryFutureExt};
 
 #[derive(Debug)]
 pub struct HoveredLinkState {
@@ -131,6 +132,47 @@ impl Editor {
         modifiers: Modifiers,
         cx: &mut ViewContext<Editor>,
     ) {
+        let selection_before_revealing = self.selections.newest::<Point>(cx);
+        let multi_buffer_snapshot = self.buffer().read(cx).snapshot(cx);
+        let before_revealing_head = selection_before_revealing.head();
+        let before_revealing_tail = selection_before_revealing.tail();
+        let before_revealing = match before_revealing_tail.cmp(&before_revealing_head) {
+            cmp::Ordering::Equal | cmp::Ordering::Less => {
+                multi_buffer_snapshot.anchor_after(before_revealing_head)
+                    ..multi_buffer_snapshot.anchor_before(before_revealing_tail)
+            }
+            cmp::Ordering::Greater => {
+                multi_buffer_snapshot.anchor_before(before_revealing_tail)
+                    ..multi_buffer_snapshot.anchor_after(before_revealing_head)
+            }
+        };
+        drop(multi_buffer_snapshot);
+
+        let reveal_task = self.cmd_click_reveal_task(point, modifiers, cx);
+        cx.spawn(|editor, mut cx| async move {
+            let definition_revealed = reveal_task.await.log_err().unwrap_or(false);
+            let find_references = editor
+                .update(&mut cx, |editor, cx| {
+                    if definition_revealed && revealed_elsewhere(editor, before_revealing, cx) {
+                        return None;
+                    }
+                    editor.find_all_references(&FindAllReferences, cx)
+                })
+                .ok()
+                .flatten();
+            if let Some(find_references) = find_references {
+                find_references.await.log_err();
+            }
+        })
+        .detach();
+    }
+
+    fn cmd_click_reveal_task(
+        &mut self,
+        point: PointForPosition,
+        modifiers: Modifiers,
+        cx: &mut ViewContext<Editor>,
+    ) -> Task<anyhow::Result<bool>> {
         if let Some(hovered_link_state) = self.hovered_link_state.take() {
             self.hide_hovered_link(cx);
             if !hovered_link_state.links.is_empty() {
@@ -138,8 +180,12 @@ impl Editor {
                     cx.focus(&self.focus_handle);
                 }
 
-                self.navigate_to_hover_links(None, hovered_link_state.links, modifiers.alt, cx);
+                return self.navigate_to_hover_links(
-                return;
+                    None,
+                    hovered_link_state.links,
+                    modifiers.alt,
+                    cx,
+                );
             }
         }
 
@@ -160,10 +206,52 @@ impl Editor {
             } else {
                 self.go_to_definition(&GoToDefinition, cx)
             }
+        } else {
+            Task::ready(Ok(false))
         }
     }
 }
 
+fn revealed_elsewhere(
+    editor: &mut Editor,
+    before_revealing: Range<Anchor>,
+    cx: &mut ViewContext<'_, Editor>,
+) -> bool {
+    let multi_buffer_snapshot = editor.buffer().read(cx).snapshot(cx);
+
+    let selection_after_revealing = editor.selections.newest::<Point>(cx);
+    let after_revealing_head = selection_after_revealing.head();
+    let after_revealing_tail = selection_after_revealing.tail();
+    let after_revealing = match after_revealing_tail.cmp(&after_revealing_head) {
+        cmp::Ordering::Equal | cmp::Ordering::Less => {
+            multi_buffer_snapshot.anchor_after(after_revealing_tail)
+                ..multi_buffer_snapshot.anchor_before(after_revealing_head)
+        }
+        cmp::Ordering::Greater => {
+            multi_buffer_snapshot.anchor_after(after_revealing_head)
+                ..multi_buffer_snapshot.anchor_before(after_revealing_tail)
+        }
+    };
+
+    let before_intersects_after_range = (before_revealing
+        .start
+        .cmp(&after_revealing.start, &multi_buffer_snapshot)
+        .is_ge()
+        && before_revealing
+            .start
+            .cmp(&after_revealing.end, &multi_buffer_snapshot)
+            .is_le())
+        || (before_revealing
+            .end
+            .cmp(&after_revealing.start, &multi_buffer_snapshot)
+            .is_ge()
+            && before_revealing
+                .end
+                .cmp(&after_revealing.end, &multi_buffer_snapshot)
+                .is_le());
+    !before_intersects_after_range
+}
+
 pub fn update_inlay_link_and_hover_points(
     snapshot: &EditorSnapshot,
     point_for_position: PointForPosition,
@@ -473,10 +561,11 @@ pub fn show_link_definition(
                 )),
             };
 
-            this.update(&mut cx, |this, cx| {
+            this.update(&mut cx, |editor, cx| {
                 // Clear any existing highlights
-                this.clear_highlights::<HoveredLinkState>(cx);
+                editor.clear_highlights::<HoveredLinkState>(cx);
-                let Some(hovered_link_state) = this.hovered_link_state.as_mut() else {
+                let Some(hovered_link_state) = editor.hovered_link_state.as_mut() else {
+                    editor.hide_hovered_link(cx);
                     return;
                 };
                 hovered_link_state.preferred_kind = preferred_kind;
@@ -485,43 +574,12 @@ pub fn show_link_definition(
                     .and_then(|(symbol_range, _)| symbol_range.clone());
 
                 if let Some((symbol_range, definitions)) = result {
-                    hovered_link_state.links = definitions.clone();
+                    hovered_link_state.links = definitions;
 
-                    let buffer_snapshot = buffer.read(cx).snapshot();
+                    let underline_hovered_link = hovered_link_state.links.len() > 0
+                        || hovered_link_state.symbol_range.is_some();
 
-                    // Only show highlight if there exists a definition to jump to that doesn't contain
+                    if underline_hovered_link {
-                    // the current location.
-                    let any_definition_does_not_contain_current_location =
-                        definitions.iter().any(|definition| {
-                            match &definition {
-                                HoverLink::Text(link) => {
-                                    if link.target.buffer == buffer {
-                                        let range = &link.target.range;
-                                        // Expand range by one character as lsp definition ranges include positions adjacent
-                                        // but not contained by the symbol range
-                                        let start = buffer_snapshot.clip_offset(
-                                            range
-                                                .start
-                                                .to_offset(&buffer_snapshot)
-                                                .saturating_sub(1),
-                                            Bias::Left,
-                                        );
-                                        let end = buffer_snapshot.clip_offset(
-                                            range.end.to_offset(&buffer_snapshot) + 1,
-                                            Bias::Right,
-                                        );
-                                        let offset = buffer_position.to_offset(&buffer_snapshot);
-                                        !(start <= offset && end >= offset)
-                                    } else {
-                                        true
-                                    }
-                                }
-                                HoverLink::InlayHint(_, _) => true,
-                                HoverLink::Url(_) => true,
-                            }
-                        });
-
-                    if any_definition_does_not_contain_current_location {
                         let style = gpui::HighlightStyle {
                             underline: Some(gpui::UnderlineStyle {
                                 thickness: px(1.),
@@ -547,15 +605,14 @@ pub fn show_link_definition(
                             });
 
                         match highlight_range {
-                            RangeInEditor::Text(text_range) => {
+                            RangeInEditor::Text(text_range) => editor
-                                this.highlight_text::<HoveredLinkState>(vec![text_range], style, cx)
+                                .highlight_text::<HoveredLinkState>(vec![text_range], style, cx),
-                            }
+                            RangeInEditor::Inlay(highlight) => editor
-                            RangeInEditor::Inlay(highlight) => this
                                 .highlight_inlays::<HoveredLinkState>(vec![highlight], style, cx),
                         }
-                    } else {
-                        this.hide_hovered_link(cx);
                     }
+                } else {
+                    editor.hide_hovered_link(cx);
                 }
             })?;
 
@@ -643,7 +700,10 @@ mod tests {
     use gpui::Modifiers;
     use indoc::indoc;
     use language::language_settings::InlayHintSettings;
-    use lsp::request::{GotoDefinition, GotoTypeDefinition};
+    use lsp::{
+        request::{GotoDefinition, GotoTypeDefinition},
+        References,
+    };
     use util::assert_set_eq;
     use workspace::item::Item;
 
@@ -1208,4 +1268,147 @@ mod tests {
         cx.simulate_click(screen_coord, Modifiers::command());
         assert_eq!(cx.opened_url(), Some("https://zed.dev/releases".into()));
     }
+
+    #[gpui::test]
+    async fn test_cmd_click_back_and_forth(cx: &mut gpui::TestAppContext) {
+        init_test(cx, |_| {});
+        let mut cx = EditorLspTestContext::new_rust(lsp::ServerCapabilities::default(), cx).await;
+        cx.set_state(indoc! {"
+            fn test() {
+                do_work();
+            }ˇ
+
+            fn do_work() {
+                test();
+            }
+        "});
+
+        // cmd-click on `test` definition and usage, and expect Zed to allow going back and forth,
+        // because cmd-click first searches for definitions to go to, and then fall backs to symbol usages to go to.
+        let definition_hover_point = cx.pixel_position(indoc! {"
+            fn testˇ() {
+                do_work();
+            }
+
+            fn do_work() {
+                test();
+            }
+        "});
+        let definition_display_point = cx.display_point(indoc! {"
+            fn testˇ() {
+                do_work();
+            }
+
+            fn do_work() {
+                test();
+            }
+        "});
+        let definition_range = cx.lsp_range(indoc! {"
+            fn «test»() {
+                do_work();
+            }
+
+            fn do_work() {
+                test();
+            }
+        "});
+        let reference_hover_point = cx.pixel_position(indoc! {"
+            fn test() {
+                do_work();
+            }
+
+            fn do_work() {
+                testˇ();
+            }
+        "});
+        let reference_display_point = cx.display_point(indoc! {"
+            fn test() {
+                do_work();
+            }
+
+            fn do_work() {
+                testˇ();
+            }
+        "});
+        let reference_range = cx.lsp_range(indoc! {"
+            fn test() {
+                do_work();
+            }
+
+            fn do_work() {
+                «test»();
+            }
+        "});
+        let expected_uri = cx.buffer_lsp_url.clone();
+        cx.lsp
+            .handle_request::<GotoDefinition, _, _>(move |params, _| {
+                let expected_uri = expected_uri.clone();
+                async move {
+                    assert_eq!(
+                        params.text_document_position_params.text_document.uri,
+                        expected_uri
+                    );
+                    let position = params.text_document_position_params.position;
+                    Ok(Some(lsp::GotoDefinitionResponse::Link(
+                        if position.line == reference_display_point.row()
+                            && position.character == reference_display_point.column()
+                        {
+                            vec![lsp::LocationLink {
+                                origin_selection_range: None,
+                                target_uri: params.text_document_position_params.text_document.uri,
+                                target_range: definition_range,
+                                target_selection_range: definition_range,
+                            }]
+                        } else {
+                            // We cannot navigate to the definition outside of its reference point
+                            Vec::new()
+                        },
+                    )))
+                }
+            });
+        let expected_uri = cx.buffer_lsp_url.clone();
+        cx.lsp.handle_request::<References, _, _>(move |params, _| {
+            let expected_uri = expected_uri.clone();
+            async move {
+                assert_eq!(
+                    params.text_document_position.text_document.uri,
+                    expected_uri
+                );
+                let position = params.text_document_position.position;
+                // Zed should not look for references if GotoDefinition works or returns non-empty result
+                assert_eq!(position.line, definition_display_point.row());
+                assert_eq!(position.character, definition_display_point.column());
+                Ok(Some(vec![lsp::Location {
+                    uri: params.text_document_position.text_document.uri,
+                    range: reference_range,
+                }]))
+            }
+        });
+
+        for _ in 0..5 {
+            cx.simulate_click(definition_hover_point, Modifiers::command());
+            cx.background_executor.run_until_parked();
+            cx.assert_editor_state(indoc! {"
+                fn test() {
+                    do_work();
+                }
+
+                fn do_work() {
+                    «testˇ»();
+                }
+            "});
+
+            cx.simulate_click(reference_hover_point, Modifiers::command());
+            cx.background_executor.run_until_parked();
+            cx.assert_editor_state(indoc! {"
+                fn «testˇ»() {
+                    do_work();
+                }
+
+                fn do_work() {
+                    test();
+                }
+            "});
+        }
+    }
 }

crates/project/src/lsp_command.rs

@@ -97,7 +97,7 @@ pub(crate) struct PerformRename {
     pub push_to_history: bool,
 }
 
-pub(crate) struct GetDefinition {
+pub struct GetDefinition {
     pub position: PointUtf16,
 }