Fix bad-hash with in-place update

Author: MichaReiser

components/salsa-macro-rules/src/setup_tracked_struct.rs

@@ -141,20 +141,30 @@ macro_rules! setup_tracked_struct {
                     revisions: &mut Self::Revisions,
                     old_fields: *mut Self::Fields<$db_lt>,
                     new_fields: Self::Fields<$db_lt>,
-                ) {
+                ) -> bool {
                     use $zalsa::UpdateFallback as _;
                     unsafe {
                         $(
                             $crate::maybe_backdate!(
-                                $field_option,
-                                $field_ty,
-                                (*old_fields).$field_index,
-                                new_fields.$field_index,
-                                revisions[$field_index],
+                                $tracked_option,
+                                $tracked_ty,
+                                (*old_fields).$absolute_tracked_index,
+                                new_fields.$absolute_tracked_index,
+                                revisions[$absolute_tracked_index],
                                 current_revision,
                                 $zalsa,
                             );
-                        )*
+                        )*;
+
+                        // If any untracked field has changed, return `true`, indicating that the tracked struct
+                        // itself should be considered changed.
+                        $(
+                            $zalsa::UpdateDispatch::<$untracked_ty>::maybe_update(
+                                &mut (*old_fields).$absolute_untracked_index,
+                                new_fields.$absolute_untracked_index,
+                            )
+                            |
+                        )* false
                     }
                 }
             }

src/tracked_struct.rs

@@ -62,7 +62,10 @@ pub trait Configuration: Sized + 'static {
     fn new_revisions(current_revision: Revision) -> Self::Revisions;
 
     /// Update the field data and, if the value has changed,
-    /// the appropriate entry in the `revisions` array.
+    /// the appropriate entry in the `revisions` array (tracked fields only).
+    ///
+    /// Returns `true` if any untracked field was updated and
+    /// the struct should be considered re-created.
     ///
     /// # Safety
     ///
@@ -87,7 +90,7 @@ pub trait Configuration: Sized + 'static {
         revisions: &mut Self::Revisions,
         old_fields: *mut Self::Fields<'db>,
         new_fields: Self::Fields<'db>,
-    );
+    ) -> bool;
 }
 // ANCHOR_END: Configuration
 
@@ -535,15 +538,24 @@ where
         // its validity invariant and any owned content also continues
         // to meet its safety invariant.
         unsafe {
-            C::update_fields(
+            if C::update_fields(
                 current_revision,
                 &mut data.revisions,
                 self.to_self_ptr(std::ptr::addr_of_mut!(data.fields)),
                 fields,
-            );
+            ) {
+                // Consider this a new tracked-struct (even though it still uses the same id)
+                // when any non-tracked field got updated.
+                // This should be rare and only ever happen if there's a hash collision
+                // which makes Salsa consider two tracked structs to still be the same
+                // even though the fields are different.
+                // See `tracked-struct-id-field-bad-hash` for more details.
+                data.created_at = current_revision;
+            }
         }
         if current_deps.durability < data.durability {
             data.revisions = C::new_revisions(current_revision);
+            data.created_at = current_revision;
         }
         data.durability = current_deps.durability;
         let swapped_out = data.updated_at.swap(Some(current_revision));

tests/tracked-struct-id-field-bad-hash.rs

@@ -50,3 +50,31 @@ fn execute() {
     input.set_field(&mut db).to(false);
     the_fn(&db, input);
 }
+
+#[salsa::tracked]
+fn create_tracked<'db>(db: &'db dyn Db, input: MyInput) -> MyTracked<'db> {
+    MyTracked::new(db, BadHash::from(input.field(db)))
+}
+
+#[salsa::tracked]
+fn with_tracked<'db>(db: &'db dyn Db, tracked: MyTracked<'db>) -> bool {
+    tracked.field(db).field
+}
+
+#[test]
+fn dependent_query() {
+    let mut db = salsa::DatabaseImpl::new();
+
+    let input = MyInput::new(&db, true);
+    let tracked = create_tracked(&db, input);
+    assert!(with_tracked(&db, tracked));
+
+    input.set_field(&mut db).to(false);
+    // We now re-run the query that creates the tracked struct.
+    // Salsa will re-use the `MyTracked` struct from the previous revision
+    // because it thinks it is unchanged because of `BadHash`'s bad hash function.
+    // That's why Salsa updates the `MyTracked` struct in-place and the struct
+    // should be considered re-created even though it still has the same id.
+    let tracked = create_tracked(&db, input);
+    assert!(!with_tracked(&db, tracked));
+}