From c01403b4b1f1702c30908388c9e21240957e41aa Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Fri, 20 Dec 2024 15:16:07 -0700 Subject: [PATCH] Reapply completion docs prefetch (#22306) Leaving release notes as N/A because it had release notes in the past in #21705 In #21286, documentation resolution was made more efficient by only resolving the current completion. However, this meant that single line documentation shown inline in the menu was missing until scrolled to. This also meant that it would wait for navigation to resolve completion docs, leading to lag for displaying documentation. This change resolves this by attempting to fetch all the completions that will be shown. It also mostly avoids re-resolving completions. It intentionally re-resolves the current selection on navigation, as some language servers will respond with more information later on. Release Notes: - N/A --- Cargo.lock | 1 + crates/editor/src/code_context_menus.rs | 164 ++++++++++----- crates/editor/src/editor.rs | 2 +- crates/editor/src/editor_tests.rs | 261 +++++++++++------------- crates/util/Cargo.toml | 1 + crates/util/src/util.rs | 74 +++++++ 6 files changed, 308 insertions(+), 195 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7b5007a703..03fa792d90 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13977,6 +13977,7 @@ dependencies = [ "futures-lite 1.13.0", "git2", "globset", + "itertools 0.13.0", "log", "rand 0.8.5", "regex", diff --git a/crates/editor/src/code_context_menus.rs b/crates/editor/src/code_context_menus.rs index 26b2fdf8ba..2432e9fc97 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -1,6 +1,3 @@ -use std::cell::RefCell; -use std::{cmp::Reverse, ops::Range, rc::Rc}; - use fuzzy::{StringMatch, StringMatchCandidate}; use gpui::{ div, px, uniform_list, AnyElement, BackgroundExecutor, Div, FontWeight, ListSizingBehavior, @@ -13,6 +10,13 @@ use lsp::LanguageServerId; use multi_buffer::{Anchor, ExcerptId}; use ordered_float::OrderedFloat; use project::{CodeAction, Completion, TaskSourceKind}; +use std::{ + cell::RefCell, + cmp::{min, Reverse}, + iter, + ops::Range, + rc::Rc, +}; use task::ResolvedTask; use ui::{prelude::*, Color, IntoElement, ListItem, Pixels, Popover, Styled}; use util::ResultExt; @@ -158,6 +162,7 @@ pub struct CompletionsMenu { scroll_handle: UniformListScrollHandle, resolve_completions: bool, show_completion_documentation: bool, + last_rendered_range: Rc>>>, } #[derive(Clone, Debug)] @@ -193,6 +198,7 @@ impl CompletionsMenu { selected_item: 0, scroll_handle: UniformListScrollHandle::new(), resolve_completions: true, + last_rendered_range: RefCell::new(None).into(), } } @@ -249,6 +255,7 @@ impl CompletionsMenu { scroll_handle: UniformListScrollHandle::new(), resolve_completions: false, show_completion_documentation: false, + last_rendered_range: RefCell::new(None).into(), } } @@ -257,11 +264,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.selected_item = 0; - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(0, provider, cx); } fn select_prev( @@ -269,15 +272,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item > 0 { - self.selected_item -= 1; - } else { - self.selected_item = self.entries.len() - 1; - } - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(self.prev_match_index(), provider, cx); } fn select_next( @@ -285,15 +280,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item + 1 < self.entries.len() { - self.selected_item += 1; - } else { - self.selected_item = 0; - } - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(self.next_match_index(), provider, cx); } fn select_last( @@ -301,11 +288,38 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.selected_item = self.entries.len() - 1; - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - self.resolve_selected_completion(provider, cx); - cx.notify(); + self.update_selection_index(self.entries.len() - 1, provider, cx); + } + + fn update_selection_index( + &mut self, + match_index: usize, + provider: Option<&dyn CompletionProvider>, + cx: &mut ViewContext, + ) { + if self.selected_item != match_index { + self.selected_item = match_index; + self.scroll_handle + .scroll_to_item(self.selected_item, ScrollStrategy::Top); + self.resolve_visible_completions(provider, cx); + cx.notify(); + } + } + + fn prev_match_index(&self) -> usize { + if self.selected_item > 0 { + self.selected_item - 1 + } else { + self.entries.len() - 1 + } + } + + fn next_match_index(&self) -> usize { + if self.selected_item + 1 < self.entries.len() { + self.selected_item + 1 + } else { + 0 + } } pub fn show_inline_completion_hint(&mut self, hint: InlineCompletionMenuHint) { @@ -330,7 +344,7 @@ impl CompletionsMenu { } } - pub fn resolve_selected_completion( + pub fn resolve_visible_completions( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, @@ -342,24 +356,76 @@ impl CompletionsMenu { return; }; - match &self.entries[self.selected_item] { - CompletionEntry::Match(entry) => { - let completion_index = entry.candidate_id; - let resolve_task = provider.resolve_completions( - self.buffer.clone(), - vec![completion_index], - self.completions.clone(), - cx, - ); + // Attempt to resolve completions for every item that will be displayed. This matters + // because single line documentation may be displayed inline with the completion. + // + // When navigating to the very beginning or end of completions, `last_rendered_range` may + // have no overlap with the completions that will be displayed, so instead use a range based + // on the last rendered count. + const APPROXIMATE_VISIBLE_COUNT: usize = 12; + let last_rendered_range = self.last_rendered_range.borrow().clone(); + let visible_count = last_rendered_range + .clone() + .map_or(APPROXIMATE_VISIBLE_COUNT, |range| range.count()); + let entry_range = if self.selected_item == 0 { + 0..min(visible_count, self.entries.len()) + } else if self.selected_item == self.entries.len() - 1 { + self.entries.len().saturating_sub(visible_count)..self.entries.len() + } else { + last_rendered_range.map_or(0..0, |range| { + min(range.start, self.entries.len())..min(range.end, self.entries.len()) + }) + }; - cx.spawn(move |editor, mut cx| async move { - if let Some(true) = resolve_task.await.log_err() { - editor.update(&mut cx, |_, cx| cx.notify()).ok(); - } - }) - .detach(); + // Expand the range to resolve more completions than are predicted to be visible, to reduce + // jank on navigation. + const EXTRA_TO_RESOLVE: usize = 4; + let entry_indices = util::iterate_expanded_and_wrapped_usize_range( + entry_range.clone(), + EXTRA_TO_RESOLVE, + EXTRA_TO_RESOLVE, + self.entries.len(), + ); + + // Avoid work by sometimes filtering out completions that already have documentation. + // This filtering doesn't happen if the completions are currently being updated. + let completions = self.completions.borrow(); + let candidate_ids = entry_indices + .flat_map(|i| Self::entry_candidate_id(&self.entries[i])) + .filter(|i| completions[*i].documentation.is_none()); + + // Current selection is always resolved even if it already has documentation, to handle + // out-of-spec language servers that return more results later. + let candidate_ids = match Self::entry_candidate_id(&self.entries[self.selected_item]) { + None => candidate_ids.collect::>(), + Some(selected_candidate_id) => iter::once(selected_candidate_id) + .chain(candidate_ids.filter(|id| *id != selected_candidate_id)) + .collect::>(), + }; + + if candidate_ids.is_empty() { + return; + } + + let resolve_task = provider.resolve_completions( + self.buffer.clone(), + candidate_ids, + self.completions.clone(), + cx, + ); + + cx.spawn(move |editor, mut cx| async move { + if let Some(true) = resolve_task.await.log_err() { + editor.update(&mut cx, |_, cx| cx.notify()).ok(); } - CompletionEntry::InlineCompletionHint { .. } => {} + }) + .detach(); + } + + fn entry_candidate_id(entry: &CompletionEntry) -> Option { + match entry { + CompletionEntry::Match(entry) => Some(entry.candidate_id), + CompletionEntry::InlineCompletionHint { .. } => None, } } @@ -408,12 +474,14 @@ impl CompletionsMenu { let selected_item = self.selected_item; let completions = self.completions.clone(); let matches = self.entries.clone(); + let last_rendered_range = self.last_rendered_range.clone(); let style = style.clone(); let list = uniform_list( cx.view().clone(), "completions", matches.len(), move |_editor, range, cx| { + last_rendered_range.borrow_mut().replace(range.clone()); let start_ix = range.start; let completions_guard = completions.borrow_mut(); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 5876f003f9..36959e055c 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3742,7 +3742,7 @@ impl Editor { if editor.focus_handle.is_focused(cx) && menu.is_some() { let mut menu = menu.unwrap(); - menu.resolve_selected_completion(editor.completion_provider.as_deref(), cx); + menu.resolve_visible_completions(editor.completion_provider.as_deref(), cx); if editor.show_inline_completions_in_menu(cx) { if let Some(hint) = editor.inline_completion_menu_hint(cx) { diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 4882d8c217..cc7d612bbf 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -25,14 +25,18 @@ use language::{ use language_settings::{Formatter, FormatterList, IndentGuideSettings}; use multi_buffer::MultiBufferIndentGuide; use parking_lot::Mutex; +use pretty_assertions::{assert_eq, assert_ne}; use project::{buffer_store::BufferChangeSet, FakeFs}; use project::{ lsp_command::SIGNATURE_HELP_HIGHLIGHT_CURRENT, project_settings::{LspSettings, ProjectSettings}, }; use serde_json::{self, json}; -use std::sync::atomic::{self, AtomicBool, AtomicUsize}; use std::{cell::RefCell, future::Future, rc::Rc, time::Instant}; +use std::{ + iter, + sync::atomic::{self, AtomicUsize}, +}; use test::{build_editor_with_project, editor_lsp_test_context::rust_lang}; use unindent::Unindent; use util::{ @@ -10787,6 +10791,62 @@ async fn test_completions_resolve_updates_labels_if_filter_text_matches( async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppContext) { init_test(cx, |_| {}); + let item_0 = lsp::CompletionItem { + label: "abs".into(), + insert_text: Some("abs".into()), + data: Some(json!({ "very": "special"})), + insert_text_mode: Some(lsp::InsertTextMode::ADJUST_INDENTATION), + text_edit: Some(lsp::CompletionTextEdit::InsertAndReplace( + lsp::InsertReplaceEdit { + new_text: "abs".to_string(), + insert: lsp::Range::default(), + replace: lsp::Range::default(), + }, + )), + ..lsp::CompletionItem::default() + }; + let items = iter::once(item_0.clone()) + .chain((11..51).map(|i| lsp::CompletionItem { + label: format!("item_{}", i), + insert_text: Some(format!("item_{}", i)), + insert_text_format: Some(lsp::InsertTextFormat::PLAIN_TEXT), + ..lsp::CompletionItem::default() + })) + .collect::>(); + + let default_commit_characters = vec!["?".to_string()]; + let default_data = json!({ "default": "data"}); + let default_insert_text_format = lsp::InsertTextFormat::SNIPPET; + let default_insert_text_mode = lsp::InsertTextMode::AS_IS; + let default_edit_range = lsp::Range { + start: lsp::Position { + line: 0, + character: 5, + }, + end: lsp::Position { + line: 0, + character: 5, + }, + }; + + let item_0_out = lsp::CompletionItem { + commit_characters: Some(default_commit_characters.clone()), + insert_text_format: Some(default_insert_text_format), + ..item_0 + }; + let items_out = iter::once(item_0_out) + .chain(items[1..].iter().map(|item| lsp::CompletionItem { + commit_characters: Some(default_commit_characters.clone()), + data: Some(default_data.clone()), + insert_text_mode: Some(default_insert_text_mode), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + range: default_edit_range, + new_text: item.label.clone(), + })), + ..item.clone() + })) + .collect::>(); + let mut cx = EditorLspTestContext::new_rust( lsp::ServerCapabilities { completion_provider: Some(lsp::CompletionOptions { @@ -10803,138 +10863,15 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"}); cx.simulate_keystroke("."); - let default_commit_characters = vec!["?".to_string()]; - let default_data = json!({ "very": "special"}); - let default_insert_text_format = lsp::InsertTextFormat::SNIPPET; - let default_insert_text_mode = lsp::InsertTextMode::AS_IS; - let default_edit_range = lsp::Range { - start: lsp::Position { - line: 0, - character: 5, - }, - end: lsp::Position { - line: 0, - character: 5, - }, - }; - - let resolve_requests_number = Arc::new(AtomicUsize::new(0)); - let expect_first_item = Arc::new(AtomicBool::new(true)); - cx.lsp - .server - .on_request::({ - let closure_default_data = default_data.clone(); - let closure_resolve_requests_number = resolve_requests_number.clone(); - let closure_expect_first_item = expect_first_item.clone(); - let closure_default_commit_characters = default_commit_characters.clone(); - move |item_to_resolve, _| { - closure_resolve_requests_number.fetch_add(1, atomic::Ordering::Release); - let default_data = closure_default_data.clone(); - let default_commit_characters = closure_default_commit_characters.clone(); - let expect_first_item = closure_expect_first_item.clone(); - async move { - if expect_first_item.load(atomic::Ordering::Acquire) { - assert_eq!( - item_to_resolve.label, "Some(2)", - "Should have selected the first item" - ); - assert_eq!( - item_to_resolve.data, - Some(json!({ "very": "special"})), - "First item should bring its own data for resolving" - ); - assert_eq!( - item_to_resolve.commit_characters, - Some(default_commit_characters), - "First item had no own commit characters and should inherit the default ones" - ); - assert!( - matches!( - item_to_resolve.text_edit, - Some(lsp::CompletionTextEdit::InsertAndReplace { .. }) - ), - "First item should bring its own edit range for resolving" - ); - assert_eq!( - item_to_resolve.insert_text_format, - Some(default_insert_text_format), - "First item had no own insert text format and should inherit the default one" - ); - assert_eq!( - item_to_resolve.insert_text_mode, - Some(lsp::InsertTextMode::ADJUST_INDENTATION), - "First item should bring its own insert text mode for resolving" - ); - Ok(item_to_resolve) - } else { - assert_eq!( - item_to_resolve.label, "vec![2]", - "Should have selected the last item" - ); - assert_eq!( - item_to_resolve.data, - Some(default_data), - "Last item has no own resolve data and should inherit the default one" - ); - assert_eq!( - item_to_resolve.commit_characters, - Some(default_commit_characters), - "Last item had no own commit characters and should inherit the default ones" - ); - assert_eq!( - item_to_resolve.text_edit, - Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { - range: default_edit_range, - new_text: "vec![2]".to_string() - })), - "Last item had no own edit range and should inherit the default one" - ); - assert_eq!( - item_to_resolve.insert_text_format, - Some(lsp::InsertTextFormat::PLAIN_TEXT), - "Last item should bring its own insert text format for resolving" - ); - assert_eq!( - item_to_resolve.insert_text_mode, - Some(default_insert_text_mode), - "Last item had no own insert text mode and should inherit the default one" - ); - - Ok(item_to_resolve) - } - } - } - }).detach(); - let completion_data = default_data.clone(); let completion_characters = default_commit_characters.clone(); cx.handle_request::(move |_, _, _| { let default_data = completion_data.clone(); let default_commit_characters = completion_characters.clone(); + let items = items.clone(); async move { Ok(Some(lsp::CompletionResponse::List(lsp::CompletionList { - items: vec![ - lsp::CompletionItem { - label: "Some(2)".into(), - insert_text: Some("Some(2)".into()), - data: Some(json!({ "very": "special"})), - insert_text_mode: Some(lsp::InsertTextMode::ADJUST_INDENTATION), - text_edit: Some(lsp::CompletionTextEdit::InsertAndReplace( - lsp::InsertReplaceEdit { - new_text: "Some(2)".to_string(), - insert: lsp::Range::default(), - replace: lsp::Range::default(), - }, - )), - ..lsp::CompletionItem::default() - }, - lsp::CompletionItem { - label: "vec![2]".into(), - insert_text: Some("vec![2]".into()), - insert_text_format: Some(lsp::InsertTextFormat::PLAIN_TEXT), - ..lsp::CompletionItem::default() - }, - ], + items, item_defaults: Some(lsp::CompletionListItemDefaults { data: Some(default_data.clone()), commit_characters: Some(default_commit_characters.clone()), @@ -10951,6 +10888,21 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo .next() .await; + let resolved_items = Arc::new(Mutex::new(Vec::new())); + cx.lsp + .server + .on_request::({ + let closure_resolved_items = resolved_items.clone(); + move |item_to_resolve, _| { + let closure_resolved_items = closure_resolved_items.clone(); + async move { + closure_resolved_items.lock().push(item_to_resolve.clone()); + Ok(item_to_resolve) + } + } + }) + .detach(); + cx.condition(|editor, _| editor.context_menu_visible()) .await; cx.run_until_parked(); @@ -10959,39 +10911,56 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo match menu.as_ref().expect("should have the completions menu") { CodeContextMenu::Completions(completions_menu) => { assert_eq!( - completion_menu_entries(&completions_menu.entries), - vec!["Some(2)", "vec![2]"] + completions_menu + .entries + .iter() + .flat_map(|c| match c { + CompletionEntry::Match(mat) => Some(mat.string.clone()), + _ => None, + }) + .collect::>(), + items_out + .iter() + .map(|completion| completion.label.clone()) + .collect::>() ); } CodeContextMenu::CodeActions(_) => panic!("Expected to have the completions menu"), } }); + // Approximate initial displayed interval is 0..12. With extra item padding of 4 this is 0..16 + // with 4 from the end. assert_eq!( - resolve_requests_number.load(atomic::Ordering::Acquire), - 1, - "While there are 2 items in the completion list, only 1 resolve request should have been sent, for the selected item" + *resolved_items.lock(), + [ + &items_out[0..16], + &items_out[items_out.len() - 4..items_out.len()] + ] + .concat() + .iter() + .cloned() + .collect::>() ); + resolved_items.lock().clear(); cx.update_editor(|editor, cx| { - editor.context_menu_first(&ContextMenuFirst, cx); + editor.context_menu_prev(&ContextMenuPrev, cx); }); cx.run_until_parked(); + // Completions that have already been resolved are skipped. assert_eq!( - resolve_requests_number.load(atomic::Ordering::Acquire), - 2, - "After re-selecting the first item, another resolve request should have been sent" - ); - - expect_first_item.store(false, atomic::Ordering::Release); - cx.update_editor(|editor, cx| { - editor.context_menu_last(&ContextMenuLast, cx); - }); - cx.run_until_parked(); - assert_eq!( - resolve_requests_number.load(atomic::Ordering::Acquire), - 3, - "After selecting the other item, another resolve request should have been sent" + *resolved_items.lock(), + [ + // Selected item is always resolved even if it was resolved before. + &items_out[items_out.len() - 1..items_out.len()], + &items_out[items_out.len() - 16..items_out.len() - 4] + ] + .concat() + .iter() + .cloned() + .collect::>() ); + resolved_items.lock().clear(); } #[gpui::test] diff --git a/crates/util/Cargo.toml b/crates/util/Cargo.toml index 2f84114409..0e75e5221e 100644 --- a/crates/util/Cargo.toml +++ b/crates/util/Cargo.toml @@ -24,6 +24,7 @@ futures-lite.workspace = true futures.workspace = true git2 = { workspace = true, optional = true } globset.workspace = true +itertools.workspace = true log.workspace = true rand = { workspace = true, optional = true } regex.workspace = true diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index 3ba42e33cd..560fe35d74 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -8,6 +8,7 @@ pub mod test; use futures::Future; +use itertools::Either; use regex::Regex; use std::sync::{LazyLock, OnceLock}; use std::{ @@ -199,6 +200,35 @@ pub fn measure(label: &str, f: impl FnOnce() -> R) -> R { } } +pub fn iterate_expanded_and_wrapped_usize_range( + range: Range, + additional_before: usize, + additional_after: usize, + wrap_length: usize, +) -> impl Iterator { + let start_wraps = range.start < additional_before; + let end_wraps = wrap_length < range.end + additional_after; + if start_wraps && end_wraps { + Either::Left(0..wrap_length) + } else if start_wraps { + let wrapped_start = (range.start + wrap_length).saturating_sub(additional_before); + if wrapped_start <= range.end { + Either::Left(0..wrap_length) + } else { + Either::Right((0..range.end + additional_after).chain(wrapped_start..wrap_length)) + } + } else if end_wraps { + let wrapped_end = range.end + additional_after - wrap_length; + if range.start <= wrapped_end { + Either::Left(0..wrap_length) + } else { + Either::Right((0..wrapped_end).chain(range.start - additional_before..wrap_length)) + } + } else { + Either::Left((range.start - additional_before)..(range.end + additional_after)) + } +} + pub trait ResultExt { type Ok; @@ -734,4 +764,48 @@ Line 2 Line 3"# ); } + + #[test] + fn test_iterate_expanded_and_wrapped_usize_range() { + // Neither wrap + assert_eq!( + iterate_expanded_and_wrapped_usize_range(2..4, 1, 1, 8).collect::>(), + (1..5).collect::>() + ); + // Start wraps + assert_eq!( + iterate_expanded_and_wrapped_usize_range(2..4, 3, 1, 8).collect::>(), + ((0..5).chain(7..8)).collect::>() + ); + // Start wraps all the way around + assert_eq!( + iterate_expanded_and_wrapped_usize_range(2..4, 5, 1, 8).collect::>(), + (0..8).collect::>() + ); + // Start wraps all the way around and past 0 + assert_eq!( + iterate_expanded_and_wrapped_usize_range(2..4, 10, 1, 8).collect::>(), + (0..8).collect::>() + ); + // End wraps + assert_eq!( + iterate_expanded_and_wrapped_usize_range(3..5, 1, 4, 8).collect::>(), + (0..1).chain(2..8).collect::>() + ); + // End wraps all the way around + assert_eq!( + iterate_expanded_and_wrapped_usize_range(3..5, 1, 5, 8).collect::>(), + (0..8).collect::>() + ); + // End wraps all the way around and past the end + assert_eq!( + iterate_expanded_and_wrapped_usize_range(3..5, 1, 10, 8).collect::>(), + (0..8).collect::>() + ); + // Both start and end wrap + assert_eq!( + iterate_expanded_and_wrapped_usize_range(3..5, 4, 4, 8).collect::>(), + (0..8).collect::>() + ); + } }