Set panic as default fallible system param behavior (#16638)

Fixes: #16578

This is a patch fix, proper fix requires a breaking change.

Added `Panic` enum variant and using is as the system meta default.
Warn once behavior can be enabled same way disabling panic (originally
disabling wans) is.

To fix an issue with the current architecture, where **all** combinator
system params get checked together,
combinator systems only check params of the first system.
This will result in old, panicking behavior on subsequent systems and
will be fixed in 0.16.

Ran unit tests and `fallible_params` example.

---------

Co-authored-by: François Mockers <[email protected]>
Co-authored-by: François Mockers <[email protected]>

Author: MiniaczQ

crates/bevy_ecs/src/change_detection.rs

@@ -544,7 +544,7 @@ impl<'w> From<TicksMut<'w>> for Ticks<'w> {
 /// If you need a unique mutable borrow, use [`ResMut`] instead.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<Res<T>>`] instead if the resource might not always exist.
 pub struct Res<'w, T: ?Sized + Resource> {
@@ -622,7 +622,7 @@ impl_debug!(Res<'w, T>, Resource);
 /// If you need a shared borrow, use [`Res`] instead.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<ResMut<T>>`] instead if the resource might not always exist.
 pub struct ResMut<'w, T: ?Sized + Resource> {
@@ -683,7 +683,7 @@ impl<'w, T: Resource> From<ResMut<'w, T>> for Mut<'w, T> {
 /// over to another thread.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if non-send resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<NonSendMut<T>>`] instead if the resource might not always exist.
 pub struct NonSendMut<'w, T: ?Sized + 'static> {

crates/bevy_ecs/src/schedule/condition.rs

@@ -80,7 +80,7 @@ pub trait Condition<Marker, In: SystemInput = ()>: sealed::Condition<Marker, In>
     ///
     /// # Examples
     ///
-    /// ```
+    /// ```should_panic
     /// use bevy_ecs::prelude::*;
     ///
     /// #[derive(Resource, PartialEq)]
@@ -90,7 +90,7 @@ pub trait Condition<Marker, In: SystemInput = ()>: sealed::Condition<Marker, In>
     /// # let mut world = World::new();
     /// # fn my_system() {}
     /// app.add_systems(
-    ///     // The `resource_equals` run condition will fail since we don't initialize `R`,
+    ///     // The `resource_equals` run condition will panic 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

@@ -186,7 +186,7 @@ mod tests {
         self as bevy_ecs,
         prelude::{IntoSystemConfigs, IntoSystemSetConfigs, Resource, Schedule, SystemSet},
         schedule::ExecutorKind,
-        system::{Commands, In, IntoSystem, Res},
+        system::{Commands, Res, WithParamWarnPolicy},
         world::World,
     };
 
@@ -215,15 +215,11 @@ mod tests {
         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| {
+                (|mut commands: Commands| {
                     commands.insert_resource(R2);
-                },
+                })
+                .param_warn_once(),
             )
                 .chain(),
         );
@@ -246,18 +242,20 @@ mod tests {
         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.configure_sets(S1.run_if((|_: Res<R1>| true).param_warn_once()));
         schedule.add_systems((
             // System gets skipped if system set run conditions fail validation.
             (|mut commands: Commands| {
                 commands.insert_resource(R1);
             })
+            .param_warn_once()
             .in_set(S1),
             // System gets skipped if run conditions fail validation.
             (|mut commands: Commands| {
                 commands.insert_resource(R2);
             })
-            .run_if(|_: Res<R2>| true),
+            .param_warn_once()
+            .run_if((|_: Res<R2>| true).param_warn_once()),
         ));
         schedule.run(&mut world);
         assert!(world.get_resource::<R1>().is_none());

crates/bevy_ecs/src/system/combinator.rs

@@ -214,7 +214,7 @@ where
     #[inline]
     unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
         // SAFETY: Delegate to other `System` implementations.
-        unsafe { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) }
+        unsafe { self.a.validate_param_unsafe(world) }
     }
 
     fn initialize(&mut self, world: &mut World) {
@@ -433,7 +433,7 @@ where
 
     unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
         // SAFETY: Delegate to other `System` implementations.
-        unsafe { self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world) }
+        unsafe { self.a.validate_param_unsafe(world) }
     }
 
     fn validate_param(&mut self, world: &World) -> bool {

crates/bevy_ecs/src/system/function_system.rs

@@ -60,7 +60,7 @@ impl SystemMeta {
             is_send: true,
             has_deferred: false,
             last_run: Tick::new(0),
-            param_warn_policy: ParamWarnPolicy::Once,
+            param_warn_policy: ParamWarnPolicy::Panic,
             #[cfg(feature = "trace")]
             system_span: info_span!("system", name = name),
             #[cfg(feature = "trace")]
@@ -190,6 +190,8 @@ impl SystemMeta {
 /// State machine for emitting warnings when [system params are invalid](System::validate_param).
 #[derive(Clone, Copy)]
 pub enum ParamWarnPolicy {
+    /// Stop app with a panic.
+    Panic,
     /// No warning should ever be emitted.
     Never,
     /// The warning will be emitted once and status will update to [`Self::Never`].
@@ -200,6 +202,7 @@ impl ParamWarnPolicy {
     /// Advances the warn policy after validation failed.
     #[inline]
     fn advance(&mut self) {
+        // Ignore `Panic` case, because it stops execution before this function gets called.
         *self = Self::Never;
     }
 
@@ -209,15 +212,21 @@ impl ParamWarnPolicy {
     where
         P: SystemParam,
     {
-        if matches!(self, Self::Never) {
-            return;
+        match self {
+            Self::Panic => panic!(
+                "{0} could not access system parameter {1}",
+                name,
+                disqualified::ShortName::of::<P>()
+            ),
+            Self::Once => {
+                bevy_utils::tracing::warn!(
+                    "{0} did not run because it requested inaccessible system parameter {1}",
+                    name,
+                    disqualified::ShortName::of::<P>()
+                );
+            }
+            Self::Never => {}
         }
-
-        bevy_utils::tracing::warn!(
-            "{0} did not run because it requested inaccessible system parameter {1}",
-            name,
-            disqualified::ShortName::of::<P>()
-        );
     }
 }
 
@@ -232,6 +241,11 @@ where
     /// Set warn policy.
     fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem<M, F>;
 
+    /// Warn only once about invalid system parameters.
+    fn param_warn_once(self) -> FunctionSystem<M, F> {
+        self.with_param_warn_policy(ParamWarnPolicy::Once)
+    }
+
     /// Disable all param warnings.
     fn never_param_warn(self) -> FunctionSystem<M, F> {
         self.with_param_warn_policy(ParamWarnPolicy::Never)

crates/bevy_ecs/src/system/query.rs

@@ -1648,7 +1648,7 @@ impl<'w, 'q, Q: QueryData, F: QueryFilter> From<&'q mut Query<'w, '_, Q, F>>
 /// [System parameter] that provides access to single entity's components, much like [`Query::single`]/[`Query::single_mut`].
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if zero or more than one matching entity exists.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<Single<D, F>>`] instead if zero or one matching entities can exist.
 ///
@@ -1684,7 +1684,7 @@ impl<'w, D: QueryData, F: QueryFilter> Single<'w, D, F> {
 /// [System parameter] that works very much like [`Query`] except it always contains at least one matching entity.
 ///
 /// This [`SystemParam`](crate::system::SystemParam) fails validation if no matching entities exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Much like [`Query::is_empty`] the worst case runtime will be `O(n)` where `n` is the number of *potential* matches.
 /// This can be notably expensive for queries that rely on non-archetypal filters such as [`Added`](crate::query::Added) or [`Changed`](crate::query::Changed)

crates/bevy_ecs/src/system/system.rs

@@ -451,7 +451,7 @@ mod tests {
 
         let mut world = World::default();
         // This fails because `T` has not been added to the world yet.
-        let result = world.run_system_once(system);
+        let result = world.run_system_once(system.param_warn_once());
 
         assert!(matches!(result, Err(RunSystemError::InvalidParams(_))));
     }

crates/bevy_ecs/src/system/system_param.rs

@@ -1335,7 +1335,7 @@ unsafe impl<T: SystemBuffer> SystemParam for Deferred<'_, T> {
 /// over to another thread.
 ///
 /// This [`SystemParam`] fails validation if non-send resource doesn't exist.
-/// This will cause systems that use this parameter to be skipped.
+/// /// This will cause a panic, but can be configured to do nothing or warn once.
 ///
 /// Use [`Option<NonSend<T>>`] instead if the resource might not always exist.
 pub struct NonSend<'w, T: 'static> {

crates/bevy_ecs/src/system/system_registry.rs

@@ -965,7 +965,7 @@ mod tests {
         fn system(_: Res<T>) {}
 
         let mut world = World::new();
-        let id = world.register_system_cached(system);
+        let id = world.register_system(system.param_warn_once());
         // This fails because `T` has not been added to the world yet.
         let result = world.run_system(id);
 

crates/bevy_pbr/src/render/mesh.rs

@@ -131,6 +131,10 @@ impl Plugin for MeshRenderPlugin {
         load_internal_asset!(app, SKINNING_HANDLE, "skinning.wgsl", Shader::from_wgsl);
         load_internal_asset!(app, MORPH_HANDLE, "morph.wgsl", Shader::from_wgsl);
 
+        if app.get_sub_app(RenderApp).is_none() {
+            return;
+        }
+
         app.add_systems(
             PostUpdate,
             (no_automatic_skin_batching, no_automatic_morph_batching),