Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Resolve documentation for visible completions (#21705)

Browse source 
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.
This commit is contained in:
Michael Sloan 2024-12-10 12:25:30 -07:00 • committed by GitHub
parent ab1e9bf270
commit ab595b0d55
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 286 additions and 184 deletions
Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -13930,6 +13930,7 @@ dependencies = [
"futures-lite 1.13.0",
"git2",
"globset",
"itertools 0.13.0",
"log",
"rand 0.8.5",
"regex",

136
crates/editor/src/code_context_menus.rs
View file

@ -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<bool>,
show_completion_documentation: bool,
last_rendered_range: Arc<Mutex<Option<Range<usize>>>>,
}
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<Editor>,
) {
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<Editor>,
) {
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<Editor>,
) {
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<Editor>,
) {
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);
}
pub fn resolve_selected_completion(
fn update_selection_index(
&mut self,
match_index: usize,
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
) {
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_visible_completions(
&mut self,
provider: Option<&dyn CompletionProvider>,
cx: &mut ViewContext<Editor>,
@ -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::<Vec<usize>>(),
Some(completions) => candidate_ids
.filter(|i| completions[*i].documentation.is_none())
.collect::<Vec<usize>>(),
};
// 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::<Vec<usize>>();
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();

2
crates/editor/src/editor.rs
View file

@ -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);

256
crates/editor/src/editor_tests.rs
View file

@ -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,6 +10717,62 @@ 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::<Vec<_>>();
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::<Vec<lsp::CompletionItem>>();
let mut cx = EditorLspTestContext::new_rust(
lsp::ServerCapabilities {
completion_provider: Some(lsp::CompletionOptions {
@ -10733,138 +10789,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::<lsp::request::ResolveCompletionItem, _, _>({
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::<lsp::request::Completion, _, _>(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::<lsp::request::ResolveCompletionItem, _, _>({
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<_>>(),
vec!["Some(2)", "vec![2]"]
.map(|c| c.string.clone())
.collect::<Vec<String>>(),
items_out
.iter()
.map(|completion| completion.label.clone())
.collect::<Vec<String>>()
);
}
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::<Vec<lsp::CompletionItem>>()
);
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::<Vec<lsp::CompletionItem>>()
);
resolved_items.lock().clear();
}
#[gpui::test]

1
crates/util/Cargo.toml
View file

@ -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

74
crates/util/src/util.rs
View file

@ -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<R>(label: &str, f: impl FnOnce() -> R) -> R {
}
}
pub fn iterate_expanded_and_wrapped_usize_range(
range: Range<usize>,
additional_before: usize,
additional_after: usize,
wrap_length: usize,
) -> impl Iterator<Item = usize> {
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<E> {
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::<Vec<usize>>(),
(1..5).collect::<Vec<usize>>()
);
// Start wraps
assert_eq!(
iterate_expanded_and_wrapped_usize_range(2..4, 3, 1, 8).collect::<Vec<usize>>(),
((0..5).chain(7..8)).collect::<Vec<usize>>()
);
// Start wraps all the way around
assert_eq!(
iterate_expanded_and_wrapped_usize_range(2..4, 5, 1, 8).collect::<Vec<usize>>(),
(0..8).collect::<Vec<usize>>()
);
// Start wraps all the way around and past 0
assert_eq!(
iterate_expanded_and_wrapped_usize_range(2..4, 10, 1, 8).collect::<Vec<usize>>(),
(0..8).collect::<Vec<usize>>()
);
// End wraps
assert_eq!(
iterate_expanded_and_wrapped_usize_range(3..5, 1, 4, 8).collect::<Vec<usize>>(),
(0..1).chain(2..8).collect::<Vec<usize>>()
);
// End wraps all the way around
assert_eq!(
iterate_expanded_and_wrapped_usize_range(3..5, 1, 5, 8).collect::<Vec<usize>>(),
(0..8).collect::<Vec<usize>>()
);
// End wraps all the way around and past the end
assert_eq!(
iterate_expanded_and_wrapped_usize_range(3..5, 1, 10, 8).collect::<Vec<usize>>(),
(0..8).collect::<Vec<usize>>()
);
// Both start and end wrap
assert_eq!(
iterate_expanded_and_wrapped_usize_range(3..5, 4, 4, 8).collect::<Vec<usize>>(),
(0..8).collect::<Vec<usize>>()
);
}
}