review feedback from Micha

Author: carljm

src/tracked_struct/struct_map.rs

@@ -99,11 +99,13 @@ where
         unsafe { C::struct_from_raw(pointer) }
     }
 
+    /// Mark the given tracked struct as valid in the current revision.
     pub fn validate<'db>(&'db self, runtime: &'db Runtime, id: Id) {
         Self::validate_in_map(&self.map, runtime, id)
     }
 
-    pub fn validate_in_map<'db>(
+    /// Internal helper to provide shared functionality for [`StructMapView`].
+    fn validate_in_map<'db>(
         map: &'db FxDashMap<Id, Alloc<Value<C>>>,
         runtime: &'db Runtime,
         id: Id,
@@ -209,6 +211,18 @@ where
         unsafe { C::struct_from_raw(data.as_raw()) }
     }
 
+    /// Lookup an existing tracked struct from the map, maybe validating it to current revision.
+    ///
+    /// Validates to current revision if the struct was last created/validated in a revision that
+    /// is still current for the struct's durability. That is, if the struct is HIGH durability
+    /// (created by a HIGH durability query) and was created in R2, and we are now at R3 but no
+    /// HIGH durability input has changed since R2, the struct is still valid and we can validate
+    /// it to R3.
+    ///
+    /// # Panics
+    ///
+    /// * If the value is not present in the map.
+    /// * If the value has not been updated in the last-changed revision for its durability.
     fn get_and_validate_last_changed<'db>(
         map: &'db FxDashMap<Id, Alloc<Value<C>>>,
         runtime: &'db Runtime,
@@ -217,20 +231,13 @@ where
         let data = map.get(&id).unwrap();
 
         // UNSAFE: We permit `&`-access in the current revision once data.created_at
-        // has been updated to the current revision (which we check below).
+        // has been updated to the current revision (which we ensure below).
         let data_ref: &Value<C> = unsafe { data.as_ref() };
 
-        // Before we drop the lock, check that the value has
-        // been updated in the most recent version in which the query that created it could have
-        // changed (based on durability), and validate to the current revision if so.
+        // Before we drop the lock, check that the value has been updated in the most recent
+        // version in which the query that created it could have changed (based on durability).
         let created_at = data_ref.created_at;
         let last_changed = runtime.last_changed_revision(data_ref.durability);
-        dbg!(
-            runtime.current_revision(),
-            created_at,
-            last_changed,
-            data_ref.durability
-        );
         assert!(
             created_at >= last_changed,
             "access to tracked struct from obsolete revision"
@@ -287,7 +294,15 @@ where
         StructMap::get_from_map(&self.map, current_revision, id)
     }
 
-    pub fn get_and_validate_last_changed<'db>(
+    /// Lookup an existing tracked struct from the map, maybe validating it to current revision.
+    ///
+    /// See [`StructMap::get_and_validate_last_changed`].
+    ///
+    /// # Panics
+    ///
+    /// * If the value is not present in the map.
+    /// * If the value has not been updated in the last-changed revision for its durability.
+    pub(super) fn get_and_validate_last_changed<'db>(
         &'db self,
         runtime: &'db Runtime,
         id: Id,

tests/tracked_struct_not_validated_due_to_durability.rs

@@ -1,3 +1,23 @@
+/// Test that high durabilities can't cause "access tracked struct from previous revision" panic.
+///
+/// The test models a situation where we have two File inputs (0, 1), where `File(0)` has LOW
+/// durability and `File(1)` has HIGH durability. We can query an `index` for each file, and a
+/// `definitions` from that index (just a sub-part of the index), and we can `infer` each file. The
+/// `index` and `definitions` queries depend only on the `File` they operate on, but the `infer`
+/// query has some other dependencies: `infer(0)` depends on `infer(1)`, and `infer(1)` also
+/// depends directly on `File(0)`.
+///
+/// The panic occurs (in versions of Salsa without a fix) because `definitions(1)` is high
+/// durability, and depends on `index(1)` which is also high durability. `index(1)` creates the
+/// tracked struct `Definition(1)`, and `infer(1)` (which is low durability) depends on
+/// `Definition.file(1)`.
+///
+/// After a change to `File(0)` (low durability), we only shallowly verify `definitions(1)` -- it
+/// passes shallow verification due to durability. We take care to mark-validated the outputs of
+/// `definitions(1)`, but we never verify `index(1)` at all (deeply or shallowly), which means we
+/// never mark `Definition(1)` validated. So when we deep-verify `infer(1)`, we try to access its
+/// dependency `Definition.file(1)`, and hit the panic because we are accessing a tracked struct
+/// that has never been re-validated or re-recreated in R2.
 use salsa::{Durability, Setter};
 
 #[salsa::db]