From 5841ac406d10b8c64296b902edcee32fa1a59c7c Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 10 Oct 2024 12:53:02 +0300 Subject: [PATCH] Fix the completions being too slow (#19013) Closes https://github.com/zed-industries/zed/issues/19005 Release Notes: - Fixed completion items inserted with a delay ([#19005](https://github.com/zed-industries/zed/issues/19005)) --------- Co-authored-by: Antonio Scandurra --- 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/lsp/src/lsp.rs | 3 +- crates/vim/src/normal/repeat.rs | 6 +- 9 files changed, 65 insertions(+), 145 deletions(-) diff --git a/crates/collab/src/tests/editor_tests.rs b/crates/collab/src/tests/editor_tests.rs index e35cf87380..f9bc21efb1 100644 --- a/crates/collab/src/tests/editor_tests.rs +++ b/crates/collab/src/tests/editor_tests.rs @@ -379,75 +379,51 @@ 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 { - 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), + 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() }) }, ); - 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. + cx_a.executor().run_until_parked(); + buffer_a.read_with(cx_a, |buffer, _| { assert_eq!( buffer.text(), @@ -540,15 +516,9 @@ 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 f03d6137fa..3a3361cda1 100644 --- a/crates/copilot/src/copilot_completion_provider.rs +++ b/crates/copilot/src/copilot_completion_provider.rs @@ -363,12 +363,10 @@ 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() - }) - .await - .unwrap(); - - cx.update_editor(|editor, cx| { + editor + .confirm_completion(&Default::default(), cx) + .unwrap() + .detach(); 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 1f009c7d79..0dbf36d49e 100644 --- a/crates/editor/src/debounced_delay.rs +++ b/crates/editor/src/debounced_delay.rs @@ -1,4 +1,4 @@ -use std::{ops::ControlFlow, time::Duration}; +use std::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,22 +23,17 @@ impl DebouncedDelay { F: 'static + Send + FnOnce(&mut Editor, &mut ViewContext) -> Task<()>, { if let Some(channel) = self.cancel_channel.take() { - channel.send(ControlFlow::Break(())).ok(); + _ = channel.send(()); } - 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! { - interrupt = receiver => { - match interrupt { - Ok(ControlFlow::Break(())) | Err(_) => return, - Ok(ControlFlow::Continue(())) => {}, - } - } + _ = receiver => return, _ = timer => {} } @@ -47,11 +42,4 @@ 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 5b5401af17..e2818bcd96 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -4459,49 +4459,16 @@ impl Editor { &mut self, item_ix: Option, intent: CompletionIntent, - cx: &mut ViewContext, - ) -> Option>> { + cx: &mut ViewContext, + ) -> Option>> { + use language::ToOffset as _; + 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))?; @@ -4660,7 +4627,11 @@ impl Editor { // so we should automatically call signature_help self.show_signature_help(&ShowSignatureHelp, cx); } - Some(apply_edits) + + Some(cx.foreground_executor().spawn(async move { + apply_edits.await?; + Ok(()) + })) } 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 5ab2c9e3a8..092afb394e 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.ˇ + one.second_completionˇ two three "}); @@ -8029,9 +8029,9 @@ async fn test_completion(cx: &mut gpui::TestAppContext) { cx.assert_editor_state(indoc! {" one.second_completionˇ two - thoverlapping additional editree - - additional edit"}); + three + 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 siˇ - three siˇ + two sixth_completionˇ + three sixth_completionˇ additional edit "}); @@ -8133,11 +8133,9 @@ async fn test_completion(cx: &mut gpui::TestAppContext) { .confirm_completion(&ConfirmCompletion::default(), cx) .unwrap() }); - cx.assert_editor_state("editor.cloˇ"); + cx.assert_editor_state("editor.closeˇ"); handle_resolve_completion_request(&mut cx, None).await; apply_additional_edits.await.unwrap(); - cx.assert_editor_state(indoc! {" - editor.closeˇ"}); } #[gpui::test] @@ -10142,7 +10140,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.ˇ; }"}); + cx.assert_editor_state(indoc! {"fn main() { let a = 2.Some(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 da23243188..269e9e9e03 100644 --- a/crates/editor/src/hover_popover.rs +++ b/crates/editor/src/hover_popover.rs @@ -810,7 +810,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(false), + resolve_provider: Some(true), ..Default::default() }), ..Default::default() @@ -902,15 +902,12 @@ mod tests { assert_eq!(counter.load(atomic::Ordering::Acquire), 1); //apply a completion and check it was successfully applied - let () = cx - .update_editor(|editor, cx| { - editor.context_menu_next(&Default::default(), cx); - editor - .confirm_completion(&ConfirmCompletion::default(), cx) - .unwrap() - }) - .await - .unwrap(); + let _apply_additional_edits = cx.update_editor(|editor, cx| { + editor.context_menu_next(&Default::default(), cx); + editor + .confirm_completion(&ConfirmCompletion::default(), cx) + .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 9406a48796..7234d97c5b 100644 --- a/crates/editor/src/test/editor_test_context.rs +++ b/crates/editor/src/test/editor_test_context.rs @@ -13,7 +13,6 @@ 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/lsp/src/lsp.rs b/crates/lsp/src/lsp.rs index e380da052d..8d67855e39 100644 --- a/crates/lsp/src/lsp.rs +++ b/crates/lsp/src/lsp.rs @@ -631,7 +631,8 @@ impl LanguageServer { "filterText".to_string(), "labelDetails".to_string(), "tags".to_string(), - "textEdit".to_string(), + // NB: Do not have this resolved, otherwise Zed becomes slow to complete things + // "textEdit".to_string(), ], }), insert_replace_support: Some(true), diff --git a/crates/vim/src/normal/repeat.rs b/crates/vim/src/normal/repeat.rs index 7f06c424f3..e0e34ceda7 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(false), + resolve_provider: Some(true), ..Default::default() }), ..Default::default() @@ -432,9 +432,7 @@ mod test { request.next().await; cx.condition(|editor, _| editor.context_menu_visible()) .await; - cx.simulate_keystrokes("down enter"); - cx.run_until_parked(); - cx.simulate_keystrokes("! escape"); + cx.simulate_keystrokes("down enter ! escape"); cx.assert_state( indoc! {"