Improved Command Errors

benches/benches/bevy_ecs/world/commands.rs

@@ -2,7 +2,6 @@ use core::hint::black_box;
 
 use bevy_ecs::{
     component::Component,
-    result::Result,
     system::{Command, Commands},
     world::{CommandQueue, World},
 };
@@ -137,18 +136,16 @@ struct FakeCommandA;
 struct FakeCommandB(u64);
 
 impl Command for FakeCommandA {
-    fn apply(self, world: &mut World) -> Result {
+    fn apply(self, world: &mut World) {
         black_box(self);
         black_box(world);
-        Ok(())
     }
 }
 
 impl Command for FakeCommandB {
-    fn apply(self, world: &mut World) -> Result {
+    fn apply(self, world: &mut World) {
         black_box(self);
         black_box(world);
-        Ok(())
     }
 }
 
@@ -183,10 +180,9 @@ pub fn fake_commands(criterion: &mut Criterion) {
 struct SizedCommand<T: Default + Send + Sync + 'static>(T);
 
 impl<T: Default + Send + Sync + 'static> Command for SizedCommand<T> {
-    fn apply(self, world: &mut World) -> Result {
+    fn apply(self, world: &mut World) {
         black_box(self);
         black_box(world);
-        Ok(())
     }
 }
 

crates/bevy_ecs/Cargo.toml

@@ -28,6 +28,9 @@ bevy_reflect = ["dep:bevy_reflect"]
 ## Extends reflection support to functions.
 reflect_functions = ["bevy_reflect", "bevy_reflect/functions"]
 
+## Use the configurable global error handler as the default error handler
+configurable_error_handler = []
+
 # Debugging Features
 
 ## Enables `tracing` integration, allowing spans and other metrics to be reported

crates/bevy_ecs/src/reflect/entity_commands.rs

@@ -169,30 +169,24 @@ pub trait ReflectCommandExt {
 
 impl ReflectCommandExt for EntityCommands<'_> {
     fn insert_reflect(&mut self, component: Box<dyn PartialReflect>) -> &mut Self {
-        self.queue(move |entity: Entity, world: &mut World| {
-            if let Ok(mut entity) = world.get_entity_mut(entity) {
-                entity.insert_reflect(component);
-            }
+        self.queue(move |mut entity: EntityWorldMut| {
+            entity.insert_reflect(component);
         })
     }
 
     fn insert_reflect_with_registry<T: Resource + AsRef<TypeRegistry>>(
         &mut self,
         component: Box<dyn PartialReflect>,
     ) -> &mut Self {
-        self.queue(move |entity: Entity, world: &mut World| {
-            if let Ok(mut entity) = world.get_entity_mut(entity) {
-                entity.insert_reflect_with_registry::<T>(component);
-            }
+        self.queue(move |mut entity: EntityWorldMut| {
+            entity.insert_reflect_with_registry::<T>(component);
         })
     }
 
     fn remove_reflect(&mut self, component_type_path: impl Into<Cow<'static, str>>) -> &mut Self {
         let component_type_path: Cow<'static, str> = component_type_path.into();
-        self.queue(move |entity: Entity, world: &mut World| {
-            if let Ok(mut entity) = world.get_entity_mut(entity) {
-                entity.remove_reflect(component_type_path);
-            }
+        self.queue(move |mut entity: EntityWorldMut| {
+            entity.remove_reflect(component_type_path);
         })
     }
 
@@ -201,10 +195,8 @@ impl ReflectCommandExt for EntityCommands<'_> {
         component_type_path: impl Into<Cow<'static, str>>,
     ) -> &mut Self {
         let component_type_path: Cow<'static, str> = component_type_path.into();
-        self.queue(move |entity: Entity, world: &mut World| {
-            if let Ok(mut entity) = world.get_entity_mut(entity) {
-                entity.remove_reflect_with_registry::<T>(component_type_path);
-            }
+        self.queue(move |mut entity: EntityWorldMut| {
+            entity.remove_reflect_with_registry::<T>(component_type_path);
         })
     }
 }

crates/bevy_ecs/src/system/commands/command.rs

@@ -12,16 +12,18 @@ use crate::{
     entity::Entity,
     event::{Event, Events},
     observer::TriggerTargets,
-    result::Result,
+    result::{Error, Result},
     schedule::ScheduleLabel,
-    system::{CommandError, IntoSystem, Resource, SystemId, SystemInput},
+    system::{error_handler, IntoSystem, Resource, SystemId, SystemInput},
     world::{FromWorld, SpawnBatchIter, World},
 };
 
 /// A [`World`] mutation.
 ///
 /// Should be used with [`Commands::queue`](crate::system::Commands::queue).
 ///
+/// The `Out` generic parameter is the returned "output" of the command.
+///
 /// # Usage
 ///
 /// ```
@@ -34,102 +36,70 @@ use crate::{
 /// struct AddToCounter(u64);
 ///
 /// impl Command for AddToCounter {
-///     fn apply(self, world: &mut World) -> Result {
+///     fn apply(self, world: &mut World) {
 ///         let mut counter = world.get_resource_or_insert_with(Counter::default);
 ///         counter.0 += self.0;
-///         Ok(())
 ///     }
 /// }
 ///
 /// fn some_system(mut commands: Commands) {
 ///     commands.queue(AddToCounter(42));
 /// }
 /// ```
-///
-/// # Note on Generic
-///
-/// The `Marker` generic is necessary to allow multiple blanket implementations
-/// of `Command` for closures, like so:
-/// ```ignore (This would conflict with the real implementations)
-/// impl Command for FnOnce(&mut World)
-/// impl Command<Result> for FnOnce(&mut World) -> Result
-/// ```
-/// Without the generic, Rust would consider the two implementations to be conflicting.
-///
-/// The type used for `Marker` has no connection to anything else in the implementation.
-pub trait Command<Marker = ()>: Send + 'static {
+pub trait Command<Out = ()>: Send + 'static {
     /// Applies this command, causing it to mutate the provided `world`.
     ///
     /// This method is used to define what a command "does" when it is ultimately applied.
     /// Because this method takes `self`, you can store data or settings on the type that implements this trait.
     /// This data is set by the system or other source of the command, and then ultimately read in this method.
-    fn apply(self, world: &mut World) -> Result;
+    fn apply(self, world: &mut World) -> Out;
+}
 
-    /// Applies this command and converts any resulting error into a [`CommandError`].
-    ///
-    /// Overwriting this method allows an implementor to return a `CommandError` directly
-    /// and avoid erasing the error's type.
-    fn apply_internal(self, world: &mut World) -> Result<(), CommandError>
-    where
-        Self: Sized,
-    {
-        match self.apply(world) {
-            Ok(_) => Ok(()),
-            Err(error) => Err(CommandError::CommandFailed(error)),
-        }
+impl<F, Out> Command<Out> for F
+where
+    F: FnOnce(&mut World) -> Out + Send + 'static,
+{
+    fn apply(self, world: &mut World) -> Out {
+        self(world)
     }
+}
 
-    /// Returns a new [`Command`] that, when applied, will apply the original command
-    /// and pass any resulting error to the provided `error_handler`.
-    fn with_error_handling(
-        self,
-        error_handler: Option<fn(&mut World, CommandError)>,
-    ) -> impl Command
+/// Takes a [`Command`] that returns a Result and uses a given error handler function to convert it into
+/// a [`Command`] that internally handles an error if it occurs and returns `()`.
+pub trait HandleError<Out = ()> {
+    /// Takes a [`Command`] that returns a Result and uses a given error handler function to convert it into
+    /// a [`Command`] that internally handles an error if it occurs and returns `()`.
+    fn handle_error_with(self, error_handler: fn(&mut World, Error)) -> impl Command;
+    /// Takes a [`Command`] that returns a Result and uses the default error handler function to convert it into
+    /// a [`Command`] that internally handles an error if it occurs and returns `()`.
+    fn handle_error(self) -> impl Command
     where
         Self: Sized,
     {
-        move |world: &mut World| {
-            if let Err(error) = self.apply_internal(world) {
-                // TODO: Pass the error to the global error handler if `error_handler` is `None`.
-                let error_handler = error_handler.unwrap_or(|_, error| panic!("{error}"));
-                error_handler(world, error);
-            }
-        }
-    }
-}
-
-impl<F> Command for F
-where
-    F: FnOnce(&mut World) + Send + 'static,
-{
-    fn apply(self, world: &mut World) -> Result {
-        self(world);
-        Ok(())
+        self.handle_error_with(error_handler::default())
     }
 }
 
-impl<F> Command<Result> for F
-where
-    F: FnOnce(&mut World) -> Result + Send + 'static,
-{
-    fn apply(self, world: &mut World) -> Result {
-        self(world)
+impl<C: Command<Result<T, E>>, T, E: Into<Error>> HandleError<Result<T, E>> for C {
+    fn handle_error_with(self, error_handler: fn(&mut World, Error)) -> impl Command {
+        move |world: &mut World| match self.apply(world) {
+            Ok(_) => {}
+            Err(err) => (error_handler)(world, err.into()),
+        }
     }
 }
 
-/// Necessary to avoid erasing the type of the `CommandError` in
-/// [`EntityCommand::with_entity`](crate::system::EntityCommand::with_entity).
-impl<F> Command<(Result, CommandError)> for F
-where
-    F: FnOnce(&mut World) -> Result<(), CommandError> + Send + 'static,
-{
-    fn apply(self, world: &mut World) -> Result {
-        self(world)?;
-        Ok(())
+impl<C: Command> HandleError for C {
+    #[inline]
+    fn handle_error_with(self, _error_handler: fn(&mut World, Error)) -> impl Command {
+        self
     }
-
-    fn apply_internal(self, world: &mut World) -> Result<(), CommandError> {
-        self(world)
+    #[inline]
+    fn handle_error(self) -> impl Command
+    where
+        Self: Sized,
+    {
+        self
     }
 }