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

@@ -642,10 +642,7 @@ impl Server {
                         app_state.config.openai_api_key.clone(),
                     )
                 })
-            })
+            });
-            .add_request_handler(user_handler(
-                forward_read_only_project_request::<proto::GetSignatureHelp>,
-            ));
 
         Arc::new(server)
     }

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<SignatureHelp>;
+    type Response = Option<SignatureHelp>;
     type LspRequest = lsp::SignatureHelpRequest;
     type ProtoRequest = proto::GetSignatureHelp;
 
@@ -1283,10 +1283,7 @@ impl LspCommand for GetSignatureHelp {
         mut cx: AsyncAppContext,
     ) -> Result<Self::Response> {
         let language = buffer.update(&mut cx, |buffer, _| buffer.language().cloned())?;
-        Ok(message
+        Ok(message.and_then(|message| SignatureHelp::new(message, language)))
-            .into_iter()
-            .filter_map(|message| SignatureHelp::new(message, language.clone()))
-            .collect())
     }
 
     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
+            signature_help: response
-                .into_iter()
+                .map(|signature_help| lsp_to_proto_signature(signature_help.original_data)),
-                .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(),
         }
     }
 
@@ -1364,33 +1335,14 @@ impl LspCommand for GetSignatureHelp {
         self,
         response: proto::GetSignatureHelpResponse,
         _: Model<Project>,
-        _: Model<Buffer>,
+        buffer: Model<Buffer>,
-        _: AsyncAppContext,
+        mut cx: AsyncAppContext,
     ) -> Result<Self::Response> {
+        let language = buffer.update(&mut cx, |buffer, _| buffer.language().cloned())?;
         Ok(response
-            .entries
+            .signature_help
-            .into_iter()
+            .map(|proto_help| proto_to_lsp_signature(proto_help))
-            .map(|proto_entry| SignatureHelp {
+            .and_then(|lsp_help| SignatureHelp::new(lsp_help, language)))
-                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())
     }
 
     fn buffer_id_from_proto(message: &Self::ProtoRequest) -> Result<BufferId> {

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<usize>, MarkdownHighlight)>,
+    pub(super) original_data: lsp::SignatureHelp,
 }
 
 impl SignatureHelp {
-    pub fn new(
+    pub fn new(help: lsp::SignatureHelp, language: Option<Arc<Language>>) -> Option<Self> {
-        lsp::SignatureHelp {
+        let function_options_count = help.signatures.len();
-            signatures,
-            active_signature,
-            active_parameter,
-            ..
-        }: lsp::SignatureHelp,
-        language: Option<Arc<Language>>,
-    ) -> Option<Self> {
-        let function_options_count = signatures.len();
 
-        let signature_information = active_signature
+        let signature_information = help
-            .and_then(|active_signature| signatures.get(active_signature as usize))
+            .active_signature
-            .or_else(|| signatures.first())?;
+            .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())
+            .map_or(0, |parameters| parameters.len());
-            .unwrap_or(0);
         let mut highlight_start = 0;
         let (markdown, mut highlights): (Vec<_>, Vec<_>) = signature_information
             .parameters
             .as_ref()?
             .iter()
             .enumerate()
-            .filter_map(|(i, parameter_information)| {
+            .map(|(i, parameter_information)| {
-                let string = match parameter_information.label.clone() {
+                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::<String>(),
                 };
-                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,
+                            highlight_start..(highlight_start + label_length),
-                            Some((
+                            SIGNATURE_HELP_HIGHLIGHT_CURRENT,
-                                highlight_start..(highlight_start + string_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()))
+            Some(Self {
-        };
+                markdown,
-
+                highlights: highlights.into_iter().flatten().collect(),
-        result.map(|(markdown, highlights)| Self {
+                original_data: help,
-            markdown,
+            })
-            highlights,
+        }
-        })
     }
 }
 
+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<lsp::Documentation> {
+    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::{

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::<LinkedEditingRange>);
-        client.add_model_request_handler(Self::handle_signature_help);
     }
 
     pub fn local(
@@ -5464,33 +5463,52 @@ impl Project {
                     .collect::<Vec<_>>()
             })
         } else if let Some(project_id) = self.remote_id() {
-            let position_anchor = buffer
+            let request_task = self.client().request(proto::MultiLspQuery {
-                .read(cx)
+                buffer_id: buffer.read(cx).remote_id().into(),
-                .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(),
                 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 {
+            cx.spawn(|weak_project, cx| async move {
-                let Some(response) = request.await.log_err() else {
+                let Some(project) = weak_project.upgrade() else {
                     return Vec::new();
                 };
-                let Some(project) = project.upgrade() else {
+                join_all(
-                    return Vec::new();
+                    request_task
-                };
+                        .await
-                GetSignatureHelp::response_from_proto(
+                        .log_err()
-                    GetSignatureHelp { position },
+                        .map(|response| response.responses)
-                    response,
+                        .unwrap_or_default()
-                    project,
+                        .into_iter()
-                    buffer,
+                        .filter_map(|lsp_response| match lsp_response.response? {
-                    cx,
+                            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()
+                .into_iter()
-                .unwrap_or_default()
+                .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<Self>,
-        envelope: TypedEnvelope<proto::GetSignatureHelp>,
-        mut cx: AsyncAppContext,
-    ) -> Result<proto::GetSignatureHelpResponse> {
-        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,

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 SignatureInformation signatures = 1;
-    repeated HighlightedRange highlights = 2;
+    optional uint32 active_signature = 2;
+    optional uint32 active_parameter = 3;
 }
 
-message HighlightedRange {
+message SignatureInformation {
-    Range range = 1;
+    string label = 1;
-    MarkdownHighlight highlight = 2;
+    optional Documentation documentation = 2;
+    repeated ParameterInformation parameters = 3;
+    optional uint32 active_parameter = 4;
 }
 
-message MarkdownHighlight {
+message Documentation {
-    bool italic = 1;
+    oneof content {
-    bool underline = 2;
+        string value = 1;
-    bool strikethrough = 3;
+        MarkupContent markup_content = 2;
-    float weight = 4;
+    }
+}
+
+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;
     }
 }