From df889128208b915b678c50abbb329658823ef6cb Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 7 Jun 2026 16:16:21 +0200 Subject: [PATCH 1/2] feat: add `previous()` to access a tracked function's prior memoized value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a generated `previous()` associated function on every tracked function, returning a reference to the value memoized for the active key in the previous revision (or `None` if the function has never completed for that key, or its prior result is provisional). This lets a tracked function build its new result on top of its own prior result while re-executing. Reading the previous value replays that result's dependencies into the current execution — durability, dependency edges, untracked-read flag, changed-at revision, and tracked-struct ids — so incremental validation stays correct and previously-created tracked structs remain live. --- .../salsa-macro-rules/src/setup_tracked_fn.rs | 30 ++ src/active_query.rs | 32 +- src/function.rs | 1 + src/function/previous.rs | 55 ++ src/input.rs | 1 + src/table/memo.rs | 1 + src/tracked_struct.rs | 1 + src/zalsa_local.rs | 29 +- tests/tracked_fn_previous.rs | 491 ++++++++++++++++++ 9 files changed, 638 insertions(+), 3 deletions(-) create mode 100644 src/function/previous.rs create mode 100644 tests/tracked_fn_previous.rs diff --git a/components/salsa-macro-rules/src/setup_tracked_fn.rs b/components/salsa-macro-rules/src/setup_tracked_fn.rs index 5fb3142bb..2ecc93e1e 100644 --- a/components/salsa-macro-rules/src/setup_tracked_fn.rs +++ b/components/salsa-macro-rules/src/setup_tracked_fn.rs @@ -469,6 +469,36 @@ macro_rules! setup_tracked_fn { } } + /// Returns a reference to this function's memoized value from the previous + /// revision, or `None` if there is no usable previous value. + /// + /// This is only meaningful while the function is re-executing for the current + /// key: it lets the new computation build on its own prior result. Reading the + /// previous value replays that result's dependencies into the current execution, + /// so incremental validation stays correct. + /// + /// Returns `None` when the function has never completed for this key, when the + /// previous result is provisional (for example, mid-cycle), or when the memoized + /// value has been evicted (for example, due to an LRU capacity limit). + /// + /// Values accumulated by the previous execution itself are not replayed; a + /// function that accumulates must push its values again even when it returns + /// its previous result. + /// + /// # Panics + /// + /// Panics if called outside of this tracked function's own execution. + pub fn previous<$db_lt>( + $db: &$db_lt dyn $Db, + ) -> Option<&$db_lt $output_ty> { + use ::salsa::plumbing as $zalsa; + + $zalsa::attach($db, || { + let (zalsa, zalsa_local) = $db.zalsas(); + $Configuration::fn_ingredient_($db, zalsa).previous(zalsa, zalsa_local) + }) + } + $zalsa::macro_if! { $is_specifiable => pub fn specify<$db_lt>( $db: &$db_lt dyn $Db, diff --git a/src/active_query.rs b/src/active_query.rs index a67f0c69d..fa7da4cc5 100644 --- a/src/active_query.rs +++ b/src/active_query.rs @@ -11,7 +11,9 @@ use crate::key::DatabaseKeyIndex; use crate::runtime::Stamp; use crate::sync::atomic::AtomicBool; use crate::tracked_struct::{Disambiguator, DisambiguatorMap, IdentityHash, IdentityMap}; -use crate::zalsa_local::{OriginAndExtra, QueryEdge, QueryRevisions, QueryRevisionsExtra}; +use crate::zalsa_local::{ + OriginAndExtra, QueryEdge, QueryEdges, QueryRevisions, QueryRevisionsExtra, +}; use crate::{ Id, cycle::{CycleHeads, IterationStamp}, @@ -72,7 +74,7 @@ impl ActiveQuery { &mut self, durability: Durability, changed_at: Revision, - edges: crate::zalsa_local::QueryEdges<'_>, + edges: QueryEdges<'_>, untracked_read: bool, active_tracked_ids: &[(Identity, Id)], ) { @@ -93,6 +95,32 @@ impl ActiveQuery { .seed(active_tracked_ids.iter().map(|(id, _)| id)); } + pub(super) fn add_previous_memo_read( + &mut self, + durability: Durability, + current_revision: Revision, + edges: QueryEdges<'_>, + untracked_read: bool, + tracked_struct_ids: &[(Identity, Id)], + #[cfg(feature = "accumulator")] accumulated_inputs: InputAccumulatedValues, + ) { + self.input_outputs.extend(edges); + self.durability = self.durability.min(durability); + self.changed_at = self.changed_at.max(current_revision); + self.untracked_read |= untracked_read; + #[cfg(feature = "accumulator")] + { + self.accumulated_inputs |= accumulated_inputs; + } + + // Mark all tracked structs from the previous revision as active and reserve + // their identities so that newly created structs receive later disambiguators. + self.tracked_struct_ids + .mark_all_active(tracked_struct_ids.iter().copied()); + self.disambiguator_map + .seed(tracked_struct_ids.iter().map(|(id, _)| id)); + } + pub(super) fn take_cycle_heads(&mut self) -> CycleHeads { std::mem::take(&mut self.cycle_heads) } diff --git a/src/function.rs b/src/function.rs index d66cf2f96..cf2bdfc59 100644 --- a/src/function.rs +++ b/src/function.rs @@ -34,6 +34,7 @@ mod fetch; mod inputs; mod maybe_changed_after; mod memo; +mod previous; mod specify; mod sync; diff --git a/src/function/previous.rs b/src/function/previous.rs new file mode 100644 index 000000000..72bc86985 --- /dev/null +++ b/src/function/previous.rs @@ -0,0 +1,55 @@ +use crate::function::{Configuration, IngredientImpl}; +use crate::zalsa::Zalsa; +use crate::zalsa_local::ZalsaLocal; + +impl IngredientImpl +where + C: Configuration, +{ + /// Returns the value memoized for the active key in the previous revision, + /// replaying that result's dependencies into the active query. + /// + /// # Panics + /// + /// Panics if the active query is not an execution of this tracked function. + pub fn previous<'db>( + &'db self, + zalsa: &'db Zalsa, + zalsa_local: &'db ZalsaLocal, + ) -> Option<&'db C::Output<'db>> { + let Some((database_key_index, _)) = zalsa_local.active_query() else { + panic!( + "cannot access previous memoized value for {} outside of its tracked function", + C::DEBUG_NAME, + ); + }; + + if database_key_index.ingredient_index() != self.index { + panic!( + "cannot access previous memoized value for {} while executing {database_key_index:?}", + C::DEBUG_NAME, + ); + } + + let id = database_key_index.key_index(); + let memo_ingredient_index = self.memo_ingredient_index(zalsa, id); + let memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index)?; + + if memo.header.may_be_provisional() { + return None; + } + + let value = memo.value.as_ref()?; + let revisions = &memo.header.revisions; + zalsa_local.report_previous_memo_read( + revisions.durability, + revisions.origin().edges(), + revisions.is_derived_untracked(), + revisions.tracked_struct_ids(), + #[cfg(feature = "accumulator")] + revisions.accumulated_inputs.load(), + zalsa.current_revision(), + ); + Some(value) + } +} diff --git a/src/input.rs b/src/input.rs index ee02fece5..e46e58780 100644 --- a/src/input.rs +++ b/src/input.rs @@ -272,6 +272,7 @@ pub struct StructEntry<'db, C> where C: Configuration, { + #[cfg_attr(not(feature = "salsa_unstable"), expect(dead_code))] value: &'db Value, key: DatabaseKeyIndex, } diff --git a/src/table/memo.rs b/src/table/memo.rs index 309b48e51..ce55111f5 100644 --- a/src/table/memo.rs +++ b/src/table/memo.rs @@ -111,6 +111,7 @@ impl LazyMemoEntries { self.as_mut_slice()?.get_mut(index) } + #[cfg(feature = "salsa_unstable")] fn iter(&self) -> std::slice::Iter<'_, MemoEntry> { self.as_slice().unwrap_or_default().iter() } diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs index dbe3b0d6c..e88343fb3 100644 --- a/src/tracked_struct.rs +++ b/src/tracked_struct.rs @@ -942,6 +942,7 @@ pub struct StructEntry<'db, C> where C: Configuration, { + #[cfg_attr(not(feature = "salsa_unstable"), expect(dead_code))] value: &'db Value, key: DatabaseKeyIndex, } diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index 7073087a4..c9ff6e94f 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -15,7 +15,7 @@ use thin_vec::ThinVec; #[cfg(feature = "accumulator")] use crate::accumulator::{ Accumulator, - accumulated_map::{AccumulatedMap, AtomicInputAccumulatedValues}, + accumulated_map::{AccumulatedMap, AtomicInputAccumulatedValues, InputAccumulatedValues}, }; use crate::active_query::{CompletedQuery, DetachedInputOutputs, QueryCompletion, QueryStack}; use crate::cycle::{AtomicIterationStamp, CycleHeads, IterationStamp, empty_cycle_heads}; @@ -392,6 +392,33 @@ impl ZalsaLocal { } } + pub(crate) fn report_previous_memo_read( + &self, + durability: Durability, + edges: QueryEdges<'_>, + untracked_read: bool, + tracked_struct_ids: &[(Identity, Id)], + #[cfg(feature = "accumulator")] accumulated_inputs: InputAccumulatedValues, + current_revision: Revision, + ) { + // SAFETY: We do not access the query stack reentrantly. + unsafe { + self.with_query_stack_unchecked_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.add_previous_memo_read( + durability, + current_revision, + edges, + untracked_read, + tracked_struct_ids, + #[cfg(feature = "accumulator")] + accumulated_inputs, + ); + } + }) + } + } + /// Register that the current query read an untracked value /// /// # Parameters diff --git a/tests/tracked_fn_previous.rs b/tests/tracked_fn_previous.rs new file mode 100644 index 000000000..fd1f215a5 --- /dev/null +++ b/tests/tracked_fn_previous.rs @@ -0,0 +1,491 @@ +#![cfg(feature = "inventory")] + +mod common; + +use common::{DiscardLoggerDatabase, LogDatabase, LoggerDatabase}; +use expect_test::expect; +use salsa::Setter; +use test_log::test; + +#[salsa::input(debug)] +struct Number { + #[returns(copy)] + value: u32, +} + +#[salsa::input(debug)] +struct Switch { + #[returns(copy)] + value: u32, + #[returns(copy)] + use_previous: bool, +} + +#[salsa::tracked] +struct PreviousNode<'db> { + #[returns(copy)] + value: u32, +} + +#[salsa::tracked] +struct NamedNode<'db> { + name: String, + #[tracked] + #[returns(copy)] + value: u32, +} + +#[salsa::tracked(returns(copy))] +fn previous_plus_one(db: &dyn LogDatabase, input: Number) -> u32 { + db.push_log("previous_plus_one".to_owned()); + + let Some(previous) = previous_plus_one::previous(db) else { + return input.value(db); + }; + + previous + 1 +} + +#[salsa::tracked(returns(copy))] +fn read_previous_or_value(db: &dyn LogDatabase, input: Switch) -> u32 { + db.push_log("read_previous_or_value".to_owned()); + + if input.use_previous(db) { + let previous = read_previous_or_value::previous(db).unwrap(); + return *previous; + } + + input.value(db) +} + +#[salsa::tracked(returns(copy))] +fn depends_on_previous_plus_one(db: &dyn LogDatabase, input: Number) -> u32 { + db.push_log("depends_on_previous_plus_one".to_owned()); + previous_plus_one(db, input) +} + +#[salsa::tracked(returns(copy))] +fn depends_on_read_previous_or_value(db: &dyn LogDatabase, input: Switch) -> u32 { + db.push_log("depends_on_read_previous_or_value".to_owned()); + read_previous_or_value(db, input) +} + +#[salsa::tracked] +fn previous_node(db: &dyn LogDatabase, input: Switch) -> PreviousNode<'_> { + db.push_log("previous_node".to_owned()); + + if input.use_previous(db) { + let previous = previous_node::previous(db).unwrap(); + return *previous; + } + + PreviousNode::new(db, input.value(db)) +} + +#[salsa::tracked] +fn previous_then_recreate_same_identity( + db: &dyn LogDatabase, + input: Switch, +) -> (NamedNode<'_>, NamedNode<'_>) { + db.push_log("previous_then_recreate_same_identity".to_owned()); + + if input.use_previous(db) { + let previous = previous_then_recreate_same_identity::previous(db).unwrap(); + let previous = previous.0; + previous.value(db); + let current = NamedNode::new(db, "node".to_owned(), input.value(db)); + return (previous, current); + } + + let node = NamedNode::new(db, "node".to_owned(), input.value(db)); + (node, node) +} + +#[salsa::tracked] +fn recreate_same_identity_then_previous( + db: &dyn LogDatabase, + input: Switch, +) -> (NamedNode<'_>, NamedNode<'_>) { + db.push_log("recreate_same_identity_then_previous".to_owned()); + + if input.use_previous(db) { + let current = NamedNode::new(db, "node".to_owned(), input.value(db)); + let previous = recreate_same_identity_then_previous::previous(db).unwrap(); + let previous = previous.0; + return (current, previous); + } + + let node = NamedNode::new(db, "node".to_owned(), input.value(db)); + (node, node) +} + +#[salsa::tracked] +fn duplicate_nodes_after_previous(db: &dyn LogDatabase, input: Switch) -> Vec> { + db.push_log("duplicate_nodes_after_previous".to_owned()); + + if input.use_previous(db) { + let previous = duplicate_nodes_after_previous::previous(db).unwrap(); + let first = previous[0]; + let second = previous[1]; + let third = NamedNode::new(db, "same".to_owned(), input.value(db)); + let fourth = NamedNode::new(db, "same".to_owned(), input.value(db) + 1); + return vec![first, second, third, fourth]; + } + + vec![ + NamedNode::new(db, "same".to_owned(), 10), + NamedNode::new(db, "same".to_owned(), 11), + ] +} + +#[salsa::tracked] +fn mixed_order_duplicate_nodes(db: &dyn LogDatabase, input: Switch) -> Vec> { + db.push_log("mixed_order_duplicate_nodes".to_owned()); + + if input.use_previous(db) { + let first = NamedNode::new(db, "same".to_owned(), input.value(db)); + let previous = mixed_order_duplicate_nodes::previous(db).unwrap(); + let second = previous[0]; + let third = previous[1]; + let fourth = NamedNode::new(db, "same".to_owned(), input.value(db) + 1); + return vec![first, second, third, fourth]; + } + + vec![ + NamedNode::new(db, "same".to_owned(), 10), + NamedNode::new(db, "same".to_owned(), 11), + ] +} + +#[salsa::tracked] +fn maybe_previous_node(db: &dyn LogDatabase, input: Switch) -> Option> { + db.push_log("maybe_previous_node".to_owned()); + + if input.use_previous(db) { + let previous = maybe_previous_node::previous(db).unwrap(); + return *previous; + } + + if input.value(db) == 0 { + return None; + } + + Some(NamedNode::new(db, "node".to_owned(), input.value(db))) +} + +#[cfg(feature = "accumulator")] +#[salsa::accumulator] +#[derive(Debug)] +struct Note(u32); + +#[cfg(feature = "accumulator")] +#[salsa::tracked(returns(copy))] +fn accumulates_note(db: &dyn LogDatabase, input: Number) -> u32 { + db.push_log("accumulates_note".to_owned()); + salsa::Accumulator::accumulate(Note(input.value(db)), db); + input.value(db) +} + +#[cfg(feature = "accumulator")] +#[salsa::tracked(returns(copy))] +fn previous_with_accumulating_dep(db: &dyn LogDatabase, switch: Switch, input: Number) -> u32 { + db.push_log("previous_with_accumulating_dep".to_owned()); + + if switch.use_previous(db) { + let previous = previous_with_accumulating_dep::previous(db).unwrap(); + return *previous; + } + + accumulates_note(db, input) +} + +#[salsa::tracked] +fn source_node(db: &dyn LogDatabase, input: Number) -> NamedNode<'_> { + db.push_log("source_node".to_owned()); + NamedNode::new(db, "source".to_owned(), input.value(db)) +} + +#[salsa::tracked(returns(copy))] +fn read_source_or_previous(db: &dyn LogDatabase, switch: Switch, input: Number) -> u32 { + db.push_log("read_source_or_previous".to_owned()); + + if switch.use_previous(db) { + let previous = read_source_or_previous::previous(db).unwrap(); + return *previous; + } + + source_node(db, input).value(db) +} + +#[test] +fn previous_value_is_available_as_a_reference() { + let mut db = LoggerDatabase::default(); + let input = Number::new(&db, 1); + + assert_eq!(previous_plus_one(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "previous_plus_one", + ]"#]]); + + input.set_value(&mut db).to(2); + + assert_eq!(previous_plus_one(&db, input), 2); + db.assert_logs(expect![[r#" + [ + "previous_plus_one", + ]"#]]); +} + +#[test] +fn previous_value_replays_previous_dependencies() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + assert_eq!(read_previous_or_value(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "read_previous_or_value", + ]"#]]); + + input.set_use_previous(&mut db).to(true); + assert_eq!(read_previous_or_value(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "read_previous_or_value", + ]"#]]); + + input.set_value(&mut db).to(2); + assert_eq!(read_previous_or_value(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "read_previous_or_value", + ]"#]]); +} + +#[test] +fn previous_value_marks_current_query_changed() { + let mut db = LoggerDatabase::default(); + let input = Number::new(&db, 1); + + assert_eq!(depends_on_previous_plus_one(&db, input), 1); + db.clear_logs(); + + input.set_value(&mut db).to(2); + + assert_eq!(depends_on_previous_plus_one(&db, input), 2); + db.assert_logs(expect![[r#" + [ + "previous_plus_one", + "depends_on_previous_plus_one", + ]"#]]); +} + +#[test] +fn previous_value_preserves_early_cutoff_for_dependents() { + // A query that returns its own previous value keeps a stable output, so backdating must + // pull its `changed_at` back and spare dependents from re-executing, even though the + // query itself re-runs because an input it reads changed. + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + assert_eq!(depends_on_read_previous_or_value(&db, input), 1); + db.clear_logs(); + + input.set_use_previous(&mut db).to(true); + assert_eq!(depends_on_read_previous_or_value(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "read_previous_or_value", + ]"#]]); +} + +#[test] +fn previous_tracked_struct_output_remains_live() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + let first = previous_node(&db, input); + assert_eq!(first.value(&db), 1); + db.clear_logs(); + + input.set_use_previous(&mut db).to(true); + let second = previous_node(&db, input); + + assert_eq!(second.value(&db), 1); + db.assert_logs(expect![[r#" + [ + "previous_node", + ]"#]]); +} + +#[test] +fn previous_read_before_recreating_same_identity_preserves_old_tracked_field() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + let nodes = previous_then_recreate_same_identity(&db, input); + assert_eq!((nodes.0.value(&db), nodes.1.value(&db)), (1, 1)); + db.clear_logs(); + + input.set_value(&mut db).to(2); + input.set_use_previous(&mut db).to(true); + + let nodes = previous_then_recreate_same_identity(&db, input); + assert_eq!((nodes.0.value(&db), nodes.1.value(&db)), (1, 2)); + db.assert_logs(expect![[r#" + [ + "previous_then_recreate_same_identity", + ]"#]]); +} + +#[test] +fn recreating_same_identity_before_previous_updates_previous_entity() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + let nodes = recreate_same_identity_then_previous(&db, input); + assert_eq!((nodes.0.value(&db), nodes.1.value(&db)), (1, 1)); + db.clear_logs(); + + input.set_value(&mut db).to(2); + input.set_use_previous(&mut db).to(true); + + let nodes = recreate_same_identity_then_previous(&db, input); + assert_eq!((nodes.0.value(&db), nodes.1.value(&db)), (2, 2)); + db.assert_logs(expect![[r#" + [ + "recreate_same_identity_then_previous", + ]"#]]); +} + +#[test] +fn previous_disambiguators_prevent_duplicate_identity_collisions() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 20, false); + + let nodes = duplicate_nodes_after_previous(&db, input); + assert_eq!( + nodes.iter().map(|node| node.value(&db)).collect::>(), + [10, 11] + ); + db.clear_logs(); + + input.set_use_previous(&mut db).to(true); + let nodes = duplicate_nodes_after_previous(&db, input); + + assert_eq!( + nodes.iter().map(|node| node.value(&db)).collect::>(), + [10, 11, 20, 21] + ); + db.assert_logs(expect![[r#" + [ + "duplicate_nodes_after_previous", + ]"#]]); +} + +#[test] +fn duplicate_identity_before_previous_documents_live_entity_aliasing() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 20, false); + + let nodes = mixed_order_duplicate_nodes(&db, input); + assert_eq!( + nodes.iter().map(|node| node.value(&db)).collect::>(), + [10, 11] + ); + db.clear_logs(); + + input.set_use_previous(&mut db).to(true); + let nodes = mixed_order_duplicate_nodes(&db, input); + + assert_eq!( + nodes.iter().map(|node| node.value(&db)).collect::>(), + [20, 20, 11, 21] + ); + db.assert_logs(expect![[r#" + [ + "mixed_order_duplicate_nodes", + ]"#]]); +} + +#[test] +fn previous_liveness_is_not_permanent() { + let mut db = DiscardLoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + assert_eq!(maybe_previous_node(&db, input).unwrap().value(&db), 1); + db.clear_logs(); + + input.set_use_previous(&mut db).to(true); + assert_eq!(maybe_previous_node(&db, input).unwrap().value(&db), 1); + db.assert_logs(expect![[r#" + [ + "maybe_previous_node", + ]"#]]); + + input.set_use_previous(&mut db).to(false); + input.set_value(&mut db).to(0); + assert!(maybe_previous_node(&db, input).is_none()); + db.assert_logs(expect![[r#" + [ + "maybe_previous_node", + "salsa_event(WillDiscardStaleOutput { execute_key: maybe_previous_node(Id(0)), output_key: NamedNode(Id(400)) })", + "salsa_event(DidDiscard { key: NamedNode(Id(400)) })", + ]"#]]); +} + +#[test] +fn previous_replays_tracked_field_dependencies() { + let mut db = LoggerDatabase::default(); + let switch = Switch::new(&db, 0, false); + let input = Number::new(&db, 1); + + assert_eq!(read_source_or_previous(&db, switch, input), 1); + db.clear_logs(); + + switch.set_use_previous(&mut db).to(true); + assert_eq!(read_source_or_previous(&db, switch, input), 1); + db.clear_logs(); + + input.set_value(&mut db).to(2); + assert_eq!(read_source_or_previous(&db, switch, input), 1); + db.assert_logs(expect![[r#" + [ + "source_node", + "read_source_or_previous", + ]"#]]); +} + +#[cfg(feature = "accumulator")] +#[test] +fn previous_replays_accumulated_inputs() { + let mut db = LoggerDatabase::default(); + let switch = Switch::new(&db, 0, false); + let input = Number::new(&db, 1); + + assert_eq!(previous_with_accumulating_dep(&db, switch, input), 1); + let notes = previous_with_accumulating_dep::accumulated::(&db, switch, input); + assert_eq!(notes.len(), 1); + assert_eq!(notes[0].0, 1); + db.clear_logs(); + + switch.set_use_previous(&mut db).to(true); + assert_eq!(previous_with_accumulating_dep(&db, switch, input), 1); + let notes = previous_with_accumulating_dep::accumulated::(&db, switch, input); + assert_eq!(notes.len(), 1); + assert_eq!(notes[0].0, 1); + db.assert_logs(expect![[r#" + [ + "previous_with_accumulating_dep", + ]"#]]); +} + +#[test] +#[should_panic( + expected = "cannot access previous memoized value for previous_plus_one outside of its tracked function" +)] +fn previous_panics_outside_of_same_query() { + let db = LoggerDatabase::default(); + previous_plus_one::previous(&db); +} From d0890528ec2cb6b2f843a52d337c9593389c2a2b Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 3 Jul 2026 13:24:05 +0200 Subject: [PATCH 2/2] Add more tests --- .../salsa-macro-rules/src/setup_tracked_fn.rs | 4 + tests/tracked_fn_previous.rs | 259 +++++++++++++++++- 2 files changed, 262 insertions(+), 1 deletion(-) diff --git a/components/salsa-macro-rules/src/setup_tracked_fn.rs b/components/salsa-macro-rules/src/setup_tracked_fn.rs index 2ecc93e1e..f8efde2f1 100644 --- a/components/salsa-macro-rules/src/setup_tracked_fn.rs +++ b/components/salsa-macro-rules/src/setup_tracked_fn.rs @@ -485,6 +485,10 @@ macro_rules! setup_tracked_fn { /// function that accumulates must push its values again even when it returns /// its previous result. /// + /// For a specifiable function, the previous value may have been assigned via + /// `specify` rather than computed by this function; the two cases are + /// indistinguishable here. + /// /// # Panics /// /// Panics if called outside of this tracked function's own execution. diff --git a/tests/tracked_fn_previous.rs b/tests/tracked_fn_previous.rs index fd1f215a5..74e9e2793 100644 --- a/tests/tracked_fn_previous.rs +++ b/tests/tracked_fn_previous.rs @@ -4,7 +4,7 @@ mod common; use common::{DiscardLoggerDatabase, LogDatabase, LoggerDatabase}; use expect_test::expect; -use salsa::Setter; +use salsa::{Database as _, Durability, Setter}; use test_log::test; #[salsa::input(debug)] @@ -173,6 +173,113 @@ fn maybe_previous_node(db: &dyn LogDatabase, input: Switch) -> Option u32 { + previous_plus_one::previous(db); + input.value(db) +} + +#[salsa::tracked(returns(copy), cycle_fn=count_recover, cycle_initial=count_initial)] +fn count_to_three(db: &dyn LogDatabase, input: Number) -> u32 { + db.push_log("count_to_three".to_owned()); + + assert!( + count_to_three::previous(db).is_none(), + "provisional memos must not be exposed as previous values" + ); + + let current = count_to_three(db, input); + if current < input.value(db) { + current + 1 + } else { + current + } +} + +fn count_initial(_db: &dyn LogDatabase, _id: salsa::Id, _input: Number) -> u32 { + 0 +} + +fn count_recover( + _db: &dyn LogDatabase, + _cycle: &salsa::Cycle, + _last_provisional_value: &u32, + value: u32, + _input: Number, +) -> u32 { + value +} + +#[salsa::tracked(returns(copy), lru = 1)] +fn lru_previous_or_value(db: &dyn LogDatabase, input: Number) -> u32 { + db.push_log("lru_previous_or_value".to_owned()); + + match lru_previous_or_value::previous(db) { + Some(previous) => previous + 100, + None => input.value(db), + } +} + +#[salsa::tracked(returns(copy))] +fn untracked_or_previous(db: &dyn LogDatabase, input: Switch) -> u32 { + db.push_log("untracked_or_previous".to_owned()); + + if input.use_previous(db) { + let previous = untracked_or_previous::previous(db).unwrap(); + return *previous; + } + + db.report_untracked_read(); + input.value(db) +} + +#[salsa::tracked(returns(copy))] +fn durable_previous_or_value(db: &dyn LogDatabase, switch: Switch, input: Number) -> u32 { + db.push_log("durable_previous_or_value".to_owned()); + + if switch.use_previous(db) { + let previous = durable_previous_or_value::previous(db).unwrap(); + return *previous; + } + + input.value(db) +} + +#[salsa::tracked(returns(copy))] +fn previous_twice(db: &dyn LogDatabase, input: Switch) -> u32 { + db.push_log("previous_twice".to_owned()); + + if input.use_previous(db) { + let first = *previous_twice::previous(db).unwrap(); + let second = *previous_twice::previous(db).unwrap(); + assert_eq!(first, second); + return first; + } + + input.value(db) +} + +#[salsa::tracked] +fn make_node(db: &dyn LogDatabase, input: Number) -> NamedNode<'_> { + db.push_log("make_node".to_owned()); + + let node = NamedNode::new(db, "specified".to_owned(), input.value(db)); + if input.value(db) < 10 { + specified_or_previous::specify(db, node, 100); + } + node +} + +#[salsa::tracked(returns(copy), specify)] +fn specified_or_previous<'db>(db: &'db dyn LogDatabase, node: NamedNode<'db>) -> u32 { + db.push_log("specified_or_previous".to_owned()); + + match specified_or_previous::previous(db) { + Some(previous) => previous + 1, + None => node.value(db), + } +} + #[cfg(feature = "accumulator")] #[salsa::accumulator] #[derive(Debug)] @@ -489,3 +596,153 @@ fn previous_panics_outside_of_same_query() { let db = LoggerDatabase::default(); previous_plus_one::previous(&db); } + +#[test] +#[should_panic( + expected = "cannot access previous memoized value for previous_plus_one while executing" +)] +fn previous_panics_inside_different_query() { + let db = LoggerDatabase::default(); + let input = Number::new(&db, 1); + reads_previous_of_other(&db, input); +} + +#[test] +fn previous_is_none_for_provisional_memos() { + // `count_to_three` asserts on every iteration that `previous()` returns `None`, + // covering both the missing-memo case (iteration one) and the provisional-memo + // case (later iterations). + let db = LoggerDatabase::default(); + let input = Number::new(&db, 3); + + assert_eq!(count_to_three(&db, input), 3); + db.assert_logs(expect![[r#" + [ + "count_to_three", + "count_to_three", + "count_to_three", + "count_to_three", + ]"#]]); +} + +#[test] +fn previous_is_none_after_lru_eviction() { + let mut db = LoggerDatabase::default(); + let evicted = Number::new(&db, 1); + let retained = Number::new(&db, 2); + + assert_eq!(lru_previous_or_value(&db, evicted), 1); + assert_eq!(lru_previous_or_value(&db, retained), 2); + db.clear_logs(); + + // The LRU (capacity one) evicts `evicted`'s value at the revision boundary; the memo + // survives without a value, so re-execution sees no previous value. + db.synthetic_write(Durability::LOW); + + assert_eq!(lru_previous_or_value(&db, evicted), 1); + db.assert_logs(expect![[r#" + [ + "lru_previous_or_value", + ]"#]]); +} + +#[test] +fn previous_replays_untracked_reads() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + assert_eq!(untracked_or_previous(&db, input), 1); + db.clear_logs(); + + input.set_use_previous(&mut db).to(true); + assert_eq!(untracked_or_previous(&db, input), 1); + db.clear_logs(); + + // The previous execution performed an untracked read, so the replayed memo must + // re-execute in every new revision even though no tracked input changed. + db.synthetic_write(Durability::HIGH); + assert_eq!(untracked_or_previous(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "untracked_or_previous", + ]"#]]); +} + +#[test] +fn previous_replays_low_durability() { + let mut db = LoggerDatabase::default(); + let switch = Switch::new(&db, 0, false); + switch + .set_use_previous(&mut db) + .with_durability(Durability::HIGH) + .to(false); + let input = Number::new(&db, 1); + + assert_eq!(durable_previous_or_value(&db, switch, input), 1); + db.clear_logs(); + + switch + .set_use_previous(&mut db) + .with_durability(Durability::HIGH) + .to(true); + assert_eq!(durable_previous_or_value(&db, switch, input), 1); + db.clear_logs(); + + // The new execution only read high-durability inputs, but the replayed previous + // result depended on the low-durability `input`. If the replay failed to lower the + // memo's durability, this low-durability change would shortcut validation and + // never re-execute the query. + input.set_value(&mut db).to(5); + assert_eq!(durable_previous_or_value(&db, switch, input), 1); + db.assert_logs(expect![[r#" + [ + "durable_previous_or_value", + ]"#]]); +} + +#[test] +fn previous_twice_in_one_execution_is_idempotent() { + let mut db = LoggerDatabase::default(); + let input = Switch::new(&db, 1, false); + + assert_eq!(previous_twice(&db, input), 1); + db.clear_logs(); + + input.set_use_previous(&mut db).to(true); + assert_eq!(previous_twice(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "previous_twice", + ]"#]]); + + input.set_value(&mut db).to(2); + assert_eq!(previous_twice(&db, input), 1); + db.assert_logs(expect![[r#" + [ + "previous_twice", + ]"#]]); +} + +#[test] +fn previous_sees_value_assigned_via_specify() { + let mut db = LoggerDatabase::default(); + let input = Number::new(&db, 1); + + let node = *make_node(&db, input); + assert_eq!(specified_or_previous(&db, node), 100); + db.assert_logs(expect![[r#" + [ + "make_node", + ]"#]]); + + // In the new revision the value is no longer specified, so the function executes + // for the first time; its "previous" value is the stale specify-assigned memo. + input.set_value(&mut db).to(20); + let node = *make_node(&db, input); + assert_eq!(specified_or_previous(&db, node), 101); + db.assert_logs(expect![[r#" + [ + "make_node", + "specified_or_previous", + ]"#]]); +}