Replace PreconditionFailed with types for each transaction.

This allows the transactions to give much more specific information
about precondition failures, while still not allocating anything.

CHANGELOG.md

@@ -116,6 +116,8 @@
 
     - `op::Operation::Paint` has been removed. Use `Operation::Neighbors` and `Operation::DestroyTo` to get the same effect.
 
+    - `transaction::PreconditionFailed` no longer exists. It has been replaced with error types specific to each transaction type.
+
 ## 0.7.1 (2024-01-27)
 
 - `all-is-cubes` library:

all-is-cubes/src/behavior.rs

@@ -577,38 +577,35 @@ impl<H: BehaviorHost> Transaction for BehaviorSetTransaction<H> {
     type Target = BehaviorSet<H>;
     type CommitCheck = CommitCheck;
     type Output = transaction::NoOutput;
-    type Mismatch = transaction::PreconditionFailed;
+    type Mismatch = BehaviorTransactionMismatch;
 
     #[allow(ambiguous_wide_pointer_comparisons)] // The hazards should be okay for this use case
-    fn check(
-        &self,
-        target: &BehaviorSet<H>,
-    ) -> Result<Self::CommitCheck, transaction::PreconditionFailed> {
+    fn check(&self, target: &BehaviorSet<H>) -> Result<Self::CommitCheck, Self::Mismatch> {
         let Self { replace, insert } = self;
         // TODO: need to compare replacement preconditions
-        for (key, Replace { old, new: _ }) in replace {
+        for (&key, Replace { old, new: _ }) in replace {
             if let Some(BehaviorSetEntry {
                 attachment,
                 behavior,
                 waker: _,
-            }) = target.members.get(key)
+            }) = target.members.get(&key)
             {
-                if attachment != &old.attachment {
-                    return Err(transaction::PreconditionFailed {
-                        location: "BehaviorSet",
-                        problem: "existing behavior attachment is not as expected",
-                    });
-                }
-                if !Arc::ptr_eq(behavior, &old.behavior) {
-                    return Err(transaction::PreconditionFailed {
-                        location: "BehaviorSet",
-                        problem: "existing behavior value is not as expected",
+                let wrong_attachment = attachment != &old.attachment;
+                let wrong_value = !Arc::ptr_eq(behavior, &old.behavior);
+                if wrong_attachment || wrong_value {
+                    return Err(BehaviorTransactionMismatch {
+                        key,
+                        key_not_found: false,
+                        wrong_attachment,
+                        wrong_value,
                     });
                 }
             } else {
-                return Err(transaction::PreconditionFailed {
-                    location: "BehaviorSet",
-                    problem: "behavior(s) not found",
+                return Err(BehaviorTransactionMismatch {
+                    key,
+                    key_not_found: true,
+                    wrong_attachment: false,
+                    wrong_value: false,
                 });
             }
         }
@@ -775,10 +772,17 @@ pub struct MergeCheck {
     _private: (),
 }
 
+/// Transaction precondition error type for a [`BehaviorSet`].
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct BehaviorTransactionMismatch {
+    key: Key,
+    // These should probably really be an ErrorKind-style enum
+    key_not_found: bool,
+    wrong_attachment: bool,
+    wrong_value: bool,
+}
+
 /// Transaction conflict error type for a [`BehaviorSet`].
-//---
-// Currently private internals, because we will probably want to have a better strategy
-// for addressing behaviors than indices.
 #[derive(Clone, Debug, Eq, PartialEq, displaydoc::Display)]
 #[non_exhaustive]
 #[displaydoc("tried to replace the same behavior slot, {key}, twice")]
@@ -787,9 +791,37 @@ pub struct BehaviorTransactionConflict {
 }
 
 crate::util::cfg_should_impl_error! {
+    impl std::error::Error for BehaviorTransactionMismatch {}
     impl std::error::Error for BehaviorTransactionConflict {}
 }
 
+impl fmt::Display for BehaviorTransactionMismatch {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let &Self {
+            key,
+            key_not_found,
+            wrong_attachment,
+            wrong_value,
+        } = self;
+        write!(f, "behavior {key} ")?;
+        if key_not_found {
+            write!(f, "not found")?;
+        } else {
+            write!(f, "does not have a matching ")?;
+            if wrong_attachment && wrong_value {
+                write!(f, "attachment or value")?;
+            } else if wrong_attachment {
+                write!(f, "attachment")?;
+            } else if wrong_value {
+                write!(f, "value")?;
+            } else {
+                write!(f, "<error in error details>")?;
+            }
+        }
+        Ok(())
+    }
+}
+
 #[cfg(test)]
 pub(crate) use testing::*;
 #[cfg(test)]
@@ -1128,7 +1160,8 @@ mod tests {
         let mut set = BehaviorSet::<Space>::new();
         let attachment =
             SpaceBehaviorAttachment::new(GridAab::from_lower_size([0, 0, 0], [1, 1, 1]));
-        BehaviorSetTransaction::insert(attachment, Arc::new(NoopBehavior(1)))
+        let correct_old_behavior = Arc::new(NoopBehavior(1));
+        BehaviorSetTransaction::insert(attachment, correct_old_behavior.clone())
             .execute(&mut set, &mut no_outputs)
             .unwrap();
         let key = *set.members.keys().next().unwrap();
@@ -1151,9 +1184,11 @@ mod tests {
         );
         assert_eq!(
             transaction.check(&set).unwrap_err(),
-            transaction::PreconditionFailed {
-                location: "BehaviorSet",
-                problem: "existing behavior value is not as expected"
+            BehaviorTransactionMismatch {
+                key,
+                key_not_found: false,
+                wrong_attachment: false,
+                wrong_value: true,
             }
         );
 
@@ -1167,21 +1202,23 @@ mod tests {
                         [100, 0, 0],
                         [1, 1, 1],
                     )),
-                    behavior: Arc::new(NoopBehavior(1)),
+                    behavior: correct_old_behavior,
                     waker: None,
                 },
                 new: Some(BehaviorSetEntry {
                     attachment,
-                    behavior: Arc::new(NoopBehavior(1)),
+                    behavior: Arc::new(NoopBehavior(4)),
                     waker: None,
                 }),
             },
         );
         assert_eq!(
             transaction.check(&set).unwrap_err(),
-            transaction::PreconditionFailed {
-                location: "BehaviorSet",
-                problem: "existing behavior attachment is not as expected"
+            BehaviorTransactionMismatch {
+                key,
+                key_not_found: false,
+                wrong_attachment: true,
+                wrong_value: false,
             }
         );
     }

all-is-cubes/src/block/block_def.rs

@@ -318,18 +318,12 @@ impl Transaction for BlockDefTransaction {
     type Target = BlockDef;
     type CommitCheck = ();
     type Output = transaction::NoOutput;
-    type Mismatch = transaction::PreconditionFailed;
+    type Mismatch = BlockDefMismatch;
 
-    fn check(
-        &self,
-        target: &BlockDef,
-    ) -> Result<Self::CommitCheck, transaction::PreconditionFailed> {
+    fn check(&self, target: &BlockDef) -> Result<Self::CommitCheck, Self::Mismatch> {
         if let Some(old) = &self.old {
             if target.state.block != *old {
-                return Err(transaction::PreconditionFailed {
-                    location: "BlockDef",
-                    problem: "existing block not as expected",
-                });
+                return Err(BlockDefMismatch::Unexpected);
             }
         }
         Ok(())
@@ -378,6 +372,14 @@ impl transaction::Merge for BlockDefTransaction {
     }
 }
 
+/// Transaction precondition error type for a [`BlockDefTransaction`].
+#[derive(Clone, Debug, Eq, PartialEq, displaydoc::Display)]
+#[non_exhaustive]
+pub enum BlockDefMismatch {
+    /// old definition not as expected
+    Unexpected,
+}
+
 /// Transaction conflict error type for a [`BlockDefTransaction`].
 // ---
 // TODO: this is identical to `CubeConflict` but for the names
@@ -391,6 +393,7 @@ pub struct BlockDefConflict {
 }
 
 crate::util::cfg_should_impl_error! {
+    impl std::error::Error for BlockDefMismatch {}
     impl std::error::Error for BlockDefConflict {}
 }
 

all-is-cubes/src/character.rs

@@ -24,9 +24,7 @@ use crate::physics::{Body, BodyStepInfo, BodyTransaction, Contact, Velocity};
 use crate::save::schema;
 use crate::space::{CubeTransaction, Space};
 use crate::time::Tick;
-use crate::transaction::{
-    self, CommitError, Merge, PreconditionFailed, Transaction, Transactional,
-};
+use crate::transaction::{self, CommitError, Merge, Transaction, Transactional};
 use crate::universe::{Handle, HandleVisitor, UniverseTransaction, VisitHandles};
 use crate::util::{ConciseDebug, Refmt as _, StatusText};
 
@@ -733,19 +731,24 @@ impl Transaction for CharacterTransaction {
         <BehaviorSetTransaction<Character> as Transaction>::CommitCheck,
     );
     type Output = transaction::NoOutput;
-    type Mismatch = PreconditionFailed;
+    type Mismatch = CharacterTransactionMismatch;
 
-    fn check(&self, target: &Character) -> Result<Self::CommitCheck, PreconditionFailed> {
+    fn check(&self, target: &Character) -> Result<Self::CommitCheck, Self::Mismatch> {
         let Self {
             set_space: _, // no check needed
             body,
             inventory,
             behaviors,
         } = self;
         Ok((
-            body.check(&target.body)?,
-            inventory.check(&target.inventory)?,
-            behaviors.check(&target.behaviors)?,
+            body.check(&target.body)
+                .map_err(CharacterTransactionMismatch::Body)?,
+            inventory
+                .check(&target.inventory)
+                .map_err(CharacterTransactionMismatch::Inventory)?,
+            behaviors
+                .check(&target.behaviors)
+                .map_err(CharacterTransactionMismatch::Behaviors)?,
         ))
     }
 
@@ -819,6 +822,18 @@ impl Merge for CharacterTransaction {
     }
 }
 
+/// Transaction precondition error type for a [`CharacterTransaction`].
+#[derive(Clone, Debug, Eq, PartialEq, displaydoc::Display)]
+#[non_exhaustive]
+pub enum CharacterTransactionMismatch {
+    /// in character body
+    Body(<BodyTransaction as Transaction>::Mismatch),
+    /// in character inventory
+    Inventory(<InventoryTransaction as Transaction>::Mismatch),
+    /// in character behaviors
+    Behaviors(<BehaviorSetTransaction<Character> as Transaction>::Mismatch),
+}
+
 /// Transaction conflict error type for a [`CharacterTransaction`].
 #[derive(Clone, Debug, Eq, PartialEq, displaydoc::Display)]
 #[non_exhaustive]
@@ -834,6 +849,16 @@ pub enum CharacterTransactionConflict {
 }
 
 crate::util::cfg_should_impl_error! {
+    impl std::error::Error for CharacterTransactionMismatch {
+        fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+            match self {
+                CharacterTransactionMismatch::Body(e) => Some(e),
+                CharacterTransactionMismatch::Inventory(e) => Some(e),
+                CharacterTransactionMismatch::Behaviors(e) => Some(e),
+            }
+        }
+    }
+
     impl std::error::Error for CharacterTransactionConflict {
         fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
             match self {

all-is-cubes/src/inv/inventory.rs

@@ -10,7 +10,7 @@ use crate::block::Block;
 use crate::character::{Character, CharacterTransaction, Cursor};
 use crate::inv::{Icons, Tool, ToolError, ToolInput};
 use crate::linking::BlockProvider;
-use crate::transaction::{CommitError, Merge, PreconditionFailed, Transaction};
+use crate::transaction::{CommitError, Merge, Transaction};
 use crate::universe::{Handle, HandleVisitor, UniverseTransaction, VisitHandles};
 
 /// A collection of [`Tool`]s (items).
@@ -362,9 +362,9 @@ impl Transaction for InventoryTransaction {
     type Target = Inventory;
     type CommitCheck = Option<InventoryCheck>;
     type Output = InventoryChange;
-    type Mismatch = PreconditionFailed;
+    type Mismatch = InventoryMismatch;
 
-    fn check(&self, inventory: &Inventory) -> Result<Self::CommitCheck, PreconditionFailed> {
+    fn check(&self, inventory: &Inventory) -> Result<Self::CommitCheck, Self::Mismatch> {
         // Don't do the expensive copy if we have one already
         if self.replace.is_empty() && self.insert.is_empty() {
             return Ok(None);
@@ -383,16 +383,10 @@ impl Transaction for InventoryTransaction {
         for (&index, (old, new)) in self.replace.iter() {
             match slots.get_mut(index) {
                 None => {
-                    return Err(PreconditionFailed {
-                        location: "Inventory",
-                        problem: "slot out of bounds",
-                    });
+                    return Err(InventoryMismatch::OutOfBounds);
                 }
                 Some(actual_old) if actual_old != old => {
-                    return Err(PreconditionFailed {
-                        location: "Inventory",
-                        problem: "old slot not as expected",
-                    }); // TODO: it would be nice to squeeze in the slot number
+                    return Err(InventoryMismatch::UnexpectedSlot(index));
                 }
                 Some(slot) => {
                     *slot = new.clone();
@@ -413,10 +407,7 @@ impl Transaction for InventoryTransaction {
                 }
             }
             if new_stack != Slot::Empty {
-                return Err(PreconditionFailed {
-                    location: "Inventory",
-                    problem: "insufficient empty slots",
-                });
+                return Err(InventoryMismatch::Full);
             }
         }
 
@@ -471,6 +462,20 @@ pub struct InventoryCheck {
     change: InventoryChange,
 }
 
+/// Transaction precondition error type for an [`InventoryTransaction`].
+#[derive(Clone, Debug, Eq, PartialEq, displaydoc::Display)]
+#[non_exhaustive]
+pub enum InventoryMismatch {
+    /// insufficient empty slots
+    Full,
+
+    /// slot out of bounds
+    OutOfBounds,
+
+    /// contents of slot {0} not as expected
+    UnexpectedSlot(usize),
+}
+
 /// Transaction conflict error type for an [`InventoryTransaction`].
 #[derive(Clone, Debug, Eq, PartialEq, displaydoc::Display)]
 #[non_exhaustive]
@@ -483,6 +488,7 @@ pub enum InventoryConflict {
 }
 
 crate::util::cfg_should_impl_error! {
+    impl std::error::Error for InventoryMismatch {}
     impl std::error::Error for InventoryConflict {}
 }