convert DatabaseSlot to unsafe trait

The unsafe impl now asserts that the `DatabaseSlot` implementor type
is indeed `Send+Sync` if `DB::DatabaseData` is `Send+Sync`. Since our
query keys/values are a part of database-data, this means that `Slot`
must be `Send+Sync` if the key/value are `Send+Sync`. We test this
with a function that will cause compliation to fail if we accidentally
introduce an `Rc<T>` etc.

components/salsa-macros/src/query_group.rs

@@ -367,7 +367,9 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream
             #[derive(Default, Debug)]
             #trait_vis struct #qt;
 
-            impl<#db> salsa::Query<#db> for #qt
+            // Unsafe proof obligation: that our key/value are a part
+            // of the `GroupData`.
+            unsafe impl<#db> salsa::Query<#db> for #qt
             where
                 DB: #trait_name + #requires,
                 DB: salsa::plumbing::HasQueryGroup<#group_struct>,

src/dependency.rs

@@ -4,8 +4,11 @@ use std::fmt::Debug;
 use std::hash::Hasher;
 use std::sync::Arc;
 
-/// Each kind of query exports a "slot".
-pub(crate) trait DatabaseSlot<DB: Database>: Debug {
+/// Unsafe proof obligations:
+///
+/// - If `DB::DatabaseData: Send + Sync`, then `Self: Send + Sync`
+/// - If `DB: 'static` and `DB::DatabaseData: 'static`, then `Self: 'static`
+pub(crate) unsafe trait DatabaseSlot<DB: Database>: Debug {
     /// Returns true if the value of this query may have changed since
     /// the given revision.
     fn maybe_changed_since(&self, db: &DB, revision: Revision) -> bool;
@@ -13,6 +16,7 @@ pub(crate) trait DatabaseSlot<DB: Database>: Debug {
 
 pub(crate) struct Dependency<DB: Database> {
     slot: Arc<dyn DatabaseSlot<DB> + Send + Sync>,
+    phantom: std::marker::PhantomData<Arc<DB::DatabaseData>>,
 }
 
 impl<DB: Database> Dependency<DB> {
@@ -22,7 +26,10 @@ impl<DB: Database> Dependency<DB> {
         // Hiding these bounds behind a trait object is a total hack
         // but I just want to see how well this works. -nikomatsakis
         let slot: Arc<dyn DatabaseSlot<DB> + Send + Sync> = unsafe { std::mem::transmute(slot) };
-        Self { slot }
+        Self {
+            slot,
+            phantom: std::marker::PhantomData,
+        }
     }
 
     fn raw_slot(&self) -> *const dyn DatabaseSlot<DB> {

src/derived.rs

@@ -49,7 +49,7 @@ where
 {
 }
 
-pub trait MemoizationPolicy<DB, Q>
+pub trait MemoizationPolicy<DB, Q>: Send + Sync + 'static
 where
     Q: QueryFunction<DB>,
     DB: Database,

src/derived/slot.rs

@@ -755,7 +755,11 @@ where
     }
 }
 
-impl<DB, Q, MP> DatabaseSlot<DB> for Slot<DB, Q, MP>
+// The unsafe obligation here is for us to assert that `Slot<DB, Q,
+// MP>` is `Send + Sync + 'static`, assuming `Q::Key` and `Q::Value`
+// are. We assert this with the `check_send_sync` and `check_static`
+// functions below.
+unsafe impl<DB, Q, MP> DatabaseSlot<DB> for Slot<DB, Q, MP>
 where
     Q: QueryFunction<DB>,
     DB: Database + HasQueryGroup<Q::Group>,
@@ -933,3 +937,38 @@ where
         maybe_changed
     }
 }
+
+/// Check that `Slot<DB, Q, MP>: Send + Sync` as long as
+/// `DB::DatabaseData: Send + Sync`, which in turn implies that
+/// `Q::Key: Send + Sync`, `Q::Value: Send + Sync`.
+#[allow(dead_code)]
+fn check_send_sync<DB, Q, MP>()
+where
+    Q: QueryFunction<DB>,
+    DB: Database + HasQueryGroup<Q::Group>,
+    MP: MemoizationPolicy<DB, Q>,
+    DB::DatabaseData: Send + Sync,
+    Q::Key: Send + Sync,
+    Q::Value: Send + Sync,
+{
+    fn is_send_sync<T: Send + Sync>() {}
+    is_send_sync::<Slot<DB, Q, MP>>();
+}
+
+/// Check that `Slot<DB, Q, MP>: 'static` as long as
+/// `DB::DatabaseData: 'static`, which in turn implies that
+/// `Q::Key: 'static`, `Q::Value: 'static`.
+#[allow(dead_code)]
+fn check_static<DB, Q, MP>()
+where
+    Q: QueryFunction<DB>,
+    DB: Database + HasQueryGroup<Q::Group>,
+    MP: MemoizationPolicy<DB, Q>,
+    DB: 'static,
+    DB::DatabaseData: 'static,
+    Q::Key: 'static,
+    Q::Value: 'static,
+{
+    fn is_static<T: 'static>() {}
+    is_static::<Slot<DB, Q, MP>>();
+}

src/doctest.rs

@@ -0,0 +1,121 @@
+#![allow(dead_code)]
+
+/// Test that a database with a key/value that is not `Send` will,
+/// indeed, not be `Send`.
+///
+/// ```compile_fail,E0277
+/// use std::rc::Rc;
+///
+/// #[salsa::query_group(NoSendSyncStorage)]
+/// trait NoSendSyncDatabase: salsa::Database {
+///     fn no_send_sync_value(&self, key: bool) -> Rc<bool>;
+///     fn no_send_sync_key(&self, key: Rc<bool>) -> bool;
+/// }
+///
+/// fn no_send_sync_value(_db: &impl NoSendSyncDatabase, key: bool) -> Rc<bool> {
+///     Rc::new(key)
+/// }
+///
+/// fn no_send_sync_key(_db: &impl NoSendSyncDatabase, key: Rc<bool>) -> bool {
+///     *key
+/// }
+///
+/// #[salsa::database(NoSendSyncStorage)]
+/// #[derive(Default)]
+/// struct DatabaseImpl {
+///     runtime: salsa::Runtime<DatabaseImpl>,
+/// }
+///
+/// impl salsa::Database for DatabaseImpl {
+///     fn salsa_runtime(&self) -> &salsa::Runtime<DatabaseImpl> {
+///         &self.runtime
+///     }
+/// }
+///
+/// fn is_send<T: Send>(_: T) { }
+///
+/// fn assert_send() {
+///    is_send(DatabaseImpl::default());
+/// }
+/// ```
+fn test_key_not_send_db_not_send() {}
+
+/// Test that a database with a key/value that is not `Sync` will not
+/// be `Send`.
+///
+/// ```compile_fail,E0277
+/// use std::rc::Rc;
+///
+/// #[salsa::query_group(NoSendSyncStorage)]
+/// trait NoSendSyncDatabase: salsa::Database {
+///     fn no_send_sync_value(&self, key: bool) -> Cell<bool>;
+///     fn no_send_sync_key(&self, key: Cell<bool>) -> bool;
+/// }
+///
+/// fn no_send_sync_value(_db: &impl NoSendSyncDatabase, key: bool) -> Cell<bool> {
+///     Cell::new(key)
+/// }
+///
+/// fn no_send_sync_key(_db: &impl NoSendSyncDatabase, key: Cell<bool>) -> bool {
+///     *key
+/// }
+///
+/// #[salsa::database(NoSendSyncStorage)]
+/// #[derive(Default)]
+/// struct DatabaseImpl {
+///     runtime: salsa::Runtime<DatabaseImpl>,
+/// }
+///
+/// impl salsa::Database for DatabaseImpl {
+///     fn salsa_runtime(&self) -> &salsa::Runtime<DatabaseImpl> {
+///         &self.runtime
+///     }
+/// }
+///
+/// fn is_send<T: Send>(_: T) { }
+///
+/// fn assert_send() {
+///    is_send(DatabaseImpl::default());
+/// }
+/// ```
+fn test_key_not_sync_db_not_send() {}
+
+/// Test that a database with a key/value that is not `Sync` will
+/// not be `Sync`.
+///
+/// ```compile_fail,E0277
+/// use std::rc::Rc;
+///
+/// #[salsa::query_group(NoSendSyncStorage)]
+/// trait NoSendSyncDatabase: salsa::Database {
+///     fn no_send_sync_value(&self, key: bool) -> Cell<bool>;
+///     fn no_send_sync_key(&self, key: Cell<bool>) -> bool;
+/// }
+///
+/// fn no_send_sync_value(_db: &impl NoSendSyncDatabase, key: bool) -> Cell<bool> {
+///     Cell::new(key)
+/// }
+///
+/// fn no_send_sync_key(_db: &impl NoSendSyncDatabase, key: Cell<bool>) -> bool {
+///     *key
+/// }
+///
+/// #[salsa::database(NoSendSyncStorage)]
+/// #[derive(Default)]
+/// struct DatabaseImpl {
+///     runtime: salsa::Runtime<DatabaseImpl>,
+/// }
+///
+/// impl salsa::Database for DatabaseImpl {
+///     fn salsa_runtime(&self) -> &salsa::Runtime<DatabaseImpl> {
+///         &self.runtime
+///     }
+/// }
+///
+/// fn is_sync<T: Sync>(_: T) { }
+///
+/// fn assert_send() {
+///    is_sync(DatabaseImpl::default());
+/// }
+/// ```
+fn test_key_not_sync_db_not_sync() {}

src/input.rs

@@ -201,7 +201,11 @@ where
     }
 }
 
-impl<DB, Q> DatabaseSlot<DB> for Slot<DB, Q>
+// Unsafe proof obligation: `Slot<DB, Q>` is Send + Sync if the query
+// key/value is Send + Sync (also, that we introduce no
+// references). These are tested by the `check_send_sync` and
+// `check_static` helpers below.
+unsafe impl<DB, Q> DatabaseSlot<DB> for Slot<DB, Q>
 where
     Q: Query<DB>,
     DB: Database,
@@ -220,6 +224,39 @@ where
     }
 }
 
+/// Check that `Slot<DB, Q, MP>: Send + Sync` as long as
+/// `DB::DatabaseData: Send + Sync`, which in turn implies that
+/// `Q::Key: Send + Sync`, `Q::Value: Send + Sync`.
+#[allow(dead_code)]
+fn check_send_sync<DB, Q>()
+where
+    Q: Query<DB>,
+    DB: Database,
+    DB::DatabaseData: Send + Sync,
+    Q::Key: Send + Sync,
+    Q::Value: Send + Sync,
+{
+    fn is_send_sync<T: Send + Sync>() {}
+    is_send_sync::<Slot<DB, Q>>();
+}
+
+/// Check that `Slot<DB, Q, MP>: 'static` as long as
+/// `DB::DatabaseData: 'static`, which in turn implies that
+/// `Q::Key: 'static`, `Q::Value: 'static`.
+#[allow(dead_code)]
+fn check_static<DB, Q>()
+where
+    Q: Query<DB>,
+    DB: Database,
+    DB: 'static,
+    DB::DatabaseData: 'static,
+    Q::Key: 'static,
+    Q::Value: 'static,
+{
+    fn is_static<T: 'static>() {}
+    is_static::<Slot<DB, Q>>();
+}
+
 impl<DB, Q> std::fmt::Debug for Slot<DB, Q>
 where
     Q: Query<DB>,

src/interned.rs

@@ -526,7 +526,11 @@ impl<K> Slot<K> {
     }
 }
 
-impl<DB, K> DatabaseSlot<DB> for Slot<K>
+// Unsafe proof obligation: `Slot<K>` is Send + Sync if the query
+// key/value is Send + Sync (also, that we introduce no
+// references). These are tested by the `check_send_sync` and
+// `check_static` helpers below.
+unsafe impl<DB, K> DatabaseSlot<DB> for Slot<K>
 where
     DB: Database,
     K: Debug,
@@ -542,3 +546,27 @@ where
         }
     }
 }
+
+/// Check that `Slot<DB, Q, MP>: Send + Sync` as long as
+/// `DB::DatabaseData: Send + Sync`, which in turn implies that
+/// `Q::Key: Send + Sync`, `Q::Value: Send + Sync`.
+#[allow(dead_code)]
+fn check_send_sync<K>()
+where
+    K: Send + Sync,
+{
+    fn is_send_sync<T: Send + Sync>() {}
+    is_send_sync::<Slot<K>>();
+}
+
+/// Check that `Slot<DB, Q, MP>: 'static` as long as
+/// `DB::DatabaseData: 'static`, which in turn implies that
+/// `Q::Key: 'static`, `Q::Value: 'static`.
+#[allow(dead_code)]
+fn check_static<K>()
+where
+    K: 'static,
+{
+    fn is_static<T: 'static>() {}
+    is_static::<Slot<K>>();
+}

src/lib.rs

@@ -10,6 +10,7 @@
 
 mod dependency;
 mod derived;
+mod doctest;
 mod input;
 mod intern_id;
 mod interned;
@@ -404,7 +405,13 @@ where
 
 /// Trait implements by all of the "special types" associated with
 /// each of your queries.
-pub trait Query<DB: Database>: Debug + Default + Sized + 'static {
+///
+/// Unsafe trait obligation: Asserts that the Key/Value associated
+/// types for this trait are a part of the `Group::GroupData` type.
+/// In particular, `Group::GroupData: Send + Sync` must imply that
+/// `Key: Send + Sync` and `Value: Send + Sync`. This is relied upon
+/// by the dependency tracking logic.
+pub unsafe trait Query<DB: Database>: Debug + Default + Sized + 'static {
     /// Type that you you give as a parameter -- for queries with zero
     /// or more than one input, this will be a tuple.
     type Key: Clone + Debug + Hash + Eq;