From acea4e7e6fe163c0e519c26764785ab7bd37b189 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Thu, 3 Oct 2024 15:16:55 +0200 Subject: [PATCH] Better warnings about invalid parameters (#15500) # Objective System param validation warnings should be configurable and default to "warn once" (per system). Fixes: #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, With)> ``` 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 --- crates/bevy_ecs/src/lib.rs | 2 +- crates/bevy_ecs/src/schedule/executor/mod.rs | 12 --- .../src/schedule/executor/multi_threaded.rs | 5 +- .../bevy_ecs/src/schedule/executor/simple.rs | 5 - .../src/schedule/executor/single_threaded.rs | 5 - crates/bevy_ecs/src/system/adapter_system.rs | 5 +- crates/bevy_ecs/src/system/combinator.rs | 10 +- .../src/system/exclusive_function_system.rs | 3 +- crates/bevy_ecs/src/system/function_system.rs | 101 +++++++++++++++++- crates/bevy_ecs/src/system/system.rs | 2 +- crates/bevy_ecs/src/system/system_param.rs | 52 ++++++--- crates/bevy_render/src/extract_param.rs | 3 +- examples/ecs/fallible_params.rs | 29 +++-- 13 files changed, 176 insertions(+), 58 deletions(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 4dda0d4ea9e38..c481da091a77e 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/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, diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 24113aee2bee0..37fd9ff7246c8 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/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::{ diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 8782793e32f71..53453240dcb88 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/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: diff --git a/crates/bevy_ecs/src/schedule/executor/simple.rs b/crates/bevy_ecs/src/schedule/executor/simple.rs index 171125342cd78..508f1fbffd07a 100644 --- a/crates/bevy_ecs/src/schedule/executor/simple.rs +++ b/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) diff --git a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs index 93d814d3b2f83..9cc6199ce6a4a 100644 --- a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs +++ b/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) diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 8c25e57bbd91e..ef5e51c08f8e2 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/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) { diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index ce4c8785feb05..e21f35eee4e82 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/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 { diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 3075f3c55772e..8b1a06fb3a60c 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/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 } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 0614d7339ca4b..fcd6a829e8afd 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/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>) { 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

(&self) + where + P: SystemParam, + { + self.param_warn_policy.try_warn::

(&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

(&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::

() + ); + } +} + +/// Trait for manipulating warn policy of systems. +#[doc(hidden)] +pub trait WithParamWarnPolicy +where + M: 'static, + F: SystemParamFunction, + Self: Sized, +{ + /// Set warn policy. + fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem; + + /// Disable all param warnings. + fn never_param_warn(self) -> FunctionSystem { + self.with_param_warn_policy(ParamWarnPolicy::Never) + } +} + +impl WithParamWarnPolicy for F +where + M: 'static, + F: SystemParamFunction, +{ + fn with_param_warn_policy(self, param_warn_policy: ParamWarnPolicy) -> FunctionSystem { + 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] diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 60d3c58f62d47..ded89235e72ef 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/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. diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 013d75265a900..bfa2fe108d70d 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -421,7 +421,11 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo world.change_tick(), ) }; - result.is_ok() + let is_valid = result.is_ok(); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid } } @@ -483,7 +487,11 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam world.change_tick(), ) }; - !matches!(result, Err(QuerySingleError::MultipleEntities(_))) + let is_valid = !matches!(result, Err(QuerySingleError::MultipleEntities(_))); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid } } @@ -773,14 +781,18 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid } #[inline] @@ -883,14 +895,18 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid } #[inline] @@ -1429,14 +1445,18 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .non_send_resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid } #[inline] @@ -1536,14 +1556,18 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { #[inline] unsafe fn validate_param( &component_id: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to resource metadata. - unsafe { world.storages() } + let is_valid = unsafe { world.storages() } .non_send_resources .get(component_id) - .is_some_and(ResourceData::is_present) + .is_some_and(ResourceData::is_present); + if !is_valid { + system_meta.try_warn_param::(); + } + is_valid } #[inline] diff --git a/crates/bevy_render/src/extract_param.rs b/crates/bevy_render/src/extract_param.rs index 914fe32dfb6e4..89b6edef1903d 100644 --- a/crates/bevy_render/src/extract_param.rs +++ b/crates/bevy_render/src/extract_param.rs @@ -77,12 +77,13 @@ where #[inline] unsafe fn validate_param( state: &Self::State, - _system_meta: &SystemMeta, + system_meta: &SystemMeta, world: UnsafeWorldCell, ) -> bool { // SAFETY: Read-only access to world data registered in `init_state`. let result = unsafe { world.get_resource_by_id(state.main_world_state) }; let Some(main_world) = result else { + system_meta.try_warn_param::<&World>(); return false; }; // SAFETY: Type is guaranteed by `SystemState`. diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index 397e883f9b2b9..a3ca04b9a0094 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -20,9 +20,22 @@ fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, setup) - // We add all the systems one after another. - // We don't need to use run conditions here. - .add_systems(Update, (user_input, move_targets, move_pointer).chain()) + // Systems that fail parameter validation will emit warnings. + // The default policy is to emit a warning once per system. + // This is good for catching unexpected behavior, but can + // lead to spam. You can disable invalid param warnings + // per system using the `.never_param_warn()` method. + .add_systems( + Update, + ( + user_input, + move_targets.never_param_warn(), + move_pointer.never_param_warn(), + ) + .chain(), + ) + // We will leave this systems with default warning policy. + .add_systems(Update, do_nothing_fail_validation) .run(); } @@ -67,9 +80,9 @@ fn setup(mut commands: Commands, asset_server: Res) { )); } -// System that reads user input. -// If user presses 'A' we spawn a new random enemy. -// If user presses 'R' we remove a random enemy (if any exist). +/// System that reads user input. +/// If user presses 'A' we spawn a new random enemy. +/// If user presses 'R' we remove a random enemy (if any exist). fn user_input( mut commands: Commands, enemies: Query>, @@ -146,3 +159,7 @@ fn move_pointer( player_transform.rotate_axis(Dir3::Z, player.rotation_speed * time.delta_seconds()); } } + +/// This system always fails param validation, because we never +/// create an entity with both [`Player`] and [`Enemy`] components. +fn do_nothing_fail_validation(_: Single<(), (With, With)>) {}