diff --git a/crates/squawk_ide/src/goto_definition.rs b/crates/squawk_ide/src/goto_definition.rs index 2c72657c..b7f45c44 100644 --- a/crates/squawk_ide/src/goto_definition.rs +++ b/crates/squawk_ide/src/goto_definition.rs @@ -66,7 +66,6 @@ pub fn goto_definition(db: &dyn Db, file: File, offset: TextSize) -> SmallVec<[L for file_id in [FileId::Current, FileId::Builtins] { let file = match file_id { FileId::Current => source_file, - // TODO: we should salsa this FileId::Builtins => &parse_builtins(db).tree(), }; // TODO: we should salsa this @@ -98,7 +97,6 @@ pub fn goto_definition(db: &dyn Db, file: File, offset: TextSize) -> SmallVec<[L for file_id in [FileId::Current, FileId::Builtins] { let file = match file_id { FileId::Current => source_file, - // TODO: we should salsa this FileId::Builtins => &parse_builtins(db).tree(), }; // TODO: we should salsa this diff --git a/crates/squawk_server/src/dispatch.rs b/crates/squawk_server/src/dispatch.rs index 0e2aea12..97fc1ef6 100644 --- a/crates/squawk_server/src/dispatch.rs +++ b/crates/squawk_server/src/dispatch.rs @@ -1,5 +1,7 @@ // Based on https://github.com/rust-lang/rust-analyzer/blob/2efc80078029894eec0699f62ec8d5c1a56af763/crates/rust-analyzer/src/handlers/dispatch.rs#L277C5-L277C5 +use std::panic::UnwindSafe; + use anyhow::Result; use log::{error, info}; use lsp_server::{RequestId, Response}; @@ -61,7 +63,7 @@ impl<'a> RequestDispatcher<'a> { pub(crate) fn on(self, handler: fn(&Snapshot, R::Params) -> Result) -> Self where R: LspRequest, - R::Params: Send + 'static, + R::Params: Send + 'static + UnwindSafe, { self.on_with_thread_intent::(ThreadIntent::Worker, handler) } @@ -72,7 +74,7 @@ impl<'a> RequestDispatcher<'a> { ) -> Self where R: LspRequest, - R::Params: Send + 'static, + R::Params: Send + 'static + UnwindSafe, { self.on_with_thread_intent::(ThreadIntent::LatencySensitive, handler) } @@ -84,14 +86,17 @@ impl<'a> RequestDispatcher<'a> { ) -> Self where R: LspRequest, - R::Params: Send + 'static, + R::Params: Send + 'static + UnwindSafe, { if let Some((id, params)) = self.parse::() { let snapshot = self.global_state.snapshot(); self.global_state.task_pool.handle.spawn(intent, move || { - let result = handler(&snapshot, params); - result_to_response::(id, result) + crate::panic::catch_unwind(|| { + let result = handler(&snapshot, params); + result_to_response::(id.clone(), result) + }) + .unwrap_or_else(|error| panic_response(id, &error)) }); } @@ -105,6 +110,28 @@ impl<'a> RequestDispatcher<'a> { } } +fn panic_response(id: RequestId, error: &crate::panic::PanicError) -> Response { + // Check if the request was canceled due to some modifications to the salsa database. + if error.payload.downcast_ref::().is_some() { + // TODO: trigger retries when we have that setup, we'll reenque the task + log::debug!( + "request id={} was cancelled by salsa, sending content modified", + id + ); + Response::new_err( + id, + lsp_server::ErrorCode::ContentModified as i32, + "content modified".to_string(), + ) + } else { + Response::new_err( + id, + lsp_server::ErrorCode::InternalError as i32, + "request handler error".to_string(), + ) + } +} + fn result_to_response(id: RequestId, result: Result) -> Response where R: LspRequest, diff --git a/crates/squawk_server/src/lib.rs b/crates/squawk_server/src/lib.rs index d121151b..a28fbc47 100644 --- a/crates/squawk_server/src/lib.rs +++ b/crates/squawk_server/src/lib.rs @@ -5,6 +5,7 @@ mod handlers; mod ignore; mod lint; mod lsp_utils; +mod panic; mod server; pub use server::run; diff --git a/crates/squawk_server/src/panic.rs b/crates/squawk_server/src/panic.rs new file mode 100644 index 00000000..8aca6dd4 --- /dev/null +++ b/crates/squawk_server/src/panic.rs @@ -0,0 +1,248 @@ +// via: https://github.com/charliermarsh/ruff/blob/5011b253c1aca2a1906762cf45414d0fda1a088a/crates/ruff_db/src/panic.rs +// +// MIT License +// +// Copyright (c) 2022 Charles Marsh +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +use std::any::Any; +use std::backtrace::BacktraceStatus; +use std::cell::Cell; +use std::panic::Location; +use std::sync::OnceLock; + +#[derive(Debug)] +pub(crate) struct PanicError { + pub(crate) location: Option, + pub(crate) payload: Payload, + pub(crate) backtrace: Option, + pub(crate) salsa_backtrace: Option, +} + +#[derive(Debug)] +pub(crate) struct Payload(Box); + +impl Payload { + pub(crate) fn as_str(&self) -> Option<&str> { + if let Some(s) = self.0.downcast_ref::() { + Some(s) + } else if let Some(s) = self.0.downcast_ref::<&str>() { + Some(s) + } else { + None + } + } + + pub(crate) fn downcast_ref(&self) -> Option<&R> { + self.0.downcast_ref::() + } +} + +impl PanicError { + #[expect(unused)] + pub(crate) fn to_diagnostic_message(&self, path: Option) -> String { + use std::fmt::Write; + + let mut message = String::new(); + message.push_str("Panicked"); + + if let Some(location) = &self.location { + let _ = write!(&mut message, " at {location}"); + } + + if let Some(path) = path { + let _ = write!(&mut message, " when checking `{path}`"); + } + + if let Some(payload) = self.payload.as_str() { + let _ = write!(&mut message, ": `{payload}`"); + } + + message + } +} + +impl std::fmt::Display for PanicError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "panicked at")?; + if let Some(location) = &self.location { + write!(f, " {location}")?; + } + if let Some(payload) = self.payload.as_str() { + write!(f, ":\n{payload}")?; + } + if let Some(query_trace) = self.salsa_backtrace.as_ref() { + let _ = writeln!(f, "{query_trace}"); + } + + if let Some(backtrace) = &self.backtrace { + match backtrace.status() { + BacktraceStatus::Disabled => { + writeln!( + f, + "\nrun with `RUST_BACKTRACE=1` environment variable to display a backtrace" + )?; + } + BacktraceStatus::Captured => { + writeln!(f, "\nBacktrace: {backtrace}")?; + } + _ => {} + } + } + + Ok(()) + } +} + +#[derive(Default)] +struct CapturedPanicInfo { + backtrace: Option, + location: Option, + salsa_backtrace: Option, +} + +thread_local! { + static CAPTURE_PANIC_INFO: Cell = const { Cell::new(false) }; + static LAST_BACKTRACE: Cell = const { + Cell::new(CapturedPanicInfo { backtrace: None, location: None, salsa_backtrace: None }) + }; +} + +fn install_hook() { + static ONCE: OnceLock<()> = OnceLock::new(); + ONCE.get_or_init(|| { + let prev = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + let should_capture = CAPTURE_PANIC_INFO.with(Cell::get); + if !should_capture { + return (*prev)(info); + } + + let location = info.location().map(Location::to_string); + let backtrace = Some(std::backtrace::Backtrace::capture()); + + LAST_BACKTRACE.set(CapturedPanicInfo { + backtrace, + location, + salsa_backtrace: salsa::Backtrace::capture(), + }); + })); + }); +} + +/// Invokes a closure, capturing and returning the cause of an unwinding panic if one occurs. +/// +/// ### Thread safety +/// +/// This is implemented by installing a custom [panic hook](std::panic::set_hook). This panic hook +/// is a global resource. The hook that we install captures panic info in a thread-safe manner, +/// and also ensures that any threads that are _not_ currently using this `catch_unwind` wrapper +/// still use the previous hook (typically the default hook, which prints out panic information to +/// stderr). +/// +/// We assume that there is nothing else running in this process that needs to install a competing +/// panic hook. We are careful to install our custom hook only once, and we do not ever restore +/// the previous hook (since you can always retain the previous hook's behavior by not calling this +/// wrapper). +pub(crate) fn catch_unwind(f: F) -> Result +where + F: FnOnce() -> R + std::panic::UnwindSafe, +{ + install_hook(); + let prev_should_capture = CAPTURE_PANIC_INFO.replace(true); + let result = std::panic::catch_unwind(f).map_err(|payload| { + // Try to get the backtrace and location from our custom panic hook. + // The custom panic hook only runs once when `panic!` is called (or similar). It doesn't + // run when the panic is propagated with `std::panic::resume_unwind`. The panic hook + // is also not called when the panic is raised with `std::panic::resum_unwind` as is the + // case for salsa unwinds (see the ignored test below). + // Because of that, always take the payload from `catch_unwind` because it may have been transformed + // by an inner `std::panic::catch_unwind` handlers and only use the information + // from the custom handler to enrich the error with the backtrace and location. + let CapturedPanicInfo { + location, + backtrace, + salsa_backtrace, + } = LAST_BACKTRACE.with(Cell::take); + + PanicError { + location, + payload: Payload(payload), + backtrace, + salsa_backtrace, + } + }); + CAPTURE_PANIC_INFO.set(prev_should_capture); + result +} + +#[cfg(test)] +mod tests { + use salsa::{Database, Durability}; + + #[test] + #[ignore = "super::catch_unwind installs a custom panic handler, which could effect test isolation"] + fn no_backtrace_for_salsa_cancelled() { + #[salsa::input] + struct Input { + value: u32, + } + + #[salsa::tracked] + fn test_query(db: &dyn Database, input: Input) -> u32 { + loop { + // This should throw a cancelled error + let _ = input.value(db); + } + } + + let db = salsa::DatabaseImpl::new(); + + let input = Input::new(&db, 42); + + let result = std::thread::scope(move |scope| { + { + let mut db = db.clone(); + scope.spawn(move || { + // This will cancel the other thread by throwing a `salsa::Cancelled` error. + db.synthetic_write(Durability::MEDIUM); + }); + } + + { + scope.spawn(move || { + super::catch_unwind(|| { + test_query(&db, input); + }) + }) + } + .join() + .unwrap() + }); + + match result { + Ok(_) => panic!("Expected query to panic"), + Err(err) => { + // Panics triggered with `resume_unwind` have no backtrace. + assert!(err.backtrace.is_none()); + } + } + } +}