From 6a09fcad742cea58060526e51bc5ae0adeb805d1 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Tue, 3 Dec 2024 23:23:55 +0100 Subject: [PATCH 1/8] go back to panic --- crates/bevy_ecs/src/system/function_system.rs | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 7c5806abbcd6a..8b5c1ad741dbb 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/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")] @@ -141,6 +141,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`]. @@ -151,6 +153,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; } @@ -160,15 +163,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::

() + ), + Self::Once => { + bevy_utils::tracing::warn!( + "{0} did not run because it requested inaccessible system parameter {1}", + name, + disqualified::ShortName::of::

() + ); + } + Self::Never => {} } - - bevy_utils::tracing::warn!( - "{0} did not run because it requested inaccessible system parameter {1}", - name, - disqualified::ShortName::of::

() - ); } } @@ -183,6 +192,11 @@ where /// Set warn policy. fn with_param_warn_policy(self, warn_policy: ParamWarnPolicy) -> FunctionSystem; + /// Warn only once about invalid system parameters. + fn param_warn_once(self) -> FunctionSystem { + self.with_param_warn_policy(ParamWarnPolicy::Once) + } + /// Disable all param warnings. fn never_param_warn(self) -> FunctionSystem { self.with_param_warn_policy(ParamWarnPolicy::Never) From c5d7248312f80fffcf43e32dee9ec3b8a113fa36 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Tue, 3 Dec 2024 23:44:45 +0100 Subject: [PATCH 2/8] adjust tests --- crates/bevy_ecs/src/schedule/executor/mod.rs | 7 +++++-- crates/bevy_ecs/src/system/system.rs | 2 +- crates/bevy_ecs/src/system/system_registry.rs | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index 37fd9ff7246c8..e5705c3137244 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/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, In, IntoSystem, Res, WithParamWarnPolicy}, world::World, }; @@ -219,7 +219,8 @@ mod tests { (|mut commands: Commands| { commands.insert_resource(R1); }) - .pipe(|_: In<()>, _: Res| {}), + .param_warn_once() + .pipe((|_: In<()>, _: Res| {}).param_warn_once()), // This system depends on a system that is always skipped. |mut commands: Commands| { commands.insert_resource(R2); @@ -252,11 +253,13 @@ mod tests { (|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); }) + .param_warn_once() .run_if(|_: Res| true), )); schedule.run(&mut world); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index e68287c699880..73f30ff0d8a41 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/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(_)))); } diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs index 573194713f8ba..c3286d0234515 100644 --- a/crates/bevy_ecs/src/system/system_registry.rs +++ b/crates/bevy_ecs/src/system/system_registry.rs @@ -965,7 +965,7 @@ mod tests { fn system(_: Res) {} let mut world = World::new(); - let id = world.register_system_cached(system); + let id = world.register_system_cached(system.param_warn_once()); // This fails because `T` has not been added to the world yet. let result = world.run_system(id); From 555d12bae0ad1b9f50c565e997b250b3ced99c26 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Tue, 3 Dec 2024 23:56:37 +0100 Subject: [PATCH 3/8] update example --- examples/ecs/fallible_params.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index 15a75944f01ae..129a3859d495f 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -20,22 +20,20 @@ fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, setup) - // 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. + // Default system policy is to panic if parameters fail to be fetched. + // We overwrite that configuration, to either warn us once or never. + // This is good for catching unexpected behavior without crashing the app, + // but can lead to spam. .add_systems( Update, ( - user_input, + user_input.param_warn_once(), 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) + .add_systems(Update, do_nothing_fail_validation.param_warn_once()) .run(); } From 405c85012c2b34a3fbc63c086d63a4de005cd3fe Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Tue, 3 Dec 2024 23:58:47 +0100 Subject: [PATCH 4/8] update docs --- crates/bevy_ecs/src/change_detection.rs | 6 +++--- crates/bevy_ecs/src/system/query.rs | 4 ++-- crates/bevy_ecs/src/system/system_param.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 025c3804e73e2..72cf8c6784edb 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -544,7 +544,7 @@ impl<'w> From> 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>`] 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>`] instead if the resource might not always exist. pub struct ResMut<'w, T: ?Sized + Resource> { @@ -683,7 +683,7 @@ impl<'w, T: Resource> From> 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>`] instead if the resource might not always exist. pub struct NonSendMut<'w, T: ?Sized + 'static> { diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 26440c0c643f4..7279b8a154394 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/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>`] 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) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 7519a1e30a089..e752170027d87 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1337,7 +1337,7 @@ unsafe impl 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>`] instead if the resource might not always exist. pub struct NonSend<'w, T: 'static> { From 82d3611fe60cf516428a2151efba6326d5a9e717 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 8 Dec 2024 18:20:14 +0100 Subject: [PATCH 5/8] remove combinator system param checking --- crates/bevy_ecs/src/system/combinator.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index e21f35eee4e82..a312ace6ef9bd 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/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 { From 1330797255f11dba050484162bd0de7ff91e7548 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 8 Dec 2024 18:27:48 +0100 Subject: [PATCH 6/8] fix test --- crates/bevy_ecs/src/system/system_registry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs index c3286d0234515..99801ccefeca9 100644 --- a/crates/bevy_ecs/src/system/system_registry.rs +++ b/crates/bevy_ecs/src/system/system_registry.rs @@ -965,7 +965,7 @@ mod tests { fn system(_: Res) {} let mut world = World::new(); - let id = world.register_system_cached(system.param_warn_once()); + 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); From f2cbfe816d050671949a9165e9f72af217c2dfae Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 8 Dec 2024 18:38:42 +0100 Subject: [PATCH 7/8] fix executor tests --- crates/bevy_ecs/src/schedule/executor/mod.rs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index e5705c3137244..a8fa3a4007925 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/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, WithParamWarnPolicy}, + system::{Commands, Res, WithParamWarnPolicy}, world::World, }; @@ -215,16 +215,11 @@ mod tests { schedule.set_executor_kind(executor); schedule.add_systems( ( - // Combined systems get skipped together. - (|mut commands: Commands| { - commands.insert_resource(R1); - }) - .param_warn_once() - .pipe((|_: In<()>, _: Res| {}).param_warn_once()), // This system depends on a system that is always skipped. - |mut commands: Commands| { + (|mut commands: Commands| { commands.insert_resource(R2); - }, + }) + .param_warn_once(), ) .chain(), ); @@ -247,7 +242,7 @@ mod tests { let mut world = World::new(); let mut schedule = Schedule::default(); schedule.set_executor_kind(executor); - schedule.configure_sets(S1.run_if(|_: Res| true)); + schedule.configure_sets(S1.run_if((|_: Res| true).param_warn_once())); schedule.add_systems(( // System gets skipped if system set run conditions fail validation. (|mut commands: Commands| { @@ -260,7 +255,7 @@ mod tests { commands.insert_resource(R2); }) .param_warn_once() - .run_if(|_: Res| true), + .run_if((|_: Res| true).param_warn_once()), )); schedule.run(&mut world); assert!(world.get_resource::().is_none()); From ce753d13874005c85815cae20360a9f4b2d5fc90 Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Sun, 8 Dec 2024 18:48:50 +0100 Subject: [PATCH 8/8] fix few doc tests --- crates/bevy_ecs/src/schedule/condition.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 6956d2715069f..e6ddaa9590bc9 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -80,7 +80,7 @@ pub trait Condition: sealed::Condition /// /// # Examples /// - /// ``` + /// ```should_panic /// use bevy_ecs::prelude::*; /// /// #[derive(Resource, PartialEq)] @@ -90,7 +90,7 @@ pub trait Condition: sealed::Condition /// # 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` in a system. /// my_system.run_if(resource_equals(R(0))), /// ); @@ -131,7 +131,7 @@ pub trait Condition: sealed::Condition /// /// # Examples /// - /// ``` + /// ```should_panic /// use bevy_ecs::prelude::*; /// /// #[derive(Resource, PartialEq)] @@ -141,7 +141,7 @@ pub trait Condition: sealed::Condition /// # 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` in a system. /// my_system.run_if(resource_equals(R(0))), /// );