Revert "Assert size for interned Value" & Mark Slot trait as unsafe (#915)

* Revert "Assert size for interned `Value`"

This reverts commit 793dc41.

* Mark `Slot` trait as unsafe

Author: Veykril

src/input.rs

@@ -290,7 +290,8 @@ pub trait HasBuilder {
     type Builder;
 }
 
-impl<C> Slot for Value<C>
+// SAFETY: `Value<C>` is our private type branded over the unique configuration `C`.
+unsafe impl<C> Slot for Value<C>
 where
     C: Configuration,
 {

src/interned.rs

@@ -100,19 +100,15 @@ impl<C: Configuration> Default for IngredientShard<C> {
 
 // SAFETY: `LinkedListLink` is `!Sync`, however, the linked list is only accessed through the
 // ingredient lock, and values are only ever linked to a single list on the ingredient.
-unsafe impl<Fields: Sync> Sync for Value<Fields> {}
+unsafe impl<C: Configuration> Sync for Value<C> {}
 
-intrusive_adapter!(ValueAdapter<C> = UnsafeRef<ValueForConfiguration<C>>: ValueForConfiguration<C> { link: LinkedListLink } where C: Configuration);
-
-#[cfg(not(feature = "shuttle"))]
-#[cfg(target_pointer_width = "64")]
-const _: [(); std::mem::size_of::<Value<()>>()] = [(); std::mem::size_of::<[usize; 7]>()];
-
-#[allow(type_alias_bounds)]
-type ValueForConfiguration<C: Configuration> = Value<C::Fields<'static>>;
+intrusive_adapter!(ValueAdapter<C> = UnsafeRef<Value<C>>: Value<C> { link: LinkedListLink } where C: Configuration);
 
 /// Struct storing the interned fields.
-pub struct Value<Fields> {
+pub struct Value<C>
+where
+    C: Configuration,
+{
     /// The index of the shard containing this value.
     shard: u16,
 
@@ -123,7 +119,7 @@ pub struct Value<Fields> {
     ///
     /// These are valid for read-only access as long as the lock is held
     /// or the value has been validated in the current revision.
-    fields: UnsafeCell<Fields>,
+    fields: UnsafeCell<C::Fields<'static>>,
 
     /// Memos attached to this interned value.
     ///
@@ -182,10 +178,13 @@ impl ValueShared {
     }
 }
 
-impl<Fields> Value<Fields> {
+impl<C> Value<C>
+where
+    C: Configuration,
+{
     /// Fields of this interned struct.
     #[cfg(feature = "salsa_unstable")]
-    pub fn fields(&self) -> &Fields {
+    pub fn fields(&self) -> &C::Fields<'static> {
         // SAFETY: The fact that this function is safe is technically unsound. However, interned
         // values are only exposed if they have been validated in the current revision, which
         // ensures that they are not reused while being accessed.
@@ -574,9 +573,7 @@ where
             .unwrap_or((Durability::MAX, Revision::max()));
 
         // Allocate the value slot.
-        let id = zalsa_local.allocate(zalsa, self.ingredient_index, |id| ValueForConfiguration::<
-            C,
-        > {
+        let id = zalsa_local.allocate(zalsa, self.ingredient_index, |id| Value::<C> {
             shard: shard_index as u16,
             link: LinkedListLink::new(),
             memos: UnsafeCell::new(MemoTable::default()),
@@ -589,7 +586,7 @@ where
             }),
         });
 
-        let value = Self::table_get(zalsa, id);
+        let value = zalsa.table().get::<Value<C>>(id);
         // SAFETY: We hold the lock for the shard containing the value.
         let value_shared = unsafe { &mut *value.shared.get() };
 
@@ -608,7 +605,7 @@ where
         shard.key_map.insert_unique(hash, id, hasher);
 
         debug_assert_eq!(hash, {
-            let value = Self::table_get(zalsa, id);
+            let value = zalsa.table().get::<Value<C>>(id);
 
             // SAFETY: We hold the lock for the shard containing the value.
             unsafe { self.hasher.hash_one(&*value.fields.get()) }
@@ -687,7 +684,7 @@ where
     unsafe fn value_hash<'db>(&'db self, id: Id, zalsa: &'db Zalsa) -> u64 {
         // This closure is only called if the table is resized. So while it's expensive
         // to lookup all values, it will only happen rarely.
-        let value = Self::table_get(zalsa, id);
+        let value = zalsa.table().get::<Value<C>>(id);
 
         // SAFETY: We hold the lock for the shard containing the value.
         unsafe { self.hasher.hash_one(&*value.fields.get()) }
@@ -702,12 +699,12 @@ where
         id: Id,
         key: &Key,
         zalsa: &'db Zalsa,
-        found_value: &Cell<Option<&'db ValueForConfiguration<C>>>,
+        found_value: &Cell<Option<&'db Value<C>>>,
     ) -> bool
     where
         C::Fields<'db>: HashEqLike<Key>,
     {
-        let value = Self::table_get(zalsa, id);
+        let value = zalsa.table().get::<Value<C>>(id);
         found_value.set(Some(value));
 
         // SAFETY: We hold the lock for the shard containing the value.
@@ -725,7 +722,7 @@ where
     /// Lookup the data for an interned value based on its ID.
     pub fn data<'db>(&'db self, db: &'db dyn Database, id: Id) -> &'db C::Fields<'db> {
         let zalsa = db.zalsa();
-        let value = Self::table_get(zalsa, id);
+        let value = zalsa.table().get::<Value<C>>(id);
 
         debug_assert!(
             {
@@ -767,16 +764,8 @@ where
     pub fn entries<'db>(
         &'db self,
         db: &'db dyn crate::Database,
-    ) -> impl Iterator<Item = &'db ValueForConfiguration<C>> {
-        db.zalsa().table().slots_of::<ValueForConfiguration<C>>()
-    }
-
-    #[inline]
-    fn table_get(zalsa: &Zalsa, id: Id) -> &ValueForConfiguration<C>
-    where
-        C: Configuration,
-    {
-        zalsa.table().get::<ValueForConfiguration<C>>(id)
+    ) -> impl Iterator<Item = &'db Value<C>> {
+        db.zalsa().table().slots_of::<Value<C>>()
     }
 }
 
@@ -805,7 +794,7 @@ where
         let current_revision = zalsa.current_revision();
         self.revision_queue.record(current_revision);
 
-        let value = zalsa.table().get::<ValueForConfiguration<C>>(input);
+        let value = zalsa.table().get::<Value<C>>(input);
 
         // SAFETY: `value.shard` is guaranteed to be in-bounds for `self.shards`.
         let _shard = unsafe { self.shards.get_unchecked(value.shard as usize) }.lock();
@@ -854,7 +843,11 @@ where
     }
 }
 
-impl<Fields: Send + Sync + 'static> Slot for Value<Fields> {
+// SAFETY: `Value<C>` is our private type branded over the unique configuration `C`.
+unsafe impl<C> Slot for Value<C>
+where
+    C: Configuration,
+{
     #[inline(always)]
     unsafe fn memos(&self, _current_revision: Revision) -> &MemoTable {
         // SAFETY: The fact that we have a reference to the `Value` means it must

src/table.rs

@@ -30,7 +30,11 @@ pub struct Table {
     non_full_pages: Mutex<FxHashMap<IngredientIndex, Vec<PageIndex>>>,
 }
 
-pub(crate) trait Slot: Any + Send + Sync {
+/// # Safety
+///
+/// Implementors of this trait need to make sure that their type is unique with respect to
+/// their owning ingredient as the allocation strategy relies on this.
+pub(crate) unsafe trait Slot: Any + Send + Sync {
     /// Access the [`MemoTable`][] for this slot.
     ///
     /// # Safety condition

src/tracked_struct.rs

@@ -912,7 +912,8 @@ where
     }
 }
 
-impl<C> Slot for Value<C>
+// SAFETY: `Value<C>` is our private type branded over the unique configuration `C`.
+unsafe impl<C> Slot for Value<C>
 where
     C: Configuration,
 {