Better warnings about invalid parameters (bevyengine#15500)

# Objective

System param validation warnings should be configurable and default to
"warn once" (per system).

Fixes: bevyengine#15391

## Solution

`SystemMeta` is given a new `ParamWarnPolicy` field.
The policy decides whether warnings will be emitted by each system param
when it fails validation.
The policy is updated by the system after param validation fails.

Example warning:
```
2024-09-30T18:10:04.740749Z  WARN bevy_ecs::system::function_system: System fallible_params::do_nothing_fail_validation will not run because it requested inaccessible system parameter Single<(), (With<Player>, With<Enemy>)>
```

Currently, only the first invalid parameter is displayed.

Warnings can be disabled on function systems using
`.param_never_warn()`.
(there is also `.with_param_warn_policy(policy)`)

## Testing

Ran `fallible_params` example.

---------

Co-authored-by: SpecificProtagonist <[email protected]>

Author: 2 people

crates/bevy_ecs/src/lib.rs

@@ -60,7 +60,7 @@ pub mod prelude {
             Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef, IntoSystem, Local,
             NonSend, NonSendMut, ParallelCommands, ParamSet, Populated, Query, ReadOnlySystem, Res,
             ResMut, Resource, Single, System, SystemIn, SystemInput, SystemParamBuilder,
-            SystemParamFunction,
+            SystemParamFunction, WithParamWarnPolicy,
         },
         world::{
             Command, EntityMut, EntityRef, EntityWorldMut, FromWorld, OnAdd, OnInsert, OnRemove,

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

@@ -180,18 +180,6 @@ mod __rust_begin_short_backtrace {
     }
 }
 
-#[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::{

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

@@ -19,7 +19,6 @@ use crate::{
     query::Access,
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
     system::BoxedSystem,
-    warn_system_skipped,
     world::{unsafe_world_cell::UnsafeWorldCell, World},
 };
 
@@ -524,7 +523,7 @@ impl ExecutorState {
     unsafe fn should_run(
         &mut self,
         system_index: usize,
-        system: &BoxedSystem,
+        system: &mut BoxedSystem,
         conditions: &mut Conditions,
         world: UnsafeWorldCell,
     ) -> bool {
@@ -575,7 +574,6 @@ impl ExecutorState {
             // - `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;
@@ -751,7 +749,6 @@ unsafe fn evaluate_and_fold_conditions(
             //   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:

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

@@ -7,7 +7,6 @@ use crate::{
     schedule::{
         executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
     },
-    warn_system_skipped,
     world::World,
 };
 
@@ -83,9 +82,6 @@ impl SystemExecutor for SimpleExecutor {
             let system = &mut schedule.systems[system_index];
             if should_run {
                 let valid_params = system.validate_param(world);
-                if !valid_params {
-                    warn_system_skipped!("System", system.name());
-                }
                 should_run &= valid_params;
             }
 
@@ -139,7 +135,6 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
         .iter_mut()
         .map(|condition| {
             if !condition.validate_param(world) {
-                warn_system_skipped!("Condition", condition.name());
                 return false;
             }
             __rust_begin_short_backtrace::readonly_run(&mut **condition, world)

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

@@ -5,7 +5,6 @@ use fixedbitset::FixedBitSet;
 
 use crate::{
     schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
-    warn_system_skipped,
     world::World,
 };
 
@@ -89,9 +88,6 @@ impl SystemExecutor for SingleThreadedExecutor {
             let system = &mut schedule.systems[system_index];
             if should_run {
                 let valid_params = system.validate_param(world);
-                if !valid_params {
-                    warn_system_skipped!("System", system.name());
-                }
                 should_run &= valid_params;
             }
 
@@ -171,7 +167,6 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
         .iter_mut()
         .map(|condition| {
             if !condition.validate_param(world) {
-                warn_system_skipped!("Condition", condition.name());
                 return false;
             }
             __rust_begin_short_backtrace::readonly_run(&mut **condition, world)

crates/bevy_ecs/src/system/adapter_system.rs

@@ -179,8 +179,9 @@ where
     }
 
     #[inline]
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
-        self.system.validate_param_unsafe(world)
+    unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
+        // SAFETY: Delegate to other `System` implementations.
+        unsafe { self.system.validate_param_unsafe(world) }
     }
 
     fn initialize(&mut self, world: &mut crate::prelude::World) {

crates/bevy_ecs/src/system/combinator.rs

@@ -212,8 +212,9 @@ where
     }
 
     #[inline]
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
-        self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world)
+    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) }
     }
 
     fn initialize(&mut self, world: &mut World) {
@@ -430,8 +431,9 @@ where
         self.b.queue_deferred(world);
     }
 
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
-        self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world)
+    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) }
     }
 
     fn validate_param(&mut self, world: &World) -> bool {

crates/bevy_ecs/src/system/exclusive_function_system.rs

@@ -150,7 +150,8 @@ where
     }
 
     #[inline]
-    unsafe fn validate_param_unsafe(&self, _world: UnsafeWorldCell) -> bool {
+    unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool {
+        // All exclusive system params are always available.
         true
     }
 

crates/bevy_ecs/src/system/function_system.rs

@@ -43,6 +43,7 @@ pub struct SystemMeta {
     is_send: bool,
     has_deferred: bool,
     pub(crate) last_run: Tick,
+    param_warn_policy: ParamWarnPolicy,
     #[cfg(feature = "trace")]
     pub(crate) system_span: Span,
     #[cfg(feature = "trace")]
@@ -59,6 +60,7 @@ impl SystemMeta {
             is_send: true,
             has_deferred: false,
             last_run: Tick::new(0),
+            param_warn_policy: ParamWarnPolicy::Once,
             #[cfg(feature = "trace")]
             system_span: info_span!("system", name = name),
             #[cfg(feature = "trace")]
@@ -75,6 +77,7 @@ impl SystemMeta {
     /// Sets the name of of this system.
     ///
     /// Useful to give closure systems more readable and unique names for debugging and tracing.
+    #[inline]
     pub fn set_name(&mut self, new_name: impl Into<Cow<'static, str>>) {
         let new_name: Cow<'static, str> = new_name.into();
         #[cfg(feature = "trace")]
@@ -108,9 +111,94 @@ impl SystemMeta {
 
     /// Marks the system as having deferred buffers like [`Commands`](`super::Commands`)
     /// This lets the scheduler insert [`apply_deferred`](`crate::prelude::apply_deferred`) systems automatically.
+    #[inline]
     pub fn set_has_deferred(&mut self) {
         self.has_deferred = true;
     }
+
+    /// Changes the warn policy.
+    #[inline]
+    pub(crate) fn set_param_warn_policy(&mut self, warn_policy: ParamWarnPolicy) {
+        self.param_warn_policy = warn_policy;
+    }
+
+    /// Advances the warn policy after validation failed.
+    #[inline]
+    pub(crate) fn advance_param_warn_policy(&mut self) {
+        self.param_warn_policy.advance();
+    }
+
+    /// Emits a warning about inaccessible system param if policy allows it.
+    #[inline]
+    pub fn try_warn_param<P>(&self)
+    where
+        P: SystemParam,
+    {
+        self.param_warn_policy.try_warn::<P>(&self.name);
+    }
+}
+
+/// State machine for emitting warnings when [system params are invalid](System::validate_param).
+#[derive(Clone, Copy)]
+pub enum ParamWarnPolicy {
+    /// No warning should ever be emitted.
+    Never,
+    /// The warning will be emitted once and status will update to [`Self::Never`].
+    Once,
+}
+
+impl ParamWarnPolicy {
+    /// Advances the warn policy after validation failed.
+    #[inline]
+    fn advance(&mut self) {
+        *self = Self::Never;
+    }
+
+    /// Emits a warning about inaccessible system param if policy allows it.
+    #[inline]
+    fn try_warn<P>(&self, name: &str)
+    where
+        P: SystemParam,
+    {
+        if matches!(self, Self::Never) {
+            return;
+        }
+
+        bevy_utils::tracing::warn!(
+            "{0} did not run because it requested inaccessible system parameter {1}",
+            name,
+            disqualified::ShortName::of::<P>()
+        );
+    }
+}
+
+/// Trait for manipulating warn policy of systems.
+#[doc(hidden)]
+pub trait WithParamWarnPolicy<M, F>
+where
+    M: 'static,
+    F: SystemParamFunction<M>,
+    Self: Sized,
+{
+    /// Set warn policy.
+    fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem<M, F>;
+
+    /// Disable all param warnings.
+    fn never_param_warn(self) -> FunctionSystem<M, F> {
+        self.with_param_warn_policy(ParamWarnPolicy::Never)
+    }
+}
+
+impl<M, F> WithParamWarnPolicy<M, F> for F
+where
+    M: 'static,
+    F: SystemParamFunction<M>,
+{
+    fn with_param_warn_policy(self, param_warn_policy: ParamWarnPolicy) -> FunctionSystem<M, F> {
+        let mut system = IntoSystem::into_system(self);
+        system.system_meta.set_param_warn_policy(param_warn_policy);
+        system
+    }
 }
 
 // TODO: Actually use this in FunctionSystem. We should probably only do this once Systems are constructed using a World reference
@@ -657,9 +745,18 @@ where
     }
 
     #[inline]
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
+    unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
         let param_state = self.param_state.as_ref().expect(Self::PARAM_MESSAGE);
-        F::Param::validate_param(param_state, &self.system_meta, world)
+        // SAFETY:
+        // - The caller has invoked `update_archetype_component_access`, which will panic
+        //   if the world does not match.
+        // - All world accesses used by `F::Param` have been registered, so the caller
+        //   will ensure that there are no data access conflicts.
+        let is_valid = unsafe { F::Param::validate_param(param_state, &self.system_meta, world) };
+        if !is_valid {
+            self.system_meta.advance_param_warn_policy();
+        }
+        is_valid
     }
 
     #[inline]

crates/bevy_ecs/src/system/system.rs

@@ -117,7 +117,7 @@ pub trait System: Send + Sync + 'static {
     /// - The method [`System::update_archetype_component_access`] must be called at some
     ///   point before this one, with the same exact [`World`]. If [`System::update_archetype_component_access`]
     ///   panics (or otherwise does not return for any reason), this method must not be called.
-    unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool;
+    unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool;
 
     /// Safe version of [`System::validate_param_unsafe`].
     /// that runs on exclusive, single-threaded `world` pointer.