require reset for new revisions

src/accumulator.rs

@@ -8,7 +8,7 @@ use std::{
 use crate::{
     cycle::CycleRecoveryStrategy,
     hash::FxDashMap,
-    ingredient::{fmt_index, Ingredient, IngredientRequiresReset, Jar},
+    ingredient::{fmt_index, Ingredient, Jar},
     key::DependencyIndex,
     runtime::local_state::QueryOrigin,
     storage::IngredientIndex,
@@ -183,6 +183,10 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
         }
     }
 
+    fn requires_reset_for_new_revision(&self) -> bool {
+        false
+    }
+
     fn reset_for_new_revision(&mut self) {
         panic!("unexpected reset on accumulator")
     }
@@ -196,10 +200,6 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
     }
 }
 
-impl<A: Accumulator> IngredientRequiresReset for IngredientImpl<A> {
-    const RESET_ON_NEW_REVISION: bool = false;
-}
-
 impl<A> std::fmt::Debug for IngredientImpl<A>
 where
     A: Accumulator,

src/function.rs

@@ -4,7 +4,7 @@ use crossbeam::atomic::AtomicCell;
 
 use crate::{
     cycle::CycleRecoveryStrategy,
-    ingredient::{fmt_index, IngredientRequiresReset},
+    ingredient::fmt_index,
     key::DatabaseKeyIndex,
     runtime::local_state::QueryOrigin,
     salsa_struct::SalsaStructInDb,
@@ -254,6 +254,10 @@ where
         // Since its `verified_at` field has not changed, it will be considered dirty if it is invoked.
     }
 
+    fn requires_reset_for_new_revision(&self) -> bool {
+        true
+    }
+
     fn reset_for_new_revision(&mut self) {
         std::mem::take(&mut self.deleted_entries);
     }
@@ -293,10 +297,3 @@ where
             .finish()
     }
 }
-
-impl<C> IngredientRequiresReset for IngredientImpl<C>
-where
-    C: Configuration,
-{
-    const RESET_ON_NEW_REVISION: bool = true;
-}

src/ingredient.rs

@@ -65,6 +65,10 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync {
     /// since only function ingredients push themselves onto the active query stack.)
     fn cycle_recovery_strategy(&self) -> CycleRecoveryStrategy;
 
+    /// Returns true if `reset_for_new_revision` should be called when new revisions start.
+    /// Invoked once when ingredient is added and not after that.
+    fn requires_reset_for_new_revision(&self) -> bool;
+
     /// Invoked when a new revision is about to start.
     /// This moment is important because it means that we have an `&mut`-reference to the
     /// database, and hence any pre-existing `&`-references must have expired.
@@ -128,11 +132,3 @@ pub(crate) fn fmt_index(
         write!(fmt, "{}()", debug_name)
     }
 }
-
-/// Defines a const indicating if an ingredient needs to be reset each round.
-/// This const probably *should* be a member of `Ingredient` trait but then `Ingredient` would
-/// not be dyn-safe.
-pub trait IngredientRequiresReset {
-    /// If this is true, then `reset_for_new_revision` will be called every new revision.
-    const RESET_ON_NEW_REVISION: bool;
-}

src/input.rs

@@ -15,7 +15,7 @@ use struct_map::StructMap;
 use crate::{
     cycle::CycleRecoveryStrategy,
     id::{AsId, FromId},
-    ingredient::{fmt_index, Ingredient, IngredientRequiresReset},
+    ingredient::{fmt_index, Ingredient},
     key::{DatabaseKeyIndex, DependencyIndex},
     plumbing::{Jar, Stamp},
     runtime::{local_state::QueryOrigin, Runtime},
@@ -225,6 +225,10 @@ impl<C: Configuration> Ingredient for IngredientImpl<C> {
         );
     }
 
+    fn requires_reset_for_new_revision(&self) -> bool {
+        false
+    }
+
     fn reset_for_new_revision(&mut self) {
         panic!("unexpected call to `reset_for_new_revision`")
     }
@@ -240,10 +244,6 @@ impl<C: Configuration> Ingredient for IngredientImpl<C> {
     }
 }
 
-impl<C: Configuration> IngredientRequiresReset for IngredientImpl<C> {
-    const RESET_ON_NEW_REVISION: bool = false;
-}
-
 impl<C: Configuration> std::fmt::Debug for IngredientImpl<C> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.debug_struct(std::any::type_name::<Self>())

src/input/input_field.rs

@@ -1,5 +1,5 @@
 use crate::cycle::CycleRecoveryStrategy;
-use crate::ingredient::{fmt_index, Ingredient, IngredientRequiresReset};
+use crate::ingredient::{fmt_index, Ingredient};
 use crate::input::Configuration;
 use crate::runtime::local_state::QueryOrigin;
 use crate::storage::IngredientIndex;
@@ -86,6 +86,10 @@ where
         panic!("unexpected call: input fields are never deleted");
     }
 
+    fn requires_reset_for_new_revision(&self) -> bool {
+        false
+    }
+
     fn reset_for_new_revision(&mut self) {
         panic!("unexpected call: input fields don't register for resets");
     }
@@ -95,13 +99,6 @@ where
     }
 }
 
-impl<C> IngredientRequiresReset for FieldIngredientImpl<C>
-where
-    C: Configuration,
-{
-    const RESET_ON_NEW_REVISION: bool = false;
-}
-
 impl<C> std::fmt::Debug for FieldIngredientImpl<C>
 where
     C: Configuration,

src/interned.rs

@@ -7,7 +7,7 @@ use std::ptr::NonNull;
 use crate::alloc::Alloc;
 use crate::durability::Durability;
 use crate::id::AsId;
-use crate::ingredient::{fmt_index, IngredientRequiresReset};
+use crate::ingredient::fmt_index;
 use crate::key::DependencyIndex;
 use crate::plumbing::Jar;
 use crate::runtime::local_state::QueryOrigin;
@@ -253,6 +253,10 @@ where
         );
     }
 
+    fn requires_reset_for_new_revision(&self) -> bool {
+        false
+    }
+
     fn reset_for_new_revision(&mut self) {
         // Interned ingredients do not, normally, get deleted except when they are "reset" en masse.
         // There ARE methods (e.g., `clear_deleted_entries` and `remove`) for deleting individual
@@ -269,13 +273,6 @@ where
     }
 }
 
-impl<C> IngredientRequiresReset for IngredientImpl<C>
-where
-    C: Configuration,
-{
-    const RESET_ON_NEW_REVISION: bool = false;
-}
-
 impl<C> std::fmt::Debug for IngredientImpl<C>
 where
     C: Configuration,

src/storage.rs

@@ -236,6 +236,9 @@ struct Shared<Db: Database> {
     ///
     /// Immutable unless the mutex on `ingredients_map` is held.
     ingredients_vec: ConcurrentVec<Box<dyn Ingredient>>,
+
+    /// Indices of ingredients that require reset when a new revision starts.
+    ingredients_requiring_reset: ConcurrentVec<IngredientIndex>,
 }
 
 // ANCHOR: default
@@ -247,6 +250,7 @@ impl<Db: Database> Default for Storage<Db> {
                 nonce: NONCE.nonce(),
                 jar_map: Default::default(),
                 ingredients_vec: Default::default(),
+                ingredients_requiring_reset: Default::default(),
             },
             runtime: Runtime::default(),
         }
@@ -276,6 +280,11 @@ impl<Db: Database> Storage<Db> {
             let ingredients = jar.create_ingredients(index);
             for ingredient in ingredients {
                 let expected_index = ingredient.ingredient_index();
+
+                if ingredient.requires_reset_for_new_revision() {
+                    self.shared.ingredients_requiring_reset.push(expected_index);
+                }
+
                 let actual_index = self
                     .shared
                     .ingredients_vec
@@ -288,6 +297,7 @@ impl<Db: Database> Storage<Db> {
                     expected_index,
                     actual_index,
                 );
+                
             }
             index
         })
@@ -308,6 +318,10 @@ impl<Db: Database> Storage<Db> {
     ) -> (&mut dyn Ingredient, &mut Runtime) {
         self.runtime.new_revision();
 
+        for index in self.shared.ingredients_requiring_reset.iter() {
+            self.shared.ingredients_vec.get_mut(index.as_usize()).unwrap().reset_for_new_revision();
+        }
+
         (
             &mut **self
                 .shared
@@ -337,7 +351,7 @@ unsafe impl<I> Sync for IngredientCache<I> where I: Ingredient + Sync {}
 impl<I> Default for IngredientCache<I>
 where
     I: Ingredient,
- {
+{
     fn default() -> Self {
         Self::new()
     }

src/tracked_struct.rs

@@ -8,7 +8,7 @@ use crate::{
     cycle::CycleRecoveryStrategy,
     hash::FxDashMap,
     id::AsId,
-    ingredient::{fmt_index, Ingredient, IngredientRequiresReset, Jar},
+    ingredient::{fmt_index, Ingredient, Jar},
     ingredient_list::IngredientList,
     key::{DatabaseKeyIndex, DependencyIndex},
     runtime::{local_state::QueryOrigin, Runtime},
@@ -470,6 +470,10 @@ where
         self.delete_entity(db.as_salsa_database(), stale_output_key.unwrap());
     }
 
+    fn requires_reset_for_new_revision(&self) -> bool {
+        true
+    }
+
     fn reset_for_new_revision(&mut self) {
         self.struct_map.drop_deleted_entries();
     }
@@ -494,13 +498,6 @@ where
     }
 }
 
-impl<C> IngredientRequiresReset for IngredientImpl<C>
-where
-    C: Configuration,
-{
-    const RESET_ON_NEW_REVISION: bool = true;
-}
-
 impl<C> Value<C>
 where
     C: Configuration,

src/tracked_struct/tracked_field.rs

@@ -1,9 +1,6 @@
 use crate::{
-    id::AsId,
-    ingredient::{Ingredient, IngredientRequiresReset},
-    key::DependencyIndex,
-    storage::IngredientIndex,
-    Database, Id, Runtime,
+    id::AsId, ingredient::Ingredient, key::DependencyIndex, storage::IngredientIndex, Database, Id,
+    Runtime,
 };
 
 use super::{struct_map::StructMapView, Configuration};
@@ -119,6 +116,10 @@ where
         panic!("tracked field ingredients are not registered as dependent")
     }
 
+    fn requires_reset_for_new_revision(&self) -> bool {
+        false
+    }
+
     fn reset_for_new_revision(&mut self) {
         panic!("tracked field ingredients do not require reset")
     }
@@ -149,10 +150,3 @@ where
             .finish()
     }
 }
-
-impl<C> IngredientRequiresReset for FieldIngredientImpl<C>
-where
-    C: Configuration,
-{
-    const RESET_ON_NEW_REVISION: bool = false;
-}

tests/deletion-drops.rs

@@ -0,0 +1,99 @@
+//! Basic deletion test:
+//!
+//! * entities not created in a revision are deleted, as is any memoized data keyed on them.
+
+mod common;
+
+use salsa::{Database, Setter};
+use test_log::test;
+
+#[salsa::input]
+struct MyInput {
+    identity: u32,
+}
+
+#[salsa::tracked]
+struct MyTracked<'db> {
+    #[id]
+    identifier: u32,
+
+    #[return_ref]
+    field: Bomb,
+}
+
+thread_local! {
+    static DROPPED: std::cell::RefCell<Vec<u32>> = std::cell::RefCell::new(vec![]);
+}
+
+fn dropped() -> Vec<u32> {
+    DROPPED.with(|d| d.borrow().clone())
+}
+
+#[derive(Clone, Debug, PartialEq, Eq)]
+struct Bomb {
+    identity: u32,
+}
+
+impl Drop for Bomb {
+    fn drop(&mut self) {
+        DROPPED.with(|d| d.borrow_mut().push(self.identity));
+    }
+}
+
+#[salsa::tracked]
+impl MyInput {
+    #[salsa::tracked]
+    fn create_tracked_struct(self, db: &dyn Database) -> MyTracked<'_> {
+        MyTracked::new(
+            db,
+            self.identity(db),
+            Bomb {
+                identity: self.identity(db),
+            },
+        )
+    }
+}
+
+#[test]
+fn deletion_drops() {
+    let mut db = salsa::default_database();
+
+    let input = MyInput::new(&db, 22);
+
+    expect_test::expect![[r#"
+        []
+    "#]]
+    .assert_debug_eq(&dropped());
+
+    let tracked_struct = input.create_tracked_struct(&db);
+    assert_eq!(tracked_struct.field(&db).identity, 22);
+
+    expect_test::expect![[r#"
+        []
+    "#]]
+    .assert_debug_eq(&dropped());
+
+    input.set_identity(&mut db).to(44);
+
+    expect_test::expect![[r#"
+        []
+    "#]]
+    .assert_debug_eq(&dropped());
+
+    let tracked_struct = input.create_tracked_struct(&db);
+    assert_eq!(tracked_struct.field(&db).identity, 44);
+
+    expect_test::expect![[r#"
+        []
+    "#]]
+    .assert_debug_eq(&dropped());
+
+    input.set_identity(&mut db).to(66);
+
+    expect_test::expect![[r#"
+        [
+            22,
+        ]
+    "#]]
+    .assert_debug_eq(&dropped());
+}