From aec1030f286bad9c8be408e3e37dc3f5cd10dd71 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 23 Mar 2026 18:33:05 +0100 Subject: [PATCH 1/2] [ty] Fix Salsa panic propagation --- crates/ruff_db/src/panic.rs | 4 ++++ crates/ty_project/src/lib.rs | 36 +++++++++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/crates/ruff_db/src/panic.rs b/crates/ruff_db/src/panic.rs index d91c1f7fe7a847..f5c176d5f44dfa 100644 --- a/crates/ruff_db/src/panic.rs +++ b/crates/ruff_db/src/panic.rs @@ -32,6 +32,10 @@ impl Payload { } impl PanicError { + pub fn resume_unwind(self) -> ! { + std::panic::resume_unwind(self.payload.0) + } + pub fn to_diagnostic_message(&self, path: Option) -> String { use std::fmt::Write; diff --git a/crates/ty_project/src/lib.rs b/crates/ty_project/src/lib.rs index d62af21b6c569f..7923f2904b3d1e 100644 --- a/crates/ty_project/src/lib.rs +++ b/crates/ty_project/src/lib.rs @@ -20,8 +20,7 @@ use ruff_db::parsed::parsed_module; use ruff_db::source::{SourceTextError, source_text}; use ruff_db::system::{SystemPath, SystemPathBuf}; use rustc_hash::FxHashSet; -use salsa::Durability; -use salsa::Setter; +use salsa::{Database, Durability, Setter}; use std::backtrace::BacktraceStatus; use std::collections::hash_set; use std::iter::FusedIterator; @@ -319,6 +318,9 @@ impl Project { for file in &files { let db = db.clone(); let reporter = &*reporter; + + db.unwind_if_revision_cancelled(); + scope.spawn(move |_| { let check_file_span = tracing::debug_span!(parent: project_span, "check_file", ?file); @@ -646,10 +648,9 @@ pub(crate) fn check_file_impl(db: &dyn Db, file: File) -> Result { + Ok(type_check_diagnostics) => { diagnostics.extend(type_check_diagnostics); } - Ok(None) => {} Err(diagnostic) => diagnostics.push(diagnostic), } } @@ -749,16 +750,29 @@ enum IOErrorKind { SourceText(#[from] SourceTextError), } -fn catch(db: &dyn Db, file: File, f: F) -> Result, Diagnostic> +fn catch(db: &dyn Db, file: File, f: F) -> Result where F: FnOnce() -> R + UnwindSafe, { - match ruff_db::panic::catch_unwind(|| { - // Ignore salsa errors - salsa::Cancelled::catch(f).ok() - }) { + match ruff_db::panic::catch_unwind(f) { Ok(result) => Ok(result), Err(error) => { + match error.payload.downcast_ref::() { + Some(salsa::Cancelled::PropagatedPanic) | None => { + // Add a diagnostic (fall through) for + // propagated Salsa panics (query A depends on query B and query B panics) + // or any non Salsa panic (logical error). + // + // The propagated Salsa panic isn't very actionalbe for users + // but it can be useful to know that file A failed to type check + // because file B panicked (both files will have a panicked diagnostic). + } + // For normal cancellations, resume the panic + Some(_) => { + error.resume_unwind(); + } + } + let message = error.to_diagnostic_message(Some(file.path(db))); let mut diagnostic = Diagnostic::new(DiagnosticId::Panic, Severity::Fatal, message); diagnostic.add_bug_sub_diagnostics("%5Bpanic%5D"); @@ -790,6 +804,10 @@ where }); } + // Report an untracked read because Salsa didn't carry over + // the dependencies of any query called by `f` because it panicked. + db.report_untracked_read(); + Err(diagnostic) } } From 93809b2d178489d051a2743269b72903e3e361a4 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 24 Mar 2026 09:53:21 +0100 Subject: [PATCH 2/2] Handle `PropagatedPanic` because of a pending write --- crates/ty_project/src/lib.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/crates/ty_project/src/lib.rs b/crates/ty_project/src/lib.rs index 7923f2904b3d1e..a5362100602d3d 100644 --- a/crates/ty_project/src/lib.rs +++ b/crates/ty_project/src/lib.rs @@ -758,16 +758,24 @@ where Ok(result) => Ok(result), Err(error) => { match error.payload.downcast_ref::() { - Some(salsa::Cancelled::PropagatedPanic) | None => { - // Add a diagnostic (fall through) for - // propagated Salsa panics (query A depends on query B and query B panics) - // or any non Salsa panic (logical error). + None => { + // Add a diagnostic (by not early returning) for + // any non Salsa panic (a bug in ty) + } + Some(salsa::Cancelled::PropagatedPanic) => { + // Add a diagnostic for propagated Salsa panics. That is, query `A` + // running on thread `a` depends on query `B` running on thread `b` + // and query `B` panics. However, avoid adding such a diagnostic + // if query `B` panicked because of a cancellation by calling + // `unwind_if_revision_cancelled`. // - // The propagated Salsa panic isn't very actionalbe for users + // The propagated Salsa panic isn't very actionable for users, // but it can be useful to know that file A failed to type check - // because file B panicked (both files will have a panicked diagnostic). + // because file B panicked (both files will have a panic-diagnostic). + db.unwind_if_revision_cancelled(); } - // For normal cancellations, resume the panic + + // For any pending write or local cancellation, resume the panic to abort the outer query. Some(_) => { error.resume_unwind(); }