Merge #9937 #9985

9937: internal: incentivize rust-analyzed developers to fix panics r=matklad a=matklad



9985: minor: Fix another “a”/“an” typo r=Veykril a=steffahn

Follow-up to #9984.

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Co-authored-by: Frank Steffahn <frank.steffahn@stu.uni-kiel.de>

crates/ide/src/references.rs

@@ -1,6 +1,6 @@
 //! This module implements a reference search.
 //! First, the element at the cursor position must be either an `ast::Name`
-//! or `ast::NameRef`. If it's a `ast::NameRef`, at the classification step we
+//! or `ast::NameRef`. If it's an `ast::NameRef`, at the classification step we
 //! try to resolve the direct tree parent of this element, otherwise we
 //! already have a definition and just need to get its HIR together with
 //! some information that is needed for further steps of searching.

crates/rust-analyzer/src/dispatch.rs

@@ -1,5 +1,5 @@
 //! A visitor for downcasting arbitrary request (JSON) into a specific type.
-use std::{fmt, panic};
+use std::{fmt, panic, thread};
 
 use serde::{de::DeserializeOwned, Serialize};
 
@@ -32,7 +32,7 @@ impl<'a> RequestDispatcher<'a> {
         };
         let global_state = panic::AssertUnwindSafe(&mut *self.global_state);
 
-        let response = panic::catch_unwind(move || {
+        let result = panic::catch_unwind(move || {
             let _ = &global_state;
             let panic::AssertUnwindSafe(global_state) = global_state;
             let _pctx = stdx::panic_context::enter(format!(
@@ -41,10 +41,10 @@ impl<'a> RequestDispatcher<'a> {
                 R::METHOD,
                 params
             ));
-            let result = f(global_state, params);
+            f(global_state, params)
-            result_to_response::<R>(id, result)
+        });
-        })
+        let response = result_to_response::<R>(id, result);
-        .map_err(|_err| format!("sync task {:?} panicked", R::METHOD))?;
+
         self.global_state.respond(response);
         Ok(self)
     }
@@ -56,7 +56,7 @@ impl<'a> RequestDispatcher<'a> {
     ) -> &mut Self
     where
         R: lsp_types::request::Request + 'static,
-        R::Params: DeserializeOwned + Send + fmt::Debug + 'static,
+        R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug + 'static,
         R::Result: Serialize + 'static,
     {
         let (id, params) = match self.parse::<R>() {
@@ -66,16 +66,18 @@ impl<'a> RequestDispatcher<'a> {
 
         self.global_state.task_pool.handle.spawn({
             let world = self.global_state.snapshot();
-
             move || {
-                let _pctx = stdx::panic_context::enter(format!(
+                let result = panic::catch_unwind(move || {
-                    "\nversion: {}\nrequest: {} {:#?}",
+                    let _pctx = stdx::panic_context::enter(format!(
-                    env!("REV"),
+                        "\nversion: {}\nrequest: {} {:#?}",
-                    R::METHOD,
+                        env!("REV"),
-                    params
+                        R::METHOD,
-                ));
+                        params
-                let result = f(world, params);
+                    ));
-                Task::Response(result_to_response::<R>(id, result))
+                    f(world, params)
+                });
+                let response = result_to_response::<R>(id, result);
+                Task::Response(response)
             }
         });
 
@@ -122,7 +124,7 @@ impl<'a> RequestDispatcher<'a> {
 
 fn result_to_response<R>(
     id: lsp_server::RequestId,
-    result: Result<R::Result>,
+    result: thread::Result<Result<R::Result>>,
 ) -> lsp_server::Response
 where
     R: lsp_types::request::Request + 'static,
@@ -130,8 +132,8 @@ where
     R::Result: Serialize + 'static,
 {
     match result {
-        Ok(resp) => lsp_server::Response::new_ok(id, &resp),
+        Ok(Ok(resp)) => lsp_server::Response::new_ok(id, &resp),
-        Err(e) => match e.downcast::<LspError>() {
+        Ok(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) {
@@ -149,6 +151,21 @@ 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/global_state.rs

@@ -116,6 +116,8 @@ pub(crate) struct GlobalStateSnapshot {
     pub(crate) workspaces: Arc<Vec<ProjectWorkspace>>,
 }
 
+impl std::panic::UnwindSafe for GlobalStateSnapshot {}
+
 impl GlobalState {
     pub(crate) fn new(sender: Sender<lsp_server::Message>, config: Config) -> GlobalState {
         let loader = {
@@ -262,6 +264,12 @@ impl GlobalState {
     }
     pub(crate) fn respond(&mut self, response: lsp_server::Response) {
         if let Some((method, start)) = self.req_queue.incoming.complete(response.id.clone()) {
+            if let Some(err) = &response.error {
+                if err.message.starts_with("server panicked") {
+                    self.poke_rust_analyzer_developer(format!("{}, check the log", err.message))
+                }
+            }
+
             let duration = start.elapsed();
             log::info!("handled {} - ({}) in {:0.2?}", method, response.id, duration);
             self.send(response.into());

crates/rust-analyzer/src/lsp_utils.rs

@@ -42,6 +42,25 @@ impl GlobalState {
         )
     }
 
+    /// rust-analyzer is resilient -- if it fails, this doesn't usually affect
+    /// the user experience. Part of that is that we deliberately hide panics
+    /// from the user.
+    ///
+    /// We do however want to pester rust-analyzer developers with panics and
+    /// other "you really gotta fix that" messages. The current strategy is to
+    /// be noisy for "from source" builds or when profiling is enabled.
+    ///
+    /// It's unclear if making from source `cargo xtask install` builds more
+    /// panicky is a good idea, let's see if we can keep our awesome bleeding
+    /// edge users from being upset!
+    pub(crate) fn poke_rust_analyzer_developer(&mut self, message: String) {
+        let from_source_build = env!("REV").contains("dev");
+        let profiling_enabled = std::env::var("RA_PROFILE").is_ok();
+        if from_source_build || profiling_enabled {
+            self.show_message(lsp_types::MessageType::Error, message)
+        }
+    }
+
     pub(crate) fn report_progress(
         &mut self,
         title: &str,

crates/rust-analyzer/src/main_loop.rs

@@ -1,7 +1,7 @@
 //! The main loop of `rust-analyzer` responsible for dispatching LSP
 //! requests/replies and notifications back to the client.
 use std::{
-    env, fmt,
+    fmt,
     sync::Arc,
     time::{Duration, Instant},
 };
@@ -487,12 +487,10 @@ impl GlobalState {
         let loop_duration = loop_start.elapsed();
         if loop_duration > Duration::from_millis(100) {
             log::warn!("overly long loop turn: {:?}", loop_duration);
-            if env::var("RA_PROFILE").is_ok() {
+            self.poke_rust_analyzer_developer(format!(
-                self.show_message(
+                "overly long loop turn: {:?}",
-                    lsp_types::MessageType::Error,
+                loop_duration
-                    format!("overly long loop turn: {:?}", loop_duration),
+            ));
-                )
-            }
         }
         Ok(())
     }