Add Transaction::Mismatch type parameter.

Currently it is always still `PreconditionFailed`, but in the future, transactions will have individual error types. The purpose of this feature is to be able to translate transaction failures into in-game error reports such as highlighting which cubes in a Space are involved, rather than having only generic and textual information.
kpreid · Jun 19, 2024 · 449d02f · 449d02f
1 parent 80f60bb
commit 449d02f
Show file tree

Hide file tree

Showing 12 changed files with 73 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -28,6 +28,7 @@
     - `op::Operation::Neighbors` allows an operation to modify nearby blocks.
     - `op::Operation::DestroyTo` allows an operation to express replacing a block when the replacement is less important than `Operation::Become`.
 
+    - `transaction::Transaction::check()` now returns an error type specified by the new `Mismatch` associated type, rather than necessarily `PreconditionFailed`.
     - `transaction::Transactional::transact()` provides a convenient way to create and immediately execute transactions.
 
 - `all-is-cubes-mesh` library:

diff --git a/all-is-cubes/src/behavior.rs b/all-is-cubes/src/behavior.rs
@@ -577,6 +577,7 @@ impl<H: BehaviorHost> Transaction for BehaviorSetTransaction<H> {
     type Target = BehaviorSet<H>;
     type CommitCheck = CommitCheck;
     type Output = transaction::NoOutput;
+    type Mismatch = transaction::PreconditionFailed;
 
     #[allow(ambiguous_wide_pointer_comparisons)] // The hazards should be okay for this use case
     fn check(

diff --git a/all-is-cubes/src/block/block_def.rs b/all-is-cubes/src/block/block_def.rs
@@ -318,6 +318,7 @@ impl Transaction for BlockDefTransaction {
     type Target = BlockDef;
     type CommitCheck = ();
     type Output = transaction::NoOutput;
+    type Mismatch = transaction::PreconditionFailed;
 
     fn check(
         &self,

diff --git a/all-is-cubes/src/character.rs b/all-is-cubes/src/character.rs
@@ -733,6 +733,7 @@ impl Transaction for CharacterTransaction {
         <BehaviorSetTransaction<Character> as Transaction>::CommitCheck,
     );
     type Output = transaction::NoOutput;
+    type Mismatch = PreconditionFailed;
 
     fn check(&self, target: &Character) -> Result<Self::CommitCheck, PreconditionFailed> {
         let Self {

diff --git a/all-is-cubes/src/inv/inventory.rs b/all-is-cubes/src/inv/inventory.rs
@@ -362,6 +362,7 @@ impl Transaction for InventoryTransaction {
     type Target = Inventory;
     type CommitCheck = Option<InventoryCheck>;
     type Output = InventoryChange;
+    type Mismatch = PreconditionFailed;
 
     fn check(&self, inventory: &Inventory) -> Result<Self::CommitCheck, PreconditionFailed> {
         // Don't do the expensive copy if we have one already

diff --git a/all-is-cubes/src/physics/body.rs b/all-is-cubes/src/physics/body.rs
@@ -704,6 +704,7 @@ impl Transaction for BodyTransaction {
     type Target = Body;
     type CommitCheck = ();
     type Output = transaction::NoOutput;
+    type Mismatch = transaction::PreconditionFailed;
 
     fn check(&self, _body: &Body) -> Result<Self::CommitCheck, transaction::PreconditionFailed> {
         // No conflicts currently possible.

diff --git a/all-is-cubes/src/space/space_txn.rs b/all-is-cubes/src/space/space_txn.rs
@@ -175,6 +175,7 @@ impl Transaction for SpaceTransaction {
     type Target = Space;
     type CommitCheck = <BehaviorSetTransaction<Space> as Transaction>::CommitCheck;
     type Output = NoOutput;
+    type Mismatch = PreconditionFailed;
 
     fn check(&self, space: &Space) -> Result<Self::CommitCheck, PreconditionFailed> {
         for (

diff --git a/all-is-cubes/src/transaction.rs b/all-is-cubes/src/transaction.rs
@@ -52,12 +52,25 @@ pub trait Transaction: Merge {
     /// owner's [`Notifier`](crate::listen::Notifier).
     type Output;
 
+    /// Error type describing a precondition not met, returned by [`Self::check()`].
+    ///
+    /// This type should be cheap to construct and drop (hopefully `Copy`) if at all
+    /// possible, because checks may be done very frequently during simulation; not every
+    /// such failure is an error of interest to the user.
+    ///
+    /// Accordingly, it might not describe the _entire_ set of unmet preconditions,
+    /// but only one example from it, so as to avoid needing to allocate a
+    /// data structure of arbitrary size.
+    ///
+    /// This type should implement [`std::error::Error`] when possible.
+    type Mismatch: fmt::Debug + fmt::Display + 'static;
+
     /// Checks whether the target's current state meets the preconditions and returns
-    /// [`Err`] if it does not. (TODO: Informative error return type.)
+    /// [`Err`] if it does not.
     ///
     /// If the preconditions are met, returns [`Ok`] containing data to be passed to
     /// [`Transaction::commit`].
-    fn check(&self, target: &Self::Target) -> Result<Self::CommitCheck, PreconditionFailed>;
+    fn check(&self, target: &Self::Target) -> Result<Self::CommitCheck, Self::Mismatch>;
 
     /// Perform the mutations specified by this transaction. The `check` value should have
     /// been created by a prior call to [`Transaction::check()`].
@@ -190,7 +203,7 @@ pub trait Merge: Sized {
 
 /// Error type from [`Transaction::execute()`] and [`Transactional::transact()`].
 #[allow(clippy::exhaustive_enums)]
-pub enum ExecuteError<Txn: Merge = UniverseTransaction> {
+pub enum ExecuteError<Txn: Transaction = UniverseTransaction> {
     /// A conflict was discovered between parts that were to be assembled into the transaction.
     ///
     /// This error cannot be produced by [`Transaction::execute()`], but only by
@@ -199,7 +212,7 @@ pub enum ExecuteError<Txn: Merge = UniverseTransaction> {
 
     /// The transaction's preconditions were not met; it does not apply to the current
     /// state of the target. No change has been made.
-    Check(PreconditionFailed),
+    Check(<Txn as Transaction>::Mismatch),
 
     /// An unexpected error occurred while applying the transaction's effects.
     /// See the documentation of [`Transaction::commit()`] for the unfortunate
@@ -210,7 +223,7 @@ pub enum ExecuteError<Txn: Merge = UniverseTransaction> {
 // Manual impl required to set proper associated type bounds.
 impl<Txn> Clone for ExecuteError<Txn>
 where
-    Txn: Merge<Conflict: Clone>,
+    Txn: Transaction<Mismatch: Clone> + Merge<Conflict: Clone>,
 {
     fn clone(&self) -> Self {
         match self {
@@ -224,7 +237,7 @@ where
 crate::util::cfg_should_impl_error! {
     impl<Txn> std::error::Error for ExecuteError<Txn>
     where
-        Txn: Merge<Conflict: std::error::Error + 'static>,
+        Txn: Transaction<Mismatch: std::error::Error + 'static> + Merge<Conflict: std::error::Error + 'static>,
     {
         fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
             match self {
@@ -238,7 +251,7 @@ crate::util::cfg_should_impl_error! {
 
 impl<Txn> fmt::Debug for ExecuteError<Txn>
 where
-    Txn: Merge<Conflict: fmt::Debug>,
+    Txn: Transaction<Mismatch: fmt::Debug> + Merge<Conflict: fmt::Debug>,
 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
@@ -251,7 +264,7 @@ where
 
 impl<Txn> fmt::Display for ExecuteError<Txn>
 where
-    Txn: Merge<Conflict: fmt::Display>,
+    Txn: Transaction<Mismatch: fmt::Display> + Merge<Conflict: fmt::Display>,
 {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {

diff --git a/all-is-cubes/src/transaction/generic.rs b/all-is-cubes/src/transaction/generic.rs
@@ -2,7 +2,7 @@ use alloc::collections::BTreeMap;
 use core::hash::Hash;
 use core::{fmt, mem};
 
-use crate::transaction::{Merge, NoOutput, PreconditionFailed, Transaction};
+use crate::transaction::{Merge, NoOutput, Transaction};
 
 /// Transaction conflict error type for transactions on map types such as [`BTreeMap`].
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -167,16 +167,22 @@ macro_rules! impl_transaction_for_tuple {
                     $( <[<Tr $name>] as Transaction>::CommitCheck, )*
                 );
                 type Output = NoOutput;
+                type Mismatch = [< TupleError $count >]<
+                $( <[<Tr $name >] as Transaction>::Mismatch, )*
+            >;
 
                 fn check(
                     &self,
                     #[allow(unused_variables)] // empty tuple case
                     target: &($( [<Tr $name>]::Target, )*),
-                ) -> Result<Self::CommitCheck, PreconditionFailed> {
+                ) -> Result<Self::CommitCheck, Self::Mismatch> {
                     let ($( [<txn_ $name>], )*) = self;
                     let ($( [<target_ $name>], )*) = target;
                     Ok((
-                        $( [<txn_ $name>].check([<target_ $name>])?, )*
+                        $(
+                            [<txn_ $name>].check([<target_ $name>])
+                                .map_err([< TupleError $count >]::[<At $name>])?,
+                        )*
                     ))
                 }
 
@@ -225,6 +231,15 @@ macro_rules! impl_transaction_for_tuple {
                 }
             }
 
+            #[doc = concat!("Transaction precondition error type for tuples of length ", $count, ".")]
+            #[derive(Clone, Copy, Debug, Eq, PartialEq)]
+            #[allow(clippy::exhaustive_enums)]
+            pub enum [< TupleError $count >]<$( [<E $name>], )*> {
+                $(
+                    #[doc = concat!("Error at tuple element ", $name, ".")]
+                    [<At $name>]([<E $name>]),
+                )*
+            }
             #[doc = concat!("Transaction conflict error type for tuples of length ", $count, ".")]
             #[derive(Clone, Copy, Debug, Eq, PartialEq)]
             #[allow(clippy::exhaustive_enums)]
@@ -238,6 +253,15 @@ macro_rules! impl_transaction_for_tuple {
             // TODO: TupleConflict should have its own message to report the position,
             // instead of delegating.
             crate::util::cfg_should_impl_error! {
+                impl<$( [<E $name>]: std::error::Error, )*> std::error::Error for
+                        [< TupleError $count >]<$( [<E $name>], )*> {
+                    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+                        match *self {
+                            $( Self::[<At $name>](ref [<e $name>]) => [<e $name>].source(), )*
+                        }
+                    }
+                }
+
                 impl<$( [<C $name>]: std::error::Error, )*> std::error::Error for
                         [< TupleConflict $count >]<$( [<C $name>], )*> {
                     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
@@ -248,6 +272,15 @@ macro_rules! impl_transaction_for_tuple {
                 }
             }
 
+            impl<$( [<E $name>]: fmt::Display, )*> fmt::Display for
+                    [< TupleError $count >]<$( [<E $name>], )*> {
+                fn fmt(&self, #[allow(unused)] f: &mut fmt::Formatter<'_>) -> fmt::Result {
+                    match *self {
+                        $( Self::[<At $name>](ref [<e $name>]) => [<e $name>].fmt(f), )*
+                    }
+                }
+            }
+
             impl<$( [<C $name>]: fmt::Display, )*> fmt::Display for
                     [< TupleConflict $count >]<$( [<C $name>], )*> {
                 fn fmt(&self, #[allow(unused)] f: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -275,10 +308,10 @@ impl_transaction_for_tuple!(6: 0, 1, 2, 3, 4, 5);
 impl Transaction for () {
     type Target = ();
     type CommitCheck = ();
-
     type Output = core::convert::Infallible;
+    type Mismatch = core::convert::Infallible;
 
-    fn check(&self, (): &()) -> Result<Self::CommitCheck, PreconditionFailed> {
+    fn check(&self, (): &()) -> Result<Self::CommitCheck, Self::Mismatch> {
         Ok(())
     }
 

diff --git a/all-is-cubes/src/universe/handle.rs b/all-is-cubes/src/universe/handle.rs
@@ -227,6 +227,8 @@ impl<T: 'static> Handle<T> {
     ) -> Result<(), ExecuteError<<T as Transactional>::Transaction>>
     where
         T: Transactional,
+        // TODO: relax this bound and use a wrapper type instead, when custom error types are actually in use
+        T::Transaction: Transaction<Mismatch = PreconditionFailed>,
     {
         let outcome: Result<
             Result<(), ExecuteError<<T as Transactional>::Transaction>>,

diff --git a/all-is-cubes/src/universe/members.rs b/all-is-cubes/src/universe/members.rs
@@ -308,6 +308,7 @@ macro_rules! member_enums_and_impls {
             type Target = ();
             type CommitCheck = ut::AnyTransactionCheck;
             type Output = transaction::NoOutput;
+            type Mismatch = transaction::PreconditionFailed;
 
             fn check(
                 &self,

diff --git a/all-is-cubes/src/universe/universe_txn.rs b/all-is-cubes/src/universe/universe_txn.rs
@@ -51,10 +51,13 @@ pub(in crate::universe) struct TransactionInUniverse<O: Transactional + 'static>
 impl<O> Transaction for TransactionInUniverse<O>
 where
     O: Transactional + 'static,
+    // TODO: relax this bound and use a wrapper type instead, when custom error types are actually in use
+    O::Transaction: Transaction<Mismatch = PreconditionFailed>,
 {
     type Target = ();
     type CommitCheck = TransactionInUniverseCheck<O>;
     type Output = <O::Transaction as Transaction>::Output;
+    type Mismatch = PreconditionFailed;
 
     fn check(&self, _dummy_target: &()) -> Result<Self::CommitCheck, PreconditionFailed> {
         let guard = self
@@ -403,6 +406,7 @@ impl Transaction for UniverseTransaction {
     type Target = Universe;
     type CommitCheck = UniverseCommitCheck;
     type Output = transaction::NoOutput;
+    type Mismatch = PreconditionFailed;
 
     fn check(&self, target: &Universe) -> Result<Self::CommitCheck, PreconditionFailed> {
         if let Some(universe_id) = self.universe_id() {