Unnamed repository; edit this file 'description' to name the repository.
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 <[email protected]>
Co-authored-by: Frank Steffahn <[email protected]>
| -rw-r--r-- | crates/ide/src/references.rs | 2 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/dispatch.rs | 55 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/global_state.rs | 8 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/lsp_utils.rs | 19 | ||||
| -rw-r--r-- | crates/rust-analyzer/src/main_loop.rs | 12 |
5 files changed, 69 insertions, 27 deletions
diff --git a/crates/ide/src/references.rs b/crates/ide/src/references.rs index d7a2b45484..395a29587b 100644 --- a/crates/ide/src/references.rs +++ b/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. diff --git a/crates/rust-analyzer/src/dispatch.rs b/crates/rust-analyzer/src/dispatch.rs index 609459b7ed..031c76c772 100644 --- a/crates/rust-analyzer/src/dispatch.rs +++ b/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); - result_to_response::<R>(id, result) - }) - .map_err(|_err| format!("sync task {:?} panicked", R::METHOD))?; + f(global_state, params) + }); + let response = result_to_response::<R>(id, result); + 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!( - "\nversion: {}\nrequest: {} {:#?}", - env!("REV"), - R::METHOD, - params - )); - let result = f(world, params); - Task::Response(result_to_response::<R>(id, result)) + let result = panic::catch_unwind(move || { + let _pctx = stdx::panic_context::enter(format!( + "\nversion: {}\nrequest: {} {:#?}", + env!("REV"), + R::METHOD, + params + )); + 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), - Err(e) => match e.downcast::<LspError>() { + Ok(Ok(resp)) => lsp_server::Response::new_ok(id, &resp), + 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) + } } } diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs index da224ed588..c2b71327da 100644 --- a/crates/rust-analyzer/src/global_state.rs +++ b/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()); diff --git a/crates/rust-analyzer/src/lsp_utils.rs b/crates/rust-analyzer/src/lsp_utils.rs index 087c26a71f..29fa3e2d0e 100644 --- a/crates/rust-analyzer/src/lsp_utils.rs +++ b/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, diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs index 42eff1875e..c2aba3ad70 100644 --- a/crates/rust-analyzer/src/main_loop.rs +++ b/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.show_message( - lsp_types::MessageType::Error, - format!("overly long loop turn: {:?}", loop_duration), - ) - } + self.poke_rust_analyzer_developer(format!( + "overly long loop turn: {:?}", + loop_duration + )); } Ok(()) } |