From 7d5048e909b113d3c987ceec9cff90a862be6e6d Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 2 Apr 2024 14:31:58 -0700 Subject: [PATCH] Revert PR #6924 - go to reference when there's only one (#10094) This PR reverts #6924, for the reasons stated in https://github.com/zed-industries/zed/pull/6924#issuecomment-2033076300. It also fixes an issue were the `find_all_references_task_sources` wasn't cleaned up in all cases. Release Notes: - N/A --------- Co-authored-by: Marshall Co-authored-by: Marshall Bowers --- crates/editor/src/editor.rs | 99 +++++--------------- crates/editor/src/editor_tests.rs | 99 -------------------- crates/editor/src/hover_links.rs | 148 +----------------------------- crates/util/src/util.rs | 1 + 4 files changed, 24 insertions(+), 323 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 64c9bbc2d8dbd2aa11c4b88170ed983319a017fe..6363202c910b7eced11c70245e61b5a83435290a 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -125,7 +125,7 @@ use ui::{ h_flex, prelude::*, ButtonSize, ButtonStyle, IconButton, IconName, IconSize, ListItem, Popover, Tooltip, }; -use util::{maybe, post_inc, RangeExt, ResultExt, TryFutureExt}; +use util::{defer, maybe, post_inc, RangeExt, ResultExt, TryFutureExt}; use workspace::Toast; use workspace::{ searchable::SearchEvent, ItemNavHistory, SplitDirection, ViewId, Workspace, WorkspaceId, @@ -7853,9 +7853,10 @@ impl Editor { Bias::Left }, ); + match self .find_all_references_task_sources - .binary_search_by(|task_anchor| task_anchor.cmp(&head_anchor, &multi_buffer_snapshot)) + .binary_search_by(|anchor| anchor.cmp(&head_anchor, &multi_buffer_snapshot)) { Ok(_) => { log::info!( @@ -7873,66 +7874,27 @@ impl Editor { let workspace = self.workspace()?; let project = workspace.read(cx).project().clone(); let references = project.update(cx, |project, cx| project.references(&buffer, head, cx)); - let open_task = cx.spawn(|editor, mut cx| async move { - let mut locations = references.await?; - let snapshot = buffer.update(&mut cx, |buffer, _| buffer.snapshot())?; - let head_offset = text::ToOffset::to_offset(&head, &snapshot); - - // LSP may return references that contain the item itself we requested `find_all_references` for (eg. rust-analyzer) - // So we will remove it from locations - // If there is only one reference, we will not do this filter cause it may make locations empty - if locations.len() > 1 { - cx.update(|cx| { - locations.retain(|location| { - // fn foo(x : i64) { - // ^ - // println!(x); - // } - // It is ok to find reference when caret being at ^ (the end of the word) - // So we turn offset into inclusive to include the end of the word - !location - .range - .to_offset(location.buffer.read(cx)) - .to_inclusive() - .contains(&head_offset) + Some(cx.spawn(|editor, mut cx| async move { + let _cleanup = defer({ + let mut cx = cx.clone(); + move || { + let _ = editor.update(&mut cx, |editor, _| { + if let Ok(i) = + editor + .find_all_references_task_sources + .binary_search_by(|anchor| { + anchor.cmp(&head_anchor, &multi_buffer_snapshot) + }) + { + editor.find_all_references_task_sources.remove(i); + } }); - })?; - } + } + }); + let locations = references.await?; if locations.is_empty() { - return Ok(()); - } - - // If there is one reference, just open it directly - if locations.len() == 1 { - let target = locations.pop().unwrap(); - - return editor.update(&mut cx, |editor, cx| { - let range = target.range.to_offset(target.buffer.read(cx)); - let range = editor.range_for_match(&range); - - if Some(&target.buffer) == editor.buffer().read(cx).as_singleton().as_ref() { - editor.change_selections(Some(Autoscroll::fit()), cx, |s| { - s.select_ranges([range]); - }); - } else { - cx.window_context().defer(move |cx| { - let target_editor: View = - workspace.update(cx, |workspace, cx| { - workspace.open_project_item( - workspace.active_pane().clone(), - target.buffer.clone(), - cx, - ) - }); - target_editor.update(cx, |target_editor, cx| { - target_editor.change_selections(Some(Autoscroll::fit()), cx, |s| { - s.select_ranges([range]); - }) - }) - }) - } - }); + return anyhow::Ok(()); } workspace.update(&mut cx, |workspace, cx| { @@ -7952,24 +7914,7 @@ impl Editor { Self::open_locations_in_multibuffer( workspace, locations, replica_id, title, false, cx, ); - })?; - - Ok(()) - }); - Some(cx.spawn(|editor, mut cx| async move { - open_task.await?; - editor.update(&mut cx, |editor, _| { - if let Ok(i) = - editor - .find_all_references_task_sources - .binary_search_by(|task_anchor| { - task_anchor.cmp(&head_anchor, &multi_buffer_snapshot) - }) - { - editor.find_all_references_task_sources.remove(i); - } - })?; - anyhow::Ok(()) + }) })) } diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 87037e57c28c1a05e255d9b8118441d48b0c9770..b6abacb2a94e38e11d63d4368af2eb174ff98ac3 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -8498,105 +8498,6 @@ async fn test_document_format_with_prettier(cx: &mut gpui::TestAppContext) { ); } -#[gpui::test] -async fn test_find_all_references(cx: &mut gpui::TestAppContext) { - init_test(cx, |_| {}); - - let mut cx = EditorLspTestContext::new_rust( - lsp::ServerCapabilities { - document_formatting_provider: Some(lsp::OneOf::Left(true)), - ..Default::default() - }, - cx, - ) - .await; - - cx.set_state(indoc! {" - fn foo(«paramˇ»: i64) { - println!(param); - } - "}); - - cx.lsp - .handle_request::(move |_, _| async move { - Ok(Some(vec![ - lsp::Location { - uri: lsp::Url::from_file_path("/root/dir/file.rs").unwrap(), - range: lsp::Range::new(lsp::Position::new(0, 7), lsp::Position::new(0, 12)), - }, - lsp::Location { - uri: lsp::Url::from_file_path("/root/dir/file.rs").unwrap(), - range: lsp::Range::new(lsp::Position::new(1, 13), lsp::Position::new(1, 18)), - }, - ])) - }); - - let references = cx - .update_editor(|editor, cx| editor.find_all_references(&FindAllReferences, cx)) - .unwrap(); - - cx.executor().run_until_parked(); - - cx.executor().start_waiting(); - references.await.unwrap(); - - cx.assert_editor_state(indoc! {" - fn foo(param: i64) { - println!(«paramˇ»); - } - "}); - - let references = cx - .update_editor(|editor, cx| editor.find_all_references(&FindAllReferences, cx)) - .unwrap(); - - cx.executor().run_until_parked(); - - cx.executor().start_waiting(); - references.await.unwrap(); - - cx.assert_editor_state(indoc! {" - fn foo(«paramˇ»: i64) { - println!(param); - } - "}); - - cx.set_state(indoc! {" - fn foo(param: i64) { - let a = param; - let aˇ = param; - let a = param; - println!(param); - } - "}); - - cx.lsp - .handle_request::(move |_, _| async move { - Ok(Some(vec![lsp::Location { - uri: lsp::Url::from_file_path("/root/dir/file.rs").unwrap(), - range: lsp::Range::new(lsp::Position::new(2, 8), lsp::Position::new(2, 9)), - }])) - }); - - let references = cx - .update_editor(|editor, cx| editor.find_all_references(&FindAllReferences, cx)) - .unwrap(); - - cx.executor().run_until_parked(); - - cx.executor().start_waiting(); - references.await.unwrap(); - - cx.assert_editor_state(indoc! {" - fn foo(param: i64) { - let a = param; - let «aˇ» = param; - let a = param; - println!(param); - } - "}); -} - #[gpui::test] async fn test_addition_reverts(cx: &mut gpui::TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/editor/src/hover_links.rs b/crates/editor/src/hover_links.rs index 7475b8d09f1f65ffbaa384454883c9c2cf93ecbd..83b767e8be315ce3cc2903ff84c696703b9d6011 100644 --- a/crates/editor/src/hover_links.rs +++ b/crates/editor/src/hover_links.rs @@ -702,10 +702,7 @@ mod tests { use gpui::Modifiers; use indoc::indoc; use language::language_settings::InlayHintSettings; - use lsp::{ - request::{GotoDefinition, GotoTypeDefinition}, - References, - }; + use lsp::request::{GotoDefinition, GotoTypeDefinition}; use util::assert_set_eq; use workspace::item::Item; @@ -1270,147 +1267,4 @@ mod tests { cx.simulate_click(screen_coord, Modifiers::secondary_key()); 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::(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::(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::secondary_key()); - 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::secondary_key()); - cx.background_executor.run_until_parked(); - cx.assert_editor_state(indoc! {" - fn «testˇ»() { - do_work(); - } - - fn do_work() { - test(); - } - "}); - } - } } diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index 5754c17b645d8fe6d503d8d2799779ef36c9070b..a8755ef2b97ba683bd0c17ff46c2f3389d1e658b 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -367,6 +367,7 @@ impl Drop for Deferred { } /// Run the given function when the returned value is dropped (unless it's cancelled). +#[must_use] pub fn defer(f: F) -> Deferred { Deferred(Some(f)) }