From 9ce989a704b3b13543472ede2e52efca52764a63 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Sat, 13 Jul 2024 04:06:01 +0300 Subject: [PATCH] Tidy up collab-related signature help data (#14377) Follow-up of https://github.com/zed-industries/zed/pull/12909 * Fully preserve LSP data when sending it via collab, and only strip it on the client. * Avoid extra custom request handlers, and extend multi LSP server query protocol instead. Release Notes: - N/A --- crates/collab/src/rpc.rs | 5 +- crates/project/src/lsp_command.rs | 72 ++----- .../project/src/lsp_command/signature_help.rs | 179 ++++++++++++++---- crates/project/src/project.rs | 135 +++++++------ crates/proto/proto/zed.proto | 45 +++-- 5 files changed, 269 insertions(+), 167 deletions(-) diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 64e0ed2990..42e5c7e94f 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -642,10 +642,7 @@ impl Server { app_state.config.openai_api_key.clone(), ) }) - }) - .add_request_handler(user_handler( - forward_read_only_project_request::, - )); + }); Arc::new(server) } diff --git a/crates/project/src/lsp_command.rs b/crates/project/src/lsp_command.rs index a06af063ac..b81f1acf7a 100644 --- a/crates/project/src/lsp_command.rs +++ b/crates/project/src/lsp_command.rs @@ -10,10 +10,9 @@ use async_trait::async_trait; use client::proto::{self, PeerId}; use clock::Global; use futures::future; -use gpui::{AppContext, AsyncAppContext, FontWeight, Model}; +use gpui::{AppContext, AsyncAppContext, Model}; use language::{ language_settings::{language_settings, InlayHintKind}, - markdown::{MarkdownHighlight, MarkdownHighlightStyle}, point_from_lsp, point_to_lsp, proto::{deserialize_anchor, deserialize_version, serialize_anchor, serialize_version}, range_from_lsp, range_to_lsp, Anchor, Bias, Buffer, BufferSnapshot, CachedLspAdapter, CharKind, @@ -24,6 +23,7 @@ use lsp::{ DocumentHighlightKind, LanguageServer, LanguageServerId, LinkedEditingRangeServerCapabilities, OneOf, ServerCapabilities, }; +use signature_help::{lsp_to_proto_signature, proto_to_lsp_signature}; use std::{cmp::Reverse, ops::Range, path::Path, sync::Arc}; use text::{BufferId, LineEnding}; @@ -1242,7 +1242,7 @@ impl LspCommand for GetDocumentHighlights { #[async_trait(?Send)] impl LspCommand for GetSignatureHelp { - type Response = Vec; + type Response = Option; type LspRequest = lsp::SignatureHelpRequest; type ProtoRequest = proto::GetSignatureHelp; @@ -1283,10 +1283,7 @@ impl LspCommand for GetSignatureHelp { mut cx: AsyncAppContext, ) -> Result { let language = buffer.update(&mut cx, |buffer, _| buffer.language().cloned())?; - Ok(message - .into_iter() - .filter_map(|message| SignatureHelp::new(message, language.clone())) - .collect()) + Ok(message.and_then(|message| SignatureHelp::new(message, language))) } fn to_proto(&self, project_id: u64, buffer: &Buffer) -> Self::ProtoRequest { @@ -1329,34 +1326,8 @@ impl LspCommand for GetSignatureHelp { _: &mut AppContext, ) -> proto::GetSignatureHelpResponse { proto::GetSignatureHelpResponse { - entries: response - .into_iter() - .map(|signature_help| proto::SignatureHelp { - rendered_text: signature_help.markdown, - highlights: signature_help - .highlights - .into_iter() - .filter_map(|(range, highlight)| { - let MarkdownHighlight::Style(highlight) = highlight else { - return None; - }; - - Some(proto::HighlightedRange { - range: Some(proto::Range { - start: range.start as u64, - end: range.end as u64, - }), - highlight: Some(proto::MarkdownHighlight { - italic: highlight.italic, - underline: highlight.underline, - strikethrough: highlight.strikethrough, - weight: highlight.weight.0, - }), - }) - }) - .collect(), - }) - .collect(), + signature_help: response + .map(|signature_help| lsp_to_proto_signature(signature_help.original_data)), } } @@ -1364,33 +1335,14 @@ impl LspCommand for GetSignatureHelp { self, response: proto::GetSignatureHelpResponse, _: Model, - _: Model, - _: AsyncAppContext, + buffer: Model, + mut cx: AsyncAppContext, ) -> Result { + let language = buffer.update(&mut cx, |buffer, _| buffer.language().cloned())?; Ok(response - .entries - .into_iter() - .map(|proto_entry| SignatureHelp { - markdown: proto_entry.rendered_text, - highlights: proto_entry - .highlights - .into_iter() - .filter_map(|highlight| { - let proto_highlight = highlight.highlight?; - let range = highlight.range?; - Some(( - range.start as usize..range.end as usize, - MarkdownHighlight::Style(MarkdownHighlightStyle { - italic: proto_highlight.italic, - underline: proto_highlight.underline, - strikethrough: proto_highlight.strikethrough, - weight: FontWeight(proto_highlight.weight), - }), - )) - }) - .collect(), - }) - .collect()) + .signature_help + .map(|proto_help| proto_to_lsp_signature(proto_help)) + .and_then(|lsp_help| SignatureHelp::new(lsp_help, language))) } fn buffer_id_from_proto(message: &Self::ProtoRequest) -> Result { diff --git a/crates/project/src/lsp_command/signature_help.rs b/crates/project/src/lsp_command/signature_help.rs index 2d3daaeada..e1f0529a80 100644 --- a/crates/project/src/lsp_command/signature_help.rs +++ b/crates/project/src/lsp_command/signature_help.rs @@ -5,6 +5,7 @@ use language::{ markdown::{MarkdownHighlight, MarkdownHighlightStyle}, Language, }; +use rpc::proto::{self, documentation}; pub const SIGNATURE_HELP_HIGHLIGHT_CURRENT: MarkdownHighlight = MarkdownHighlight::Style(MarkdownHighlightStyle { @@ -26,38 +27,31 @@ pub const SIGNATURE_HELP_HIGHLIGHT_OVERLOAD: MarkdownHighlight = pub struct SignatureHelp { pub markdown: String, pub highlights: Vec<(Range, MarkdownHighlight)>, + pub(super) original_data: lsp::SignatureHelp, } impl SignatureHelp { - pub fn new( - lsp::SignatureHelp { - signatures, - active_signature, - active_parameter, - .. - }: lsp::SignatureHelp, - language: Option>, - ) -> Option { - let function_options_count = signatures.len(); + pub fn new(help: lsp::SignatureHelp, language: Option>) -> Option { + let function_options_count = help.signatures.len(); - let signature_information = active_signature - .and_then(|active_signature| signatures.get(active_signature as usize)) - .or_else(|| signatures.first())?; + let signature_information = help + .active_signature + .and_then(|active_signature| help.signatures.get(active_signature as usize)) + .or_else(|| help.signatures.first())?; let str_for_join = ", "; let parameter_length = signature_information .parameters .as_ref() - .map(|parameters| parameters.len()) - .unwrap_or(0); + .map_or(0, |parameters| parameters.len()); let mut highlight_start = 0; let (markdown, mut highlights): (Vec<_>, Vec<_>) = signature_information .parameters .as_ref()? .iter() .enumerate() - .filter_map(|(i, parameter_information)| { - let string = match parameter_information.label.clone() { + .map(|(i, parameter_information)| { + let label = match parameter_information.label.clone() { lsp::ParameterLabel::Simple(string) => string, lsp::ParameterLabel::LabelOffsets(offset) => signature_information .label @@ -66,33 +60,28 @@ impl SignatureHelp { .take((offset[1] - offset[0]) as usize) .collect::(), }; - let string_length = string.len(); + let label_length = label.len(); - let result = if let Some(active_parameter) = active_parameter { + let highlights = help.active_parameter.and_then(|active_parameter| { if i == active_parameter as usize { Some(( - string, - Some(( - highlight_start..(highlight_start + string_length), - SIGNATURE_HELP_HIGHLIGHT_CURRENT, - )), + highlight_start..(highlight_start + label_length), + SIGNATURE_HELP_HIGHLIGHT_CURRENT, )) } else { - Some((string, None)) + None } - } else { - Some((string, None)) - }; + }); if i != parameter_length { - highlight_start += string_length + str_for_join.len(); + highlight_start += label_length + str_for_join.len(); } - result + (label, highlights) }) .unzip(); - let result = if markdown.is_empty() { + if markdown.is_empty() { None } else { let markdown = markdown.join(str_for_join); @@ -112,16 +101,130 @@ impl SignatureHelp { format!("```{language_name}\n{markdown}") }; - Some((markdown, highlights.into_iter().flatten().collect())) - }; - - result.map(|(markdown, highlights)| Self { - markdown, - highlights, - }) + Some(Self { + markdown, + highlights: highlights.into_iter().flatten().collect(), + original_data: help, + }) + } } } +pub fn lsp_to_proto_signature(lsp_help: lsp::SignatureHelp) -> proto::SignatureHelp { + proto::SignatureHelp { + signatures: lsp_help + .signatures + .into_iter() + .map(|signature| proto::SignatureInformation { + label: signature.label, + documentation: signature + .documentation + .map(|documentation| lsp_to_proto_documentation(documentation)), + parameters: signature + .parameters + .unwrap_or_default() + .into_iter() + .map(|parameter_info| proto::ParameterInformation { + label: Some(match parameter_info.label { + lsp::ParameterLabel::Simple(label) => { + proto::parameter_information::Label::Simple(label) + } + lsp::ParameterLabel::LabelOffsets(offsets) => { + proto::parameter_information::Label::LabelOffsets( + proto::LabelOffsets { + start: offsets[0], + end: offsets[1], + }, + ) + } + }), + documentation: parameter_info.documentation.map(lsp_to_proto_documentation), + }) + .collect(), + active_parameter: signature.active_parameter, + }) + .collect(), + active_signature: lsp_help.active_signature, + active_parameter: lsp_help.active_parameter, + } +} + +fn lsp_to_proto_documentation(documentation: lsp::Documentation) -> proto::Documentation { + proto::Documentation { + content: Some(match documentation { + lsp::Documentation::String(string) => proto::documentation::Content::Value(string), + lsp::Documentation::MarkupContent(content) => { + proto::documentation::Content::MarkupContent(proto::MarkupContent { + is_markdown: matches!(content.kind, lsp::MarkupKind::Markdown), + value: content.value, + }) + } + }), + } +} + +pub fn proto_to_lsp_signature(proto_help: proto::SignatureHelp) -> lsp::SignatureHelp { + lsp::SignatureHelp { + signatures: proto_help + .signatures + .into_iter() + .map(|signature| lsp::SignatureInformation { + label: signature.label, + documentation: signature.documentation.and_then(proto_to_lsp_documentation), + parameters: Some( + signature + .parameters + .into_iter() + .filter_map(|parameter_info| { + Some(lsp::ParameterInformation { + label: match parameter_info.label? { + proto::parameter_information::Label::Simple(string) => { + lsp::ParameterLabel::Simple(string) + } + proto::parameter_information::Label::LabelOffsets(offsets) => { + lsp::ParameterLabel::LabelOffsets([ + offsets.start, + offsets.end, + ]) + } + }, + documentation: parameter_info + .documentation + .and_then(proto_to_lsp_documentation), + }) + }) + .collect(), + ), + active_parameter: signature.active_parameter, + }) + .collect(), + active_signature: proto_help.active_signature, + active_parameter: proto_help.active_parameter, + } +} + +fn proto_to_lsp_documentation(documentation: proto::Documentation) -> Option { + let documentation = { + Some(match documentation.content? { + documentation::Content::Value(string) => lsp::Documentation::String(string), + documentation::Content::MarkupContent(markup) => { + lsp::Documentation::MarkupContent(if markup.is_markdown { + lsp::MarkupContent { + kind: lsp::MarkupKind::Markdown, + value: markup.value, + } + } else { + lsp::MarkupContent { + kind: lsp::MarkupKind::PlainText, + value: markup.value, + } + }) + } + }) + }; + documentation +} + #[cfg(test)] mod tests { use crate::lsp_command::signature_help::{ diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 69c31c8929..c3d30b9ecf 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -695,7 +695,6 @@ impl Project { client.add_model_request_handler(Self::handle_task_context_for_location); client.add_model_request_handler(Self::handle_task_templates); client.add_model_request_handler(Self::handle_lsp_command::); - client.add_model_request_handler(Self::handle_signature_help); } pub fn local( @@ -5464,33 +5463,52 @@ impl Project { .collect::>() }) } else if let Some(project_id) = self.remote_id() { - let position_anchor = buffer - .read(cx) - .anchor_at(buffer.read(cx).point_utf16_to_offset(position), Bias::Right); - let request = self.client.request(proto::GetSignatureHelp { - project_id, - position: Some(serialize_anchor(&position_anchor)), - buffer_id: buffer.read(cx).remote_id().to_proto(), + let request_task = self.client().request(proto::MultiLspQuery { + buffer_id: buffer.read(cx).remote_id().into(), version: serialize_version(&buffer.read(cx).version()), + project_id, + strategy: Some(proto::multi_lsp_query::Strategy::All( + proto::AllLanguageServers {}, + )), + request: Some(proto::multi_lsp_query::Request::GetSignatureHelp( + GetSignatureHelp { position }.to_proto(project_id, buffer.read(cx)), + )), }); let buffer = buffer.clone(); - cx.spawn(move |project, cx| async move { - let Some(response) = request.await.log_err() else { + cx.spawn(|weak_project, cx| async move { + let Some(project) = weak_project.upgrade() else { return Vec::new(); }; - let Some(project) = project.upgrade() else { - return Vec::new(); - }; - GetSignatureHelp::response_from_proto( - GetSignatureHelp { position }, - response, - project, - buffer, - cx, + join_all( + request_task + .await + .log_err() + .map(|response| response.responses) + .unwrap_or_default() + .into_iter() + .filter_map(|lsp_response| match lsp_response.response? { + proto::lsp_response::Response::GetSignatureHelpResponse(response) => { + Some(response) + } + unexpected => { + debug_panic!("Unexpected response: {unexpected:?}"); + None + } + }) + .map(|signature_response| { + let response = GetSignatureHelp { position }.response_from_proto( + signature_response, + project.clone(), + buffer.clone(), + cx.clone(), + ); + async move { response.await.log_err().flatten() } + }), ) .await - .log_err() - .unwrap_or_default() + .into_iter() + .flatten() + .collect() }) } else { Task::ready(Vec::new()) @@ -8356,6 +8374,48 @@ impl Project { .collect(), }) } + Some(proto::multi_lsp_query::Request::GetSignatureHelp(get_signature_help)) => { + let get_signature_help = GetSignatureHelp::from_proto( + get_signature_help, + project.clone(), + buffer.clone(), + cx.clone(), + ) + .await?; + + let all_signatures = project + .update(&mut cx, |project, cx| { + project.request_multiple_lsp_locally( + &buffer, + Some(get_signature_help.position), + |server_capabilities| { + server_capabilities.signature_help_provider.is_some() + }, + get_signature_help, + cx, + ) + })? + .await + .into_iter(); + + project.update(&mut cx, |project, cx| proto::MultiLspQueryResponse { + responses: all_signatures + .map(|signature_help| proto::LspResponse { + response: Some( + proto::lsp_response::Response::GetSignatureHelpResponse( + GetSignatureHelp::response_to_proto( + signature_help, + project, + sender_id, + &buffer_version, + cx, + ), + ), + ), + }) + .collect(), + }) + } None => anyhow::bail!("empty multi lsp query request"), } } @@ -9322,39 +9382,6 @@ impl Project { Ok(proto::TaskTemplatesResponse { templates }) } - async fn handle_signature_help( - project: Model, - envelope: TypedEnvelope, - mut cx: AsyncAppContext, - ) -> Result { - let sender_id = envelope.original_sender_id()?; - let buffer_id = BufferId::new(envelope.payload.buffer_id)?; - let buffer = project.update(&mut cx, |project, cx| { - project.buffer_store.read(cx).get_existing(buffer_id) - })??; - let response = GetSignatureHelp::from_proto( - envelope.payload.clone(), - project.clone(), - buffer.clone(), - cx.clone(), - ) - .await?; - let help_response = project - .update(&mut cx, |project, cx| { - project.signature_help(&buffer, response.position, cx) - })? - .await; - project.update(&mut cx, |project, cx| { - GetSignatureHelp::response_to_proto( - help_response, - project, - sender_id, - &buffer.read(cx).version(), - cx, - ) - }) - } - async fn try_resolve_code_action( lang_server: &LanguageServer, action: &mut CodeAction, diff --git a/crates/proto/proto/zed.proto b/crates/proto/proto/zed.proto index ec23aaccbb..ebf98983f9 100644 --- a/crates/proto/proto/zed.proto +++ b/crates/proto/proto/zed.proto @@ -945,24 +945,45 @@ message GetSignatureHelp { } message GetSignatureHelpResponse { - repeated SignatureHelp entries = 1; + optional SignatureHelp signature_help = 1; } message SignatureHelp { - string rendered_text = 1; - repeated HighlightedRange highlights = 2; + repeated SignatureInformation signatures = 1; + optional uint32 active_signature = 2; + optional uint32 active_parameter = 3; } -message HighlightedRange { - Range range = 1; - MarkdownHighlight highlight = 2; +message SignatureInformation { + string label = 1; + optional Documentation documentation = 2; + repeated ParameterInformation parameters = 3; + optional uint32 active_parameter = 4; } -message MarkdownHighlight { - bool italic = 1; - bool underline = 2; - bool strikethrough = 3; - float weight = 4; +message Documentation { + oneof content { + string value = 1; + MarkupContent markup_content = 2; + } +} + +enum MarkupKind { + PlainText = 0; + Markdown = 1; +} + +message ParameterInformation { + oneof label { + string simple = 1; + LabelOffsets label_offsets = 2; + } + optional Documentation documentation = 3; +} + +message LabelOffsets { + uint32 start = 1; + uint32 end = 2; } message GetHover { @@ -2166,6 +2187,7 @@ message MultiLspQuery { oneof request { GetHover get_hover = 5; GetCodeActions get_code_actions = 6; + GetSignatureHelp get_signature_help = 7; } } @@ -2184,6 +2206,7 @@ message LspResponse { oneof response { GetHoverResponse get_hover_response = 1; GetCodeActionsResponse get_code_actions_response = 2; + GetSignatureHelpResponse get_signature_help_response = 3; } }