From a62a2fa8f7372799a3f66955e39c7da8ed97cf76 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 9 Oct 2024 17:18:20 +0300 Subject: [PATCH] Always wait for completion resolve before applying the completion edits (#18907) After https://github.com/rust-lang/rust-analyzer/pull/18167 and certain people who type and complete rapidly, it turned out that we have not waited for `completionItem/resolve` to finish before applying the completion results. Release Notes: - Fixed completion items applied improperly on fast typing --- crates/collab/src/tests/editor_tests.rs | 86 +++++++++++++------ .../src/copilot_completion_provider.rs | 10 ++- crates/editor/src/debounced_delay.rs | 22 +++-- crates/editor/src/editor.rs | 47 ++++++++-- crates/editor/src/editor_tests.rs | 18 ++-- crates/editor/src/hover_popover.rs | 17 ++-- crates/editor/src/test/editor_test_context.rs | 1 + crates/vim/src/normal/repeat.rs | 6 +- 8 files changed, 144 insertions(+), 63 deletions(-) diff --git a/crates/collab/src/tests/editor_tests.rs b/crates/collab/src/tests/editor_tests.rs index f9bc21efb1..e35cf87380 100644 --- a/crates/collab/src/tests/editor_tests.rs +++ b/crates/collab/src/tests/editor_tests.rs @@ -379,51 +379,75 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu .next() .await .unwrap(); - cx_a.executor().finish_waiting(); - // Open the buffer on the host. let buffer_a = project_a .update(cx_a, |p, cx| p.open_buffer((worktree_id, "main.rs"), cx)) .await .unwrap(); - cx_a.executor().run_until_parked(); buffer_a.read_with(cx_a, |buffer, _| { assert_eq!(buffer.text(), "fn main() { a. }") }); - // Confirm a completion on the guest. - editor_b.update(cx_b, |editor, cx| { - assert!(editor.context_menu_visible()); - editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx); - assert_eq!(editor.text(cx), "fn main() { a.first_method() }"); - }); - // Return a resolved completion from the host's language server. // The resolved completion has an additional text edit. fake_language_server.handle_request::( |params, _| async move { - assert_eq!(params.label, "first_method(…)"); - Ok(lsp::CompletionItem { - label: "first_method(…)".into(), - detail: Some("fn(&mut self, B) -> C".into()), - text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { - new_text: "first_method($1)".to_string(), - range: lsp::Range::new(lsp::Position::new(0, 14), lsp::Position::new(0, 14)), - })), - additional_text_edits: Some(vec![lsp::TextEdit { - new_text: "use d::SomeTrait;\n".to_string(), - range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)), - }]), - insert_text_format: Some(lsp::InsertTextFormat::SNIPPET), - ..Default::default() + Ok(match params.label.as_str() { + "first_method(…)" => lsp::CompletionItem { + label: "first_method(…)".into(), + detail: Some("fn(&mut self, B) -> C".into()), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + new_text: "first_method($1)".to_string(), + range: lsp::Range::new( + lsp::Position::new(0, 14), + lsp::Position::new(0, 14), + ), + })), + additional_text_edits: Some(vec![lsp::TextEdit { + new_text: "use d::SomeTrait;\n".to_string(), + range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)), + }]), + insert_text_format: Some(lsp::InsertTextFormat::SNIPPET), + ..Default::default() + }, + "second_method(…)" => lsp::CompletionItem { + label: "second_method(…)".into(), + detail: Some("fn(&mut self, C) -> D".into()), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + new_text: "second_method()".to_string(), + range: lsp::Range::new( + lsp::Position::new(0, 14), + lsp::Position::new(0, 14), + ), + })), + insert_text_format: Some(lsp::InsertTextFormat::SNIPPET), + additional_text_edits: Some(vec![lsp::TextEdit { + new_text: "use d::SomeTrait;\n".to_string(), + range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)), + }]), + ..Default::default() + }, + _ => panic!("unexpected completion label: {:?}", params.label), }) }, ); - - // The additional edit is applied. + cx_a.executor().finish_waiting(); cx_a.executor().run_until_parked(); + // Confirm a completion on the guest. + editor_b + .update(cx_b, |editor, cx| { + assert!(editor.context_menu_visible()); + editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx) + }) + .unwrap() + .await + .unwrap(); + cx_a.executor().run_until_parked(); + cx_b.executor().run_until_parked(); + + // The additional edit is applied. buffer_a.read_with(cx_a, |buffer, _| { assert_eq!( buffer.text(), @@ -516,9 +540,15 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu cx_b.executor().run_until_parked(); // When accepting the completion, the snippet is insert. + editor_b + .update(cx_b, |editor, cx| { + assert!(editor.context_menu_visible()); + editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx) + }) + .unwrap() + .await + .unwrap(); editor_b.update(cx_b, |editor, cx| { - assert!(editor.context_menu_visible()); - editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx); assert_eq!( editor.text(cx), "use d::SomeTrait;\nfn main() { a.first_method(); a.third_method(, , ) }" diff --git a/crates/copilot/src/copilot_completion_provider.rs b/crates/copilot/src/copilot_completion_provider.rs index 3a3361cda1..f03d6137fa 100644 --- a/crates/copilot/src/copilot_completion_provider.rs +++ b/crates/copilot/src/copilot_completion_provider.rs @@ -363,10 +363,12 @@ mod tests { // Confirming a completion inserts it and hides the context menu, without showing // the copilot suggestion afterwards. - editor - .confirm_completion(&Default::default(), cx) - .unwrap() - .detach(); + editor.confirm_completion(&Default::default(), cx).unwrap() + }) + .await + .unwrap(); + + cx.update_editor(|editor, cx| { assert!(!editor.context_menu_visible()); assert!(!editor.has_active_inline_completion(cx)); assert_eq!(editor.text(cx), "one.completion_a\ntwo\nthree\n"); diff --git a/crates/editor/src/debounced_delay.rs b/crates/editor/src/debounced_delay.rs index 0dbf36d49e..1f009c7d79 100644 --- a/crates/editor/src/debounced_delay.rs +++ b/crates/editor/src/debounced_delay.rs @@ -1,4 +1,4 @@ -use std::time::Duration; +use std::{ops::ControlFlow, time::Duration}; use futures::{channel::oneshot, FutureExt}; use gpui::{Task, ViewContext}; @@ -7,7 +7,7 @@ use crate::Editor; pub struct DebouncedDelay { task: Option>, - cancel_channel: Option>, + cancel_channel: Option>>, } impl DebouncedDelay { @@ -23,17 +23,22 @@ impl DebouncedDelay { F: 'static + Send + FnOnce(&mut Editor, &mut ViewContext) -> Task<()>, { if let Some(channel) = self.cancel_channel.take() { - _ = channel.send(()); + channel.send(ControlFlow::Break(())).ok(); } - let (sender, mut receiver) = oneshot::channel::<()>(); + let (sender, mut receiver) = oneshot::channel::>(); self.cancel_channel = Some(sender); drop(self.task.take()); self.task = Some(cx.spawn(move |model, mut cx| async move { let mut timer = cx.background_executor().timer(delay).fuse(); futures::select_biased! { - _ = receiver => return, + interrupt = receiver => { + match interrupt { + Ok(ControlFlow::Break(())) | Err(_) => return, + Ok(ControlFlow::Continue(())) => {}, + } + } _ = timer => {} } @@ -42,4 +47,11 @@ impl DebouncedDelay { } })); } + + pub fn start_now(&mut self) -> Option> { + if let Some(channel) = self.cancel_channel.take() { + channel.send(ControlFlow::Continue(())).ok(); + } + self.task.take() + } } diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 2f7c8ce5d7..412c7d3051 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -4427,16 +4427,49 @@ impl Editor { &mut self, item_ix: Option, intent: CompletionIntent, - cx: &mut ViewContext, - ) -> Option>> { - use language::ToOffset as _; - + cx: &mut ViewContext, + ) -> Option>> { let completions_menu = if let ContextMenu::Completions(menu) = self.hide_context_menu(cx)? { menu } else { return None; }; + let mut resolve_task_store = completions_menu + .selected_completion_documentation_resolve_debounce + .lock(); + let selected_completion_resolve = resolve_task_store.start_now(); + let menu_pre_resolve = self + .completion_documentation_pre_resolve_debounce + .start_now(); + drop(resolve_task_store); + + Some(cx.spawn(|editor, mut cx| async move { + match (selected_completion_resolve, menu_pre_resolve) { + (None, None) => {} + (Some(resolve), None) | (None, Some(resolve)) => resolve.await, + (Some(resolve_1), Some(resolve_2)) => { + futures::join!(resolve_1, resolve_2); + } + } + if let Some(apply_edits_task) = editor.update(&mut cx, |editor, cx| { + editor.apply_resolved_completion(completions_menu, item_ix, intent, cx) + })? { + apply_edits_task.await?; + } + Ok(()) + })) + } + + fn apply_resolved_completion( + &mut self, + completions_menu: CompletionsMenu, + item_ix: Option, + intent: CompletionIntent, + cx: &mut ViewContext<'_, Editor>, + ) -> Option>>> { + use language::ToOffset as _; + let mat = completions_menu .matches .get(item_ix.unwrap_or(completions_menu.selected_item))?; @@ -4595,11 +4628,7 @@ impl Editor { // so we should automatically call signature_help self.show_signature_help(&ShowSignatureHelp, cx); } - - Some(cx.foreground_executor().spawn(async move { - apply_edits.await?; - Ok(()) - })) + Some(apply_edits) } pub fn toggle_code_actions(&mut self, action: &ToggleCodeActions, cx: &mut ViewContext) { diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 092afb394e..5ab2c9e3a8 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -7996,7 +7996,7 @@ async fn test_completion(cx: &mut gpui::TestAppContext) { .unwrap() }); cx.assert_editor_state(indoc! {" - one.second_completionˇ + one.ˇ two three "}); @@ -8029,9 +8029,9 @@ async fn test_completion(cx: &mut gpui::TestAppContext) { cx.assert_editor_state(indoc! {" one.second_completionˇ two - three - additional edit - "}); + thoverlapping additional editree + + additional edit"}); cx.set_state(indoc! {" one.second_completion @@ -8091,8 +8091,8 @@ async fn test_completion(cx: &mut gpui::TestAppContext) { }); cx.assert_editor_state(indoc! {" one.second_completion - two sixth_completionˇ - three sixth_completionˇ + two siˇ + three siˇ additional edit "}); @@ -8133,9 +8133,11 @@ async fn test_completion(cx: &mut gpui::TestAppContext) { .confirm_completion(&ConfirmCompletion::default(), cx) .unwrap() }); - cx.assert_editor_state("editor.closeˇ"); + cx.assert_editor_state("editor.cloˇ"); handle_resolve_completion_request(&mut cx, None).await; apply_additional_edits.await.unwrap(); + cx.assert_editor_state(indoc! {" + editor.closeˇ"}); } #[gpui::test] @@ -10140,7 +10142,7 @@ async fn test_completions_with_additional_edits(cx: &mut gpui::TestAppContext) { .confirm_completion(&ConfirmCompletion::default(), cx) .unwrap() }); - cx.assert_editor_state(indoc! {"fn main() { let a = 2.Some(2)ˇ; }"}); + cx.assert_editor_state(indoc! {"fn main() { let a = 2.ˇ; }"}); cx.handle_request::(move |_, _, _| { let task_completion_item = completion_item.clone(); diff --git a/crates/editor/src/hover_popover.rs b/crates/editor/src/hover_popover.rs index f6eb837ae8..e20b6a3dc8 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -821,7 +821,7 @@ mod tests { hover_provider: Some(lsp::HoverProviderCapability::Simple(true)), completion_provider: Some(lsp::CompletionOptions { trigger_characters: Some(vec![".".to_string(), ":".to_string()]), - resolve_provider: Some(true), + resolve_provider: Some(false), ..Default::default() }), ..Default::default() @@ -913,12 +913,15 @@ mod tests { assert_eq!(counter.load(atomic::Ordering::Acquire), 1); //apply a completion and check it was successfully applied - let _apply_additional_edits = cx.update_editor(|editor, cx| { - editor.context_menu_next(&Default::default(), cx); - editor - .confirm_completion(&ConfirmCompletion::default(), cx) - .unwrap() - }); + let () = cx + .update_editor(|editor, cx| { + editor.context_menu_next(&Default::default(), cx); + editor + .confirm_completion(&ConfirmCompletion::default(), cx) + .unwrap() + }) + .await + .unwrap(); cx.assert_editor_state(indoc! {" one.second_completionˇ two diff --git a/crates/editor/src/test/editor_test_context.rs b/crates/editor/src/test/editor_test_context.rs index 7234d97c5b..9406a48796 100644 --- a/crates/editor/src/test/editor_test_context.rs +++ b/crates/editor/src/test/editor_test_context.rs @@ -13,6 +13,7 @@ use itertools::Itertools; use language::{Buffer, BufferSnapshot, LanguageRegistry}; use multi_buffer::{ExcerptRange, ToPoint}; use parking_lot::RwLock; +use pretty_assertions::assert_eq; use project::{FakeFs, Project}; use std::{ any::TypeId, diff --git a/crates/vim/src/normal/repeat.rs b/crates/vim/src/normal/repeat.rs index e0e34ceda7..7f06c424f3 100644 --- a/crates/vim/src/normal/repeat.rs +++ b/crates/vim/src/normal/repeat.rs @@ -387,7 +387,7 @@ mod test { lsp::ServerCapabilities { completion_provider: Some(lsp::CompletionOptions { trigger_characters: Some(vec![".".to_string(), ":".to_string()]), - resolve_provider: Some(true), + resolve_provider: Some(false), ..Default::default() }), ..Default::default() @@ -432,7 +432,9 @@ mod test { request.next().await; cx.condition(|editor, _| editor.context_menu_visible()) .await; - cx.simulate_keystrokes("down enter ! escape"); + cx.simulate_keystrokes("down enter"); + cx.run_until_parked(); + cx.simulate_keystrokes("! escape"); cx.assert_state( indoc! {"