Fallible systems (#16589)

# Objective

Error handling in bevy is hard. See for reference
#11562,
#10874 and
#12660. The goal of this PR is
to make it better, by allowing users to optionally return `Result` from
systems as outlined by Cart in
<#14275 (comment)>.

## Solution

This PR introduces a new `ScheuleSystem` type to represent systems that
can be added to schedules. Instances of this type contain either an
infallible `BoxedSystem<(), ()>` or a fallible `BoxedSystem<(),
Result>`. `ScheuleSystem` implements `System<In = (), Out = Result>` and
replaces all uses of `BoxedSystem` in schedules. The async executor now
receives a result after executing a system, which for infallible systems
is always `Ok(())`. Currently it ignores this result, but more useful
error handling could also be implemented.

Aliases for `Error` and `Result` have been added to the `bevy_ecs`
prelude, as well as const `OK` which new users may find more friendly
than `Ok(())`.

## Testing

- Currently there are not actual semantics changes that really require
new tests, but I added a basic one just to make sure we don't break
stuff in the future.
- The behavior of existing systems is totally unchanged, including
logging.
- All of the existing systems tests pass, and I have not noticed
anything strange while playing with the examples

## Showcase

The following minimal example prints "hello world" once, then completes.

```rust
use bevy::prelude::*;

fn main() {
    App::new().add_systems(Update, hello_world_system).run();
}

fn hello_world_system() -> Result {
    println!("hello world");
    Err("string")?;
    println!("goodbye world");
    OK
}
```

## Migration Guide

This change should be pretty much non-breaking, except for users who
have implemented their own custom executors. Those users should use
`ScheduleSystem` in place of `BoxedSystem<(), ()>` and import the
`System` trait where needed. They can choose to do whatever they wish
with the result.

## Current Work

+ [x] Fix tests & doc comments
+ [x] Write more tests
+ [x] Add examples
+ [X] Draft release notes

## Draft Release Notes

As of this release, systems can now return results.

First a bit of background: Bevy has hisotrically expected systems to
return the empty type `()`. While this makes sense in the context of the
ecs, it's at odds with how error handling is typically done in rust:
returning `Result::Error` to indicate failure, and using the
short-circuiting `?` operator to propagate that error up the call stack
to where it can be properly handled. Users of functional languages will
tell you this is called "monadic error handling".

Not being able to return `Results` from systems left bevy users with a
quandry. They could add custom error handling logic to every system, or
manually pipe every system into an error handler, or perhaps sidestep
the issue with some combination of fallible assignents, logging, macros,
and early returns. Often, users would just litter their systems with
unwraps and possible panics.

While any one of these approaches might be fine for a particular user,
each of them has their own drawbacks, and none makes good use of the
language. Serious issues could also arrise when two different crates
used by the same project made different choices about error handling.

Now, by returning results, systems can defer error handling to the
application itself. It looks like this:

```rust
// Previous, handling internally
app.add_systems(my_system)
fn my_system(window: Query<&Window>) {
   let Ok(window) = query.get_single() else {
       return;
   };
   // ... do something to the window here
}

// Previous, handling externally
app.add_systems(my_system.pipe(my_error_handler))
fn my_system(window: Query<&Window>) -> Result<(), impl Error> {
   let window = query.get_single()?;
   // ... do something to the window here
   Ok(())
}

// Previous, panicking
app.add_systems(my_system)
fn my_system(window: Query<&Window>) {
   let window = query.single();
   // ... do something to the window here
}

// Now 
app.add_systems(my_system)
fn my_system(window: Query<&Window>) -> Result {
    let window = query.get_single()?;
    // ... do something to the window here
    Ok(())
}
```

There are currently some limitations. Systems must either return `()` or
`Result<(), Box<dyn Error + Send + Sync + 'static>>`, with no
in-between. Results are also ignored by default, and though implementing
a custom handler is possible, it involves writing your own custom ecs
executor (which is *not* recomended).

Systems should return errors when they cannot perform their normal
behavior. In turn, errors returned to the executor while running the
schedule will (eventually) be treated as unexpected. Users and library
authors should prefer to return errors for anything that disrupts the
normal expected behavior of a system, and should only handle expected
cases internally.

We have big plans for improving error handling further:
+ Allowing users to change the error handling logic of the default
executors.
+ Adding source tracking and optional backtraces to errors.
+ Possibly adding tracing-levels (Error/Warn/Info/Debug/Trace) to
errors.
+ Generally making the default error logging more helpful and
inteligent.
+ Adding monadic system combininators for fallible systems.
+ Possibly removing all panicking variants from our api.

---------

Co-authored-by: Zachary Harrold <[email protected]>

Author: NthTensor

Cargo.toml

@@ -2031,6 +2031,17 @@ description = "Systems are skipped if their parameters cannot be acquired"
 category = "ECS (Entity Component System)"
 wasm = false
 
+[[example]]
+name = "fallible_systems"
+path = "examples/ecs/fallible_systems.rs"
+doc-scrape-examples = true
+
+[package.metadata.example.fallible_systems]
+name = "Fallible Systems"
+description = "Systems that return results to handle errors"
+category = "ECS (Entity Component System)"
+wasm = false
+
 [[example]]
 name = "startup_system"
 path = "examples/ecs/startup_system.rs"

crates/bevy_ecs/src/lib.rs

@@ -1,5 +1,7 @@
 // FIXME(11590): remove this once the lint is fixed
 #![allow(unsafe_op_in_unsafe_fn)]
+// TODO: remove once Edition 2024 is released
+#![allow(dependency_on_unit_never_type_fallback)]
 #![doc = include_str!("../README.md")]
 // `rustdoc_internals` is needed for `#[doc(fake_variadics)]`
 #![allow(internal_features)]
@@ -30,6 +32,7 @@ pub mod query;
 #[cfg(feature = "bevy_reflect")]
 pub mod reflect;
 pub mod removal_detection;
+pub mod result;
 pub mod schedule;
 pub mod storage;
 pub mod system;
@@ -53,6 +56,7 @@ pub mod prelude {
         observer::{CloneEntityWithObserversExt, Observer, Trigger},
         query::{Added, AnyOf, Changed, Has, Or, QueryBuilder, QueryState, With, Without},
         removal_detection::RemovedComponents,
+        result::{Error, Result},
         schedule::{
             apply_deferred, common_conditions::*, ApplyDeferred, Condition, IntoSystemConfigs,
             IntoSystemSet, IntoSystemSetConfigs, Schedule, Schedules, SystemSet,

crates/bevy_ecs/src/result.rs

@@ -0,0 +1,7 @@
+//! Contains error and result helpers for use in fallible systems.
+
+/// A dynamic error type for use in fallible systems.
+pub type Error = Box<dyn core::error::Error + Send + Sync + 'static>;
+
+/// A result type for use in fallible systems.
+pub type Result<T = (), E = Error> = core::result::Result<T, E>;

crates/bevy_ecs/src/schedule/config.rs

@@ -1,13 +1,14 @@
 use variadics_please::all_tuples;
 
 use crate::{
+    result::Result,
     schedule::{
         condition::{BoxedCondition, Condition},
         graph::{Ambiguity, Dependency, DependencyKind, GraphInfo},
         set::{InternedSystemSet, IntoSystemSet, SystemSet},
         Chain,
     },
-    system::{BoxedSystem, IntoSystem, System},
+    system::{BoxedSystem, IntoSystem, ScheduleSystem, System},
 };
 
 fn new_condition<M>(condition: impl Condition<M>) -> BoxedCondition {
@@ -47,7 +48,7 @@ pub struct NodeConfig<T> {
 }
 
 /// Stores configuration for a single system.
-pub type SystemConfig = NodeConfig<BoxedSystem>;
+pub type SystemConfig = NodeConfig<ScheduleSystem>;
 
 /// A collections of generic [`NodeConfig`]s.
 pub enum NodeConfigs<T> {
@@ -65,10 +66,10 @@ pub enum NodeConfigs<T> {
 }
 
 /// A collection of [`SystemConfig`].
-pub type SystemConfigs = NodeConfigs<BoxedSystem>;
+pub type SystemConfigs = NodeConfigs<ScheduleSystem>;
 
 impl SystemConfigs {
-    fn new_system(system: BoxedSystem) -> Self {
+    fn new_system(system: ScheduleSystem) -> Self {
         // include system in its default sets
         let sets = system.default_system_sets().into_iter().collect();
         Self::NodeConfig(SystemConfig {
@@ -517,18 +518,41 @@ impl IntoSystemConfigs<()> for SystemConfigs {
     }
 }
 
-impl<Marker, F> IntoSystemConfigs<Marker> for F
+#[doc(hidden)]
+pub struct Infallible;
+
+impl<F, Marker> IntoSystemConfigs<(Infallible, Marker)> for F
 where
     F: IntoSystem<(), (), Marker>,
 {
     fn into_configs(self) -> SystemConfigs {
-        SystemConfigs::new_system(Box::new(IntoSystem::into_system(self)))
+        let boxed_system = Box::new(IntoSystem::into_system(self));
+        SystemConfigs::new_system(ScheduleSystem::Infallible(boxed_system))
     }
 }
 
 impl IntoSystemConfigs<()> for BoxedSystem<(), ()> {
     fn into_configs(self) -> SystemConfigs {
-        SystemConfigs::new_system(self)
+        SystemConfigs::new_system(ScheduleSystem::Infallible(self))
+    }
+}
+
+#[doc(hidden)]
+pub struct Fallible;
+
+impl<F, Marker> IntoSystemConfigs<(Fallible, Marker)> for F
+where
+    F: IntoSystem<(), Result, Marker>,
+{
+    fn into_configs(self) -> SystemConfigs {
+        let boxed_system = Box::new(IntoSystem::into_system(self));
+        SystemConfigs::new_system(ScheduleSystem::Fallible(boxed_system))
+    }
+}
+
+impl IntoSystemConfigs<()> for BoxedSystem<(), Result> {
+    fn into_configs(self) -> SystemConfigs {
+        SystemConfigs::new_system(ScheduleSystem::Fallible(self))
     }
 }
 

crates/bevy_ecs/src/schedule/executor/mod.rs

@@ -19,7 +19,7 @@ use crate::{
     prelude::{IntoSystemSet, SystemSet},
     query::Access,
     schedule::{BoxedCondition, InternedSystemSet, NodeId, SystemTypeSet},
-    system::{BoxedSystem, System, SystemIn},
+    system::{ScheduleSystem, System, SystemIn},
     world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World},
 };
 
@@ -67,7 +67,7 @@ pub struct SystemSchedule {
     /// List of system node ids.
     pub(super) system_ids: Vec<NodeId>,
     /// Indexed by system node id.
-    pub(super) systems: Vec<BoxedSystem>,
+    pub(super) systems: Vec<ScheduleSystem>,
     /// Indexed by system node id.
     pub(super) system_conditions: Vec<Vec<BoxedCondition>>,
     /// Indexed by system node id.
@@ -140,9 +140,8 @@ pub const apply_deferred: ApplyDeferred = ApplyDeferred;
 pub struct ApplyDeferred;
 
 /// Returns `true` if the [`System`] is an instance of [`ApplyDeferred`].
-pub(super) fn is_apply_deferred(system: &BoxedSystem) -> bool {
-    // deref to use `System::type_id` instead of `Any::type_id`
-    system.as_ref().type_id() == TypeId::of::<ApplyDeferred>()
+pub(super) fn is_apply_deferred(system: &ScheduleSystem) -> bool {
+    system.type_id() == TypeId::of::<ApplyDeferred>()
 }
 
 impl System for ApplyDeferred {
@@ -247,19 +246,18 @@ mod __rust_begin_short_backtrace {
     use core::hint::black_box;
 
     use crate::{
-        system::{ReadOnlySystem, System},
+        result::Result,
+        system::{ReadOnlySystem, ScheduleSystem, System},
         world::{unsafe_world_cell::UnsafeWorldCell, World},
     };
 
     /// # Safety
     /// See `System::run_unsafe`.
     #[inline(never)]
-    pub(super) unsafe fn run_unsafe(
-        system: &mut dyn System<In = (), Out = ()>,
-        world: UnsafeWorldCell,
-    ) {
-        system.run_unsafe((), world);
+    pub(super) unsafe fn run_unsafe(system: &mut ScheduleSystem, world: UnsafeWorldCell) -> Result {
+        let result = system.run_unsafe((), world);
         black_box(());
+        result
     }
 
     /// # Safety
@@ -273,9 +271,10 @@ mod __rust_begin_short_backtrace {
     }
 
     #[inline(never)]
-    pub(super) fn run(system: &mut dyn System<In = (), Out = ()>, world: &mut World) {
-        system.run((), world);
+    pub(super) fn run(system: &mut ScheduleSystem, world: &mut World) -> Result {
+        let result = system.run((), world);
         black_box(());
+        result
     }
 
     #[inline(never)]

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

@@ -18,7 +18,7 @@ use crate::{
     prelude::Resource,
     query::Access,
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
-    system::BoxedSystem,
+    system::{ScheduleSystem, System},
     world::{unsafe_world_cell::UnsafeWorldCell, World},
 };
 
@@ -29,7 +29,7 @@ use super::__rust_begin_short_backtrace;
 /// Borrowed data used by the [`MultiThreadedExecutor`].
 struct Environment<'env, 'sys> {
     executor: &'env MultiThreadedExecutor,
-    systems: &'sys [SyncUnsafeCell<BoxedSystem>],
+    systems: &'sys [SyncUnsafeCell<ScheduleSystem>],
     conditions: SyncUnsafeCell<Conditions<'sys>>,
     world_cell: UnsafeWorldCell<'env>,
 }
@@ -269,7 +269,7 @@ impl<'scope, 'env: 'scope, 'sys> Context<'scope, 'env, 'sys> {
         &self,
         system_index: usize,
         res: Result<(), Box<dyn Any + Send>>,
-        system: &BoxedSystem,
+        system: &ScheduleSystem,
     ) {
         // tell the executor that the system finished
         self.environment
@@ -459,7 +459,7 @@ impl ExecutorState {
     fn can_run(
         &mut self,
         system_index: usize,
-        system: &mut BoxedSystem,
+        system: &mut ScheduleSystem,
         conditions: &mut Conditions,
         world: UnsafeWorldCell,
     ) -> bool {
@@ -523,7 +523,7 @@ impl ExecutorState {
     unsafe fn should_run(
         &mut self,
         system_index: usize,
-        system: &mut BoxedSystem,
+        system: &mut ScheduleSystem,
         conditions: &mut Conditions,
         world: UnsafeWorldCell,
     ) -> bool {
@@ -603,8 +603,9 @@ impl ExecutorState {
                 // access the world data used by the system.
                 // - `update_archetype_component_access` has been called.
                 unsafe {
-                    __rust_begin_short_backtrace::run_unsafe(
-                        &mut **system,
+                    // TODO: implement an error-handling API instead of suppressing a possible failure.
+                    let _ = __rust_begin_short_backtrace::run_unsafe(
+                        system,
                         context.environment.world_cell,
                     );
                 };
@@ -650,7 +651,8 @@ impl ExecutorState {
                 // that no other systems currently have access to the world.
                 let world = unsafe { context.environment.world_cell.world_mut() };
                 let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
-                    __rust_begin_short_backtrace::run(&mut **system, world);
+                    // TODO: implement an error-handling API instead of suppressing a possible failure.
+                    let _ = __rust_begin_short_backtrace::run(system, world);
                 }));
                 context.system_completed(system_index, res, system);
             };
@@ -710,7 +712,7 @@ impl ExecutorState {
 
 fn apply_deferred(
     unapplied_systems: &FixedBitSet,
-    systems: &[SyncUnsafeCell<BoxedSystem>],
+    systems: &[SyncUnsafeCell<ScheduleSystem>],
     world: &mut World,
 ) -> Result<(), Box<dyn Any + Send>> {
     for system_index in unapplied_systems.ones() {

crates/bevy_ecs/src/schedule/executor/simple.rs

@@ -7,6 +7,7 @@ use crate::{
     schedule::{
         executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
     },
+    system::System,
     world::World,
 };
 
@@ -100,7 +101,8 @@ impl SystemExecutor for SimpleExecutor {
             }
 
             let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
-                __rust_begin_short_backtrace::run(&mut **system, world);
+                // TODO: implement an error-handling API instead of suppressing a possible failure.
+                let _ = __rust_begin_short_backtrace::run(system, world);
             }));
             if let Err(payload) = res {
                 eprintln!("Encountered a panic in system `{}`!", &*system.name());
@@ -119,7 +121,7 @@ impl SystemExecutor for SimpleExecutor {
 
 impl SimpleExecutor {
     /// Creates a new simple executor for use in a [`Schedule`](crate::schedule::Schedule).
-    /// This calls each system in order and immediately calls [`System::apply_deferred`](crate::system::System::apply_deferred).
+    /// This calls each system in order and immediately calls [`System::apply_deferred`].
     pub const fn new() -> Self {
         Self {
             evaluated_sets: FixedBitSet::new(),

crates/bevy_ecs/src/schedule/executor/single_threaded.rs

@@ -5,6 +5,7 @@ use fixedbitset::FixedBitSet;
 
 use crate::{
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
+    system::System,
     world::World,
 };
 
@@ -108,14 +109,18 @@ impl SystemExecutor for SingleThreadedExecutor {
 
             let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
                 if system.is_exclusive() {
-                    __rust_begin_short_backtrace::run(&mut **system, world);
+                    // TODO: implement an error-handling API instead of suppressing a possible failure.
+                    let _ = __rust_begin_short_backtrace::run(system, world);
                 } else {
                     // Use run_unsafe to avoid immediately applying deferred buffers
                     let world = world.as_unsafe_world_cell();
                     system.update_archetype_component_access(world);
                     // SAFETY: We have exclusive, single-threaded access to the world and
                     // update_archetype_component_access is being called immediately before this.
-                    unsafe { __rust_begin_short_backtrace::run_unsafe(&mut **system, world) };
+                    unsafe {
+                        // TODO: implement an error-handling API instead of suppressing a possible failure.
+                        let _ = __rust_begin_short_backtrace::run_unsafe(system, world);
+                    };
                 }
             }));
             if let Err(payload) = res {