Reduce runtime panics through SystemParam validation (#15276)

# Objective

The goal of this PR is to introduce `SystemParam` validation in order to
reduce runtime panics.

Fixes #15265

## Solution

`SystemParam` now has a new method `validate_param(...) -> bool`, which
takes immutable variants of `get_param` arguments. The returned value
indicates whether the parameter can be acquired from the world. If
parameters cannot be acquired for a system, it won't be executed,
similarly to run conditions. This reduces panics when using params like
`Res`, `ResMut`, etc. as well as allows for new, ergonomic params like
#15264 or #15302.

Param validation happens at the level of executors. All validation
happens directly before executing a system, in case of normal systems
they are skipped, in case of conditions they return false.

Warning about system skipping is primitive and subject to change in
subsequent PRs.

## Testing

Two executor tests check that all executors:
- skip systems which have invalid parameters:
  - piped systems get skipped together,
  - dependent systems still run correctly,
- skip systems with invalid run conditions:
  - system conditions have invalid parameters,
  - system set conditions have invalid parameters.

Author: MiniaczQ

crates/bevy_ecs/macros/src/lib.rs

@@ -271,6 +271,15 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
                     <(#(#param,)*) as SystemParam>::apply(state, system_meta, world);
                 }
 
+                #[inline]
+                unsafe fn validate_param<'w, 's>(
+                    state: &'s Self::State,
+                    system_meta: &SystemMeta,
+                    world: UnsafeWorldCell<'w>,
+                ) -> bool {
+                    <(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world)
+                }
+
                 #[inline]
                 unsafe fn get_param<'w, 's>(
                     state: &'s mut Self::State,
@@ -512,6 +521,16 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
                     <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::queue(&mut state.state, system_meta, world);
                 }
 
+                #[inline]
+                unsafe fn validate_param<'w, 's>(
+                    state: &'s Self::State,
+                    system_meta: &#path::system::SystemMeta,
+                    world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
+                ) -> bool {
+                    <(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world)
+                }
+
+                #[inline]
                 unsafe fn get_param<'w, 's>(
                     state: &'s mut Self::State,
                     system_meta: &#path::system::SystemMeta,

crates/bevy_ecs/src/schedule/condition.rs

@@ -79,7 +79,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
     ///
     /// # Examples
     ///
-    /// ```should_panic
+    /// ```
     /// use bevy_ecs::prelude::*;
     ///
     /// #[derive(Resource, PartialEq)]
@@ -89,7 +89,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
     /// # let mut world = World::new();
     /// # fn my_system() {}
     /// app.add_systems(
-    ///     // The `resource_equals` run condition will panic since we don't initialize `R`,
+    ///     // The `resource_equals` run condition will fail since we don't initialize `R`,
     ///     // just like if we used `Res<R>` in a system.
     ///     my_system.run_if(resource_equals(R(0))),
     /// );
@@ -130,7 +130,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
     ///
     /// # Examples
     ///
-    /// ```should_panic
+    /// ```
     /// use bevy_ecs::prelude::*;
     ///
     /// #[derive(Resource, PartialEq)]
@@ -140,7 +140,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
     /// # let mut world = World::new();
     /// # fn my_system() {}
     /// app.add_systems(
-    ///     // The `resource_equals` run condition will panic since we don't initialize `R`,
+    ///     // The `resource_equals` run condition will fail since we don't initialize `R`,
     ///     // just like if we used `Res<R>` in a system.
     ///     my_system.run_if(resource_equals(R(0))),
     /// );

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

@@ -176,3 +176,100 @@ mod __rust_begin_short_backtrace {
         black_box(system.run((), world))
     }
 }
+
+#[macro_export]
+/// Emits a warning about system being skipped.
+macro_rules! warn_system_skipped {
+    ($ty:literal, $sys:expr) => {
+        bevy_utils::tracing::warn!(
+            "{} {} was skipped due to inaccessible system parameters.",
+            $ty,
+            $sys
+        )
+    };
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::{
+        self as bevy_ecs,
+        prelude::{IntoSystemConfigs, IntoSystemSetConfigs, Resource, Schedule, SystemSet},
+        schedule::ExecutorKind,
+        system::{Commands, In, IntoSystem, Res},
+        world::World,
+    };
+
+    #[derive(Resource)]
+    struct R1;
+
+    #[derive(Resource)]
+    struct R2;
+
+    const EXECUTORS: [ExecutorKind; 3] = [
+        ExecutorKind::Simple,
+        ExecutorKind::SingleThreaded,
+        ExecutorKind::MultiThreaded,
+    ];
+
+    #[test]
+    fn invalid_system_param_skips() {
+        for executor in EXECUTORS {
+            invalid_system_param_skips_core(executor);
+        }
+    }
+
+    fn invalid_system_param_skips_core(executor: ExecutorKind) {
+        let mut world = World::new();
+        let mut schedule = Schedule::default();
+        schedule.set_executor_kind(executor);
+        schedule.add_systems(
+            (
+                // Combined systems get skipped together.
+                (|mut commands: Commands| {
+                    commands.insert_resource(R1);
+                })
+                .pipe(|_: In<()>, _: Res<R1>| {}),
+                // This system depends on a system that is always skipped.
+                |mut commands: Commands| {
+                    commands.insert_resource(R2);
+                },
+            )
+                .chain(),
+        );
+        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]
+    fn invalid_condition_param_skips_system() {
+        for executor in EXECUTORS {
+            invalid_condition_param_skips_system_core(executor);
+        }
+    }
+
+    fn invalid_condition_param_skips_system_core(executor: ExecutorKind) {
+        let mut world = World::new();
+        let mut schedule = Schedule::default();
+        schedule.set_executor_kind(executor);
+        schedule.configure_sets(S1.run_if(|_: Res<R1>| true));
+        schedule.add_systems((
+            // System gets skipped if system set run conditions fail validation.
+            (|mut commands: Commands| {
+                commands.insert_resource(R1);
+            })
+            .in_set(S1),
+            // System gets skipped if run conditions fail validation.
+            (|mut commands: Commands| {
+                commands.insert_resource(R2);
+            })
+            .run_if(|_: Res<R2>| true),
+        ));
+        schedule.run(&mut world);
+        assert!(world.get_resource::<R1>().is_none());
+        assert!(world.get_resource::<R2>().is_none());
+    }
+}

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

@@ -19,6 +19,7 @@ use crate::{
     query::Access,
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
     system::BoxedSystem,
+    warn_system_skipped,
     world::{unsafe_world_cell::UnsafeWorldCell, World},
 };
 
@@ -519,15 +520,16 @@ impl ExecutorState {
     ///   the system's conditions: this includes conditions for the system
     ///   itself, and conditions for any of the system's sets.
     /// * `update_archetype_component` must have been called with `world`
-    ///   for each run condition in `conditions`.
+    ///   for the system as well as system and system set's run conditions.
     unsafe fn should_run(
         &mut self,
         system_index: usize,
-        _system: &BoxedSystem,
+        system: &BoxedSystem,
         conditions: &mut Conditions,
         world: UnsafeWorldCell,
     ) -> bool {
         let mut should_run = !self.skipped_systems.contains(system_index);
+
         for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() {
             if self.evaluated_sets.contains(set_idx) {
                 continue;
@@ -566,6 +568,19 @@ impl ExecutorState {
 
         should_run &= system_conditions_met;
 
+        // SAFETY:
+        // - 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) };
+
+        if !valid_params {
+            warn_system_skipped!("System", system.name());
+            self.skipped_systems.insert(system_index);
+        }
+
+        should_run &= valid_params;
+
         should_run
     }
 
@@ -731,8 +746,18 @@ unsafe fn evaluate_and_fold_conditions(
     conditions
         .iter_mut()
         .map(|condition| {
-            // SAFETY: The caller ensures that `world` has permission to
-            // access any data required by the condition.
+            // SAFETY:
+            // - 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) } {
+                warn_system_skipped!("Condition", condition.name());
+                return false;
+            }
+            // SAFETY:
+            // - The caller ensures that `world` has permission to read any data
+            //   required by the condition.
+            // - `update_archetype_component_access` has been called for condition.
             unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) }
         })
         .fold(true, |acc, res| acc && res)

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

@@ -7,6 +7,7 @@ use crate::{
     schedule::{
         executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
     },
+    warn_system_skipped,
     world::World,
 };
 
@@ -79,6 +80,15 @@ impl SystemExecutor for SimpleExecutor {
 
             should_run &= system_conditions_met;
 
+            let system = &mut schedule.systems[system_index];
+            let valid_params = system.validate_param(world);
+
+            if !valid_params {
+                warn_system_skipped!("System", system.name());
+            }
+
+            should_run &= valid_params;
+
             #[cfg(feature = "trace")]
             should_run_span.exit();
 
@@ -89,7 +99,6 @@ impl SystemExecutor for SimpleExecutor {
                 continue;
             }
 
-            let system = &mut schedule.systems[system_index];
             if is_apply_deferred(system) {
                 continue;
             }
@@ -128,7 +137,13 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
     #[allow(clippy::unnecessary_fold)]
     conditions
         .iter_mut()
-        .map(|condition| __rust_begin_short_backtrace::readonly_run(&mut **condition, world))
+        .map(|condition| {
+            if !condition.validate_param(world) {
+                warn_system_skipped!("Condition", condition.name());
+                return false;
+            }
+            __rust_begin_short_backtrace::readonly_run(&mut **condition, world)
+        })
         .fold(true, |acc, res| acc && res)
 }
 

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

@@ -5,6 +5,7 @@ use std::panic::AssertUnwindSafe;
 
 use crate::{
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
+    warn_system_skipped,
     world::World,
 };
 
@@ -85,6 +86,15 @@ impl SystemExecutor for SingleThreadedExecutor {
 
             should_run &= system_conditions_met;
 
+            let system = &mut schedule.systems[system_index];
+            let valid_params = system.validate_param(world);
+
+            if !valid_params {
+                warn_system_skipped!("System", system.name());
+            }
+
+            should_run &= valid_params;
+
             #[cfg(feature = "trace")]
             should_run_span.exit();
 
@@ -95,7 +105,6 @@ impl SystemExecutor for SingleThreadedExecutor {
                 continue;
             }
 
-            let system = &mut schedule.systems[system_index];
             if is_apply_deferred(system) {
                 self.apply_deferred(schedule, world);
                 continue;
@@ -160,6 +169,12 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
     #[allow(clippy::unnecessary_fold)]
     conditions
         .iter_mut()
-        .map(|condition| __rust_begin_short_backtrace::readonly_run(&mut **condition, world))
+        .map(|condition| {
+            if !condition.validate_param(world) {
+                warn_system_skipped!("Condition", condition.name());
+                return false;
+            }
+            __rust_begin_short_backtrace::readonly_run(&mut **condition, world)
+        })
         .fold(true, |acc, res| acc && res)
 }

crates/bevy_ecs/src/system/adapter_system.rs

@@ -132,6 +132,11 @@ where
         self.system.queue_deferred(world);
     }
 
+    #[inline]
+    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
+        self.system.validate_param_unsafe(world)
+    }
+
     fn initialize(&mut self, world: &mut crate::prelude::World) {
         self.system.initialize(world);
     }

crates/bevy_ecs/src/system/combinator.rs

@@ -193,6 +193,7 @@ where
         )
     }
 
+    #[inline]
     fn apply_deferred(&mut self, world: &mut World) {
         self.a.apply_deferred(world);
         self.b.apply_deferred(world);
@@ -204,6 +205,11 @@ where
         self.b.queue_deferred(world);
     }
 
+    #[inline]
+    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
+        self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world)
+    }
+
     fn initialize(&mut self, world: &mut World) {
         self.a.initialize(world);
         self.b.initialize(world);