From e482fcde5b5872e75c31d75887f04e7ab1557dca Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 20 Aug 2024 14:56:19 +0300 Subject: [PATCH] Fall back to FindAllReferences if GoToDefinition have not navigated (#16512) Follow-up of https://github.com/zed-industries/zed/pull/9243 Release Notes: - N/A --------- Co-authored-by: Alex Kladov --- crates/editor/src/editor.rs | 193 +++++++++++++++++------------- crates/editor/src/editor_tests.rs | 121 +++++++++++++++++++ crates/editor/src/hover_links.rs | 12 +- crates/project/src/project.rs | 1 + 4 files changed, 237 insertions(+), 90 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 915a07fbd7..6cbdb0daaa 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -266,6 +266,22 @@ pub enum Direction { Next, } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum Navigated { + Yes, + No, +} + +impl Navigated { + pub fn from_bool(yes: bool) -> Navigated { + if yes { + Navigated::Yes + } else { + Navigated::No + } + } +} + pub fn init_settings(cx: &mut AppContext) { EditorSettings::register(cx); } @@ -1561,6 +1577,7 @@ pub(crate) struct NavigationData { scroll_top_row: u32, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] enum GotoDefinitionKind { Symbol, Declaration, @@ -4843,7 +4860,7 @@ impl Editor { let range = Anchor { buffer_id, - excerpt_id: excerpt_id, + excerpt_id, text_anchor: start, }..Anchor { buffer_id, @@ -9020,15 +9037,28 @@ impl Editor { &mut self, _: &GoToDefinition, cx: &mut ViewContext, - ) -> Task> { - self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, false, cx) + ) -> Task> { + let definition = self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, false, cx); + let references = self.find_all_references(&FindAllReferences, cx); + cx.background_executor().spawn(async move { + if definition.await? == Navigated::Yes { + return Ok(Navigated::Yes); + } + if let Some(references) = references { + if references.await? == Navigated::Yes { + return Ok(Navigated::Yes); + } + } + + Ok(Navigated::No) + }) } pub fn go_to_declaration( &mut self, _: &GoToDeclaration, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { self.go_to_definition_of_kind(GotoDefinitionKind::Declaration, false, cx) } @@ -9036,7 +9066,7 @@ impl Editor { &mut self, _: &GoToDeclaration, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { self.go_to_definition_of_kind(GotoDefinitionKind::Declaration, true, cx) } @@ -9044,7 +9074,7 @@ impl Editor { &mut self, _: &GoToImplementation, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { self.go_to_definition_of_kind(GotoDefinitionKind::Implementation, false, cx) } @@ -9052,7 +9082,7 @@ impl Editor { &mut self, _: &GoToImplementationSplit, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { self.go_to_definition_of_kind(GotoDefinitionKind::Implementation, true, cx) } @@ -9060,7 +9090,7 @@ impl Editor { &mut self, _: &GoToTypeDefinition, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { self.go_to_definition_of_kind(GotoDefinitionKind::Type, false, cx) } @@ -9068,7 +9098,7 @@ impl Editor { &mut self, _: &GoToDefinitionSplit, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, true, cx) } @@ -9076,7 +9106,7 @@ impl Editor { &mut self, _: &GoToTypeDefinitionSplit, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { self.go_to_definition_of_kind(GotoDefinitionKind::Type, true, cx) } @@ -9085,16 +9115,16 @@ impl Editor { kind: GotoDefinitionKind, split: bool, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { let Some(workspace) = self.workspace() else { - return Task::ready(Ok(false)); + return Task::ready(Ok(Navigated::No)); }; let buffer = self.buffer.read(cx); let head = self.selections.newest::(cx).head(); let (buffer, head) = if let Some(text_anchor) = buffer.text_anchor_for_position(head, cx) { text_anchor } else { - return Task::ready(Ok(false)); + return Task::ready(Ok(Navigated::No)); }; let project = workspace.read(cx).project().clone(); @@ -9153,7 +9183,7 @@ impl Editor { mut definitions: Vec, split: bool, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { // If there is one definition, just open it directly if definitions.len() == 1 { let definition = definitions.pop().unwrap(); @@ -9169,77 +9199,61 @@ impl Editor { }; cx.spawn(|editor, mut cx| async move { let target = target_task.await.context("target resolution task")?; - if let Some(target) = target { - editor.update(&mut cx, |editor, cx| { - let Some(workspace) = editor.workspace() else { - return false; - }; - let pane = workspace.read(cx).active_pane().clone(); + let Some(target) = target else { + return Ok(Navigated::No); + }; + editor.update(&mut cx, |editor, cx| { + let Some(workspace) = editor.workspace() else { + return Navigated::No; + }; + let pane = workspace.read(cx).active_pane().clone(); - let range = target.range.to_offset(target.buffer.read(cx)); - let range = editor.range_for_match(&range); + let range = target.range.to_offset(target.buffer.read(cx)); + let range = editor.range_for_match(&range); - /// If select range has more than one line, we - /// just point the cursor to range.start. - fn check_multiline_range( - buffer: &Buffer, - range: Range, - ) -> Range { - if buffer.offset_to_point(range.start).row - == buffer.offset_to_point(range.end).row - { - range - } else { - range.start..range.start - } - } + if Some(&target.buffer) == editor.buffer.read(cx).as_singleton().as_ref() { + let buffer = target.buffer.read(cx); + let range = check_multiline_range(buffer, range); + editor.change_selections(Some(Autoscroll::focused()), cx, |s| { + s.select_ranges([range]); + }); + } else { + cx.window_context().defer(move |cx| { + let target_editor: View = + workspace.update(cx, |workspace, cx| { + let pane = if split { + workspace.adjacent_pane(cx) + } else { + workspace.active_pane().clone() + }; - if Some(&target.buffer) == editor.buffer.read(cx).as_singleton().as_ref() { - let buffer = target.buffer.read(cx); - let range = check_multiline_range(buffer, range); - editor.change_selections(Some(Autoscroll::focused()), cx, |s| { - s.select_ranges([range]); - }); - } else { - cx.window_context().defer(move |cx| { - let target_editor: View = - workspace.update(cx, |workspace, cx| { - let pane = if split { - workspace.adjacent_pane(cx) - } else { - workspace.active_pane().clone() - }; - - workspace.open_project_item( - pane, - target.buffer.clone(), - true, - true, - cx, - ) - }); - target_editor.update(cx, |target_editor, cx| { - // When selecting a definition in a different buffer, disable the nav history - // to avoid creating a history entry at the previous cursor location. - pane.update(cx, |pane, _| pane.disable_history()); - let buffer = target.buffer.read(cx); - let range = check_multiline_range(buffer, range); - target_editor.change_selections( - Some(Autoscroll::focused()), + workspace.open_project_item( + pane, + target.buffer.clone(), + true, + true, cx, - |s| { - s.select_ranges([range]); - }, - ); - pane.update(cx, |pane, _| pane.enable_history()); + ) }); + target_editor.update(cx, |target_editor, cx| { + // When selecting a definition in a different buffer, disable the nav history + // to avoid creating a history entry at the previous cursor location. + pane.update(cx, |pane, _| pane.disable_history()); + let buffer = target.buffer.read(cx); + let range = check_multiline_range(buffer, range); + target_editor.change_selections( + Some(Autoscroll::focused()), + cx, + |s| { + s.select_ranges([range]); + }, + ); + pane.update(cx, |pane, _| pane.enable_history()); }); - } - true - }) - } else { - Ok(false) - } + }); + } + Navigated::Yes + }) }) } else if !definitions.is_empty() { let replica_id = self.replica_id(cx); @@ -9289,7 +9303,7 @@ impl Editor { .context("location tasks")?; let Some(workspace) = workspace else { - return Ok(false); + return Ok(Navigated::No); }; let opened = workspace .update(&mut cx, |workspace, cx| { @@ -9299,10 +9313,10 @@ impl Editor { }) .ok(); - anyhow::Ok(opened.is_some()) + anyhow::Ok(Navigated::from_bool(opened.is_some())) }) } else { - Task::ready(Ok(false)) + Task::ready(Ok(Navigated::No)) } } @@ -9361,7 +9375,7 @@ impl Editor { &mut self, _: &FindAllReferences, cx: &mut ViewContext, - ) -> Option>> { + ) -> Option>> { let multi_buffer = self.buffer.read(cx); let selection = self.selections.newest::(cx); let head = selection.head(); @@ -9416,7 +9430,7 @@ impl Editor { let locations = references.await?; if locations.is_empty() { - return anyhow::Ok(()); + return anyhow::Ok(Navigated::No); } workspace.update(&mut cx, |workspace, cx| { @@ -9436,6 +9450,7 @@ impl Editor { Self::open_locations_in_multibuffer( workspace, locations, replica_id, title, false, cx, ); + Navigated::Yes }) })) } @@ -13277,3 +13292,13 @@ fn hunk_status(hunk: &DiffHunk) -> DiffHunkStatus { DiffHunkStatus::Modified } } + +/// If select range has more than one line, we +/// just point the cursor to range.start. +fn check_multiline_range(buffer: &Buffer, range: Range) -> Range { + if buffer.offset_to_point(range.start).row == buffer.offset_to_point(range.end).row { + range + } else { + range.start..range.start + } +} diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 091eca5e8e..b0b36dffb4 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -13221,6 +13221,127 @@ let foo = 15;"#, }); } +#[gpui::test] +async fn test_goto_definition_with_find_all_references_fallback(cx: &mut gpui::TestAppContext) { + init_test(cx, |_| {}); + let mut cx = EditorLspTestContext::new_rust( + lsp::ServerCapabilities { + definition_provider: Some(lsp::OneOf::Left(true)), + references_provider: Some(lsp::OneOf::Left(true)), + ..lsp::ServerCapabilities::default() + }, + cx, + ) + .await; + + let set_up_lsp_handlers = |empty_go_to_definition: bool, cx: &mut EditorLspTestContext| { + let go_to_definition = cx.lsp.handle_request::( + move |params, _| async move { + if empty_go_to_definition { + Ok(None) + } else { + Ok(Some(lsp::GotoDefinitionResponse::Scalar(lsp::Location { + uri: params.text_document_position_params.text_document.uri, + range: lsp::Range::new(lsp::Position::new(4, 3), lsp::Position::new(4, 6)), + }))) + } + }, + ); + let references = + cx.lsp + .handle_request::(move |params, _| async move { + Ok(Some(vec![lsp::Location { + uri: params.text_document_position.text_document.uri, + range: lsp::Range::new(lsp::Position::new(0, 8), lsp::Position::new(0, 11)), + }])) + }); + (go_to_definition, references) + }; + + cx.set_state( + &r#"fn one() { + let mut a = ˇtwo(); + } + + fn two() {}"# + .unindent(), + ); + set_up_lsp_handlers(false, &mut cx); + let navigated = cx + .update_editor(|editor, cx| editor.go_to_definition(&GoToDefinition, cx)) + .await + .expect("Failed to navigate to definition"); + assert_eq!( + navigated, + Navigated::Yes, + "Should have navigated to definition from the GetDefinition response" + ); + cx.assert_editor_state( + &r#"fn one() { + let mut a = two(); + } + + fn «twoˇ»() {}"# + .unindent(), + ); + + let editors = cx.update_workspace(|workspace, cx| { + workspace.items_of_type::(cx).collect::>() + }); + cx.update_editor(|_, test_editor_cx| { + assert_eq!( + editors.len(), + 1, + "Initially, only one, test, editor should be open in the workspace" + ); + assert_eq!( + test_editor_cx.view(), + editors.last().expect("Asserted len is 1") + ); + }); + + set_up_lsp_handlers(true, &mut cx); + let navigated = cx + .update_editor(|editor, cx| editor.go_to_definition(&GoToDefinition, cx)) + .await + .expect("Failed to navigate to lookup references"); + assert_eq!( + navigated, + Navigated::Yes, + "Should have navigated to references as a fallback after empty GoToDefinition response" + ); + // We should not change the selections in the existing file, + // if opening another milti buffer with the references + cx.assert_editor_state( + &r#"fn one() { + let mut a = two(); + } + + fn «twoˇ»() {}"# + .unindent(), + ); + let editors = cx.update_workspace(|workspace, cx| { + workspace.items_of_type::(cx).collect::>() + }); + cx.update_editor(|_, test_editor_cx| { + assert_eq!( + editors.len(), + 2, + "After falling back to references search, we open a new editor with the results" + ); + let references_fallback_text = editors + .into_iter() + .find(|new_editor| new_editor != test_editor_cx.view()) + .expect("Should have one non-test editor now") + .read(test_editor_cx) + .text(test_editor_cx); + assert_eq!( + references_fallback_text, "fn one() {\n let mut a = two();\n}", + "Should use the range from the references response and not the GoToDefinition one" + ); + }); +} + fn empty_range(row: usize, column: usize) -> Range { let point = DisplayPoint::new(DisplayRow(row as u32), column as u32); point..point diff --git a/crates/editor/src/hover_links.rs b/crates/editor/src/hover_links.rs index 910415f313..deefae9d8c 100644 --- a/crates/editor/src/hover_links.rs +++ b/crates/editor/src/hover_links.rs @@ -2,7 +2,7 @@ use crate::{ hover_popover::{self, InlayHover}, scroll::ScrollAmount, Anchor, Editor, EditorSnapshot, FindAllReferences, GoToDefinition, GoToTypeDefinition, InlayId, - PointForPosition, SelectPhase, + Navigated, PointForPosition, SelectPhase, }; use gpui::{px, AppContext, AsyncWindowContext, Model, Modifiers, Task, ViewContext}; use language::{Bias, ToOffset}; @@ -157,10 +157,10 @@ impl Editor { ) { 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 definition_revealed = reveal_task.await.log_err().unwrap_or(Navigated::No); let find_references = editor .update(&mut cx, |editor, cx| { - if definition_revealed { + if definition_revealed == Navigated::Yes { return None; } editor.find_all_references(&FindAllReferences, cx) @@ -194,7 +194,7 @@ impl Editor { point: PointForPosition, modifiers: Modifiers, cx: &mut ViewContext, - ) -> Task> { + ) -> Task> { if let Some(hovered_link_state) = self.hovered_link_state.take() { self.hide_hovered_link(cx); if !hovered_link_state.links.is_empty() { @@ -211,7 +211,7 @@ impl Editor { .read(cx) .text_anchor_for_position(current_position, cx) else { - return Task::ready(Ok(false)); + return Task::ready(Ok(Navigated::No)); }; let links = hovered_link_state .links @@ -247,7 +247,7 @@ impl Editor { self.go_to_definition(&GoToDefinition, cx) } } else { - Task::ready(Ok(false)) + Task::ready(Ok(Navigated::No)) } } } diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 04ecad7fc5..423c9410e1 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -232,6 +232,7 @@ pub struct Project { cached_shell_environments: HashMap>, } +#[derive(Debug)] pub enum LanguageServerToQuery { Primary, Other(LanguageServerId),