Define system param validation on a per-system parameter basis (#18504)

# Objective

When introduced, `Single` was intended to simply be silently skipped,
allowing for graceful and efficient handling of systems during invalid
game states (such as when the player is dead).

However, this also caused missing resources to *also* be silently
skipped, leading to confusing and very hard to debug failures. In
0.15.1, this behavior was reverted to a panic, making missing resources
easier to debug, but largely making `Single` (and `Populated`)
worthless, as they would panic during expected game states.

Ultimately, the consensus is that this behavior should differ on a
per-system-param basis. However, there was no sensible way to *do* that
before this PR.

## Solution

Swap `SystemParam::validate_param` from a `bool` to:

```rust
/// The outcome of system / system param validation,
/// used by system executors to determine what to do with a system.
pub enum ValidationOutcome {
    /// All system parameters were validated successfully and the system can be run.
    Valid,
    /// At least one system parameter failed validation, and an error must be handled.
    /// By default, this will result in1 a panic. See [crate::error] for more information.
    ///
    /// This is the default behavior, and is suitable for system params that should *always* be valid,
    /// either because sensible fallback behavior exists (like [`Query`] or because
    /// failures in validation should be considered a bug in the user's logic that must be immediately addressed (like [`Res`]).
    Invalid,
    /// At least one system parameter failed validation, but the system should be skipped due to [`ValidationBehavior::Skip`].
    /// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`].
    Skipped,
}
```
Then, inside of each `SystemParam` implementation, return either Valid,
Invalid or Skipped.

Currently, only `Single`, `Option<Single>` and `Populated` use the
`Skipped` behavior. Other params (like resources) retain their current
failing

## Testing

Messed around with the fallible_params example. Added a pair of tests:
one for panicking when resources are missing, and another for properly
skipping `Single` and `Populated` system params.

## To do

- [x] get #18454 merged
- [x] fix the todo!() in the macro-powered tuple implementation (please
help 🥺)
- [x] test
- [x] write a migration guide
- [x] update the example comments

## Migration Guide

Various system and system parameter validation methods
(`SystemParam::validate_param`, `System::validate_param` and
`System::validate_param_unsafe`) now return and accept a
`ValidationOutcome` enum, rather than a `bool`. The previous `true`
values map to `ValidationOutcome::Valid`, while `false` maps to
`ValidationOutcome::Invalid`.

However, if you wrote a custom schedule executor, you should now respect
the new `ValidationOutcome::Skipped` parameter, skipping any systems
whose validation was skipped. By contrast, `ValidationOutcome::Invalid`
systems should also be skipped, but you should call the
`default_error_handler` on them first, which by default will result in a
panic.

If you are implementing a custom `SystemParam`, you should consider
whether failing system param validation is an error or an expected
state, and choose between `Invalid` and `Skipped` accordingly. In Bevy
itself, `Single` and `Populated` now once again skip the system when
their conditions are not met. This is the 0.15.0 behavior, but stands in
contrast to the 0.15.1 behavior, where they would panic.

---------

Co-authored-by: MiniaczQ <[email protected]>
Co-authored-by: Dmytro Banin <[email protected]>
Co-authored-by: Chris Russell <[email protected]>

Author: 4 people

crates/bevy_ecs/macros/src/lib.rs

@@ -429,7 +429,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
                     state: &'s Self::State,
                     system_meta: &#path::system::SystemMeta,
                     world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
-                ) -> bool {
+                ) -> #path::system::ValidationOutcome {
                     <(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world)
                 }
 

crates/bevy_ecs/src/observer/runner.rs

@@ -7,7 +7,7 @@ use crate::{
     observer::{ObserverDescriptor, ObserverTrigger},
     prelude::*,
     query::DebugCheckedUnwrap,
-    system::{IntoObserverSystem, ObserverSystem, SystemParamValidationError},
+    system::{IntoObserverSystem, ObserverSystem, SystemParamValidationError, ValidationOutcome},
     world::DeferredWorld,
 };
 use bevy_ptr::PtrMut;
@@ -405,25 +405,27 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     // - system is the same type erased system from above
     unsafe {
         (*system).update_archetype_component_access(world);
-        if (*system).validate_param_unsafe(world) {
-            if let Err(err) = (*system).run_unsafe(trigger, world) {
-                error_handler(
-                    err,
-                    ErrorContext::Observer {
-                        name: (*system).name(),
-                        last_run: (*system).get_last_run(),
-                    },
-                );
-            };
-            (*system).queue_deferred(world.into_deferred());
-        } else {
-            error_handler(
+        match (*system).validate_param_unsafe(world) {
+            ValidationOutcome::Valid => {
+                if let Err(err) = (*system).run_unsafe(trigger, world) {
+                    error_handler(
+                        err,
+                        ErrorContext::Observer {
+                            name: (*system).name(),
+                            last_run: (*system).get_last_run(),
+                        },
+                    );
+                };
+                (*system).queue_deferred(world.into_deferred());
+            }
+            ValidationOutcome::Invalid => error_handler(
                 SystemParamValidationError.into(),
                 ErrorContext::Observer {
                     name: (*system).name(),
                     last_run: (*system).get_last_run(),
                 },
-            );
+            ),
+            ValidationOutcome::Skipped => (),
         }
     }
 }

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

@@ -20,7 +20,7 @@ use crate::{
     prelude::{IntoSystemSet, SystemSet},
     query::Access,
     schedule::{BoxedCondition, InternedSystemSet, NodeId, SystemTypeSet},
-    system::{ScheduleSystem, System, SystemIn},
+    system::{ScheduleSystem, System, SystemIn, ValidationOutcome},
     world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World},
 };
 
@@ -221,10 +221,10 @@ impl System for ApplyDeferred {
 
     fn queue_deferred(&mut self, _world: DeferredWorld) {}
 
-    unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool {
+    unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> ValidationOutcome {
         // This system is always valid to run because it doesn't do anything,
         // and only used as a marker for the executor.
-        true
+        ValidationOutcome::Valid
     }
 
     fn initialize(&mut self, _world: &mut World) {}
@@ -312,49 +312,97 @@ mod __rust_begin_short_backtrace {
 #[cfg(test)]
 mod tests {
     use crate::{
-        prelude::{IntoScheduleConfigs, Resource, Schedule, SystemSet},
+        prelude::{Component, Resource, Schedule},
         schedule::ExecutorKind,
-        system::Commands,
+        system::{Populated, Res, ResMut, Single},
         world::World,
     };
 
-    #[derive(Resource)]
-    struct R1;
-
-    #[derive(Resource)]
-    struct R2;
+    #[derive(Component)]
+    struct TestComponent;
 
     const EXECUTORS: [ExecutorKind; 3] = [
         ExecutorKind::Simple,
         ExecutorKind::SingleThreaded,
         ExecutorKind::MultiThreaded,
     ];
 
+    #[derive(Resource, Default)]
+    struct TestState {
+        populated_ran: bool,
+        single_ran: bool,
+    }
+
+    fn set_single_state(mut _single: Single<&TestComponent>, mut state: ResMut<TestState>) {
+        state.single_ran = true;
+    }
+
+    fn set_populated_state(
+        mut _populated: Populated<&TestComponent>,
+        mut state: ResMut<TestState>,
+    ) {
+        state.populated_ran = true;
+    }
+
     #[test]
-    fn invalid_system_param_skips() {
+    #[expect(clippy::print_stdout, reason = "std and println are allowed in tests")]
+    fn single_and_populated_skipped_and_run() {
         for executor in EXECUTORS {
-            invalid_system_param_skips_core(executor);
+            std::println!("Testing executor: {:?}", executor);
+
+            let mut world = World::new();
+            world.init_resource::<TestState>();
+
+            let mut schedule = Schedule::default();
+            schedule.set_executor_kind(executor);
+            schedule.add_systems((set_single_state, set_populated_state));
+            schedule.run(&mut world);
+
+            let state = world.get_resource::<TestState>().unwrap();
+            assert!(!state.single_ran);
+            assert!(!state.populated_ran);
+
+            world.spawn(TestComponent);
+
+            schedule.run(&mut world);
+            let state = world.get_resource::<TestState>().unwrap();
+            assert!(state.single_ran);
+            assert!(state.populated_ran);
         }
     }
 
-    fn invalid_system_param_skips_core(executor: ExecutorKind) {
+    fn look_for_missing_resource(_res: Res<TestState>) {}
+
+    #[test]
+    #[should_panic]
+    fn missing_resource_panics_simple() {
         let mut world = World::new();
         let mut schedule = Schedule::default();
-        schedule.set_executor_kind(executor);
-        schedule.add_systems(
-            (
-                // This system depends on a system that is always skipped.
-                (|mut commands: Commands| {
-                    commands.insert_resource(R2);
-                }),
-            )
-                .chain(),
-        );
+
+        schedule.set_executor_kind(ExecutorKind::Simple);
+        schedule.add_systems(look_for_missing_resource);
+        schedule.run(&mut world);
+    }
+
+    #[test]
+    #[should_panic]
+    fn missing_resource_panics_single_threaded() {
+        let mut world = World::new();
+        let mut schedule = Schedule::default();
+
+        schedule.set_executor_kind(ExecutorKind::SingleThreaded);
+        schedule.add_systems(look_for_missing_resource);
         schedule.run(&mut world);
-        assert!(world.get_resource::<R1>().is_none());
-        assert!(world.get_resource::<R2>().is_some());
     }
 
-    #[derive(SystemSet, Hash, Debug, PartialEq, Eq, Clone)]
-    struct S1;
+    #[test]
+    #[should_panic]
+    fn missing_resource_panics_multi_threaded() {
+        let mut world = World::new();
+        let mut schedule = Schedule::default();
+
+        schedule.set_executor_kind(ExecutorKind::MultiThreaded);
+        schedule.add_systems(look_for_missing_resource);
+        schedule.run(&mut world);
+    }
 }

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::{ScheduleSystem, SystemParamValidationError},
+    system::{ScheduleSystem, SystemParamValidationError, ValidationOutcome},
     world::{unsafe_world_cell::UnsafeWorldCell, World},
 };
 
@@ -581,18 +581,24 @@ impl ExecutorState {
             // - The caller ensures that `world` has permission to read any data
             //   required by the system.
             // - `update_archetype_component_access` has been called for system.
-            let valid_params = unsafe { system.validate_param_unsafe(world) };
+            let valid_params = match unsafe { system.validate_param_unsafe(world) } {
+                ValidationOutcome::Valid => true,
+                ValidationOutcome::Invalid => {
+                    error_handler(
+                        SystemParamValidationError.into(),
+                        ErrorContext::System {
+                            name: system.name(),
+                            last_run: system.get_last_run(),
+                        },
+                    );
+                    false
+                }
+                ValidationOutcome::Skipped => false,
+            };
             if !valid_params {
-                error_handler(
-                    SystemParamValidationError.into(),
-                    ErrorContext::System {
-                        name: system.name(),
-                        last_run: system.get_last_run(),
-                    },
-                );
-
                 self.skipped_systems.insert(system_index);
             }
+
             should_run &= valid_params;
         }
 
@@ -789,16 +795,19 @@ unsafe fn evaluate_and_fold_conditions(
             // - The caller ensures that `world` has permission to read any data
             //   required by the condition.
             // - `update_archetype_component_access` has been called for condition.
-            if !unsafe { condition.validate_param_unsafe(world) } {
-                error_handler(
-                    SystemParamValidationError.into(),
-                    ErrorContext::System {
-                        name: condition.name(),
-                        last_run: condition.get_last_run(),
-                    },
-                );
-
-                return false;
+            match unsafe { condition.validate_param_unsafe(world) } {
+                ValidationOutcome::Valid => (),
+                ValidationOutcome::Invalid => {
+                    error_handler(
+                        SystemParamValidationError.into(),
+                        ErrorContext::System {
+                            name: condition.name(),
+                            last_run: condition.get_last_run(),
+                        },
+                    );
+                    return false;
+                }
+                ValidationOutcome::Skipped => return false,
             }
             // SAFETY:
             // - The caller ensures that `world` has permission to read any data

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

@@ -12,7 +12,7 @@ use crate::{
     schedule::{
         executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
     },
-    system::SystemParamValidationError,
+    system::{SystemParamValidationError, ValidationOutcome},
     world::World,
 };
 
@@ -88,17 +88,20 @@ impl SystemExecutor for SimpleExecutor {
 
             let system = &mut schedule.systems[system_index];
             if should_run {
-                let valid_params = system.validate_param(world);
-                if !valid_params {
-                    error_handler(
-                        SystemParamValidationError.into(),
-                        ErrorContext::System {
-                            name: system.name(),
-                            last_run: system.get_last_run(),
-                        },
-                    );
-                }
-
+                let valid_params = match system.validate_param(world) {
+                    ValidationOutcome::Valid => true,
+                    ValidationOutcome::Invalid => {
+                        error_handler(
+                            SystemParamValidationError.into(),
+                            ErrorContext::System {
+                                name: system.name(),
+                                last_run: system.get_last_run(),
+                            },
+                        );
+                        false
+                    }
+                    ValidationOutcome::Skipped => false,
+                };
                 should_run &= valid_params;
             }
 
@@ -173,15 +176,19 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
     conditions
         .iter_mut()
         .map(|condition| {
-            if !condition.validate_param(world) {
-                error_handler(
-                    SystemParamValidationError.into(),
-                    ErrorContext::RunCondition {
-                        name: condition.name(),
-                        last_run: condition.get_last_run(),
-                    },
-                );
-                return false;
+            match condition.validate_param(world) {
+                ValidationOutcome::Valid => (),
+                ValidationOutcome::Invalid => {
+                    error_handler(
+                        SystemParamValidationError.into(),
+                        ErrorContext::System {
+                            name: condition.name(),
+                            last_run: condition.get_last_run(),
+                        },
+                    );
+                    return false;
+                }
+                ValidationOutcome::Skipped => return false,
             }
             __rust_begin_short_backtrace::readonly_run(&mut **condition, world)
         })