Unwind with specific type when encountering an unexpected cycle (#856)

Author: Veykril

src/cancelled.rs

@@ -23,7 +23,7 @@ impl Cancelled {
     pub(crate) fn throw(self) -> ! {
         // We use resume and not panic here to avoid running the panic
         // hook (that is, to avoid collecting and printing backtrace).
-        std::panic::resume_unwind(Box::new(self));
+        panic::resume_unwind(Box::new(self));
     }
 
     /// Runs `f`, and catches any salsa cancellation.

src/cycle.rs

@@ -49,6 +49,9 @@
 //! 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;
@@ -58,6 +61,52 @@ use crate::key::DatabaseKeyIndex;
 /// Should only be relevant in case of a badly configured cycle recovery.
 pub const MAX_ITERATIONS: u32 = 200;
 
+pub struct UnexpectedCycle(Option<crate::Backtrace>);
+
+impl fmt::Debug for UnexpectedCycle {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str("cycle detected but no cycle handler found")?;
+        if let Some(backtrace) = &self.0 {
+            f.write_str(": ")?;
+            backtrace.fmt(f)?;
+        }
+        Ok(())
+    }
+}
+
+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")?;
+        if let Some(backtrace) = &self.0 {
+            f.write_str("\n")?;
+            backtrace.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 collecting and printing backtrace).
+        panic::resume_unwind(Box::new(Self(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};
+use crate::cycle::{CycleHeads, CycleRecoveryStrategy, UnexpectedCycle};
 use crate::function::memo::Memo;
 use crate::function::sync::ClaimResult;
 use crate::function::{Configuration, IngredientImpl, VerifyResult};
@@ -139,22 +139,14 @@ where
                 }
                 // no provisional value; create/insert/return initial provisional value
                 return match C::CYCLE_STRATEGY {
-                    CycleRecoveryStrategy::Panic => db.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::Panic => UnexpectedCycle::throw(),
                     CycleRecoveryStrategy::Fixpoint => {
                         tracing::debug!(
                             "hit cycle at {database_key_index:#?}, \
                             inserting and returning fixpoint initial value"
                         );
                         let revisions = QueryRevisions::fixpoint_initial(database_key_index);
-                        let initial_value = self
-                            .initial_value(db, id)
-                            .expect("`CycleRecoveryStrategy::Fixpoint` should have initial_value");
+                        let initial_value = C::cycle_initial(db, C::id_to_input(db, id));
                         Some(self.insert_memo(
                             zalsa,
                             id,
@@ -167,9 +159,7 @@ where
                             "hit a `FallbackImmediate` cycle at {database_key_index:#?}"
                         );
                         let active_query = db.zalsa_local().push_query(database_key_index, 0);
-                        let fallback_value = self.initial_value(db, id).expect(
-                            "`CycleRecoveryStrategy::FallbackImmediate` should have initial_value",
-                        );
+                        let fallback_value = C::cycle_initial(db, C::id_to_input(db, id));
                         let mut revisions = active_query.pop();
                         revisions.cycle_heads = CycleHeads::initial(database_key_index);
                         // We need this for `cycle_heads()` to work. We will unset this in the outer `execute()`.

src/function/maybe_changed_after.rs

@@ -1,5 +1,5 @@
 use crate::accumulator::accumulated_map::InputAccumulatedValues;
-use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryStrategy};
+use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryStrategy, UnexpectedCycle};
 use crate::function::memo::Memo;
 use crate::function::sync::ClaimResult;
 use crate::function::{Configuration, IngredientImpl};
@@ -104,13 +104,7 @@ where
         let _claim_guard = match self.sync_table.try_claim(zalsa, key_index) {
             ClaimResult::Retry => return None,
             ClaimResult::Cycle => match C::CYCLE_STRATEGY {
-                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::Panic => UnexpectedCycle::throw(),
                 CycleRecoveryStrategy::FallbackImmediate => {
                     return Some(VerifyResult::unchanged());
                 }

src/function/memo.rs

@@ -3,7 +3,7 @@ use std::fmt::{Debug, Formatter};
 use std::mem::transmute;
 use std::ptr::NonNull;
 
-use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryStrategy, EMPTY_CYCLE_HEADS};
+use crate::cycle::{CycleHeadKind, CycleHeads, EMPTY_CYCLE_HEADS};
 use crate::function::{Configuration, IngredientImpl};
 use crate::key::DatabaseKeyIndex;
 use crate::loom::sync::atomic::Ordering;
@@ -82,19 +82,6 @@ impl<C: Configuration> IngredientImpl<C> {
 
         table.map_memo(memo_ingredient_index, map)
     }
-
-    pub(super) fn initial_value<'db>(
-        &'db self,
-        db: &'db C::DbView,
-        key: Id,
-    ) -> Option<C::Output<'db>> {
-        match C::CYCLE_STRATEGY {
-            CycleRecoveryStrategy::Fixpoint | CycleRecoveryStrategy::FallbackImmediate => {
-                Some(C::cycle_initial(db, C::id_to_input(db, key)))
-            }
-            CycleRecoveryStrategy::Panic => None,
-        }
-    }
 }
 
 #[derive(Debug)]

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;
+pub use self::cycle::{CycleRecoveryAction, UnexpectedCycle};
 pub use self::database::{AsDynDatabase, Database};
 pub use self::database_impl::DatabaseImpl;
 pub use self::durability::Durability;

tests/cycle.rs

@@ -5,7 +5,10 @@
 mod common;
 use common::{ExecuteValidateLoggerDatabase, LogDatabase};
 use expect_test::expect;
-use salsa::{CycleRecoveryAction, Database as Db, DatabaseImpl as DbImpl, Durability, Setter};
+use salsa::{
+    CycleRecoveryAction, Database as Db, DatabaseImpl as DbImpl, Durability, Setter,
+    UnexpectedCycle,
+};
 #[cfg(not(miri))]
 use test_log::test;
 
@@ -222,14 +225,12 @@ 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()]);
-
-    a.eval(&db);
+    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
 }
 
 /// a:Np(u10, a) -+
@@ -238,14 +239,13 @@ 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()]);
 
-    a.eval(&db);
+    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
 }
 
 /// a:Ni(a) -+
@@ -289,7 +289,6 @@ 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![]);
@@ -299,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()]);
 
-    a.eval(&db);
+    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
 }
 
 /// a:Ni(b) --> b:Xi(a)
@@ -370,7 +369,6 @@ 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![]);
@@ -383,7 +381,7 @@ fn two_indirect_panic() {
     b_in.set_inputs(&mut db).to(vec![c]);
     c_in.set_inputs(&mut db).to(vec![b]);
 
-    a.eval(&db);
+    UnexpectedCycle::catch(|| a.eval(&db)).unwrap_err();
 }
 
 /// a:Np(b) -> b:Ni(v200,c) -> c:Xp(b)

tests/cycle_initial_call_back_into_cycle.rs

@@ -1,5 +1,7 @@
 //! 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)
@@ -28,9 +30,8 @@ fn cycle_fn(
 }
 
 #[test_log::test]
-#[should_panic(expected = "dependency graph cycle")]
 fn the_test() {
     let db = salsa::DatabaseImpl::default();
 
-    query(&db);
+    UnexpectedCycle::catch(|| query(&db)).unwrap_err();
 }