From 7e7a98560e58371690d00e380d79e0bbaaf19d54 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Sat, 22 Mar 2025 14:18:17 -0400 Subject: [PATCH] LSP: Eagerly decode request results in the client Previously the `call` helper (and its related functions) returned a `serde_json::Value` which was then decoded either later in the client (see signature help and hover) or by the client's caller. This led to some unnecessary boilerplate in the client: let resp = self.call::(params); Some(async move { Ok(serde_json::from_value(resp.await?)?) }) and in the caller. It also allowed for mistakes with the types. The workspace symbol request's calling code for example mistakenly decoded a `lsp::WorkspaceSymbolResponse` as `Vec` - one of the untagged enum members (so it parsed successfully) but not the correct type. With this change, the `call` helper eagerly decodes the response to a request as the `lsp::request::Request::Result` trait item. This is similar to the old helper `request` (which has become redundant and has been eliminated) but all work is done within the same async block which avoids some awkward lifetimes. The return types of functions like `Client::text_document_range_inlay_hints` are now more verbose but it is no longer possible to accidentally decode as an incorrect type. Additionally `Client::resolve_code_action` now uses the `call_with_ref` helper to avoid an unnecessary clone. --- helix-lsp/src/client.rs | 114 +++++++----------- helix-term/src/commands.rs | 16 +-- helix-term/src/commands/lsp.rs | 89 ++++++-------- helix-term/src/handlers/completion/request.rs | 1 - helix-view/src/document.rs | 11 +- helix-view/src/editor.rs | 2 +- 6 files changed, 96 insertions(+), 137 deletions(-) diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index bf8a86952..e5a116d73 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -385,23 +385,11 @@ impl Client { self.workspace_folders.lock() } - /// Execute a RPC request on the language server. - async fn request(&self, params: R::Params) -> Result - where - R::Params: serde::Serialize, - R::Result: core::fmt::Debug, // TODO: temporary - { - // a future that resolves into the response - let json = self.call::(params).await?; - let response = serde_json::from_value(json)?; - Ok(response) - } - /// Execute a RPC request on the language server. fn call( &self, params: R::Params, - ) -> impl Future> + ) -> impl Future> where R::Params: serde::Serialize, { @@ -411,7 +399,7 @@ impl Client { fn call_with_ref( &self, params: &R::Params, - ) -> impl Future> + ) -> impl Future> where R::Params: serde::Serialize, { @@ -422,7 +410,7 @@ impl Client { &self, params: &R::Params, timeout_secs: u64, - ) -> impl Future> + ) -> impl Future> where R::Params: serde::Serialize, { @@ -458,6 +446,7 @@ impl Client { .await .map_err(|_| Error::Timeout(id))? // return Timeout .ok_or(Error::StreamClosed)? + .and_then(|value| serde_json::from_value(value).map_err(Into::into)) } } @@ -697,11 +686,11 @@ impl Client { work_done_progress_params: lsp::WorkDoneProgressParams::default(), }; - self.request::(params).await + self.call::(params).await } pub async fn shutdown(&self) -> Result<()> { - self.request::(()).await + self.call::(()).await } pub fn exit(&self) { @@ -746,7 +735,7 @@ impl Client { old_path: &Path, new_path: &Path, is_dir: bool, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.file_operations_intests(); if !capabilities.will_rename.has_interest(old_path, is_dir) { return None; @@ -763,16 +752,10 @@ impl Client { old_uri: url_from_path(old_path)?, new_uri: url_from_path(new_path)?, }]; - let request = self.call_with_timeout::( + Some(self.call_with_timeout::( &lsp::RenameFilesParams { files }, 5, - ); - - Some(async move { - let json = request.await?; - let response: Option = serde_json::from_value(json)?; - Ok(response.unwrap_or_default()) - }) + )) } pub fn did_rename(&self, old_path: &Path, new_path: &Path, is_dir: bool) -> Option<()> { @@ -1016,7 +999,7 @@ impl Client { position: lsp::Position, work_done_token: Option, context: lsp::CompletionContext, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support completion. @@ -1042,14 +1025,13 @@ impl Client { &self, completion_item: &lsp::CompletionItem, ) -> impl Future> { - let res = self.call_with_ref::(completion_item); - async move { Ok(serde_json::from_value(res.await?)?) } + self.call_with_ref::(completion_item) } pub fn resolve_code_action( &self, - code_action: lsp::CodeAction, - ) -> Option>> { + code_action: &lsp::CodeAction, + ) -> Option>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support resolving code actions. @@ -1061,7 +1043,7 @@ impl Client { _ => return None, } - Some(self.call::(code_action)) + Some(self.call_with_ref::(code_action)) } pub fn text_document_signature_help( @@ -1085,8 +1067,7 @@ impl Client { // lsp::SignatureHelpContext }; - let res = self.call::(params); - Some(async move { Ok(serde_json::from_value(res.await?)?) }) + Some(self.call::(params)) } pub fn text_document_range_inlay_hints( @@ -1094,7 +1075,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, range: lsp::Range, work_done_token: Option, - ) -> Option>> { + ) -> Option>>>> { let capabilities = self.capabilities.get().unwrap(); match capabilities.inlay_hint_provider { @@ -1140,8 +1121,7 @@ impl Client { // lsp::SignatureHelpContext }; - let res = self.call::(params); - Some(async move { Ok(serde_json::from_value(res.await?)?) }) + Some(self.call::(params)) } // formatting @@ -1151,7 +1131,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, options: lsp::FormattingOptions, work_done_token: Option, - ) -> Option>>> { + ) -> Option>>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support formatting. @@ -1184,13 +1164,7 @@ impl Client { work_done_progress_params: lsp::WorkDoneProgressParams { work_done_token }, }; - let request = self.call::(params); - - Some(async move { - let json = request.await?; - let response: Option> = serde_json::from_value(json)?; - Ok(response.unwrap_or_default()) - }) + Some(self.call::(params)) } pub fn text_document_range_formatting( @@ -1199,7 +1173,7 @@ impl Client { range: lsp::Range, options: lsp::FormattingOptions, work_done_token: Option, - ) -> Option>>> { + ) -> Option>>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support range formatting. @@ -1215,13 +1189,7 @@ impl Client { work_done_progress_params: lsp::WorkDoneProgressParams { work_done_token }, }; - let request = self.call::(params); - - Some(async move { - let json = request.await?; - let response: Option> = serde_json::from_value(json)?; - Ok(response.unwrap_or_default()) - }) + Some(self.call::(params)) } pub fn text_document_document_highlight( @@ -1229,7 +1197,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, position: lsp::Position, work_done_token: Option, - ) -> Option>> { + ) -> Option>>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support document highlight. @@ -1262,7 +1230,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, position: lsp::Position, work_done_token: Option, - ) -> impl Future> { + ) -> impl Future> { let params = lsp::GotoDefinitionParams { text_document_position_params: lsp::TextDocumentPositionParams { text_document, @@ -1282,7 +1250,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, position: lsp::Position, work_done_token: Option, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support goto-definition. @@ -1303,7 +1271,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, position: lsp::Position, work_done_token: Option, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support goto-declaration. @@ -1328,7 +1296,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, position: lsp::Position, work_done_token: Option, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support goto-type-definition. @@ -1352,7 +1320,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, position: lsp::Position, work_done_token: Option, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support goto-definition. @@ -1377,7 +1345,7 @@ impl Client { position: lsp::Position, include_declaration: bool, work_done_token: Option, - ) -> Option>> { + ) -> Option>>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support goto-reference. @@ -1406,7 +1374,7 @@ impl Client { pub fn document_symbols( &self, text_document: lsp::TextDocumentIdentifier, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support document symbols. @@ -1428,7 +1396,7 @@ impl Client { &self, text_document: lsp::TextDocumentIdentifier, position: lsp::Position, - ) -> Option>> { + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); match capabilities.rename_provider { @@ -1448,7 +1416,10 @@ impl Client { } // empty string to get all symbols - pub fn workspace_symbols(&self, query: String) -> Option>> { + pub fn workspace_symbols( + &self, + query: String, + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support workspace symbols. @@ -1471,7 +1442,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, range: lsp::Range, context: lsp::CodeActionContext, - ) -> Option>> { + ) -> Option>>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the server does not support code actions. @@ -1499,7 +1470,7 @@ impl Client { text_document: lsp::TextDocumentIdentifier, position: lsp::Position, new_name: String, - ) -> Option>> { + ) -> Option>>> { if !self.supports_feature(LanguageServerFeature::RenameSymbol) { return None; } @@ -1515,16 +1486,13 @@ impl Client { }, }; - let request = self.call::(params); - - Some(async move { - let json = request.await?; - let response: Option = serde_json::from_value(json)?; - Ok(response.unwrap_or_default()) - }) + Some(self.call::(params)) } - pub fn command(&self, command: lsp::Command) -> Option>> { + pub fn command( + &self, + command: lsp::Command, + ) -> Option>>> { let capabilities = self.capabilities.get().unwrap(); // Return early if the language server does not support executing commands. diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index a197792ef..2e15dcdcc 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -146,10 +146,10 @@ impl Context<'_> { #[inline] pub fn callback( &mut self, - call: impl Future> + 'static + Send, + call: impl Future> + 'static + Send, callback: F, ) where - T: for<'de> serde::Deserialize<'de> + Send + 'static, + T: Send + 'static, F: FnOnce(&mut Editor, &mut Compositor, T) + Send + 'static, { self.jobs.callback(make_job_callback(call, callback)); @@ -175,16 +175,15 @@ impl Context<'_> { #[inline] fn make_job_callback( - call: impl Future> + 'static + Send, + call: impl Future> + 'static + Send, callback: F, ) -> std::pin::Pin>>> where - T: for<'de> serde::Deserialize<'de> + Send + 'static, + T: Send + 'static, F: FnOnce(&mut Editor, &mut Compositor, T) + Send + 'static, { Box::pin(async move { - let json = call.await?; - let response = serde_json::from_value(json)?; + let response = call.await?; let call: job::Callback = Callback::EditorCompositor(Box::new( move |editor: &mut Editor, compositor: &mut Compositor| { callback(editor, compositor, response) @@ -4905,7 +4904,10 @@ fn format_selections(cx: &mut Context) { ) .unwrap(); - let edits = tokio::task::block_in_place(|| helix_lsp::block_on(future)).unwrap_or_default(); + let edits = tokio::task::block_in_place(|| helix_lsp::block_on(future)) + .ok() + .flatten() + .unwrap_or_default(); let transaction = helix_lsp::util::generate_transaction_from_edits(doc.text(), edits, offset_encoding); diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 6d6b9744e..8377f7c71 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -343,9 +343,7 @@ pub fn symbol_picker(cx: &mut Context) { .expect("docs with active language servers must be backed by paths"); async move { - let json = request.await?; - let response: Option = serde_json::from_value(json)?; - let symbols = match response { + let symbols = match request.await? { Some(symbols) => symbols, None => return anyhow::Ok(vec![]), }; @@ -461,30 +459,34 @@ pub fn workspace_symbol_picker(cx: &mut Context) { .unwrap(); let offset_encoding = language_server.offset_encoding(); async move { - let json = request.await?; + let symbols = request + .await? + .and_then(|resp| match resp { + lsp::WorkspaceSymbolResponse::Flat(symbols) => Some(symbols), + lsp::WorkspaceSymbolResponse::Nested(_) => None, + }) + .unwrap_or_default(); - let response: Vec<_> = - serde_json::from_value::>>(json)? - .unwrap_or_default() - .into_iter() - .filter_map(|symbol| { - let uri = match Uri::try_from(&symbol.location.uri) { - Ok(uri) => uri, - Err(err) => { - log::warn!("discarding symbol with invalid URI: {err}"); - return None; - } - }; - Some(SymbolInformationItem { - location: Location { - uri, - range: symbol.location.range, - offset_encoding, - }, - symbol, - }) + let response: Vec<_> = symbols + .into_iter() + .filter_map(|symbol| { + let uri = match Uri::try_from(&symbol.location.uri) { + Ok(uri) => uri, + Err(err) => { + log::warn!("discarding symbol with invalid URI: {err}"); + return None; + } + }; + Some(SymbolInformationItem { + location: Location { + uri, + range: symbol.location.range, + offset_encoding, + }, + symbol, }) - .collect(); + }) + .collect(); anyhow::Ok(response) } @@ -676,11 +678,8 @@ pub fn code_action(cx: &mut Context) { Some((code_action_request, language_server_id)) }) .map(|(request, ls_id)| async move { - let json = request.await?; - let response: Option = serde_json::from_value(json)?; - let mut actions = match response { - Some(a) => a, - None => return anyhow::Ok(Vec::new()), + let Some(mut actions) = request.await? else { + return anyhow::Ok(Vec::new()); }; // remove disabled code actions @@ -782,15 +781,9 @@ pub fn code_action(cx: &mut Context) { // we support lsp "codeAction/resolve" for `edit` and `command` fields let mut resolved_code_action = None; if code_action.edit.is_none() || code_action.command.is_none() { - if let Some(future) = - language_server.resolve_code_action(code_action.clone()) - { - if let Ok(response) = helix_lsp::block_on(future) { - if let Ok(code_action) = - serde_json::from_value::(response) - { - resolved_code_action = Some(code_action); - } + if let Some(future) = language_server.resolve_code_action(code_action) { + if let Ok(code_action) = helix_lsp::block_on(future) { + resolved_code_action = Some(code_action); } } } @@ -882,7 +875,7 @@ fn goto_impl(editor: &mut Editor, compositor: &mut Compositor, locations: Vec(cx: &mut Context, feature: LanguageServerFeature, request_provider: P) where P: Fn(&Client, lsp::Position, lsp::TextDocumentIdentifier) -> Option, - F: Future> + 'static + Send, + F: Future>> + 'static + Send, { let (view, doc) = current_ref!(cx.editor); let mut futures: FuturesOrdered<_> = doc @@ -891,11 +884,7 @@ where let offset_encoding = language_server.offset_encoding(); let pos = doc.position(view.id, offset_encoding); let future = request_provider(language_server, pos, doc.identifier()).unwrap(); - async move { - let json = future.await?; - let response: Option = serde_json::from_value(json)?; - anyhow::Ok((response, offset_encoding)) - } + async move { anyhow::Ok((future.await?, offset_encoding)) } }) .collect(); @@ -992,11 +981,7 @@ pub fn goto_reference(cx: &mut Context) { None, ) .unwrap(); - async move { - let json = future.await?; - let locations: Option> = serde_json::from_value(json)?; - anyhow::Ok((locations, offset_encoding)) - } + async move { anyhow::Ok((future.await?, offset_encoding)) } }) .collect(); @@ -1158,7 +1143,9 @@ pub fn rename_symbol(cx: &mut Context) { match block_on(future) { Ok(edits) => { - let _ = cx.editor.apply_workspace_edit(offset_encoding, &edits); + let _ = cx + .editor + .apply_workspace_edit(offset_encoding, &edits.unwrap_or_default()); } Err(err) => cx.editor.set_error(err.to_string()), } diff --git a/helix-term/src/handlers/completion/request.rs b/helix-term/src/handlers/completion/request.rs index 3d2a158ea..26f252a4a 100644 --- a/helix-term/src/handlers/completion/request.rs +++ b/helix-term/src/handlers/completion/request.rs @@ -303,7 +303,6 @@ fn request_completions_from_language_server( async move { let response: Option = completion_response .await - .and_then(|json| serde_json::from_value(json).map_err(helix_lsp::Error::Parse)) .inspect_err(|err| log::error!("completion request failed: {err}")) .ok() .flatten(); diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index 9b3b45486..b75aebe70 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -867,10 +867,13 @@ impl Document { )?; let fut = async move { - let edits = request.await.unwrap_or_else(|e| { - log::warn!("LSP formatting failed: {}", e); - Default::default() - }); + let edits = request + .await + .unwrap_or_else(|e| { + log::warn!("LSP formatting failed: {}", e); + Default::default() + }) + .unwrap_or_default(); Ok(helix_lsp::util::generate_transaction_from_edits( &text, edits, diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 723fb1392..65976f2c1 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -1410,7 +1410,7 @@ impl Editor { continue; }; let edit = match helix_lsp::block_on(request) { - Ok(edit) => edit, + Ok(edit) => edit.unwrap_or_default(), Err(err) => { log::error!("invalid willRename response: {err:?}"); continue;