From 3b9548e163ed64d21d56873a4ba3583bf2f44535 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 30 Oct 2020 19:38:29 +0100 Subject: Respond with JSON-RPC error if we failed to deserialize request Historically, we intentinally violated JSON-RPC spec here by hard crashing. The idea was to poke both the clients and servers to fix stuff. However, this is confusing for server implementors, and falls down in one important place -- protocol extension are not always backwards compatible, which causes crashes simply due to version mismatch. We had once such case with our own extension, and one for semantic tokens. So let's be less adventerous and just err on the err side! --- crates/rust-analyzer/src/dispatch.rs | 40 +++++++++++--------- crates/rust-analyzer/src/lib.rs | 10 +++-- crates/rust-analyzer/src/main_loop.rs | 70 +++++++++++++++++------------------ 3 files changed, 62 insertions(+), 58 deletions(-) diff --git a/crates/rust-analyzer/src/dispatch.rs b/crates/rust-analyzer/src/dispatch.rs index 7a87515e9..81b85a269 100644 --- a/crates/rust-analyzer/src/dispatch.rs +++ b/crates/rust-analyzer/src/dispatch.rs @@ -28,17 +28,16 @@ impl<'a> RequestDispatcher<'a> { { let (id, params) = match self.parse::() { Some(it) => it, - None => { - return Ok(self); - } + None => return Ok(self), }; let world = panic::AssertUnwindSafe(&mut *self.global_state); + let response = panic::catch_unwind(move || { let _pctx = stdx::panic_context::enter(format!("request: {} {:#?}", R::METHOD, params)); let result = f(world.0, params); result_to_response::(id, result) }) - .map_err(|_| format!("sync task {:?} panicked", R::METHOD))?; + .map_err(|_err| format!("sync task {:?} panicked", R::METHOD))?; self.global_state.respond(response); Ok(self) } @@ -47,7 +46,7 @@ impl<'a> RequestDispatcher<'a> { pub(crate) fn on( &mut self, f: fn(GlobalStateSnapshot, R::Params) -> Result, - ) -> Result<&mut Self> + ) -> &mut Self where R: lsp_types::request::Request + 'static, R::Params: DeserializeOwned + Send + fmt::Debug + 'static, @@ -55,9 +54,7 @@ impl<'a> RequestDispatcher<'a> { { let (id, params) = match self.parse::() { Some(it) => it, - None => { - return Ok(self); - } + None => return self, }; self.global_state.task_pool.handle.spawn({ @@ -71,7 +68,7 @@ impl<'a> RequestDispatcher<'a> { } }); - Ok(self) + self } pub(crate) fn finish(&mut self) { @@ -82,7 +79,7 @@ impl<'a> RequestDispatcher<'a> { lsp_server::ErrorCode::MethodNotFound as i32, "unknown request".to_string(), ); - self.global_state.respond(response) + self.global_state.respond(response); } } @@ -91,15 +88,24 @@ impl<'a> RequestDispatcher<'a> { R: lsp_types::request::Request + 'static, R::Params: DeserializeOwned + 'static, { - let req = self.req.take()?; - let (id, params) = match req.extract::(R::METHOD) { - Ok(it) => it, - Err(req) => { - self.req = Some(req); + let req = match &self.req { + Some(req) if req.method == R::METHOD => self.req.take().unwrap(), + _ => return None, + }; + + let res = crate::from_json(R::METHOD, req.params); + match res { + Ok(params) => return Some((req.id, params)), + Err(err) => { + let response = lsp_server::Response::new_err( + req.id, + lsp_server::ErrorCode::InvalidParams as i32, + err.to_string(), + ); + self.global_state.respond(response); return None; } - }; - Some((id, params)) + } } } diff --git a/crates/rust-analyzer/src/lib.rs b/crates/rust-analyzer/src/lib.rs index 87f72b497..ad08f1afb 100644 --- a/crates/rust-analyzer/src/lib.rs +++ b/crates/rust-analyzer/src/lib.rs @@ -37,14 +37,16 @@ mod document; pub mod lsp_ext; pub mod config; -use serde::de::DeserializeOwned; - -pub type Result> = std::result::Result; -pub use crate::{caps::server_capabilities, main_loop::main_loop}; use ide::AnalysisHost; +use serde::de::DeserializeOwned; use std::fmt; use vfs::Vfs; +pub use crate::{caps::server_capabilities, main_loop::main_loop}; + +pub type Error = Box; +pub type Result = std::result::Result; + pub fn from_json(what: &'static str, json: serde_json::Value) -> Result { let res = T::deserialize(&json) .map_err(|e| format!("Failed to deserialize {}: {}; {}", what, e, json))?; diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index ff855fe1a..53f8ca194 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/crates/rust-analyzer/src/main_loop.rs @@ -403,53 +403,49 @@ impl GlobalState { handlers::handle_matching_brace(s.snapshot(), p) })? .on_sync::(|s, p| handlers::handle_memory_usage(s, p))? - .on::(handlers::handle_analyzer_status)? - .on::(handlers::handle_syntax_tree)? - .on::(handlers::handle_expand_macro)? - .on::(handlers::handle_parent_module)? - .on::(handlers::handle_runnables)? - .on::(handlers::handle_inlay_hints)? - .on::(handlers::handle_code_action)? - .on::(handlers::handle_resolve_code_action)? - .on::(handlers::handle_hover)? - .on::(handlers::handle_open_docs)? - .on::(handlers::handle_on_type_formatting)? - .on::(handlers::handle_document_symbol)? - .on::(handlers::handle_workspace_symbol)? - .on::(handlers::handle_goto_definition)? - .on::(handlers::handle_goto_implementation)? - .on::(handlers::handle_goto_type_definition)? - .on::(handlers::handle_completion)? - .on::(handlers::handle_code_lens)? - .on::(handlers::handle_code_lens_resolve)? - .on::(handlers::handle_folding_range)? - .on::(handlers::handle_signature_help)? - .on::(handlers::handle_prepare_rename)? - .on::(handlers::handle_rename)? - .on::(handlers::handle_references)? - .on::(handlers::handle_formatting)? - .on::( - handlers::handle_document_highlight, - )? - .on::( - handlers::handle_call_hierarchy_prepare, - )? + .on::(handlers::handle_analyzer_status) + .on::(handlers::handle_syntax_tree) + .on::(handlers::handle_expand_macro) + .on::(handlers::handle_parent_module) + .on::(handlers::handle_runnables) + .on::(handlers::handle_inlay_hints) + .on::(handlers::handle_code_action) + .on::(handlers::handle_resolve_code_action) + .on::(handlers::handle_hover) + .on::(handlers::handle_open_docs) + .on::(handlers::handle_on_type_formatting) + .on::(handlers::handle_document_symbol) + .on::(handlers::handle_workspace_symbol) + .on::(handlers::handle_goto_definition) + .on::(handlers::handle_goto_implementation) + .on::(handlers::handle_goto_type_definition) + .on::(handlers::handle_completion) + .on::(handlers::handle_code_lens) + .on::(handlers::handle_code_lens_resolve) + .on::(handlers::handle_folding_range) + .on::(handlers::handle_signature_help) + .on::(handlers::handle_prepare_rename) + .on::(handlers::handle_rename) + .on::(handlers::handle_references) + .on::(handlers::handle_formatting) + .on::(handlers::handle_document_highlight) + .on::(handlers::handle_call_hierarchy_prepare) .on::( handlers::handle_call_hierarchy_incoming, - )? + ) .on::( handlers::handle_call_hierarchy_outgoing, - )? + ) .on::( handlers::handle_semantic_tokens_full, - )? + ) .on::( handlers::handle_semantic_tokens_full_delta, - )? + ) .on::( handlers::handle_semantic_tokens_range, - )? - .on::(handlers::handle_ssr)? + ) + .on::(handlers::handle_ssr) .finish(); Ok(()) } -- cgit v1.2.3