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!

Author: matklad

crates/rust-analyzer/src/dispatch.rs

@@ -28,17 +28,16 @@ impl<'a> RequestDispatcher<'a> {
     {
         let (id, params) = match self.parse::<R>() {
             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::<R>(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,17 +46,15 @@ impl<'a> RequestDispatcher<'a> {
     pub(crate) fn on<R>(
         &mut self,
         f: fn(GlobalStateSnapshot, R::Params) -> Result<R::Result>,
-    ) -> Result<&mut Self>
+    ) -> &mut Self
     where
         R: lsp_types::request::Request + 'static,
         R::Params: DeserializeOwned + Send + fmt::Debug + 'static,
         R::Result: Serialize + 'static,
     {
         let (id, params) = match self.parse::<R>() {
             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::Params>(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))
+        }
     }
 }
 

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<T, E = Box<dyn std::error::Error + Send + Sync>> = std::result::Result<T, E>;
-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<dyn std::error::Error + Send + Sync>;
+pub type Result<T, E = Error> = std::result::Result<T, E>;
+
 pub fn from_json<T: DeserializeOwned>(what: &'static str, json: serde_json::Value) -> Result<T> {
     let res = T::deserialize(&json)
         .map_err(|e| format!("Failed to deserialize {}: {}; {}", what, e, json))?;

crates/rust-analyzer/src/main_loop.rs

@@ -403,53 +403,49 @@ impl GlobalState {
                 handlers::handle_matching_brace(s.snapshot(), p)
             })?
             .on_sync::<lsp_ext::MemoryUsage>(|s, p| handlers::handle_memory_usage(s, p))?
-            .on::<lsp_ext::AnalyzerStatus>(handlers::handle_analyzer_status)?
-            .on::<lsp_ext::SyntaxTree>(handlers::handle_syntax_tree)?
-            .on::<lsp_ext::ExpandMacro>(handlers::handle_expand_macro)?
-            .on::<lsp_ext::ParentModule>(handlers::handle_parent_module)?
-            .on::<lsp_ext::Runnables>(handlers::handle_runnables)?
-            .on::<lsp_ext::InlayHints>(handlers::handle_inlay_hints)?
-            .on::<lsp_ext::CodeActionRequest>(handlers::handle_code_action)?
-            .on::<lsp_ext::ResolveCodeActionRequest>(handlers::handle_resolve_code_action)?
-            .on::<lsp_ext::HoverRequest>(handlers::handle_hover)?
-            .on::<lsp_ext::ExternalDocs>(handlers::handle_open_docs)?
-            .on::<lsp_types::request::OnTypeFormatting>(handlers::handle_on_type_formatting)?
-            .on::<lsp_types::request::DocumentSymbolRequest>(handlers::handle_document_symbol)?
-            .on::<lsp_types::request::WorkspaceSymbol>(handlers::handle_workspace_symbol)?
-            .on::<lsp_types::request::GotoDefinition>(handlers::handle_goto_definition)?
-            .on::<lsp_types::request::GotoImplementation>(handlers::handle_goto_implementation)?
-            .on::<lsp_types::request::GotoTypeDefinition>(handlers::handle_goto_type_definition)?
-            .on::<lsp_types::request::Completion>(handlers::handle_completion)?
-            .on::<lsp_types::request::CodeLensRequest>(handlers::handle_code_lens)?
-            .on::<lsp_types::request::CodeLensResolve>(handlers::handle_code_lens_resolve)?
-            .on::<lsp_types::request::FoldingRangeRequest>(handlers::handle_folding_range)?
-            .on::<lsp_types::request::SignatureHelpRequest>(handlers::handle_signature_help)?
-            .on::<lsp_types::request::PrepareRenameRequest>(handlers::handle_prepare_rename)?
-            .on::<lsp_types::request::Rename>(handlers::handle_rename)?
-            .on::<lsp_types::request::References>(handlers::handle_references)?
-            .on::<lsp_types::request::Formatting>(handlers::handle_formatting)?
-            .on::<lsp_types::request::DocumentHighlightRequest>(
-                handlers::handle_document_highlight,
-            )?
-            .on::<lsp_types::request::CallHierarchyPrepare>(
-                handlers::handle_call_hierarchy_prepare,
-            )?
+            .on::<lsp_ext::AnalyzerStatus>(handlers::handle_analyzer_status)
+            .on::<lsp_ext::SyntaxTree>(handlers::handle_syntax_tree)
+            .on::<lsp_ext::ExpandMacro>(handlers::handle_expand_macro)
+            .on::<lsp_ext::ParentModule>(handlers::handle_parent_module)
+            .on::<lsp_ext::Runnables>(handlers::handle_runnables)
+            .on::<lsp_ext::InlayHints>(handlers::handle_inlay_hints)
+            .on::<lsp_ext::CodeActionRequest>(handlers::handle_code_action)
+            .on::<lsp_ext::ResolveCodeActionRequest>(handlers::handle_resolve_code_action)
+            .on::<lsp_ext::HoverRequest>(handlers::handle_hover)
+            .on::<lsp_ext::ExternalDocs>(handlers::handle_open_docs)
+            .on::<lsp_types::request::OnTypeFormatting>(handlers::handle_on_type_formatting)
+            .on::<lsp_types::request::DocumentSymbolRequest>(handlers::handle_document_symbol)
+            .on::<lsp_types::request::WorkspaceSymbol>(handlers::handle_workspace_symbol)
+            .on::<lsp_types::request::GotoDefinition>(handlers::handle_goto_definition)
+            .on::<lsp_types::request::GotoImplementation>(handlers::handle_goto_implementation)
+            .on::<lsp_types::request::GotoTypeDefinition>(handlers::handle_goto_type_definition)
+            .on::<lsp_types::request::Completion>(handlers::handle_completion)
+            .on::<lsp_types::request::CodeLensRequest>(handlers::handle_code_lens)
+            .on::<lsp_types::request::CodeLensResolve>(handlers::handle_code_lens_resolve)
+            .on::<lsp_types::request::FoldingRangeRequest>(handlers::handle_folding_range)
+            .on::<lsp_types::request::SignatureHelpRequest>(handlers::handle_signature_help)
+            .on::<lsp_types::request::PrepareRenameRequest>(handlers::handle_prepare_rename)
+            .on::<lsp_types::request::Rename>(handlers::handle_rename)
+            .on::<lsp_types::request::References>(handlers::handle_references)
+            .on::<lsp_types::request::Formatting>(handlers::handle_formatting)
+            .on::<lsp_types::request::DocumentHighlightRequest>(handlers::handle_document_highlight)
+            .on::<lsp_types::request::CallHierarchyPrepare>(handlers::handle_call_hierarchy_prepare)
             .on::<lsp_types::request::CallHierarchyIncomingCalls>(
                 handlers::handle_call_hierarchy_incoming,
-            )?
+            )
             .on::<lsp_types::request::CallHierarchyOutgoingCalls>(
                 handlers::handle_call_hierarchy_outgoing,
-            )?
+            )
             .on::<lsp_types::request::SemanticTokensFullRequest>(
                 handlers::handle_semantic_tokens_full,
-            )?
+            )
             .on::<lsp_types::request::SemanticTokensFullDeltaRequest>(
                 handlers::handle_semantic_tokens_full_delta,
-            )?
+            )
             .on::<lsp_types::request::SemanticTokensRangeRequest>(
                 handlers::handle_semantic_tokens_range,
-            )?
-            .on::<lsp_ext::Ssr>(handlers::handle_ssr)?
+            )
+            .on::<lsp_ext::Ssr>(handlers::handle_ssr)
             .finish();
         Ok(())
     }