Improved Command Errors (#17215)

# Objective

Rework / build on #17043 to simplify the implementation. #17043 should
be merged first, and the diff from this PR will get much nicer after it
is merged (this PR is net negative LOC).

## Solution

1. Command and EntityCommand have been vastly simplified. No more marker
components. Just one function.
2. Command and EntityCommand are now generic on the return type. This
enables result-less commands to exist, and allows us to statically
distinguish between fallible and infallible commands, which allows us to
skip the "error handling overhead" for cases that don't need it.
3. There are now only two command queue variants: `queue` and
`queue_fallible`. `queue` accepts commands with no return type.
`queue_fallible` accepts commands that return a Result (specifically,
one that returns an error that can convert to
`bevy_ecs::result::Error`).
4. I've added the concept of the "default error handler", which is used
by `queue_fallible`. This is a simple direct call to the `panic()` error
handler by default. Users that want to override this can enable the
`configurable_error_handler` cargo feature, then initialize the
GLOBAL_ERROR_HANDLER value on startup. This is behind a flag because
there might be minor overhead with `OnceLock` and I'm guessing this will
be a niche feature. We can also do perf testing with OnceLock if someone
really wants it to be used unconditionally, but I don't personally feel
the need to do that.
5. I removed the "temporary error handler" on Commands (and all code
associated with it). It added more branching, made Commands bigger /
more expensive to initialize (note that we construct it at high
frequencies / treat it like a pointer type), made the code harder to
follow, and introduced a bunch of additional functions. We instead rely
on the new default error handler used in `queue_fallible` for most
things. In the event that a custom handler is required,
`handle_error_with` can be used.
6. EntityCommand now _only_ supports functions that take
`EntityWorldMut` (and all existing entity commands have been ported).
Removing the marker component from EntityCommand hinged on this change,
but I strongly believe this is for the best anyway, as this sets the
stage for more efficient batched entity commands.
7. I added `EntityWorldMut::resource` and the other variants for more
ergonomic resource access on `EntityWorldMut` (removes the need for
entity.world_scope, which also incurs entity-lookup overhead).

## Open Questions

1. I believe we could merge `queue` and `queue_fallible` into a single
`queue` which accepts both fallible and infallible commands (via the
introduction of a `QueueCommand` trait). Is this desirable?

Author: cart

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
     }
 }