From ab595b0d5575285f2351ff085c4a8862f2ddc1f2 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Tue, 10 Dec 2024 12:25:30 -0700 Subject: [PATCH] Resolve documentation for visible completions (#21705) Release Notes: - Improved LSP resolution of documentation for completions. It now queries documentation for visible completions and avoids doing too many redundant queries. --- 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. --- Cargo.lock | 1 + crates/editor/src/code_context_menus.rs | 136 +++++++++---- crates/editor/src/editor.rs | 2 +- crates/editor/src/editor_tests.rs | 256 ++++++++++-------------- crates/util/Cargo.toml | 1 + crates/util/src/util.rs | 74 +++++++ 6 files changed, 286 insertions(+), 184 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e8a6c7d0852da0eb5a70ff251611037f2a0f988f..cdb1fb589906ad69fd8d87e4055c4e684bded60e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13930,6 +13930,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 1817190e426b4eb954bfc707a01da15af5f290f6..2c76287f00bbc1248c74e411ab374f281d79d946 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -1,4 +1,9 @@ -use std::{cell::Cell, cmp::Reverse, ops::Range, sync::Arc}; +use std::{ + cell::Cell, + cmp::{min, Reverse}, + ops::Range, + sync::Arc, +}; use fuzzy::{StringMatch, StringMatchCandidate}; use gpui::{ @@ -11,8 +16,9 @@ use language::{CodeLabel, Documentation}; use lsp::LanguageServerId; use multi_buffer::{Anchor, ExcerptId}; use ordered_float::OrderedFloat; -use parking_lot::RwLock; +use parking_lot::{Mutex, RwLock}; use project::{CodeAction, Completion, TaskSourceKind}; +use std::iter; use task::ResolvedTask; use ui::{ h_flex, ActiveTheme as _, Color, FluentBuilder as _, InteractiveElement as _, IntoElement, @@ -145,6 +151,7 @@ pub struct CompletionsMenu { resolve_completions: bool, pub aside_was_displayed: Cell, show_completion_documentation: bool, + last_rendered_range: Arc>>>, } impl CompletionsMenu { @@ -173,7 +180,6 @@ impl CompletionsMenu { sort_completions, initial_position, buffer, - show_completion_documentation, completions: Arc::new(RwLock::new(completions)), match_candidates, matches: Vec::new().into(), @@ -181,6 +187,8 @@ impl CompletionsMenu { scroll_handle: UniformListScrollHandle::new(), resolve_completions: true, aside_was_displayed: Cell::new(aside_was_displayed), + show_completion_documentation, + last_rendered_range: Arc::new(Mutex::new(None)), } } @@ -236,6 +244,7 @@ impl CompletionsMenu { resolve_completions: false, aside_was_displayed: Cell::new(false), show_completion_documentation: false, + last_rendered_range: Arc::new(Mutex::new(None)), } } @@ -244,11 +253,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( @@ -256,15 +261,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item > 0 { - self.selected_item -= 1; - } else { - self.selected_item = self.matches.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( @@ -272,15 +269,7 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - if self.selected_item + 1 < self.matches.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( @@ -288,14 +277,41 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.selected_item = self.matches.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.matches.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.matches.len() - 1 + } + } + + fn next_match_index(&self) -> usize { + if self.selected_item + 1 < self.matches.len() { + self.selected_item + 1 + } else { + 0 + } } - pub fn resolve_selected_completion( + pub fn resolve_visible_completions( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, @@ -307,10 +323,59 @@ impl CompletionsMenu { return; }; - let completion_index = self.matches[self.selected_item].candidate_id; + // 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.lock().clone(); + let visible_count = last_rendered_range + .clone() + .map_or(APPROXIMATE_VISIBLE_COUNT, |range| range.count()); + let matches_range = if self.selected_item == 0 { + 0..min(visible_count, self.matches.len()) + } else if self.selected_item == self.matches.len() - 1 { + self.matches.len().saturating_sub(visible_count)..self.matches.len() + } else { + last_rendered_range.unwrap_or_else(|| self.selected_item..self.selected_item + 1) + }; + + // 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 matches_indices = util::iterate_expanded_and_wrapped_usize_range( + matches_range.clone(), + EXTRA_TO_RESOLVE, + EXTRA_TO_RESOLVE, + self.matches.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 candidate_ids = matches_indices.map(|i| self.matches[i].candidate_id); + let candidate_ids = match self.completions.try_read() { + None => candidate_ids.collect::>(), + Some(completions) => candidate_ids + .filter(|i| completions[*i].documentation.is_none()) + .collect::>(), + }; + + // Current selection is always resolved even if it already has documentation, to handle + // out-of-spec language servers that return more results later. + let selected_candidate_id = self.matches[self.selected_item].candidate_id; + let candidate_ids = iter::once(selected_candidate_id) + .chain( + candidate_ids + .into_iter() + .filter(|id| *id != selected_candidate_id), + ) + .collect::>(); + let resolve_task = provider.resolve_completions( self.buffer.clone(), - vec![completion_index], + candidate_ids, self.completions.clone(), cx, ); @@ -406,11 +471,14 @@ impl CompletionsMenu { .occlude() }); + let last_rendered_range = self.last_rendered_range.clone(); + let list = uniform_list( cx.view().clone(), "completions", matches.len(), move |_editor, range, cx| { + last_rendered_range.lock().replace(range.clone()); let start_ix = range.start; let completions_guard = completions.read(); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6039092be50ed0977fb3fe6618201f21033ae15c..1336a76f22194518bdfb3065fc3038fdde6f44cc 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3684,7 +3684,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); *context_menu = Some(CodeContextMenu::Completions(menu)); drop(context_menu); editor.discard_inline_completion(false, cx); diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 468eec6fc3c231761640eebe1a95532e749ac575..c1f15f34b32c4b0aef0017f8241f3418a607af75 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -25,15 +25,15 @@ 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::AtomicUsize; -use std::sync::atomic::{self, AtomicBool}; -use std::{cell::RefCell, future::Future, rc::Rc, time::Instant}; +use std::sync::atomic::{self, AtomicUsize}; +use std::{cell::RefCell, future::Future, iter, rc::Rc, time::Instant}; use test::editor_lsp_test_context::rust_lang; use unindent::Unindent; use util::{ @@ -10717,24 +10717,31 @@ async fn test_completions_resolve_updates_labels(cx: &mut gpui::TestAppContext) async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppContext) { init_test(cx, |_| {}); - let mut cx = EditorLspTestContext::new_rust( - lsp::ServerCapabilities { - completion_provider: Some(lsp::CompletionOptions { - trigger_characters: Some(vec![".".to_string()]), - resolve_provider: Some(true), - ..Default::default() - }), - ..Default::default() - }, - cx, - ) - .await; - - cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"}); - cx.simulate_keystroke("."); + 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!({ "very": "special"}); + 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 { @@ -10748,123 +10755,49 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo }, }; - 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 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 { + trigger_characters: Some(vec![".".to_string()]), + resolve_provider: Some(true), + ..Default::default() + }), + ..Default::default() + }, + cx, + ) + .await; + + cx.set_state(indoc! {"fn main() { let a = 2ˇ; }"}); + cx.simulate_keystroke("."); 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()), @@ -10881,6 +10814,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(); @@ -10892,40 +10840,50 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo completions_menu .matches .iter() - .map(|c| c.string.as_str()) - .collect::>(), - vec!["Some(2)", "vec![2]"] + .map(|c| c.string.clone()) + .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" - ); - - cx.update_editor(|editor, cx| { - editor.context_menu_first(&ContextMenuFirst, cx); - }); - cx.run_until_parked(); - assert_eq!( - resolve_requests_number.load(atomic::Ordering::Acquire), - 2, - "After re-selecting the first item, another resolve request should have been sent" + *resolved_items.lock(), + [ + &items_out[0..16], + &items_out[items_out.len() - 4..items_out.len()] + ] + .concat() + .iter() + .cloned() + .collect::>() ); + resolved_items.lock().clear(); - expect_first_item.store(false, atomic::Ordering::Release); cx.update_editor(|editor, cx| { - editor.context_menu_last(&ContextMenuLast, 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), - 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 2f841144092383b28075a26e0325582fdcdd4178..0e75e5221eb9a50efd0fe00453f6f5d6e08d6c2c 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 777b8b60dcdffdd5c7655553064b6f50c44b7f54..58290373e1c634b026861172165ac0f4d8a8f13b 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::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; @@ -733,4 +763,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::>() + ); + } }