From 16ab655c19d26e882dc1930bf644b5f7fef51b40 Mon Sep 17 00:00:00 2001 From: Sai Asish Y Date: Tue, 26 May 2026 01:04:19 -0700 Subject: [PATCH] fix accumulator wrongly skipped when tracked fn reused without re-execute Signed-off-by: Sai Asish Y --- src/function/maybe_changed_after.rs | 19 ++++++- tests/accumulate-behind-tracked-fn.rs | 74 +++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 tests/accumulate-behind-tracked-fn.rs diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index e6ea963eb..6d7dd1136 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -190,8 +190,23 @@ where return Some(if old_memo.revisions.changed_at > revision { VerifyResult::changed() } else { - // Returns unchanged but propagates the accumulated values - deep_verify + // Propagate accumulated values from inputs *and* from this memo itself. + // Without the own-accumulated check, a caller querying accumulated values + // would see `Empty` and skip traversing into this subtree even though + // this memo directly pushed accumulated values. + VerifyResult::unchanged_with_accumulated( + #[cfg(feature = "accumulator")] + { + let VerifyResult::Unchanged { accumulated } = deep_verify else { + unreachable!() + }; + if old_memo.revisions.accumulated().is_some() { + InputAccumulatedValues::Any + } else { + accumulated + } + }, + ) }); } diff --git a/tests/accumulate-behind-tracked-fn.rs b/tests/accumulate-behind-tracked-fn.rs new file mode 100644 index 000000000..703463311 --- /dev/null +++ b/tests/accumulate-behind-tracked-fn.rs @@ -0,0 +1,74 @@ +#![cfg(all(feature = "inventory", feature = "accumulator"))] + +//! Regression test for https://github.com/salsa-rs/salsa/issues/923. +//! +//! Accumulated values were silently dropped when a tracked fn that calls +//! another tracked fn (which does the actual accumulating) is re-used +//! without re-executing (its inputs haven't changed). + +use expect_test::expect; +use salsa::{Accumulator, Setter}; +use test_log::test; + +#[salsa::input(debug)] +struct List { + value: u32, + next: Option, +} + +#[allow(unused)] +#[salsa::accumulator] +#[derive(Copy, Clone, Debug)] +struct Integers(u32); + +/// Outer tracked fn: iterates the list and delegates accumulation to `compute_single`. +#[salsa::tracked] +fn compute(db: &dyn salsa::Database, input: List) { + compute_single(db, input); + if let Some(next) = input.next(db) { + compute(db, next); + } +} + +/// Inner tracked fn: performs the actual accumulation. +#[salsa::tracked] +fn compute_single(db: &dyn salsa::Database, input: List) { + Integers(input.value(db)).accumulate(db); +} + +#[test] +fn accumulated_values_not_lost_after_partial_reuse() { + let mut db = salsa::DatabaseImpl::new(); + + let l0 = List::new(&db, 1, None); + let l1 = List::new(&db, 10, Some(l0)); + + compute(&db, l1); + expect![[r#" + [ + Integers( + 10, + ), + Integers( + 1, + ), + ] + "#]] + .assert_debug_eq(&compute::accumulated::(&db, l1)); + + // Mutate l1; l0 is unchanged so compute(l0) / compute_single(l0) should be reused. + // Reused memos must still report their accumulated values so callers don't skip them. + l1.set_value(&mut db).to(11); + compute(&db, l1); + expect![[r#" + [ + Integers( + 11, + ), + Integers( + 1, + ), + ] + "#]] + .assert_debug_eq(&compute::accumulated::(&db, l1)); +}