panic with string message again for cycle panics (#898)

Author: carljm

src/cycle.rs

@@ -49,9 +49,6 @@
 //! cycle head may then iterate, which may result in a new set of iterations on the inner cycle,
 //! for each iteration of the outer cycle.
 
-use core::fmt;
-use std::panic;
-
 use thin_vec::{thin_vec, ThinVec};
 
 use crate::key::DatabaseKeyIndex;
@@ -62,48 +59,6 @@ use crate::sync::OnceLock;
 /// Should only be relevant in case of a badly configured cycle recovery.
 pub const MAX_ITERATIONS: IterationCount = IterationCount(200);
 
-pub struct UnexpectedCycle {
-    backtrace: std::backtrace::Backtrace,
-    query_trace: Option<crate::Backtrace>,
-}
-
-impl fmt::Display for UnexpectedCycle {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.write_str("cycle detected but no cycle handler found\n")?;
-        self.backtrace.fmt(f)?;
-        if let Some(query_trace) = &self.query_trace {
-            f.write_str("\n")?;
-            query_trace.fmt(f)?;
-        }
-        Ok(())
-    }
-}
-
-impl UnexpectedCycle {
-    pub(crate) fn throw() -> ! {
-        // We use resume and not panic here to avoid running the panic
-        // hook (that is to avoid printing the backtrace).
-        panic::resume_unwind(Box::new(Self {
-            backtrace: std::backtrace::Backtrace::capture(),
-            query_trace: crate::Backtrace::capture(),
-        }));
-    }
-
-    /// Runs `f`, and catches any salsa cycle.
-    pub fn catch<F, T>(f: F) -> Result<T, UnexpectedCycle>
-    where
-        F: FnOnce() -> T + panic::UnwindSafe,
-    {
-        match panic::catch_unwind(f) {
-            Ok(t) => Ok(t),
-            Err(payload) => match payload.downcast() {
-                Ok(cycle) => Err(*cycle),
-                Err(payload) => panic::resume_unwind(payload),
-            },
-        }
-    }
-}
-
 /// Return value from a cycle recovery function.
 #[derive(Debug)]
 pub enum CycleRecoveryAction<T> {

src/function/fetch.rs

@@ -1,4 +1,4 @@
-use crate::cycle::{CycleHeads, CycleRecoveryStrategy, IterationCount, UnexpectedCycle};
+use crate::cycle::{CycleHeads, CycleRecoveryStrategy, IterationCount};
 use crate::function::memo::Memo;
 use crate::function::sync::ClaimResult;
 use crate::function::{Configuration, IngredientImpl, VerifyResult};
@@ -161,7 +161,13 @@ where
                 }
                 // no provisional value; create/insert/return initial provisional value
                 return match C::CYCLE_STRATEGY {
-                    CycleRecoveryStrategy::Panic => UnexpectedCycle::throw(),
+                    CycleRecoveryStrategy::Panic => zalsa_local.with_query_stack(|stack| {
+                        panic!(
+                            "dependency graph cycle when querying {database_key_index:#?}, \
+                            set cycle_fn/cycle_initial to fixpoint iterate.\n\
+                            Query stack:\n{stack:#?}",
+                        );
+                    }),
                     CycleRecoveryStrategy::Fixpoint => {
                         tracing::debug!(
                             "hit cycle at {database_key_index:#?}, \

src/function/maybe_changed_after.rs

@@ -1,7 +1,5 @@
 use crate::accumulator::accumulated_map::InputAccumulatedValues;
-use crate::cycle::{
-    CycleHeads, CycleRecoveryStrategy, IterationCount, ProvisionalStatus, UnexpectedCycle,
-};
+use crate::cycle::{CycleHeads, CycleRecoveryStrategy, IterationCount, ProvisionalStatus};
 use crate::function::memo::Memo;
 use crate::function::sync::ClaimResult;
 use crate::function::{Configuration, IngredientImpl};
@@ -108,7 +106,13 @@ where
                 return None;
             }
             ClaimResult::Cycle { .. } => match C::CYCLE_STRATEGY {
-                CycleRecoveryStrategy::Panic => UnexpectedCycle::throw(),
+                CycleRecoveryStrategy::Panic => db.zalsa_local().with_query_stack(|stack| {
+                    panic!(
+                        "dependency graph cycle when validating {database_key_index:#?}, \
+                        set cycle_fn/cycle_initial to fixpoint iterate.\n\
+                        Query stack:\n{stack:#?}",
+                    );
+                }),
                 CycleRecoveryStrategy::FallbackImmediate => {
                     return Some(VerifyResult::unchanged());
                 }

src/lib.rs

@@ -42,7 +42,7 @@ pub use salsa_macros::{accumulator, db, input, interned, tracked, Supertype, Upd
 pub use self::accumulator::Accumulator;
 pub use self::active_query::Backtrace;
 pub use self::cancelled::Cancelled;
-pub use self::cycle::{CycleRecoveryAction, UnexpectedCycle};
+pub use self::cycle::CycleRecoveryAction;
 pub use self::database::{AsDynDatabase, Database};
 pub use self::database_impl::DatabaseImpl;
 pub use self::durability::Durability;

tests/cycle.rs

@@ -5,10 +5,7 @@
 mod common;
 use common::{ExecuteValidateLoggerDatabase, LogDatabase};
 use expect_test::expect;
-use salsa::{
-    CycleRecoveryAction, Database as Db, DatabaseImpl as DbImpl, Durability, Setter,
-    UnexpectedCycle,
-};
+use salsa::{CycleRecoveryAction, Database as Db, DatabaseImpl as DbImpl, Durability, Setter};
 #[cfg(not(miri))]
 use test_log::test;
 
@@ -225,12 +222,13 @@ fn value(num: u8) -> Input {
 ///
 /// Simple self-cycle, no iteration, should panic.
 #[test]
+#[should_panic(expected = "dependency graph cycle")]
 fn self_panic() {
     let mut db = DbImpl::new();
     let a_in = Inputs::new(&db, vec![]);
     let a = Input::MinPanic(a_in);
     a_in.set_inputs(&mut db).to(vec![a.clone()]);
-    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
+    a.eval(&db);
 }
 
 /// a:Np(u10, a) -+
@@ -239,13 +237,14 @@ fn self_panic() {
 ///
 /// Simple self-cycle with untracked read, no iteration, should panic.
 #[test]
+#[should_panic(expected = "dependency graph cycle")]
 fn self_untracked_panic() {
     let mut db = DbImpl::new();
     let a_in = Inputs::new(&db, vec![]);
     let a = Input::MinPanic(a_in);
     a_in.set_inputs(&mut db).to(vec![untracked(10), a.clone()]);
 
-    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
+    a.eval(&db);
 }
 
 /// a:Ni(a) -+
@@ -289,6 +288,7 @@ fn two_mixed_converge_initial_value() {
 /// Two-query cycle, one with iteration and one without.
 /// If we enter from the one with no iteration, we panic.
 #[test]
+#[should_panic(expected = "dependency graph cycle")]
 fn two_mixed_panic() {
     let mut db = DbImpl::new();
     let a_in = Inputs::new(&db, vec![]);
@@ -298,7 +298,7 @@ fn two_mixed_panic() {
     a_in.set_inputs(&mut db).to(vec![b]);
     b_in.set_inputs(&mut db).to(vec![a.clone()]);
 
-    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
+    a.eval(&db);
 }
 
 /// a:Ni(b) --> b:Xi(a)
@@ -369,6 +369,7 @@ fn two_indirect_iterate_converge_initial_value() {
 ///
 /// Two-query cycle, enter indirectly at node without iteration, panic.
 #[test]
+#[should_panic(expected = "dependency graph cycle")]
 fn two_indirect_panic() {
     let mut db = DbImpl::new();
     let a_in = Inputs::new(&db, vec![]);
@@ -381,7 +382,7 @@ fn two_indirect_panic() {
     b_in.set_inputs(&mut db).to(vec![c]);
     c_in.set_inputs(&mut db).to(vec![b]);
 
-    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
+    a.eval(&db);
 }
 
 /// a:Np(b) -> b:Ni(v200,c) -> c:Xp(b)

tests/cycle_initial_call_back_into_cycle.rs

@@ -1,7 +1,5 @@
 //! Calling back into the same cycle from your cycle initial function will trigger another cycle.
 
-use salsa::UnexpectedCycle;
-
 #[salsa::tracked]
 fn initial_value(db: &dyn salsa::Database) -> u32 {
     query(db)
@@ -30,8 +28,9 @@ fn cycle_fn(
 }
 
 #[test_log::test]
+#[should_panic(expected = "dependency graph cycle")]
 fn the_test() {
     let db = salsa::DatabaseImpl::default();
 
-    UnexpectedCycle::catch(|| query(&db)).unwrap_err();
+    query(&db);
 }