Address reviews

Veykril · Veykril · commit 8c0ec5899af1 · 2025-07-31T07:35:14.000+02:00
diff --git a/components/salsa-macro-rules/src/setup_tracked_fn.rs b/components/salsa-macro-rules/src/setup_tracked_fn.rs
@@ -318,7 +318,6 @@ macro_rules! setup_tracked_fn {
                     };
 
                     let fn_ingredient = <$zalsa::function::IngredientImpl<$Configuration>>::new(
-                        zalsa,
                         first_index,
                         memo_ingredient_indices,
                         $lru,
diff --git a/components/salsa-macros/src/db.rs b/components/salsa-macros/src/db.rs
@@ -135,7 +135,9 @@ impl DbMacro {
             #[inline(never)]
             #[doc(hidden)]
             fn zalsa_register_downcaster(&self) -> &salsa::plumbing::DatabaseDownCaster<dyn #TraitPath> {
-                salsa::plumbing::views(self).add(<Self as #TraitPath>::downcast)
+                salsa::plumbing::views(self).add::<Self, dyn #TraitPath>(unsafe {
+                    ::std::mem::transmute(<Self as #TraitPath>::downcast as fn(_) -> _)
+                })
             }
         });
         input.items.push(parse_quote! {
diff --git a/src/accumulator.rs b/src/accumulator.rs
@@ -103,7 +103,7 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
     unsafe fn maybe_changed_after(
         &self,
         _zalsa: &crate::zalsa::Zalsa,
-        _db: crate::database::RawDatabasePointer<'_>,
+        _db: crate::database::RawDatabase<'_>,
         _input: Id,
         _revision: Revision,
         _cycle_heads: &mut CycleHeads,
diff --git a/src/database.rs b/src/database.rs
@@ -6,25 +6,25 @@ use crate::zalsa::{IngredientIndex, ZalsaDatabase};
 use crate::{Durability, Revision};
 
 #[derive(Copy, Clone)]
-pub struct RawDatabasePointer<'db> {
+pub struct RawDatabase<'db> {
     pub(crate) ptr: NonNull<()>,
     _marker: std::marker::PhantomData<&'db dyn Database>,
 }
 
-impl<'db, Db: Database + ?Sized> From<&'db Db> for RawDatabasePointer<'db> {
+impl<'db, Db: Database + ?Sized> From<&'db Db> for RawDatabase<'db> {
     #[inline]
     fn from(db: &'db Db) -> Self {
-        RawDatabasePointer {
+        RawDatabase {
             ptr: NonNull::from(db).cast(),
             _marker: std::marker::PhantomData,
         }
     }
 }
 
-impl<'db, Db: Database + ?Sized> From<&'db mut Db> for RawDatabasePointer<'db> {
+impl<'db, Db: Database + ?Sized> From<&'db mut Db> for RawDatabase<'db> {
     #[inline]
     fn from(db: &'db mut Db) -> Self {
-        RawDatabasePointer {
+        RawDatabase {
             ptr: NonNull::from(db).cast(),
             _marker: std::marker::PhantomData,
         }
diff --git a/src/function.rs b/src/function.rs
@@ -10,7 +10,7 @@ use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues
 use crate::cycle::{
     empty_cycle_heads, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy, ProvisionalStatus,
 };
-use crate::database::RawDatabasePointer;
+use crate::database::RawDatabase;
 use crate::function::delete::DeletedEntries;
 use crate::function::sync::{ClaimResult, SyncTable};
 use crate::ingredient::{Ingredient, WaitForResult};
@@ -130,7 +130,6 @@ pub struct IngredientImpl<C: Configuration> {
     ///
     /// The supplied database must be be the same as the database used to construct the [`Views`]
     /// instances that this downcaster was derived from.
-    /// instances that this downcaster was derived from.
     view_caster: OnceLock<DatabaseDownCaster<C::DbView>>,
 
     sync_table: SyncTable,
@@ -155,7 +154,6 @@ where
     C: Configuration,
 {
     pub fn new(
-        _zalsa: &Zalsa,
         index: IngredientIndex,
         memo_ingredient_indices: <C::SalsaStruct<'static> as SalsaStructInDb>::MemoIngredientMap,
         lru: usize,
@@ -173,7 +171,10 @@ where
     /// Set the view-caster for this tracked function ingredient, if it has
     /// not already been initialized.
     #[inline]
-    pub fn get_or_init(&self, view_caster: impl FnOnce() -> DatabaseDownCaster<C::DbView>) -> &Self {
+    pub fn get_or_init(
+        &self,
+        view_caster: impl FnOnce() -> DatabaseDownCaster<C::DbView>,
+    ) -> &Self {
         // Note that we must set this lazily as we don't have access to the database
         // type when ingredients are registered into the `Zalsa`.
         self.view_caster.get_or_init(view_caster);
@@ -261,7 +262,7 @@ where
     unsafe fn maybe_changed_after(
         &self,
         _zalsa: &crate::zalsa::Zalsa,
-        db: RawDatabasePointer<'_>,
+        db: RawDatabase<'_>,
         input: Id,
         revision: Revision,
         cycle_heads: &mut CycleHeads,
@@ -372,7 +373,7 @@ where
 
     unsafe fn accumulated<'db>(
         &'db self,
-        db: RawDatabasePointer<'db>,
+        db: RawDatabase<'db>,
         key_index: Id,
     ) -> (Option<&'db AccumulatedMap>, InputAccumulatedValues) {
         // SAFETY: The `db` belongs to the ingredient as per caller invariant
diff --git a/src/ingredient.rs b/src/ingredient.rs
@@ -5,7 +5,7 @@ use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues
 use crate::cycle::{
     empty_cycle_heads, CycleHeads, CycleRecoveryStrategy, IterationCount, ProvisionalStatus,
 };
-use crate::database::RawDatabasePointer;
+use crate::database::RawDatabase;
 use crate::function::VerifyResult;
 use crate::runtime::Running;
 use crate::sync::Arc;
@@ -49,7 +49,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync {
     unsafe fn maybe_changed_after(
         &self,
         zalsa: &crate::zalsa::Zalsa,
-        db: crate::database::RawDatabasePointer<'_>,
+        db: crate::database::RawDatabase<'_>,
         input: Id,
         revision: Revision,
         cycle_heads: &mut CycleHeads,
@@ -167,7 +167,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync {
     /// The passed in database needs to be the same one that the ingredient was created with.
     unsafe fn accumulated<'db>(
         &'db self,
-        db: RawDatabasePointer<'db>,
+        db: RawDatabase<'db>,
         key_index: Id,
     ) -> (Option<&'db AccumulatedMap>, InputAccumulatedValues) {
         let _ = (db, key_index);
diff --git a/src/input.rs b/src/input.rs
@@ -221,7 +221,7 @@ impl<C: Configuration> Ingredient for IngredientImpl<C> {
     unsafe fn maybe_changed_after(
         &self,
         _zalsa: &crate::zalsa::Zalsa,
-        _db: crate::database::RawDatabasePointer<'_>,
+        _db: crate::database::RawDatabase<'_>,
         _input: Id,
         _revision: Revision,
         _cycle_heads: &mut CycleHeads,
diff --git a/src/input/input_field.rs b/src/input/input_field.rs
@@ -53,7 +53,7 @@ where
     unsafe fn maybe_changed_after(
         &self,
         zalsa: &crate::zalsa::Zalsa,
-        _db: crate::database::RawDatabasePointer<'_>,
+        _db: crate::database::RawDatabase<'_>,
         input: Id,
         revision: Revision,
         _cycle_heads: &mut CycleHeads,
diff --git a/src/interned.rs b/src/interned.rs
@@ -795,7 +795,7 @@ where
     unsafe fn maybe_changed_after(
         &self,
         zalsa: &crate::zalsa::Zalsa,
-        _db: crate::database::RawDatabasePointer<'_>,
+        _db: crate::database::RawDatabase<'_>,
         input: Id,
         _revision: Revision,
         _cycle_heads: &mut CycleHeads,
diff --git a/src/key.rs b/src/key.rs
@@ -36,27 +36,18 @@ impl DatabaseKeyIndex {
 
     pub(crate) fn maybe_changed_after(
         &self,
-        db: crate::database::RawDatabasePointer<'_>,
+        db: crate::database::RawDatabase<'_>,
         zalsa: &Zalsa,
         last_verified_at: crate::Revision,
         cycle_heads: &mut CycleHeads,
     ) -> VerifyResult {
         // SAFETY: The `db` belongs to the ingredient
         unsafe {
-            // heer, `db` has to be either the correct type already, or a subtype (as far as trait
+            // here, `db` has to be either the correct type already, or a subtype (as far as trait
             // hierarchy is concerned)
             zalsa
                 .lookup_ingredient(self.ingredient_index())
-                .maybe_changed_after(
-                    zalsa,
-                    // lets say we do turn this into an opaque pair of data and vtable pointer
-                    // then we also need an downcast function, from our dbview to the ingredients
-                    // dbview
-                    db,
-                    self.key_index(),
-                    last_verified_at,
-                    cycle_heads,
-                )
+                .maybe_changed_after(zalsa, db, self.key_index(), last_verified_at, cycle_heads)
         }
     }
 
diff --git a/src/parallel.rs b/src/parallel.rs
@@ -1,6 +1,6 @@
 use rayon::iter::{FromParallelIterator, IntoParallelIterator, ParallelIterator};
 
-use crate::{database::RawDatabasePointer, views::DatabaseDownCaster, Database};
+use crate::{database::RawDatabase, views::DatabaseDownCaster, Database};
 
 pub fn par_map<Db, F, T, R, C>(db: &Db, inputs: impl IntoParallelIterator<Item = T>, op: F) -> C
 where
@@ -12,7 +12,7 @@ where
 {
     let views = db.zalsa().views();
     let caster = &views.downcaster_for::<Db>();
-    let db_caster = &views.base_database_downcaster();
+    let db_caster = &views.downcaster_for::<dyn Database>();
     inputs
         .into_par_iter()
         .map_with(
@@ -23,7 +23,7 @@ where
 }
 
 struct DbForkOnClone<'views, Db: Database + ?Sized>(
-    RawDatabasePointer<'static>,
+    RawDatabase<'static>,
     &'views DatabaseDownCaster<Db>,
     &'views DatabaseDownCaster<dyn Database>,
 );
diff --git a/src/storage.rs b/src/storage.rs
@@ -2,7 +2,7 @@
 use std::marker::PhantomData;
 use std::panic::RefUnwindSafe;
 
-use crate::database::RawDatabasePointer;
+use crate::database::RawDatabase;
 use crate::sync::{Arc, Condvar, Mutex};
 use crate::zalsa::{ErasedJar, HasJar, Zalsa, ZalsaDatabase};
 use crate::zalsa_local::{self, ZalsaLocal};
@@ -246,7 +246,7 @@ unsafe impl<T: HasStorage> ZalsaDatabase for T {
     }
 
     #[inline(always)]
-    fn fork_db(&self) -> RawDatabasePointer<'static> {
+    fn fork_db(&self) -> RawDatabase<'static> {
         Box::leak(Box::new(self.clone())).into()
     }
 }
diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs
@@ -812,7 +812,7 @@ where
     unsafe fn maybe_changed_after(
         &self,
         _zalsa: &crate::zalsa::Zalsa,
-        _db: crate::database::RawDatabasePointer<'_>,
+        _db: crate::database::RawDatabase<'_>,
         _input: Id,
         _revision: Revision,
         _cycle_heads: &mut CycleHeads,
diff --git a/src/tracked_struct/tracked_field.rs b/src/tracked_struct/tracked_field.rs
@@ -58,7 +58,7 @@ where
     unsafe fn maybe_changed_after(
         &self,
         zalsa: &crate::zalsa::Zalsa,
-        _db: crate::database::RawDatabasePointer<'_>,
+        _db: crate::database::RawDatabase<'_>,
         input: Id,
         revision: crate::Revision,
         _cycle_heads: &mut CycleHeads,
diff --git a/src/views.rs b/src/views.rs
@@ -5,7 +5,7 @@ use std::{
     ptr::NonNull,
 };
 
-use crate::{database::RawDatabasePointer, Database};
+use crate::{database::RawDatabase, Database};
 
 /// A `Views` struct is associated with some specific database type
 /// (a `DatabaseImpl<U>` for some existential `U`). It contains functions
@@ -31,27 +31,21 @@ struct ViewCaster {
 }
 
 impl ViewCaster {
-    fn new<DbView: ?Sized + Any>(func: DatabaseDownCasterSigRaw<DbView>) -> ViewCaster {
+    fn new<DbView: ?Sized + Any>(func: DatabaseDownCasterSig<DbView>) -> ViewCaster {
         ViewCaster {
             target_type_id: TypeId::of::<DbView>(),
             type_name: std::any::type_name::<DbView>(),
             // SAFETY: We are type erasing for storage, taking care of unerasing before we call
             // the function pointer.
             cast: unsafe {
-                mem::transmute::<DatabaseDownCasterSigRaw<DbView>, ErasedDatabaseDownCasterSig>(
-                    func,
-                )
+                mem::transmute::<DatabaseDownCasterSig<DbView>, ErasedDatabaseDownCasterSig>(func)
             },
         }
     }
 }
 
-type ErasedDatabaseDownCasterSig = unsafe fn(RawDatabasePointer<'_>) -> NonNull<()>;
-type DatabaseDownCasterSigRaw<DbView> =
-    for<'db> unsafe fn(RawDatabasePointer<'db>) -> NonNull<DbView>;
-type DatabaseDownCasterSig<DbView> = for<'db> unsafe fn(RawDatabasePointer<'db>) -> &'db DbView;
-type DatabaseDownCasterSigMut<DbView> =
-    for<'db> unsafe fn(RawDatabasePointer<'db>) -> &'db mut DbView;
+type ErasedDatabaseDownCasterSig = unsafe fn(RawDatabase<'_>) -> NonNull<()>;
+type DatabaseDownCasterSig<DbView> = unsafe fn(RawDatabase<'_>) -> NonNull<DbView>;
 
 #[repr(transparent)]
 pub struct DatabaseDownCaster<DbView: ?Sized>(ViewCaster, PhantomData<fn() -> DbView>);
@@ -70,29 +64,30 @@ impl<DbView: ?Sized + Any> DatabaseDownCaster<DbView> {
     ///
     /// The caller must ensure that `db` is of the correct type.
     #[inline]
-    pub unsafe fn downcast_unchecked<'db>(&self, db: RawDatabasePointer<'db>) -> &'db DbView {
+    pub unsafe fn downcast_unchecked<'db>(&self, db: RawDatabase<'db>) -> &'db DbView {
         // SAFETY: The caller must ensure that `db` is of the correct type.
-        unsafe {
-            (mem::transmute::<ErasedDatabaseDownCasterSig, DatabaseDownCasterSig<DbView>>(
-                self.0.cast,
-            ))(db)
-        }
+        // The returned pointer is live for `'db` due to construction of the downcaster functions.
+        unsafe { (self.unerased_downcaster())(db).as_ref() }
     }
     /// Downcast `db` to `DbView`.
     ///
     /// # Safety
     ///
     /// The caller must ensure that `db` is of the correct type.
     #[inline]
-    pub unsafe fn downcast_mut_unchecked<'db>(
-        &self,
-        db: RawDatabasePointer<'db>,
-    ) -> &'db mut DbView {
+    pub unsafe fn downcast_mut_unchecked<'db>(&self, db: RawDatabase<'db>) -> &'db mut DbView {
         // SAFETY: The caller must ensure that `db` is of the correct type.
+        // The returned pointer is live for `'db` due to construction of the downcaster functions.
+        unsafe { (self.unerased_downcaster())(db).as_mut() }
+    }
+
+    #[inline]
+    fn unerased_downcaster(&self) -> DatabaseDownCasterSig<DbView> {
+        // SAFETY: The type-erased function pointer is guaranteed to be ABI compatible for `DbView`
         unsafe {
-            (mem::transmute::<ErasedDatabaseDownCasterSig, DatabaseDownCasterSigMut<DbView>>(
+            mem::transmute::<ErasedDatabaseDownCasterSig, DatabaseDownCasterSig<DbView>>(
                 self.0.cast,
-            ))(db)
+            )
         }
     }
 }
@@ -111,7 +106,7 @@ impl Views {
     /// Add a new downcaster to `dyn DbView`.
     pub fn add<Concrete: 'static, DbView: ?Sized + Any>(
         &self,
-        func: fn(&Concrete) -> &DbView,
+        func: fn(NonNull<Concrete>) -> NonNull<DbView>,
     ) -> &DatabaseDownCaster<DbView> {
         assert_eq!(self.source_type_id, TypeId::of::<Concrete>());
         let target_type_id = TypeId::of::<DbView>();
@@ -121,26 +116,22 @@ impl Views {
             .find(|(_, u)| u.target_type_id == target_type_id)
         {
             // SAFETY: The type-erased function pointer is guaranteed to be valid for `DbView`
-            return unsafe { &*(caster as *const ViewCaster as *const DatabaseDownCaster<DbView>) };
+            return unsafe { &*(&raw const *caster).cast::<DatabaseDownCaster<DbView>>() };
         }
 
         // SAFETY: We are type erasing the function pointer for storage, and we will unerase it
         // before we call it.
-        let caster = ViewCaster::new::<DbView>(unsafe {
-            mem::transmute::<fn(&Concrete) -> &DbView, DatabaseDownCasterSigRaw<DbView>>(func)
-        });
+        let caster = unsafe {
+            mem::transmute::<fn(NonNull<Concrete>) -> NonNull<DbView>, DatabaseDownCasterSig<DbView>>(
+                func,
+            )
+        };
+        let caster = ViewCaster::new::<DbView>(caster);
         let idx = self.view_casters.push(caster);
         // SAFETY: The type-erased function pointer is guaranteed to be valid for `DbView`
         unsafe { &*(&raw const self.view_casters[idx]).cast::<DatabaseDownCaster<DbView>>() }
     }
 
-    #[inline]
-    pub fn base_database_downcaster(&self) -> &DatabaseDownCaster<dyn Database> {
-        // SAFETY: The type-erased function pointer is guaranteed to be valid for `dyn Database`
-        // since we created it with the same type.
-        unsafe { &*((&raw const self.view_casters[0]).cast::<DatabaseDownCaster<dyn Database>>()) }
-    }
-
     /// Retrieve an downcaster function to `dyn DbView`.
     ///
     /// # Panics
diff --git a/src/zalsa.rs b/src/zalsa.rs

Original file line number	Diff line number	Diff line change
`@@ -6,25 +6,25 @@ use crate::zalsa::{IngredientIndex, ZalsaDatabase};`
`6`	`6`	`use crate::{Durability, Revision};`
`7`	`7`
`8`	`8`	`#[derive(Copy, Clone)]`
`9`		`-pub struct RawDatabasePointer<'db> {`
	`9`	`+pub struct RawDatabase<'db> {`
`10`	`10`	`pub(crate) ptr: NonNull<()>,`
`11`	`11`	`_marker: std::marker::PhantomData<&'db dyn Database>,`
`12`	`12`	`}`
`13`	`13`
`14`		`-impl<'db, Db: Database + ?Sized> From<&'db Db> for RawDatabasePointer<'db> {`
	`14`	`+impl<'db, Db: Database + ?Sized> From<&'db Db> for RawDatabase<'db> {`
`15`	`15`	`#[inline]`
`16`	`16`	`fn from(db: &'db Db) -> Self {`
`17`		`- RawDatabasePointer {`
	`17`	`+ RawDatabase {`
`18`	`18`	`ptr: NonNull::from(db).cast(),`
`19`	`19`	`_marker: std::marker::PhantomData,`
`20`	`20`	`}`
`21`	`21`	`}`
`22`	`22`	`}`
`23`	`23`
`24`		`-impl<'db, Db: Database + ?Sized> From<&'db mut Db> for RawDatabasePointer<'db> {`
	`24`	`+impl<'db, Db: Database + ?Sized> From<&'db mut Db> for RawDatabase<'db> {`
`25`	`25`	`#[inline]`
`26`	`26`	`fn from(db: &'db mut Db) -> Self {`
`27`		`- RawDatabasePointer {`
	`27`	`+ RawDatabase {`
`28`	`28`	`ptr: NonNull::from(db).cast(),`
`29`	`29`	`_marker: std::marker::PhantomData,`
`30`	`30`	`}`