From c81230405f267c5bad06c732bc99bf6b537fcf18 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 2 May 2024 10:50:01 +0200 Subject: [PATCH] typescript: Complete function calls with snippets (#11157) This allows function call (i.e. snippet) completion with `typescript-language-server`. So far that didn't work, because `typescript-language-server` doesn't respond with `insertText` when getting the completions, but only when then sending `completionItem/resolve` requests. See: https://github.com/hrsh7th/nvim-cmp/issues/646#issuecomment-992765479 What this PR does is to support text edits in the response to `completionItem/resolve`, which means updating the completion item. It then enables this feature by default for `typescript-language-server`. TODOs: - [x] Make this work over collab - [x] Test that this doesn't break existing language server support - [x] Refactor duplicated code Release Notes: - Added support for function call completion when using `typescript-language-server`. This will result in parameters being added, which can then be changed and navigated with ``. For this to work with `typescript-language-server`, the documentation for a given completion item needs to be resolved, meaning that if one types very quickly and accepts completion before `typescript-language-server` could respond with the documentation, no full function completion is used. Demo: https://github.com/zed-industries/zed/assets/1185253/c23ebe12-5902-4b50-888c-d9b8cd32965d --- crates/collab/src/tests/editor_tests.rs | 91 ++++++++++++++- .../src/chat_panel/message_editor.rs | 1 + crates/editor/src/editor.rs | 16 ++- crates/languages/src/typescript.rs | 12 ++ crates/project/src/lsp_command.rs | 68 ++++++----- crates/project/src/project.rs | 107 +++++++++++++++--- crates/rpc/proto/zed.proto | 11 +- 7 files changed, 256 insertions(+), 50 deletions(-) diff --git a/crates/collab/src/tests/editor_tests.rs b/crates/collab/src/tests/editor_tests.rs index e07ff0cdf8..624b633043 100644 --- a/crates/collab/src/tests/editor_tests.rs +++ b/crates/collab/src/tests/editor_tests.rs @@ -6,8 +6,8 @@ use call::ActiveCall; use collections::HashMap; use editor::{ actions::{ - ConfirmCodeAction, ConfirmCompletion, ConfirmRename, Redo, Rename, RevertSelectedHunks, - ToggleCodeActions, Undo, + ConfirmCodeAction, ConfirmCompletion, ConfirmRename, ContextMenuFirst, Redo, Rename, + RevertSelectedHunks, ToggleCodeActions, Undo, }, test::{ editor_hunks, @@ -444,6 +444,93 @@ async fn test_collaborating_with_completion(cx_a: &mut TestAppContext, cx_b: &mu "use d::SomeTrait;\nfn main() { a.first_method() }" ); }); + + // Now we do a second completion, this time to ensure that documentation/snippets are + // resolved + editor_b.update(cx_b, |editor, cx| { + editor.change_selections(None, cx, |s| s.select_ranges([46..46])); + editor.handle_input("; a", cx); + editor.handle_input(".", cx); + }); + + buffer_b.read_with(cx_b, |buffer, _| { + assert_eq!( + buffer.text(), + "use d::SomeTrait;\nfn main() { a.first_method(); a. }" + ); + }); + + let mut completion_response = fake_language_server + .handle_request::(|params, _| async move { + assert_eq!( + params.text_document_position.text_document.uri, + lsp::Url::from_file_path("/a/main.rs").unwrap(), + ); + assert_eq!( + params.text_document_position.position, + lsp::Position::new(1, 32), + ); + + Ok(Some(lsp::CompletionResponse::Array(vec![ + lsp::CompletionItem { + label: "third_method(…)".into(), + detail: Some("fn(&mut self, B, C, D) -> E".into()), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + // no snippet placehodlers + new_text: "third_method".to_string(), + range: lsp::Range::new( + lsp::Position::new(1, 32), + lsp::Position::new(1, 32), + ), + })), + insert_text_format: Some(lsp::InsertTextFormat::SNIPPET), + documentation: None, + ..Default::default() + }, + ]))) + }); + + // The completion now gets a new `text_edit.new_text` when resolving the completion item + let mut resolve_completion_response = fake_language_server + .handle_request::(|params, _| async move { + assert_eq!(params.label, "third_method(…)"); + Ok(lsp::CompletionItem { + label: "third_method(…)".into(), + detail: Some("fn(&mut self, B, C, D) -> E".into()), + text_edit: Some(lsp::CompletionTextEdit::Edit(lsp::TextEdit { + // Now it's a snippet + new_text: "third_method($1, $2, $3)".to_string(), + range: lsp::Range::new(lsp::Position::new(1, 32), lsp::Position::new(1, 32)), + })), + insert_text_format: Some(lsp::InsertTextFormat::SNIPPET), + documentation: Some(lsp::Documentation::String( + "this is the documentation".into(), + )), + ..Default::default() + }) + }); + + cx_b.executor().run_until_parked(); + + completion_response.next().await.unwrap(); + + editor_b.update(cx_b, |editor, cx| { + assert!(editor.context_menu_visible()); + editor.context_menu_first(&ContextMenuFirst {}, cx); + }); + + resolve_completion_response.next().await.unwrap(); + cx_b.executor().run_until_parked(); + + // When accepting the completion, the snippet is insert. + editor_b.update(cx_b, |editor, cx| { + assert!(editor.context_menu_visible()); + editor.confirm_completion(&ConfirmCompletion { item_ix: Some(0) }, cx); + assert_eq!( + editor.text(cx), + "use d::SomeTrait;\nfn main() { a.first_method(); a.third_method(, , ) }" + ); + }); } #[gpui::test(iterations = 10)] diff --git a/crates/collab_ui/src/chat_panel/message_editor.rs b/crates/collab_ui/src/chat_panel/message_editor.rs index 84014f05c6..455ad80a88 100644 --- a/crates/collab_ui/src/chat_panel/message_editor.rs +++ b/crates/collab_ui/src/chat_panel/message_editor.rs @@ -59,6 +59,7 @@ impl CompletionProvider for MessageEditorCompletionProvider { fn resolve_completions( &self, + _buffer: Model, _completion_indices: Vec, _completions: Arc>>, _cx: &mut ViewContext, diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 831196bdfa..423c35d148 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -837,6 +837,7 @@ impl CompletionsMenu { } fn pre_resolve_completion_documentation( + buffer: Model, completions: Arc>>, matches: Arc<[StringMatch]>, editor: &Editor, @@ -852,6 +853,7 @@ impl CompletionsMenu { }; let resolve_task = provider.resolve_completions( + buffer, matches.iter().map(|m| m.candidate_id).collect(), completions.clone(), cx, @@ -880,7 +882,12 @@ impl CompletionsMenu { }; let resolve_task = project.update(cx, |project, cx| { - project.resolve_completions(vec![completion_index], self.completions.clone(), cx) + project.resolve_completions( + self.buffer.clone(), + vec![completion_index], + self.completions.clone(), + cx, + ) }); let delay_ms = @@ -3463,7 +3470,7 @@ impl Editor { ) }) .collect(), - buffer, + buffer: buffer.clone(), completions: Arc::new(RwLock::new(completions.into())), matches: Vec::new().into(), selected_item: 0, @@ -3490,6 +3497,7 @@ impl Editor { .completion_documentation_pre_resolve_debounce .fire_new(delay, cx, |editor, cx| { CompletionsMenu::pre_resolve_completion_documentation( + buffer, completions, matches, editor, @@ -10213,6 +10221,7 @@ pub trait CompletionProvider { fn resolve_completions( &self, + buffer: Model, completion_indices: Vec, completions: Arc>>, cx: &mut ViewContext, @@ -10241,12 +10250,13 @@ impl CompletionProvider for Model { fn resolve_completions( &self, + buffer: Model, completion_indices: Vec, completions: Arc>>, cx: &mut ViewContext, ) -> Task> { self.update(cx, |project, cx| { - project.resolve_completions(completion_indices, completions, cx) + project.resolve_completions(buffer, completion_indices, completions, cx) }) } diff --git a/crates/languages/src/typescript.rs b/crates/languages/src/typescript.rs index 7a538dba13..9e6fed2eb5 100644 --- a/crates/languages/src/typescript.rs +++ b/crates/languages/src/typescript.rs @@ -186,6 +186,18 @@ impl LspAdapter for TypeScriptLspAdapter { }))) } + async fn workspace_configuration( + self: Arc, + _: &Arc, + _cx: &mut AsyncAppContext, + ) -> Result { + Ok(json!({ + "completions": { + "completeFunctionCalls": true + } + })) + } + fn language_ids(&self) -> HashMap { HashMap::from_iter([ ("TypeScript".into(), "typescript".into()), diff --git a/crates/project/src/lsp_command.rs b/crates/project/src/lsp_command.rs index c1b1b20ebd..4c006bb864 100644 --- a/crates/project/src/lsp_command.rs +++ b/crates/project/src/lsp_command.rs @@ -1505,18 +1505,11 @@ impl LspCommand for GetCompletions { let edit = match lsp_completion.text_edit.as_ref() { // If the language server provides a range to overwrite, then // check that the range is valid. - Some(lsp::CompletionTextEdit::Edit(edit)) => { - let range = range_from_lsp(edit.range); - let start = snapshot.clip_point_utf16(range.start, Bias::Left); - let end = snapshot.clip_point_utf16(range.end, Bias::Left); - if start != range.start.0 || end != range.end.0 { - log::info!("completion out of expected range"); - return false; + Some(completion_text_edit) => { + match parse_completion_text_edit(completion_text_edit, &snapshot) { + Some(edit) => edit, + None => return false, } - ( - snapshot.anchor_before(start)..snapshot.anchor_after(end), - edit.new_text.clone(), - ) } // If the language server does not provide a range, then infer @@ -1570,21 +1563,6 @@ impl LspCommand for GetCompletions { .clone(); (range, text) } - - Some(lsp::CompletionTextEdit::InsertAndReplace(edit)) => { - let range = range_from_lsp(edit.insert); - - let start = snapshot.clip_point_utf16(range.start, Bias::Left); - let end = snapshot.clip_point_utf16(range.end, Bias::Left); - if start != range.start.0 || end != range.end.0 { - log::info!("completion out of expected range"); - return false; - } - ( - snapshot.anchor_before(start)..snapshot.anchor_after(end), - edit.new_text.clone(), - ) - } }; completion_edits.push(edit); @@ -1684,6 +1662,44 @@ impl LspCommand for GetCompletions { } } +pub(crate) fn parse_completion_text_edit( + edit: &lsp::CompletionTextEdit, + snapshot: &BufferSnapshot, +) -> Option<(Range, String)> { + match edit { + lsp::CompletionTextEdit::Edit(edit) => { + let range = range_from_lsp(edit.range); + let start = snapshot.clip_point_utf16(range.start, Bias::Left); + let end = snapshot.clip_point_utf16(range.end, Bias::Left); + if start != range.start.0 || end != range.end.0 { + log::info!("completion out of expected range"); + None + } else { + Some(( + snapshot.anchor_before(start)..snapshot.anchor_after(end), + edit.new_text.clone(), + )) + } + } + + lsp::CompletionTextEdit::InsertAndReplace(edit) => { + let range = range_from_lsp(edit.insert); + + let start = snapshot.clip_point_utf16(range.start, Bias::Left); + let end = snapshot.clip_point_utf16(range.end, Bias::Left); + if start != range.start.0 || end != range.end.0 { + log::info!("completion out of expected range"); + None + } else { + Some(( + snapshot.anchor_before(start)..snapshot.anchor_after(end), + edit.new_text.clone(), + )) + } + } + } +} + #[async_trait(?Send)] impl LspCommand for GetCodeActions { type Response = Vec; diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index ae2b2ba57d..3c72161aef 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -98,7 +98,7 @@ use std::{ }; use task::static_source::{StaticSource, TrackedFile}; use terminals::Terminals; -use text::{Anchor, BufferId}; +use text::{Anchor, BufferId, LineEnding}; use util::{ debug_panic, defer, http::{HttpClient, Url}, @@ -5597,6 +5597,7 @@ impl Project { pub fn resolve_completions( &self, + buffer: Model, completion_indices: Vec, completions: Arc>>, cx: &mut ModelContext, @@ -5607,6 +5608,9 @@ impl Project { let is_remote = self.is_remote(); let project_id = self.remote_id(); + let buffer_id = buffer.read(cx).remote_id(); + let buffer_snapshot = buffer.read(cx).snapshot(); + cx.spawn(move |this, mut cx| async move { let mut did_resolve = false; if is_remote { @@ -5628,9 +5632,10 @@ impl Project { (server_id, completion) }; - Self::resolve_completion_documentation_remote( + Self::resolve_completion_remote( project_id, server_id, + buffer_id, completions.clone(), completion_index, completion, @@ -5665,8 +5670,9 @@ impl Project { }; did_resolve = true; - Self::resolve_completion_documentation_local( + Self::resolve_completion_local( server, + &buffer_snapshot, completions.clone(), completion_index, completion, @@ -5680,8 +5686,9 @@ impl Project { }) } - async fn resolve_completion_documentation_local( + async fn resolve_completion_local( server: Arc, + snapshot: &BufferSnapshot, completions: Arc>>, completion_index: usize, completion: lsp::CompletionItem, @@ -5702,9 +5709,9 @@ impl Project { return; }; - if let Some(lsp_documentation) = completion_item.documentation { + if let Some(lsp_documentation) = completion_item.documentation.as_ref() { let documentation = language::prepare_completion_documentation( - &lsp_documentation, + lsp_documentation, &language_registry, None, // TODO: Try to reasonably work out which language the completion is for ) @@ -5718,11 +5725,31 @@ impl Project { let completion = &mut completions[completion_index]; completion.documentation = Some(Documentation::Undocumented); } + + if let Some(text_edit) = completion_item.text_edit.as_ref() { + // Technically we don't have to parse the whole `text_edit`, since the only + // language server we currently use that does update `text_edit` in `completionItem/resolve` + // is `typescript-language-server` and they only update `text_edit.new_text`. + // But we should not rely on that. + let edit = parse_completion_text_edit(text_edit, snapshot); + + if let Some((old_range, mut new_text)) = edit { + LineEnding::normalize(&mut new_text); + + let mut completions = completions.write(); + let completion = &mut completions[completion_index]; + + completion.new_text = new_text; + completion.old_range = old_range; + } + } } - async fn resolve_completion_documentation_remote( + #[allow(clippy::too_many_arguments)] + async fn resolve_completion_remote( project_id: u64, server_id: LanguageServerId, + buffer_id: BufferId, completions: Arc>>, completion_index: usize, completion: lsp::CompletionItem, @@ -5733,6 +5760,7 @@ impl Project { project_id, language_server_id: server_id.0 as u64, lsp_completion: serde_json::to_string(&completion).unwrap().into_bytes(), + buffer_id: buffer_id.into(), }; let Some(response) = client @@ -5744,21 +5772,32 @@ impl Project { return; }; - let documentation = if response.text.is_empty() { + let documentation = if response.documentation.is_empty() { Documentation::Undocumented - } else if response.is_markdown { + } else if response.documentation_is_markdown { Documentation::MultiLineMarkdown( - markdown::parse_markdown(&response.text, &language_registry, None).await, + markdown::parse_markdown(&response.documentation, &language_registry, None).await, ) - } else if response.text.lines().count() <= 1 { - Documentation::SingleLine(response.text) + } else if response.documentation.lines().count() <= 1 { + Documentation::SingleLine(response.documentation) } else { - Documentation::MultiLinePlainText(response.text) + Documentation::MultiLinePlainText(response.documentation) }; let mut completions = completions.write(); let completion = &mut completions[completion_index]; completion.documentation = Some(documentation); + + let old_range = response + .old_start + .and_then(deserialize_anchor) + .zip(response.old_end.and_then(deserialize_anchor)); + if let Some((old_start, old_end)) = old_range { + if !response.new_text.is_empty() { + completion.new_text = response.new_text; + completion.old_range = old_start..old_end; + } + } } pub fn apply_additional_edits_for_completion( @@ -8962,19 +9001,53 @@ impl Project { })?? .await?; - let mut is_markdown = false; - let text = match completion.documentation { + let mut documentation_is_markdown = false; + let documentation = match completion.documentation { Some(lsp::Documentation::String(text)) => text, Some(lsp::Documentation::MarkupContent(lsp::MarkupContent { kind, value })) => { - is_markdown = kind == lsp::MarkupKind::Markdown; + documentation_is_markdown = kind == lsp::MarkupKind::Markdown; value } _ => String::new(), }; - Ok(proto::ResolveCompletionDocumentationResponse { text, is_markdown }) + // If we have a new buffer_id, that means we're talking to a new client + // and want to check for new text_edits in the completion too. + let mut old_start = None; + let mut old_end = None; + let mut new_text = String::default(); + if let Ok(buffer_id) = BufferId::new(envelope.payload.buffer_id) { + let buffer_snapshot = this.update(&mut cx, |this, cx| { + let buffer = this + .opened_buffers + .get(&buffer_id) + .and_then(|buffer| buffer.upgrade()) + .ok_or_else(|| anyhow!("unknown buffer id {}", buffer_id))?; + anyhow::Ok(buffer.read(cx).snapshot()) + })??; + + if let Some(text_edit) = completion.text_edit.as_ref() { + let edit = parse_completion_text_edit(text_edit, &buffer_snapshot); + + if let Some((old_range, mut text_edit_new_text)) = edit { + LineEnding::normalize(&mut text_edit_new_text); + + new_text = text_edit_new_text; + old_start = Some(serialize_anchor(&old_range.start)); + old_end = Some(serialize_anchor(&old_range.end)); + } + } + } + + Ok(proto::ResolveCompletionDocumentationResponse { + documentation, + documentation_is_markdown, + old_start, + old_end, + new_text, + }) } async fn handle_apply_code_action( diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 5dcf3ad740..15cb0e33c1 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -1024,15 +1024,22 @@ message ResolveState { } } +// This type is used to resolve more than just +// the documentation, but for backwards-compatibility +// reasons we can't rename the type. message ResolveCompletionDocumentation { uint64 project_id = 1; uint64 language_server_id = 2; bytes lsp_completion = 3; + uint64 buffer_id = 4; } message ResolveCompletionDocumentationResponse { - string text = 1; - bool is_markdown = 2; + string documentation = 1; + bool documentation_is_markdown = 2; + Anchor old_start = 3; + Anchor old_end = 4; + string new_text = 5; } message ResolveInlayHint {