use latest revision for dependencies on interned values (#908)

src/interned.rs

@@ -367,12 +367,13 @@ where
 
             // Record a dependency on the value.
             //
-            // Note that we can use `Revision::start()` here as the ID with the given generation
-            // is unique to this use of the interned slot.
+            // See `intern_id_cold` for why we need to use `current_revision` here. Note that just
+            // because this value was previously interned does not mean it was previously interned
+            // by *our query*, so the same considerations apply.
             zalsa_local.report_tracked_read_simple(
                 index,
                 value_shared.durability,
-                Revision::start(),
+                current_revision,
             );
 
             return value_shared.id;
@@ -442,15 +443,15 @@ where
                 last_interned_at,
             };
 
+            let index = self.database_key_index(value_shared.id);
+
             // Record a dependency on the new value.
             //
-            // Note that we can use `Revision::start()` here as we just incremented the ID generation,
-            // so it as if a new input has been created.
-            let index = self.database_key_index(value_shared.id);
+            // See `intern_id_cold` for why we need to use `current_revision` here.
             zalsa_local.report_tracked_read_simple(
                 index,
                 value_shared.durability,
-                Revision::start(),
+                current_revision,
             );
 
             zalsa.event(&|| {
@@ -603,7 +604,14 @@ where
         let index = self.database_key_index(id);
 
         // Record a dependency on the newly interned value.
-        zalsa_local.report_tracked_read_simple(index, durability, Revision::start());
+        //
+        // Note that the ID is unique to this use of the interned slot, so it seems logical to use
+        // `Revision::start()` here. However, it is possible that the ID we read is different from
+        // the previous execution of this query if the previous slot has been reused. In that case,
+        // the query has changed without a corresponding input changing. Using `current_revision`
+        // for dependencies on interned values encodes the fact that interned IDs are not stable
+        // across revisions.
+        zalsa_local.report_tracked_read_simple(index, durability, current_revision);
 
         zalsa.event(&|| {
             Event::new(EventKind::DidInternValue {

tests/interned-revisions.rs

@@ -233,6 +233,51 @@ fn test_reuse() {
         ]"#]]);
 }
 
+#[test]
+fn test_reuse_indirect() {
+    #[salsa::tracked]
+    fn intern<'db>(db: &'db dyn Database, input: Input, value: usize) -> Interned<'db> {
+        intern_inner(db, input, value)
+    }
+
+    #[salsa::tracked]
+    fn intern_inner<'db>(db: &'db dyn Database, input: Input, value: usize) -> Interned<'db> {
+        let _i = input.field1(db);
+        Interned::new(db, BadHash(value))
+    }
+
+    let mut db = common::EventLoggerDatabase::default();
+    let input = Input::builder(0).durability(Durability::LOW).new(&db);
+
+    // Intern `i0`.
+    let i0 = intern(&db, input, 0);
+    let i0_id = salsa::plumbing::AsId::as_id(&i0);
+    assert_eq!(i0.field1(&db).0, 0);
+
+    // Get the garbage collector to consider `i0` stale.
+    for x in 1.. {
+        db.synthetic_write(Durability::LOW);
+
+        let ix = intern(&db, input, x);
+        let ix_id = salsa::plumbing::AsId::as_id(&ix);
+
+        // We reused the slot of `i0`.
+        if ix_id.index() == i0_id.index() {
+            assert_eq!(ix.field1(&db).0, x);
+
+            // Re-intern and read `i0` from a new slot.
+            //
+            // Note that the only writes have been synthetic, so none of the query dependencies
+            // have changed directly. The interned value dependency should be enough to force
+            // the inner query to update.
+            let i0 = intern(&db, input, 0);
+            assert_eq!(i0.field1(&db).0, 0);
+
+            break;
+        }
+    }
+}
+
 #[test]
 fn test_reuse_interned_input() {
     // A query that creates an interned value.