Change Transaction’s target type from a parameter to an associated …

…type. This doesn't remove any flexibility we’re actually using, and means that we can add transaction-type-specific commit errors without making `ExecuteError` need another type parameter.
kpreid · Jun 19, 2024 · 80f60bb · 80f60bb
1 parent 605860f
commit 80f60bb
Show file tree

Hide file tree

Showing 13 changed files with 85 additions and 63 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -71,6 +71,7 @@
     - `raytracer::SpaceRaytracer::trace_scene_to_text()` and `trace_scene_to_string()` have been replaced with the single method `to_text()`.
 
     - `transaction::ExecuteError` is now generic over the type of transaction it is an error from.
+    - `transaction::Transaction` now has an associated type `Target` instead of a type parameter; it is no longer possible for a transaction type to be used with more than one target type.
 
     - Renamed `universe::URef` to `universe::Handle`. Related items have also been renamed:
         - `URefErased` to `ErasedHandle`

diff --git a/all-is-cubes/src/behavior.rs b/all-is-cubes/src/behavior.rs
@@ -573,7 +573,8 @@ impl<H: BehaviorHost> BehaviorSetTransaction<H> {
     }
 }
 
-impl<H: BehaviorHost> Transaction<BehaviorSet<H>> for BehaviorSetTransaction<H> {
+impl<H: BehaviorHost> Transaction for BehaviorSetTransaction<H> {
+    type Target = BehaviorSet<H>;
     type CommitCheck = CommitCheck;
     type Output = transaction::NoOutput;
 

diff --git a/all-is-cubes/src/block/block_def.rs b/all-is-cubes/src/block/block_def.rs
@@ -279,7 +279,7 @@ impl<'a> arbitrary::Arbitrary<'a> for BlockDef {
 #[derive(Clone, Debug, Default, Eq, Hash, PartialEq)]
 #[must_use]
 pub struct BlockDefTransaction {
-    // TODO: This struct is the second occurrence (the first is space::CubeTransaction) of a "assign to a mutable location" transaction. If we figure out how to have conveniently _composable_ transactions then we should have an `impl Transaction<&mut T> for Assign<T>` transaction (targeting `&mut` to discourage use otherwise).
+    // TODO: This struct is the second occurrence (the first is space::CubeTransaction) of a "assign to a mutable location" transaction. If we figure out how to have conveniently _composable_ transactions then we should have an `impl Transaction<Target = &mut T> for Assign<T>` transaction (targeting `&mut` to discourage use otherwise).
     /// If `None`, no precondition.
     old: Option<Block>,
     /// If `None`, no change is made and this transaction is only a precondition.
@@ -314,7 +314,8 @@ impl BlockDefTransaction {
     }
 }
 
-impl Transaction<BlockDef> for BlockDefTransaction {
+impl Transaction for BlockDefTransaction {
+    type Target = BlockDef;
     type CommitCheck = ();
     type Output = transaction::NoOutput;
 

diff --git a/all-is-cubes/src/character.rs b/all-is-cubes/src/character.rs
@@ -725,11 +725,12 @@ impl CharacterTransaction {
 }
 
 #[allow(clippy::type_complexity)]
-impl Transaction<Character> for CharacterTransaction {
+impl Transaction for CharacterTransaction {
+    type Target = Character;
     type CommitCheck = (
-        <BodyTransaction as Transaction<Body>>::CommitCheck,
-        <InventoryTransaction as Transaction<Inventory>>::CommitCheck,
-        <BehaviorSetTransaction<Character> as Transaction<BehaviorSet<Character>>>::CommitCheck,
+        <BodyTransaction as Transaction>::CommitCheck,
+        <InventoryTransaction as Transaction>::CommitCheck,
+        <BehaviorSetTransaction<Character> as Transaction>::CommitCheck,
     );
     type Output = transaction::NoOutput;
 

diff --git a/all-is-cubes/src/inv/inventory.rs b/all-is-cubes/src/inv/inventory.rs
@@ -358,7 +358,8 @@ impl InventoryTransaction {
     }
 }
 
-impl Transaction<Inventory> for InventoryTransaction {
+impl Transaction for InventoryTransaction {
+    type Target = Inventory;
     type CommitCheck = Option<InventoryCheck>;
     type Output = InventoryChange;
 

diff --git a/all-is-cubes/src/physics/body.rs b/all-is-cubes/src/physics/body.rs
@@ -700,7 +700,8 @@ impl transaction::Transactional for Body {
     type Transaction = BodyTransaction;
 }
 
-impl Transaction<Body> for BodyTransaction {
+impl Transaction for BodyTransaction {
+    type Target = Body;
     type CommitCheck = ();
     type Output = transaction::NoOutput;
 

diff --git a/all-is-cubes/src/space/space_txn.rs b/all-is-cubes/src/space/space_txn.rs
@@ -7,7 +7,7 @@ use alloc::sync::Arc;
 use alloc::vec::Vec;
 use core::{fmt, mem};
 
-use crate::behavior::{self, BehaviorSet, BehaviorSetTransaction};
+use crate::behavior::{self, BehaviorSetTransaction};
 use crate::block::Block;
 use crate::drawing::DrawingPlane;
 use crate::fluff::Fluff;
@@ -18,6 +18,9 @@ use crate::transaction::{
 };
 use crate::util::{ConciseDebug, Refmt as _};
 
+#[cfg(doc)]
+use crate::behavior::BehaviorSet;
+
 impl Transactional for Space {
     type Transaction = SpaceTransaction;
 }
@@ -168,9 +171,9 @@ impl SpaceTransaction {
     }
 }
 
-impl Transaction<Space> for SpaceTransaction {
-    type CommitCheck =
-        <BehaviorSetTransaction<Space> as Transaction<BehaviorSet<Space>>>::CommitCheck;
+impl Transaction for SpaceTransaction {
+    type Target = Space;
+    type CommitCheck = <BehaviorSetTransaction<Space> as Transaction>::CommitCheck;
     type Output = NoOutput;
 
     fn check(&self, space: &Space) -> Result<Self::CommitCheck, PreconditionFailed> {

diff --git a/all-is-cubes/src/transaction.rs b/all-is-cubes/src/transaction.rs
@@ -34,7 +34,10 @@ pub use tester::*;
 /// If a transaction implements [`Default`], then the default value should be a
 /// transaction which has no effects and always succeeds, and is cheap to create.
 #[must_use]
-pub trait Transaction<T: ?Sized>: Merge {
+pub trait Transaction: Merge {
+    /// Type of the transaction’s target (what it can be used to mutate).
+    type Target;
+
     /// Type of a value passed from [`Transaction::check`] to [`Transaction::commit`].
     /// This may be used to pass precalculated values to speed up the commit phase,
     /// or even lock guards or similar, but also makes it slightly harder to accidentally
@@ -54,7 +57,7 @@ pub trait Transaction<T: ?Sized>: Merge {
     ///
     /// If the preconditions are met, returns [`Ok`] containing data to be passed to
     /// [`Transaction::commit`].
-    fn check(&self, target: &T) -> Result<Self::CommitCheck, PreconditionFailed>;
+    fn check(&self, target: &Self::Target) -> Result<Self::CommitCheck, PreconditionFailed>;
 
     /// Perform the mutations specified by this transaction. The `check` value should have
     /// been created by a prior call to [`Transaction::check()`].
@@ -69,12 +72,12 @@ pub trait Transaction<T: ?Sized>: Merge {
     ///
     /// The target should not be mutated between the call to [`Transaction::check()`] and
     /// [`Transaction::commit()`] (including via interior mutability, however that applies
-    /// to the particular `T`). The consequences of doing so may include mutating the
+    /// to the particular `Target` type). The consequences of doing so may include mutating the
     /// wrong components, signaling an error partway through the transaction, or merely
     /// committing the transaction while its preconditions do not hold.
     fn commit(
         &self,
-        target: &mut T,
+        target: &mut Self::Target,
         check: Self::CommitCheck,
         outputs: &mut dyn FnMut(Self::Output),
     ) -> Result<(), CommitError>;
@@ -96,7 +99,7 @@ pub trait Transaction<T: ?Sized>: Merge {
     /// See also: [`Transactional::transact()`], for building a transaction through mutations.
     fn execute(
         &self,
-        target: &mut T,
+        target: &mut Self::Target,
         outputs: &mut dyn FnMut(Self::Output),
     ) -> Result<(), ExecuteError<Self>> {
         let check = self.check(target).map_err(ExecuteError::Check)?;
@@ -108,10 +111,10 @@ pub trait Transaction<T: ?Sized>: Merge {
     /// so that it can be combined with other transactions in the same universe.
     ///
     /// This is a convenience wrapper around [`UTransactional::bind`].
-    fn bind(self, target: Handle<T>) -> UniverseTransaction
+    fn bind(self, target: Handle<Self::Target>) -> UniverseTransaction
     where
         Self: Sized,
-        T: UTransactional<Transaction = Self>,
+        Self::Target: UTransactional<Transaction = Self>,
     {
         UTransactional::bind(target, self)
     }
@@ -120,9 +123,8 @@ pub trait Transaction<T: ?Sized>: Merge {
 /// Merging two transactions (or, in principle, other values) to produce one result “with
 /// the effect of both”. Merging is a commutative, fallible operation.
 ///
-/// This is a separate trait from [`Transaction`] so that a single type can implement
-/// `Transaction<Foo>` and `Transaction<Bar>` without making it ambiguous which
-/// implementation `.merge()` refers to.
+/// This is a separate trait from [`Transaction`] because some components of transactions
+/// are mergeable but not executable in isolation.
 ///
 /// TODO: Generalize to different RHS types for convenient combination?
 pub trait Merge: Sized {
@@ -355,11 +357,11 @@ crate::util::cfg_should_impl_error! {
 
 /// Specifies a canonical [`Transaction`] type for the implementing type.
 ///
-/// [`Transaction<T>`](Transaction) may be implemented by multiple types but there can
-/// be at most one `<T as Transactional>::Transaction`.
+/// For a given `T`, [`Transaction<Target = T>`] may be implemented by multiple types,
+/// but there can be at most one `<T as Transactional>::Transaction`.
 pub trait Transactional {
     /// The type of transaction which should be used with `Self`.
-    type Transaction: Transaction<Self>;
+    type Transaction: Transaction<Target = Self>;
 
     /// Convenience method for building and then applying a transaction to `self`,
     /// equivalent to the following steps:
@@ -385,7 +387,7 @@ pub trait Transactional {
             &mut Self::Transaction,
             &Self,
         ) -> Result<O, <Self::Transaction as Merge>::Conflict>,
-        Self::Transaction: Transaction<Self, Output = NoOutput> + Default,
+        Self::Transaction: Transaction<Target = Self, Output = NoOutput> + Default,
     {
         let mut transaction = Self::Transaction::default();
         let output = f(&mut transaction, self).map_err(ExecuteError::Merge)?;

diff --git a/all-is-cubes/src/transaction/generic.rs b/all-is-cubes/src/transaction/generic.rs
@@ -157,20 +157,21 @@ macro_rules! impl_transaction_for_tuple {
             /// A tuple of transactions may act as a transaction on tuples.
             ///
             /// TODO: This functionality is not currently used and is of dubious value.
-            impl<$( [<Ta $name>], )* $( [<Tr $name>] ),*>
-                Transaction<($( [<Ta $name>], )*)> for ($( [<Tr $name>], )*)
+            impl<$( [<Tr $name>] ),*>
+                Transaction for ($( [<Tr $name>], )*)
             where
-                $( [<Tr $name>]: Transaction<[<Ta $name>], Output = NoOutput>  ),*
+                $( [<Tr $name>]: Transaction<Output = NoOutput>  ),*
             {
+                type Target = ($( [<Tr $name>]::Target, )*);
                 type CommitCheck = (
-                    $( <[<Tr $name>] as Transaction<[<Ta $name>]>>::CommitCheck, )*
+                    $( <[<Tr $name>] as Transaction>::CommitCheck, )*
                 );
                 type Output = NoOutput;
 
                 fn check(
                     &self,
                     #[allow(unused_variables)] // empty tuple case
-                    target: &($( [<Ta $name>], )*),
+                    target: &($( [<Tr $name>]::Target, )*),
                 ) -> Result<Self::CommitCheck, PreconditionFailed> {
                     let ($( [<txn_ $name>], )*) = self;
                     let ($( [<target_ $name>], )*) = target;
@@ -182,7 +183,7 @@ macro_rules! impl_transaction_for_tuple {
                 fn commit(
                     &self,
                     #[allow(unused_variables)] // empty tuple case
-                    target: &mut ($( [<Ta $name>], )*),
+                    target: &mut ($( [<Tr $name>]::Target, )*),
                     check: Self::CommitCheck,
                     outputs: &mut dyn FnMut(Self::Output),
                 ) -> Result<(), super::CommitError> {
@@ -267,19 +268,23 @@ impl_transaction_for_tuple!(4: 0, 1, 2, 3);
 impl_transaction_for_tuple!(5: 0, 1, 2, 3, 4);
 impl_transaction_for_tuple!(6: 0, 1, 2, 3, 4, 5);
 
-/// Does nothing to anything.
-impl<T> Transaction<T> for () {
+/// Does nothing.
+// The empty tuple gets a special implementation because it cannot fail to commit,
+// and this is best represented without using a custom type.
+// Other than that, this is identical to the macro-generated code.
+impl Transaction for () {
+    type Target = ();
     type CommitCheck = ();
 
     type Output = core::convert::Infallible;
 
-    fn check(&self, _: &T) -> Result<Self::CommitCheck, PreconditionFailed> {
+    fn check(&self, (): &()) -> Result<Self::CommitCheck, PreconditionFailed> {
         Ok(())
     }
 
     fn commit(
         &self,
-        _: &mut T,
+        (): &mut (),
         (): Self::CommitCheck,
         _: &mut dyn FnMut(Self::Output),
     ) -> Result<(), super::CommitError> {

diff --git a/all-is-cubes/src/transaction/tester.rs b/all-is-cubes/src/transaction/tester.rs
@@ -17,15 +17,19 @@ use super::Transaction;
 /// finish with [`Self::test`].
 #[must_use]
 #[allow(missing_debug_implementations)]
-pub struct TransactionTester<'a, Tr, Ta> {
-    transactions: Vec<TransactionAndPredicate<'a, Tr, Ta>>,
-    target_factories: Vec<Box<dyn Fn() -> Ta + 'a>>,
+pub struct TransactionTester<'a, Tr>
+where
+    Tr: Transaction + Clone + Debug + 'a,
+    Tr::Target: Debug + 'a,
+{
+    transactions: Vec<TransactionAndPredicate<'a, Tr>>,
+    target_factories: Vec<Box<dyn Fn() -> Tr::Target + 'a>>,
 }
 
-impl<'a, Tr, Ta> TransactionTester<'a, Tr, Ta>
+impl<'a, Tr> TransactionTester<'a, Tr>
 where
-    Tr: Transaction<Ta> + Clone + Debug + 'a,
-    Ta: Debug + 'a,
+    Tr: Transaction + Clone + Debug + 'a,
+    Tr::Target: Debug + 'a,
 {
     #[allow(clippy::new_without_default)]
     pub fn new() -> Self {
@@ -46,7 +50,7 @@ where
     pub fn transaction(
         mut self,
         transaction: Tr,
-        predicate: impl Fn(&Ta, &Ta) -> PredicateRes + 'a,
+        predicate: impl Fn(&Tr::Target, &Tr::Target) -> PredicateRes + 'a,
     ) -> Self {
         self.transactions.push(TransactionAndPredicate {
             transaction,
@@ -59,7 +63,7 @@ where
     ///
     /// To avoid requiring the targets to implement [`Clone`], a factory function is
     /// required here.
-    pub fn target(mut self, factory: impl Fn() -> Ta + 'a) -> Self {
+    pub fn target(mut self, factory: impl Fn() -> Tr::Target + 'a) -> Self {
         self.target_factories.push(Box::new(factory));
         self
     }
@@ -112,7 +116,7 @@ where
 
     fn derived_transactions<'b>(
         &'b self,
-    ) -> impl Iterator<Item = TransactionAndPredicate<'a, Tr, Ta>> + 'b {
+    ) -> impl Iterator<Item = TransactionAndPredicate<'a, Tr>> + 'b {
         self.transactions.iter().flat_map(move |t1| {
             core::iter::once(t1.clone()).chain(
                 self.transactions
@@ -125,13 +129,13 @@ where
 
 pub type PredicateRes = Result<(), Box<dyn Error>>;
 
-struct TransactionAndPredicate<'a, Tr, Ta> {
+struct TransactionAndPredicate<'a, Tr: Transaction> {
     transaction: Tr,
     #[allow(clippy::type_complexity)] // https://github.com/rust-lang/rust-clippy/issues/9299
-    predicate: Rc<dyn Fn(&Ta, &Ta) -> PredicateRes + 'a>,
+    predicate: Rc<dyn Fn(&Tr::Target, &Tr::Target) -> PredicateRes + 'a>,
 }
 
-impl<'a, Tr: Clone, Ta> Clone for TransactionAndPredicate<'a, Tr, Ta> {
+impl<'a, Tr: Transaction + Clone> Clone for TransactionAndPredicate<'a, Tr> {
     fn clone(&self) -> Self {
         TransactionAndPredicate {
             transaction: self.transaction.clone(),
@@ -140,10 +144,9 @@ impl<'a, Tr: Clone, Ta> Clone for TransactionAndPredicate<'a, Tr, Ta> {
     }
 }
 
-impl<'a, Tr, Ta> TransactionAndPredicate<'a, Tr, Ta>
+impl<'a, Tr> TransactionAndPredicate<'a, Tr>
 where
-    Tr: Transaction<Ta>,
-    Ta: 'a,
+    Tr: Transaction<Target: 'a>,
 {
     fn try_merge(mut self, other: Self) -> Option<Self> {
         let merge_check = self.transaction.check_merge(&other.transaction).ok()?;

diff --git a/all-is-cubes/src/universe/handle.rs b/all-is-cubes/src/universe/handle.rs
@@ -223,7 +223,7 @@ impl<T: 'static> Handle<T> {
     pub fn execute(
         &self,
         transaction: &<T as Transactional>::Transaction,
-        outputs: &mut dyn FnMut(<<T as Transactional>::Transaction as Transaction<T>>::Output),
+        outputs: &mut dyn FnMut(<<T as Transactional>::Transaction as Transaction>::Output),
     ) -> Result<(), ExecuteError<<T as Transactional>::Transaction>>
     where
         T: Transactional,

diff --git a/all-is-cubes/src/universe/members.rs b/all-is-cubes/src/universe/members.rs
@@ -304,7 +304,8 @@ macro_rules! member_enums_and_impls {
             }
         }
 
-        impl transaction::Transaction<()> for AnyTransaction {
+        impl transaction::Transaction for AnyTransaction {
+            type Target = ();
             type CommitCheck = ut::AnyTransactionCheck;
             type Output = transaction::NoOutput;