From 2f2e7f03176a50b732208ab3a0c2502f6aba116e Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Fri, 13 Dec 2024 12:22:26 -0700 Subject: [PATCH] Revert "Resolve documentation for visible completions (#21705)" (#21985) This reverts commit ab595b0d5575285f2351ff085c4a8862f2ddc1f2. Release Notes: - (preview only) Fixed a panic in completions --- 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, 182 insertions(+), 288 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 86bb0fc7439276..0ee25ccb5f0a46 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13974,7 +13974,6 @@ 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 2c76287f00bbc1..1817190e426b4e 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -1,9 +1,4 @@ -use std::{ - cell::Cell, - cmp::{min, Reverse}, - ops::Range, - sync::Arc, -}; +use std::{cell::Cell, cmp::Reverse, ops::Range, sync::Arc}; use fuzzy::{StringMatch, StringMatchCandidate}; use gpui::{ @@ -16,9 +11,8 @@ use language::{CodeLabel, Documentation}; use lsp::LanguageServerId; use multi_buffer::{Anchor, ExcerptId}; use ordered_float::OrderedFloat; -use parking_lot::{Mutex, RwLock}; +use parking_lot::RwLock; use project::{CodeAction, Completion, TaskSourceKind}; -use std::iter; use task::ResolvedTask; use ui::{ h_flex, ActiveTheme as _, Color, FluentBuilder as _, InteractiveElement as _, IntoElement, @@ -151,7 +145,6 @@ pub struct CompletionsMenu { resolve_completions: bool, pub aside_was_displayed: Cell, show_completion_documentation: bool, - last_rendered_range: Arc>>>, } impl CompletionsMenu { @@ -180,6 +173,7 @@ impl CompletionsMenu { sort_completions, initial_position, buffer, + show_completion_documentation, completions: Arc::new(RwLock::new(completions)), match_candidates, matches: Vec::new().into(), @@ -187,8 +181,6 @@ 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)), } } @@ -244,7 +236,6 @@ impl CompletionsMenu { resolve_completions: false, aside_was_displayed: Cell::new(false), show_completion_documentation: false, - last_rendered_range: Arc::new(Mutex::new(None)), } } @@ -253,7 +244,11 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.update_selection_index(0, provider, cx); + self.selected_item = 0; + self.scroll_handle + .scroll_to_item(self.selected_item, ScrollStrategy::Top); + self.resolve_selected_completion(provider, cx); + cx.notify(); } fn select_prev( @@ -261,7 +256,15 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.update_selection_index(self.prev_match_index(), provider, cx); + 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(); } fn select_next( @@ -269,7 +272,15 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.update_selection_index(self.next_match_index(), provider, cx); + 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(); } fn select_last( @@ -277,41 +288,14 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - 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 - } + 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(); } - pub fn resolve_visible_completions( + pub fn resolve_selected_completion( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, @@ -323,59 +307,10 @@ impl CompletionsMenu { return; }; - // 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 completion_index = self.matches[self.selected_item].candidate_id; let resolve_task = provider.resolve_completions( self.buffer.clone(), - candidate_ids, + vec![completion_index], self.completions.clone(), cx, ); @@ -471,14 +406,11 @@ 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 844cbea336e350..4e85c20a4405d5 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3736,7 +3736,7 @@ impl Editor { if editor.focus_handle.is_focused(cx) && menu.is_some() { let mut menu = menu.unwrap(); - menu.resolve_visible_completions(editor.completion_provider.as_deref(), cx); + menu.resolve_selected_completion(editor.completion_provider.as_deref(), cx); *context_menu = Some(CodeContextMenu::Completions(menu)); drop(context_menu); cx.notify(); diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index ce020c8ca812a9..921a4c0c283801 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -25,18 +25,14 @@ 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::{ @@ -10746,62 +10742,6 @@ 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 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 { @@ -10818,15 +10758,138 @@ 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, + 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() + }, + ], item_defaults: Some(lsp::CompletionListItemDefaults { data: Some(default_data.clone()), commit_characters: Some(default_commit_characters.clone()), @@ -10843,21 +10906,6 @@ 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(); @@ -10869,50 +10917,40 @@ async fn test_completions_default_resolve_data_handling(cx: &mut gpui::TestAppCo completions_menu .matches .iter() - .map(|c| c.string.clone()) - .collect::>(), - items_out - .iter() - .map(|completion| completion.label.clone()) - .collect::>() + .map(|c| c.string.as_str()) + .collect::>(), + vec!["Some(2)", "vec![2]"] ); } 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!( - *resolved_items.lock(), - [ - &items_out[0..16], - &items_out[items_out.len() - 4..items_out.len()] - ] - .concat() - .iter() - .cloned() - .collect::>() + 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().clear(); cx.update_editor(|editor, cx| { - editor.context_menu_prev(&ContextMenuPrev, cx); + editor.context_menu_first(&ContextMenuFirst, cx); }); cx.run_until_parked(); - // Completions that have already been resolved are skipped. assert_eq!( - *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::>() + 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().clear(); } #[gpui::test] diff --git a/crates/util/Cargo.toml b/crates/util/Cargo.toml index 0e75e5221eb9a5..2f841144092383 100644 --- a/crates/util/Cargo.toml +++ b/crates/util/Cargo.toml @@ -24,7 +24,6 @@ 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 58290373e1c634..777b8b60dcdffd 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -8,7 +8,6 @@ pub mod test; use futures::Future; -use itertools::Either; use regex::Regex; use std::sync::OnceLock; use std::{ @@ -200,35 +199,6 @@ 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; @@ -763,48 +733,4 @@ 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::>() - ); - } }