Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set panic as default fallible system param behavior #16638

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/change_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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> {
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_ecs/src/schedule/condition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ pub trait Condition<Marker, In: SystemInput = ()>: sealed::Condition<Marker, In>
///
/// # Examples
///
/// ```
/// ```should_panic
/// use bevy_ecs::prelude::*;
///
/// #[derive(Resource, PartialEq)]
Expand All @@ -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))),
/// );
Expand Down Expand Up @@ -131,7 +131,7 @@ pub trait Condition<Marker, In: SystemInput = ()>: sealed::Condition<Marker, In>
///
/// # Examples
///
/// ```
/// ```should_panic
/// use bevy_ecs::prelude::*;
///
/// #[derive(Resource, PartialEq)]
Expand All @@ -141,7 +141,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))),
/// );
Expand Down
18 changes: 8 additions & 10 deletions crates/bevy_ecs/src/schedule/executor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,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,
};

Expand Down Expand Up @@ -321,15 +321,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>| {}),
Comment on lines -324 to -328
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As agreed, this is no longer supported in this scope

// This system depends on a system that is always skipped.
|mut commands: Commands| {
(|mut commands: Commands| {
commands.insert_resource(R2);
},
})
.param_warn_once(),
)
.chain(),
);
Expand All @@ -352,18 +348,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());
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/system/combinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this ok? Surely we should check that both params are valid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I should have read above in the PR discussion, can you just add a to-do or at least a comment explanation why we've decided to do this?

}

fn initialize(&mut self, world: &mut World) {
Expand Down Expand Up @@ -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) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

}

fn validate_param(&mut self, world: &World) -> bool {
Expand Down
32 changes: 23 additions & 9 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -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`].
Expand All @@ -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;
}

Expand All @@ -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>()
);
}
}

Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,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(_))));
}
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1337,7 +1337,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> {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/system_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
14 changes: 6 additions & 8 deletions examples/ecs/fallible_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Loading