From 75c53447548686c477434c95214e0e9a437593df Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 21 Jan 2025 17:55:41 +0200 Subject: [PATCH] Fix completion labels becoming overly large due to LSP completion items with newlines (#23407) Reworks https://github.com/zed-industries/zed/pull/23030 and https://github.com/zed-industries/zed/pull/15087 Closes https://github.com/zed-industries/zed/issues/23352 Closes https://github.com/zed-industries/zed/issues/23310 Zed's completion items use `label` from LSP completion items as a base to show in the list: https://github.com/zed-industries/zed/blob/d290da7dac922fdc67c4774cdd371fba23fe62e3/crates/project/src/lsp_store.rs#L4371-L4374 Besides that, certain language plugins append `detail` or `label_details.description` as a suffix: https://github.com/zed-industries/zed/blob/d290da7dac922fdc67c4774cdd371fba23fe62e3/crates/languages/src/vtsls.rs#L178-L188 Either of these 3 properties may return `\n` (or multiple) in it, spoiling Zed's completion menu, which uses `UniformList` to render those items: a uniform list uses common, minimum possible height for each element, and `\n` bloats that overly. Good approach would be to use something else: https://github.com/zed-industries/zed/issues/21403 but that has its own drawbacks and relatively hard to use instead (?). We could follow VSCode's approach and move away all but `label` from `CodeLabel.text` to the side, where the documentation is, but that does not solve the issue with `details` having newlines. So, for now, sanitize all labels and remove any newlines from them. If newlines are found, also replace whitespace sequences if there's more than 1 in a row. Later, this approach can be improved similarly to how Helix and Zed's inline completions do: rendering a "ghost" text, showing the completion's edit applied to the editor. Release Notes: - Fixed completion labels becoming overly large due to LSP completion items with newlines --- crates/editor/src/editor_tests.rs | 241 +++++++++++++++++++++++++++++ crates/language/src/language.rs | 18 +++ crates/languages/src/typescript.rs | 6 +- crates/languages/src/vtsls.rs | 6 +- crates/project/src/lsp_store.rs | 82 +++++++++- 5 files changed, 338 insertions(+), 15 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 5770d0cf1a..c9353a8e45 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -8437,6 +8437,247 @@ async fn test_completion(cx: &mut gpui::TestAppContext) { apply_additional_edits.await.unwrap(); } +#[gpui::test] +async fn test_multiline_completion(cx: &mut gpui::TestAppContext) { + init_test(cx, |_| {}); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + "/a", + json!({ + "main.ts": "a", + }), + ) + .await; + + let project = Project::test(fs, ["/a".as_ref()], cx).await; + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + let typescript_language = Arc::new(Language::new( + LanguageConfig { + name: "TypeScript".into(), + matcher: LanguageMatcher { + path_suffixes: vec!["ts".to_string()], + ..LanguageMatcher::default() + }, + line_comments: vec!["// ".into()], + ..LanguageConfig::default() + }, + Some(tree_sitter_typescript::LANGUAGE_TYPESCRIPT.into()), + )); + language_registry.add(typescript_language.clone()); + let mut fake_servers = language_registry.register_fake_lsp( + "TypeScript", + FakeLspAdapter { + capabilities: lsp::ServerCapabilities { + completion_provider: Some(lsp::CompletionOptions { + trigger_characters: Some(vec![".".to_string(), ":".to_string()]), + ..lsp::CompletionOptions::default() + }), + signature_help_provider: Some(lsp::SignatureHelpOptions::default()), + ..lsp::ServerCapabilities::default() + }, + // Emulate vtsls label generation + label_for_completion: Some(Box::new(|item, _| { + let text = if let Some(description) = item + .label_details + .as_ref() + .and_then(|label_details| label_details.description.as_ref()) + { + format!("{} {}", item.label, description) + } else if let Some(detail) = &item.detail { + format!("{} {}", item.label, detail) + } else { + item.label.clone() + }; + let len = text.len(); + Some(language::CodeLabel { + text, + runs: Vec::new(), + filter_range: 0..len, + }) + })), + ..FakeLspAdapter::default() + }, + ); + let workspace = cx.add_window(|cx| Workspace::test_new(project.clone(), cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + let worktree_id = workspace + .update(cx, |workspace, cx| { + workspace.project().update(cx, |project, cx| { + project.worktrees(cx).next().unwrap().read(cx).id() + }) + }) + .unwrap(); + let _buffer = project + .update(cx, |project, cx| { + project.open_local_buffer_with_lsp("/a/main.ts", cx) + }) + .await + .unwrap(); + let editor = workspace + .update(cx, |workspace, cx| { + workspace.open_path((worktree_id, "main.ts"), None, true, cx) + }) + .unwrap() + .await + .unwrap() + .downcast::() + .unwrap(); + let fake_server = fake_servers.next().await.unwrap(); + + let multiline_label = "StickyHeaderExcerpt {\n excerpt,\n next_excerpt_controls_present,\n next_buffer_row,\n }: StickyHeaderExcerpt<'_>,"; + let multiline_label_2 = "a\nb\nc\n"; + let multiline_detail = "[]struct {\n\tSignerId\tstruct {\n\t\tIssuer\t\t\tstring\t`json:\"issuer\"`\n\t\tSubjectSerialNumber\"`\n}}"; + let multiline_description = "d\ne\nf\n"; + let multiline_detail_2 = "g\nh\ni\n"; + + let mut completion_handle = + fake_server.handle_request::(move |params, _| async move { + Ok(Some(lsp::CompletionResponse::Array(vec![ + lsp::CompletionItem { + label: multiline_label.to_string(), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + end: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + }, + new_text: "new_text_1".to_string(), + })), + ..lsp::CompletionItem::default() + }, + lsp::CompletionItem { + label: "single line label 1".to_string(), + detail: Some(multiline_detail.to_string()), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + end: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + }, + new_text: "new_text_2".to_string(), + })), + ..lsp::CompletionItem::default() + }, + lsp::CompletionItem { + label: "single line label 2".to_string(), + label_details: Some(lsp::CompletionItemLabelDetails { + description: Some(multiline_description.to_string()), + detail: None, + }), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + end: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + }, + new_text: "new_text_2".to_string(), + })), + ..lsp::CompletionItem::default() + }, + lsp::CompletionItem { + label: multiline_label_2.to_string(), + detail: Some(multiline_detail_2.to_string()), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + end: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + }, + new_text: "new_text_3".to_string(), + })), + ..lsp::CompletionItem::default() + }, + lsp::CompletionItem { + label: "Label with many spaces and \t but without newlines".to_string(), + detail: Some( + "Details with many spaces and \t but without newlines".to_string(), + ), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + end: lsp::Position { + line: params.text_document_position.position.line, + character: params.text_document_position.position.character, + }, + }, + new_text: "new_text_4".to_string(), + })), + ..lsp::CompletionItem::default() + }, + ]))) + }); + + editor.update(cx, |editor, cx| { + editor.focus(cx); + editor.move_to_end(&MoveToEnd, cx); + editor.handle_input(".", cx); + }); + cx.run_until_parked(); + completion_handle.next().await.unwrap(); + + editor.update(cx, |editor, _| { + assert!(editor.context_menu_visible()); + if let Some(CodeContextMenu::Completions(menu)) = editor.context_menu.borrow_mut().as_ref() + { + let completion_labels = menu + .completions + .borrow() + .iter() + .map(|c| c.label.text.clone()) + .collect::>(); + assert_eq!( + completion_labels, + &[ + "StickyHeaderExcerpt { excerpt, next_excerpt_controls_present, next_buffer_row, }: StickyHeaderExcerpt<'_>,", + "single line label 1 []struct { SignerId struct { Issuer string `json:\"issuer\"` SubjectSerialNumber\"` }}", + "single line label 2 d e f ", + "a b c g h i ", + "Label with many spaces and \t but without newlines Details with many spaces and \t but without newlines", + ], + "Completion items should have their labels without newlines, also replacing excessive whitespaces. Completion items without newlines should not be altered.", + ); + + for completion in menu + .completions + .borrow() + .iter() { + assert_eq!( + completion.label.filter_range, + 0..completion.label.text.len(), + "Adjusted completion items should still keep their filter ranges for the entire label. Item: {completion:?}" + ); + } + + } else { + panic!("expected completion menu to be open"); + } + }); +} + #[gpui::test] async fn test_completion_page_up_down_keys(cx: &mut gpui::TestAppContext) { init_test(cx, |_| {}); diff --git a/crates/language/src/language.rs b/crates/language/src/language.rs index 9732939d50..8061c7efa9 100644 --- a/crates/language/src/language.rs +++ b/crates/language/src/language.rs @@ -763,6 +763,14 @@ pub struct FakeLspAdapter { pub capabilities: lsp::ServerCapabilities, pub initializer: Option>, + pub label_for_completion: Option< + Box< + dyn 'static + + Send + + Sync + + Fn(&lsp::CompletionItem, &Arc) -> Option, + >, + >, } /// Configuration of handling bracket pairs for a given language. @@ -1778,6 +1786,7 @@ impl Default for FakeLspAdapter { arguments: vec![], env: Default::default(), }, + label_for_completion: None, } } } @@ -1849,6 +1858,15 @@ impl LspAdapter for FakeLspAdapter { ) -> Result> { Ok(self.initialization_options.clone()) } + + async fn label_for_completion( + &self, + item: &lsp::CompletionItem, + language: &Arc, + ) -> Option { + let label_for_completion = self.label_for_completion.as_ref()?; + label_for_completion(item, language) + } } fn get_capture_indices(query: &Query, captures: &mut [(&str, &mut Option)]) { diff --git a/crates/languages/src/typescript.rs b/crates/languages/src/typescript.rs index 6ceb6f5898..edfdbf5f55 100644 --- a/crates/languages/src/typescript.rs +++ b/crates/languages/src/typescript.rs @@ -212,16 +212,14 @@ impl LspAdapter for TypeScriptLspAdapter { _ => None, }?; - let one_line = |s: &str| s.replace(" ", "").replace('\n', " "); - let text = if let Some(description) = item .label_details .as_ref() .and_then(|label_details| label_details.description.as_ref()) { - format!("{} {}", item.label, one_line(description)) + format!("{} {}", item.label, description) } else if let Some(detail) = &item.detail { - format!("{} {}", item.label, one_line(detail)) + format!("{} {}", item.label, detail) } else { item.label.clone() }; diff --git a/crates/languages/src/vtsls.rs b/crates/languages/src/vtsls.rs index e44e4e295f..22718e410c 100644 --- a/crates/languages/src/vtsls.rs +++ b/crates/languages/src/vtsls.rs @@ -175,16 +175,14 @@ impl LspAdapter for VtslsLspAdapter { _ => None, }?; - let one_line = |s: &str| s.replace(" ", "").replace('\n', " "); - let text = if let Some(description) = item .label_details .as_ref() .and_then(|label_details| label_details.description.as_ref()) { - format!("{} {}", item.label, one_line(description)) + format!("{} {}", item.label, description) } else if let Some(detail) = &item.detail { - format!("{} {}", item.label, one_line(detail)) + format!("{} {}", item.label, detail) } else { item.label.clone() }; diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index c8f4ec52e5..cbd93ad12a 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -4357,7 +4357,7 @@ impl LspStore { // NB: Zed does not have `details` inside the completion resolve capabilities, but certain language servers violate the spec and do not return `details` immediately, e.g. https://github.com/yioneko/vtsls/issues/213 // So we have to update the label here anyway... - let new_label = match snapshot.language() { + let mut new_label = match snapshot.language() { Some(language) => { adapter .labels_for_completions(&[completion_item.clone()], language) @@ -4373,6 +4373,7 @@ impl LspStore { completion_item.filter_text.as_deref(), ) }); + ensure_uniform_list_compatible_label(&mut new_label); let mut completions = completions.borrow_mut(); let completion = &mut completions[completion_index]; @@ -8014,15 +8015,18 @@ async fn populate_labels_for_completions( None }; + let mut label = label.unwrap_or_else(|| { + CodeLabel::plain( + lsp_completion.label.clone(), + lsp_completion.filter_text.as_deref(), + ) + }); + ensure_uniform_list_compatible_label(&mut label); + completions.push(Completion { old_range: completion.old_range, new_text: completion.new_text, - label: label.unwrap_or_else(|| { - CodeLabel::plain( - lsp_completion.label.clone(), - lsp_completion.filter_text.as_deref(), - ) - }), + label, server_id: completion.server_id, documentation, lsp_completion, @@ -8733,6 +8737,70 @@ fn include_text(server: &lsp::LanguageServer) -> Option { } } +/// Completion items are displayed in a `UniformList`. +/// Usually, those items are single-line strings, but in LSP responses, +/// completion items `label`, `detail` and `label_details.description` may contain newlines or long spaces. +/// Many language plugins construct these items by joining these parts together, and we may fall back to `CodeLabel::plain` that uses `label`. +/// All that may lead to a newline being inserted into resulting `CodeLabel.text`, which will force `UniformList` to bloat each entry to occupy more space, +/// breaking the completions menu presentation. +/// +/// Sanitize the text to ensure there are no newlines, or, if there are some, remove them and also remove long space sequences if there were newlines. +fn ensure_uniform_list_compatible_label(label: &mut CodeLabel) { + let mut new_text = String::with_capacity(label.text.len()); + let mut offset_map = vec![0; label.text.len() + 1]; + let mut last_char_was_space = false; + let mut new_idx = 0; + let mut chars = label.text.char_indices().fuse(); + let mut newlines_removed = false; + + while let Some((idx, c)) = chars.next() { + offset_map[idx] = new_idx; + + match c { + '\n' if last_char_was_space => { + newlines_removed = true; + } + '\t' | ' ' if last_char_was_space => {} + '\n' if !last_char_was_space => { + new_text.push(' '); + new_idx += 1; + last_char_was_space = true; + newlines_removed = true; + } + ' ' | '\t' => { + new_text.push(' '); + new_idx += 1; + last_char_was_space = true; + } + _ => { + new_text.push(c); + new_idx += 1; + last_char_was_space = false; + } + } + } + offset_map[label.text.len()] = new_idx; + + // Only modify the label if newlines were removed. + if !newlines_removed { + return; + } + + for (range, _) in &mut label.runs { + range.start = offset_map[range.start]; + range.end = offset_map[range.end]; + } + + if label.filter_range == (0..label.text.len()) { + label.filter_range = 0..new_text.len(); + } else { + label.filter_range.start = offset_map[label.filter_range.start]; + label.filter_range.end = offset_map[label.filter_range.end]; + } + + label.text = new_text; +} + #[cfg(test)] #[test] fn test_glob_literal_prefix() {