internal: don't shut up the compiler when it says the code's buggy

Wrapping state-modifying functions into a `catch_unwind` is wrong -- to
do retry after failure, you need to have transactional semantics!

Author: matklad

crates/rust-analyzer/src/dispatch.rs

@@ -1,4 +1,4 @@
-//! A visitor for downcasting arbitrary request (JSON) into a specific type.
+//! See [RequestDispatcher].
 use std::{fmt, panic, thread};
 
 use serde::{de::DeserializeOwned, Serialize};
@@ -10,14 +10,30 @@ use crate::{
     LspError, Result,
 };
 
+/// A visitor for routing a raw JSON request to an appropriate handler function.
+///
+/// Most requests are read-only and async and are handled on the threadpool
+/// (`on` method).
+///
+/// Some read-only requests are latency sensitive, and are immediately handled
+/// on the main loop thread (`on_sync`). These are typically typing-related
+/// requests.
+///
+/// Some requests modify the state, and are run on the main thread to get
+/// `&mut` (`on_sync_mut`).
+///
+/// Read-only requests are wrapped into `catch_unwind` -- they don't modify the
+/// state, so it's OK to recover from their failures.
 pub(crate) struct RequestDispatcher<'a> {
     pub(crate) req: Option<lsp_server::Request>,
     pub(crate) global_state: &'a mut GlobalState,
 }
 
 impl<'a> RequestDispatcher<'a> {
-    /// Dispatches the request onto the current thread
-    pub(crate) fn on_sync<R>(
+    /// Dispatches the request onto the current thread, given full access to
+    /// mutable global state. Unlike all other methods here, this one isn't
+    /// guarded by `catch_unwind`, so, please, don't make bugs :-)
+    pub(crate) fn on_sync_mut<R>(
         &mut self,
         f: fn(&mut GlobalState, R::Params) -> Result<R::Result>,
     ) -> Result<&mut Self>
@@ -26,26 +42,40 @@ impl<'a> RequestDispatcher<'a> {
         R::Params: DeserializeOwned + panic::UnwindSafe + fmt::Debug + 'static,
         R::Result: Serialize + 'static,
     {
-        let (id, params) = match self.parse::<R>() {
+        let (id, params, panic_context) = match self.parse::<R>() {
             Some(it) => it,
             None => return Ok(self),
         };
-        let global_state = panic::AssertUnwindSafe(&mut *self.global_state);
+        let _pctx = stdx::panic_context::enter(panic_context);
+
+        let result = f(&mut self.global_state, params);
+        let response = result_to_response::<R>(id, result);
+
+        self.global_state.respond(response);
+        Ok(self)
+    }
+
+    /// Dispatches the request onto the current thread.
+    pub(crate) fn on_sync<R>(
+        &mut self,
+        f: fn(GlobalStateSnapshot, R::Params) -> Result<R::Result>,
+    ) -> Result<&mut Self>
+    where
+        R: lsp_types::request::Request + 'static,
+        R::Params: DeserializeOwned + panic::UnwindSafe + fmt::Debug + 'static,
+        R::Result: Serialize + 'static,
+    {
+        let (id, params, panic_context) = match self.parse::<R>() {
+            Some(it) => it,
+            None => return Ok(self),
+        };
+        let global_state_snapshot = self.global_state.snapshot();
 
         let result = panic::catch_unwind(move || {
-            // Make sure that the whole AssertUnwindSafe is moved into the
-            // closure, and not just its field.
-            let panic::AssertUnwindSafe(global_state) = { global_state };
-
-            let _pctx = stdx::panic_context::enter(format!(
-                "\nversion: {}\nrequest: {} {:#?}",
-                env!("REV"),
-                R::METHOD,
-                params
-            ));
-            f(global_state, params)
+            let _pctx = stdx::panic_context::enter(panic_context);
+            f(global_state_snapshot, params)
         });
-        let response = result_to_response::<R>(id, result);
+        let response = thread_result_to_response::<R>(id, result);
 
         self.global_state.respond(response);
         Ok(self)
@@ -61,7 +91,7 @@ impl<'a> RequestDispatcher<'a> {
         R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug + 'static,
         R::Result: Serialize + 'static,
     {
-        let (id, params) = match self.parse::<R>() {
+        let (id, params, panic_context) = match self.parse::<R>() {
             Some(it) => it,
             None => return self,
         };
@@ -70,15 +100,10 @@ impl<'a> RequestDispatcher<'a> {
             let world = self.global_state.snapshot();
             move || {
                 let result = panic::catch_unwind(move || {
-                    let _pctx = stdx::panic_context::enter(format!(
-                        "\nversion: {}\nrequest: {} {:#?}",
-                        env!("REV"),
-                        R::METHOD,
-                        params
-                    ));
+                    let _pctx = stdx::panic_context::enter(panic_context);
                     f(world, params)
                 });
-                let response = result_to_response::<R>(id, result);
+                let response = thread_result_to_response::<R>(id, result);
                 Task::Response(response)
             }
         });
@@ -98,10 +123,10 @@ impl<'a> RequestDispatcher<'a> {
         }
     }
 
-    fn parse<R>(&mut self) -> Option<(lsp_server::RequestId, R::Params)>
+    fn parse<R>(&mut self) -> Option<(lsp_server::RequestId, R::Params, String)>
     where
         R: lsp_types::request::Request + 'static,
-        R::Params: DeserializeOwned + 'static,
+        R::Params: DeserializeOwned + fmt::Debug + 'static,
     {
         let req = match &self.req {
             Some(req) if req.method == R::METHOD => self.req.take().unwrap(),
@@ -110,7 +135,11 @@ impl<'a> RequestDispatcher<'a> {
 
         let res = crate::from_json(R::METHOD, req.params);
         match res {
-            Ok(params) => Some((req.id, params)),
+            Ok(params) => {
+                let panic_context =
+                    format!("\nversion: {}\nrequest: {} {:#?}", env!("REV"), R::METHOD, params);
+                Some((req.id, params, panic_context))
+            }
             Err(err) => {
                 let response = lsp_server::Response::new_err(
                     req.id,
@@ -124,7 +153,7 @@ impl<'a> RequestDispatcher<'a> {
     }
 }
 
-fn result_to_response<R>(
+fn thread_result_to_response<R>(
     id: lsp_server::RequestId,
     result: thread::Result<Result<R::Result>>,
 ) -> lsp_server::Response
@@ -134,8 +163,37 @@ where
     R::Result: Serialize + 'static,
 {
     match result {
-        Ok(Ok(resp)) => lsp_server::Response::new_ok(id, &resp),
-        Ok(Err(e)) => match e.downcast::<LspError>() {
+        Ok(result) => result_to_response::<R>(id, result),
+        Err(panic) => {
+            let mut message = "server panicked".to_string();
+
+            let panic_message = panic
+                .downcast_ref::<String>()
+                .map(String::as_str)
+                .or_else(|| panic.downcast_ref::<&str>().copied());
+
+            if let Some(panic_message) = panic_message {
+                message.push_str(": ");
+                message.push_str(panic_message)
+            };
+
+            lsp_server::Response::new_err(id, lsp_server::ErrorCode::InternalError as i32, message)
+        }
+    }
+}
+
+fn result_to_response<R>(
+    id: lsp_server::RequestId,
+    result: Result<R::Result>,
+) -> lsp_server::Response
+where
+    R: lsp_types::request::Request + 'static,
+    R::Params: DeserializeOwned + 'static,
+    R::Result: Serialize + 'static,
+{
+    match result {
+        Ok(resp) => lsp_server::Response::new_ok(id, &resp),
+        Err(e) => match e.downcast::<LspError>() {
             Ok(lsp_error) => lsp_server::Response::new_err(id, lsp_error.code, lsp_error.message),
             Err(e) => {
                 if is_cancelled(&*e) {
@@ -153,21 +211,6 @@ where
                 }
             }
         },
-        Err(panic) => {
-            let mut message = "server panicked".to_string();
-
-            let panic_message = panic
-                .downcast_ref::<String>()
-                .map(String::as_str)
-                .or_else(|| panic.downcast_ref::<&str>().copied());
-
-            if let Some(panic_message) = panic_message {
-                message.push_str(": ");
-                message.push_str(panic_message)
-            };
-
-            lsp_server::Response::new_err(id, lsp_server::ErrorCode::InternalError as i32, message)
-        }
     }
 }
 

crates/rust-analyzer/src/main_loop.rs

@@ -520,24 +520,20 @@ impl GlobalState {
         }
 
         RequestDispatcher { req: Some(req), global_state: self }
-            .on_sync::<lsp_ext::ReloadWorkspace>(|s, ()| {
+            .on_sync_mut::<lsp_ext::ReloadWorkspace>(|s, ()| {
                 s.fetch_workspaces_request();
                 s.fetch_workspaces_if_needed();
                 Ok(())
             })?
-            .on_sync::<lsp_ext::JoinLines>(|s, p| handlers::handle_join_lines(s.snapshot(), p))?
-            .on_sync::<lsp_ext::OnEnter>(|s, p| handlers::handle_on_enter(s.snapshot(), p))?
-            .on_sync::<lsp_types::request::Shutdown>(|s, ()| {
+            .on_sync_mut::<lsp_types::request::Shutdown>(|s, ()| {
                 s.shutdown_requested = true;
                 Ok(())
             })?
-            .on_sync::<lsp_types::request::SelectionRangeRequest>(|s, p| {
-                handlers::handle_selection_range(s.snapshot(), p)
-            })?
-            .on_sync::<lsp_ext::MatchingBrace>(|s, p| {
-                handlers::handle_matching_brace(s.snapshot(), p)
-            })?
-            .on_sync::<lsp_ext::MemoryUsage>(|s, p| handlers::handle_memory_usage(s, p))?
+            .on_sync_mut::<lsp_ext::MemoryUsage>(|s, p| handlers::handle_memory_usage(s, p))?
+            .on_sync::<lsp_ext::JoinLines>(handlers::handle_join_lines)?
+            .on_sync::<lsp_ext::OnEnter>(handlers::handle_on_enter)?
+            .on_sync::<lsp_types::request::SelectionRangeRequest>(handlers::handle_selection_range)?
+            .on_sync::<lsp_ext::MatchingBrace>(handlers::handle_matching_brace)?
             .on::<lsp_ext::AnalyzerStatus>(handlers::handle_analyzer_status)
             .on::<lsp_ext::SyntaxTree>(handlers::handle_syntax_tree)
             .on::<lsp_ext::ViewHir>(handlers::handle_view_hir)