Reduce runtime panics through SystemParam validation (#15276)

# Objective

The goal of this PR is to introduce `SystemParam` validation in order to
reduce runtime panics.

Fixes #15265

## Solution

`SystemParam` now has a new method `validate_param(...) -> bool`, which
takes immutable variants of `get_param` arguments. The returned value
indicates whether the parameter can be acquired from the world. If
parameters cannot be acquired for a system, it won't be executed,
similarly to run conditions. This reduces panics when using params like
`Res`, `ResMut`, etc. as well as allows for new, ergonomic params like
#15264 or #15302.

Param validation happens at the level of executors. All validation
happens directly before executing a system, in case of normal systems
they are skipped, in case of conditions they return false.

Warning about system skipping is primitive and subject to change in
subsequent PRs.

## Testing

Two executor tests check that all executors:
- skip systems which have invalid parameters:
  - piped systems get skipped together,
  - dependent systems still run correctly,
- skip systems with invalid run conditions:
  - system conditions have invalid parameters,
  - system set conditions have invalid parameters.
This commit is contained in:
MiniaczQ 2024-09-23 18:54:21 +02:00 committed by GitHub
parent 4d0961cc8a
commit e312da8c52
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
17 changed files with 474 additions and 36 deletions

View file

@ -271,6 +271,15 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
<(#(#param,)*) as SystemParam>::apply(state, system_meta, world); <(#(#param,)*) as SystemParam>::apply(state, system_meta, world);
} }
#[inline]
unsafe fn validate_param<'w, 's>(
state: &'s Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell<'w>,
) -> bool {
<(#(#param,)*) as SystemParam>::validate_param(state, system_meta, world)
}
#[inline] #[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
state: &'s mut Self::State, state: &'s mut Self::State,
@ -512,6 +521,16 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
<#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::queue(&mut state.state, system_meta, world); <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::queue(&mut state.state, system_meta, world);
} }
#[inline]
unsafe fn validate_param<'w, 's>(
state: &'s Self::State,
system_meta: &#path::system::SystemMeta,
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
) -> bool {
<(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world)
}
#[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
state: &'s mut Self::State, state: &'s mut Self::State,
system_meta: &#path::system::SystemMeta, system_meta: &#path::system::SystemMeta,

View file

@ -79,7 +79,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
/// ///
/// # Examples /// # Examples
/// ///
/// ```should_panic /// ```
/// use bevy_ecs::prelude::*; /// use bevy_ecs::prelude::*;
/// ///
/// #[derive(Resource, PartialEq)] /// #[derive(Resource, PartialEq)]
@ -89,7 +89,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
/// # let mut world = World::new(); /// # let mut world = World::new();
/// # fn my_system() {} /// # fn my_system() {}
/// app.add_systems( /// app.add_systems(
/// // The `resource_equals` run condition will panic since we don't initialize `R`, /// // The `resource_equals` run condition will fail since we don't initialize `R`,
/// // just like if we used `Res<R>` in a system. /// // just like if we used `Res<R>` in a system.
/// my_system.run_if(resource_equals(R(0))), /// my_system.run_if(resource_equals(R(0))),
/// ); /// );
@ -130,7 +130,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
/// ///
/// # Examples /// # Examples
/// ///
/// ```should_panic /// ```
/// use bevy_ecs::prelude::*; /// use bevy_ecs::prelude::*;
/// ///
/// #[derive(Resource, PartialEq)] /// #[derive(Resource, PartialEq)]
@ -140,7 +140,7 @@ pub trait Condition<Marker, In = ()>: sealed::Condition<Marker, In> {
/// # let mut world = World::new(); /// # let mut world = World::new();
/// # fn my_system() {} /// # fn my_system() {}
/// app.add_systems( /// app.add_systems(
/// // The `resource_equals` run condition will panic since we don't initialize `R`, /// // The `resource_equals` run condition will fail since we don't initialize `R`,
/// // just like if we used `Res<R>` in a system. /// // just like if we used `Res<R>` in a system.
/// my_system.run_if(resource_equals(R(0))), /// my_system.run_if(resource_equals(R(0))),
/// ); /// );

View file

@ -176,3 +176,100 @@ mod __rust_begin_short_backtrace {
black_box(system.run((), world)) black_box(system.run((), world))
} }
} }
#[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::{
self as bevy_ecs,
prelude::{IntoSystemConfigs, IntoSystemSetConfigs, Resource, Schedule, SystemSet},
schedule::ExecutorKind,
system::{Commands, In, IntoSystem, Res},
world::World,
};
#[derive(Resource)]
struct R1;
#[derive(Resource)]
struct R2;
const EXECUTORS: [ExecutorKind; 3] = [
ExecutorKind::Simple,
ExecutorKind::SingleThreaded,
ExecutorKind::MultiThreaded,
];
#[test]
fn invalid_system_param_skips() {
for executor in EXECUTORS {
invalid_system_param_skips_core(executor);
}
}
fn invalid_system_param_skips_core(executor: ExecutorKind) {
let mut world = World::new();
let mut schedule = Schedule::default();
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| {
commands.insert_resource(R2);
},
)
.chain(),
);
schedule.run(&mut world);
assert!(world.get_resource::<R1>().is_none());
assert!(world.get_resource::<R2>().is_some());
}
#[derive(SystemSet, Hash, Debug, PartialEq, Eq, Clone)]
struct S1;
#[test]
fn invalid_condition_param_skips_system() {
for executor in EXECUTORS {
invalid_condition_param_skips_system_core(executor);
}
}
fn invalid_condition_param_skips_system_core(executor: ExecutorKind) {
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.add_systems((
// System gets skipped if system set run conditions fail validation.
(|mut commands: Commands| {
commands.insert_resource(R1);
})
.in_set(S1),
// System gets skipped if run conditions fail validation.
(|mut commands: Commands| {
commands.insert_resource(R2);
})
.run_if(|_: Res<R2>| true),
));
schedule.run(&mut world);
assert!(world.get_resource::<R1>().is_none());
assert!(world.get_resource::<R2>().is_none());
}
}

View file

@ -19,6 +19,7 @@ use crate::{
query::Access, query::Access,
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule}, schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
system::BoxedSystem, system::BoxedSystem,
warn_system_skipped,
world::{unsafe_world_cell::UnsafeWorldCell, World}, world::{unsafe_world_cell::UnsafeWorldCell, World},
}; };
@ -519,15 +520,16 @@ impl ExecutorState {
/// the system's conditions: this includes conditions for the system /// the system's conditions: this includes conditions for the system
/// itself, and conditions for any of the system's sets. /// itself, and conditions for any of the system's sets.
/// * `update_archetype_component` must have been called with `world` /// * `update_archetype_component` must have been called with `world`
/// for each run condition in `conditions`. /// for the system as well as system and system set's run conditions.
unsafe fn should_run( unsafe fn should_run(
&mut self, &mut self,
system_index: usize, system_index: usize,
_system: &BoxedSystem, system: &BoxedSystem,
conditions: &mut Conditions, conditions: &mut Conditions,
world: UnsafeWorldCell, world: UnsafeWorldCell,
) -> bool { ) -> bool {
let mut should_run = !self.skipped_systems.contains(system_index); let mut should_run = !self.skipped_systems.contains(system_index);
for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() { for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() {
if self.evaluated_sets.contains(set_idx) { if self.evaluated_sets.contains(set_idx) {
continue; continue;
@ -566,6 +568,19 @@ impl ExecutorState {
should_run &= system_conditions_met; should_run &= system_conditions_met;
// SAFETY:
// - The caller ensures that `world` has permission to read any data
// required by the system.
// - `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;
should_run should_run
} }
@ -731,8 +746,18 @@ unsafe fn evaluate_and_fold_conditions(
conditions conditions
.iter_mut() .iter_mut()
.map(|condition| { .map(|condition| {
// SAFETY: The caller ensures that `world` has permission to // SAFETY:
// access any data required by the condition. // - The caller ensures that `world` has permission to read any data
// 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:
// - The caller ensures that `world` has permission to read any data
// required by the condition.
// - `update_archetype_component_access` has been called for condition.
unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) } unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) }
}) })
.fold(true, |acc, res| acc && res) .fold(true, |acc, res| acc && res)

View file

@ -7,6 +7,7 @@ use crate::{
schedule::{ schedule::{
executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule, executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
}, },
warn_system_skipped,
world::World, world::World,
}; };
@ -79,6 +80,15 @@ impl SystemExecutor for SimpleExecutor {
should_run &= system_conditions_met; should_run &= system_conditions_met;
let system = &mut schedule.systems[system_index];
let valid_params = system.validate_param(world);
if !valid_params {
warn_system_skipped!("System", system.name());
}
should_run &= valid_params;
#[cfg(feature = "trace")] #[cfg(feature = "trace")]
should_run_span.exit(); should_run_span.exit();
@ -89,7 +99,6 @@ impl SystemExecutor for SimpleExecutor {
continue; continue;
} }
let system = &mut schedule.systems[system_index];
if is_apply_deferred(system) { if is_apply_deferred(system) {
continue; continue;
} }
@ -128,7 +137,13 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
#[allow(clippy::unnecessary_fold)] #[allow(clippy::unnecessary_fold)]
conditions conditions
.iter_mut() .iter_mut()
.map(|condition| __rust_begin_short_backtrace::readonly_run(&mut **condition, world)) .map(|condition| {
if !condition.validate_param(world) {
warn_system_skipped!("Condition", condition.name());
return false;
}
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)
})
.fold(true, |acc, res| acc && res) .fold(true, |acc, res| acc && res)
} }

View file

@ -5,6 +5,7 @@ use std::panic::AssertUnwindSafe;
use crate::{ use crate::{
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule}, schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
warn_system_skipped,
world::World, world::World,
}; };
@ -85,6 +86,15 @@ impl SystemExecutor for SingleThreadedExecutor {
should_run &= system_conditions_met; should_run &= system_conditions_met;
let system = &mut schedule.systems[system_index];
let valid_params = system.validate_param(world);
if !valid_params {
warn_system_skipped!("System", system.name());
}
should_run &= valid_params;
#[cfg(feature = "trace")] #[cfg(feature = "trace")]
should_run_span.exit(); should_run_span.exit();
@ -95,7 +105,6 @@ impl SystemExecutor for SingleThreadedExecutor {
continue; continue;
} }
let system = &mut schedule.systems[system_index];
if is_apply_deferred(system) { if is_apply_deferred(system) {
self.apply_deferred(schedule, world); self.apply_deferred(schedule, world);
continue; continue;
@ -160,6 +169,12 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
#[allow(clippy::unnecessary_fold)] #[allow(clippy::unnecessary_fold)]
conditions conditions
.iter_mut() .iter_mut()
.map(|condition| __rust_begin_short_backtrace::readonly_run(&mut **condition, world)) .map(|condition| {
if !condition.validate_param(world) {
warn_system_skipped!("Condition", condition.name());
return false;
}
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)
})
.fold(true, |acc, res| acc && res) .fold(true, |acc, res| acc && res)
} }

View file

@ -132,6 +132,11 @@ where
self.system.queue_deferred(world); self.system.queue_deferred(world);
} }
#[inline]
unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
self.system.validate_param_unsafe(world)
}
fn initialize(&mut self, world: &mut crate::prelude::World) { fn initialize(&mut self, world: &mut crate::prelude::World) {
self.system.initialize(world); self.system.initialize(world);
} }

View file

@ -193,6 +193,7 @@ where
) )
} }
#[inline]
fn apply_deferred(&mut self, world: &mut World) { fn apply_deferred(&mut self, world: &mut World) {
self.a.apply_deferred(world); self.a.apply_deferred(world);
self.b.apply_deferred(world); self.b.apply_deferred(world);
@ -204,6 +205,11 @@ where
self.b.queue_deferred(world); self.b.queue_deferred(world);
} }
#[inline]
unsafe fn validate_param_unsafe(&self, world: UnsafeWorldCell) -> bool {
self.a.validate_param_unsafe(world) && self.b.validate_param_unsafe(world)
}
fn initialize(&mut self, world: &mut World) { fn initialize(&mut self, world: &mut World) {
self.a.initialize(world); self.a.initialize(world);
self.b.initialize(world); self.b.initialize(world);

View file

@ -16,8 +16,8 @@ use crate::{
observer::{Observer, TriggerEvent, TriggerTargets}, observer::{Observer, TriggerEvent, TriggerTargets},
system::{RunSystemWithInput, SystemId}, system::{RunSystemWithInput, SystemId},
world::{ world::{
command_queue::RawCommandQueue, Command, CommandQueue, EntityWorldMut, FromWorld, command_queue::RawCommandQueue, unsafe_world_cell::UnsafeWorldCell, Command, CommandQueue,
SpawnBatchIter, World, EntityWorldMut, FromWorld, SpawnBatchIter, World,
}, },
}; };
use bevy_ptr::OwningPtr; use bevy_ptr::OwningPtr;
@ -95,7 +95,9 @@ const _: () = {
// SAFETY: Only reads Entities // SAFETY: Only reads Entities
unsafe impl bevy_ecs::system::SystemParam for Commands<'_, '_> { unsafe impl bevy_ecs::system::SystemParam for Commands<'_, '_> {
type State = FetchState; type State = FetchState;
type Item<'w, 's> = Commands<'w, 's>; type Item<'w, 's> = Commands<'w, 's>;
fn init_state( fn init_state(
world: &mut World, world: &mut World,
system_meta: &mut bevy_ecs::system::SystemMeta, system_meta: &mut bevy_ecs::system::SystemMeta,
@ -107,6 +109,7 @@ const _: () = {
), ),
} }
} }
unsafe fn new_archetype( unsafe fn new_archetype(
state: &mut Self::State, state: &mut Self::State,
archetype: &bevy_ecs::archetype::Archetype, archetype: &bevy_ecs::archetype::Archetype,
@ -121,6 +124,7 @@ const _: () = {
); );
}; };
} }
fn apply( fn apply(
state: &mut Self::State, state: &mut Self::State,
system_meta: &bevy_ecs::system::SystemMeta, system_meta: &bevy_ecs::system::SystemMeta,
@ -132,6 +136,7 @@ const _: () = {
world, world,
); );
} }
fn queue( fn queue(
state: &mut Self::State, state: &mut Self::State,
system_meta: &bevy_ecs::system::SystemMeta, system_meta: &bevy_ecs::system::SystemMeta,
@ -143,13 +148,28 @@ const _: () = {
world, world,
); );
} }
#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &bevy_ecs::system::SystemMeta,
world: UnsafeWorldCell,
) -> bool {
<(Deferred<CommandQueue>, &Entities) as bevy_ecs::system::SystemParam>::validate_param(
&state.state,
system_meta,
world,
)
}
#[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
state: &'s mut Self::State, state: &'s mut Self::State,
system_meta: &bevy_ecs::system::SystemMeta, system_meta: &bevy_ecs::system::SystemMeta,
world: bevy_ecs::world::unsafe_world_cell::UnsafeWorldCell<'w>, world: UnsafeWorldCell<'w>,
change_tick: bevy_ecs::component::Tick, change_tick: bevy_ecs::component::Tick,
) -> Self::Item<'w, 's> { ) -> Self::Item<'w, 's> {
let(f0,f1,) = <(Deferred<'s,CommandQueue> , &'w Entities,)as bevy_ecs::system::SystemParam> ::get_param(&mut state.state,system_meta,world,change_tick); let(f0, f1) = <(Deferred<'s, CommandQueue>, &'w Entities) as bevy_ecs::system::SystemParam>::get_param(&mut state.state, system_meta, world, change_tick);
Commands { Commands {
queue: InternalQueue::CommandQueue(f0), queue: InternalQueue::CommandQueue(f0),
entities: f1, entities: f1,

View file

@ -144,6 +144,11 @@ where
// might have buffers to apply, but this is handled by `PipeSystem`. // might have buffers to apply, but this is handled by `PipeSystem`.
} }
#[inline]
unsafe fn validate_param_unsafe(&self, _world: UnsafeWorldCell) -> bool {
true
}
#[inline] #[inline]
fn initialize(&mut self, world: &mut World) { fn initialize(&mut self, world: &mut World) {
self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX); self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX);

View file

@ -340,6 +340,18 @@ impl<Param: SystemParam> SystemState<Param> {
Param::apply(&mut self.param_state, &self.meta, world); Param::apply(&mut self.param_state, &self.meta, world);
} }
/// Wrapper over [`SystemParam::validate_param`].
///
/// # Safety
///
/// - The passed [`UnsafeWorldCell`] must have read-only access to
/// world data in `archetype_component_access`.
/// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state).
pub unsafe fn validate_param(state: &Self, world: UnsafeWorldCell) -> bool {
// SAFETY: Delegated to existing `SystemParam` implementations.
unsafe { Param::validate_param(&state.param_state, &state.meta, world) }
}
/// Returns `true` if `world_id` matches the [`World`] that was used to call [`SystemState::new`]. /// Returns `true` if `world_id` matches the [`World`] that was used to call [`SystemState::new`].
/// Otherwise, this returns false. /// Otherwise, this returns false.
#[inline] #[inline]
@ -636,6 +648,12 @@ where
F::Param::queue(param_state, &self.system_meta, world); F::Param::queue(param_state, &self.system_meta, world);
} }
#[inline]
unsafe fn validate_param_unsafe(&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)
}
#[inline] #[inline]
fn initialize(&mut self, world: &mut World) { fn initialize(&mut self, world: &mut World) {
if let Some(id) = self.world_id { if let Some(id) = self.world_id {

View file

@ -59,11 +59,11 @@ pub trait System: Send + Sync + 'static {
/// ///
/// # Safety /// # Safety
/// ///
/// - The caller must ensure that `world` has permission to access any world data /// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data
/// registered in [`Self::archetype_component_access`]. There must be no conflicting /// registered in `archetype_component_access`. There must be no conflicting
/// simultaneous accesses while the system is running. /// simultaneous accesses while the system is running.
/// - The method [`Self::update_archetype_component_access`] must be called at some /// - The method [`System::update_archetype_component_access`] must be called at some
/// point before this one, with the same exact [`World`]. If `update_archetype_component_access` /// 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. /// panics (or otherwise does not return for any reason), this method must not be called.
unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out; unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out;
@ -94,6 +94,34 @@ pub trait System: Send + Sync + 'static {
/// of this system into the world's command buffer. /// of this system into the world's command buffer.
fn queue_deferred(&mut self, world: DeferredWorld); fn queue_deferred(&mut self, world: DeferredWorld);
/// Validates that all parameters can be acquired and that system can run without panic.
/// Built-in executors use this to prevent invalid systems from running.
///
/// However calling and respecting [`System::validate_param_unsafe`] or it's safe variant
/// is not a strict requirement, both [`System::run`] and [`System::run_unsafe`]
/// should provide their own safety mechanism to prevent undefined behavior.
///
/// # Safety
///
/// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data
/// registered in `archetype_component_access`. There must be no conflicting
/// simultaneous accesses while the system is running.
/// - 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;
/// Safe version of [`System::validate_param_unsafe`].
/// that runs on exclusive, single-threaded `world` pointer.
fn validate_param(&mut self, world: &World) -> bool {
let world_cell = world.as_unsafe_world_cell_readonly();
self.update_archetype_component_access(world_cell);
// SAFETY:
// - We have exclusive access to the entire world.
// - `update_archetype_component_access` has been called.
unsafe { self.validate_param_unsafe(world_cell) }
}
/// Initialize the system. /// Initialize the system.
fn initialize(&mut self, _world: &mut World); fn initialize(&mut self, _world: &mut World);

View file

@ -1,5 +1,4 @@
pub use crate::change_detection::{NonSendMut, Res, ResMut}; pub use crate::change_detection::{NonSendMut, Res, ResMut};
use crate::query::AccessConflicts;
use crate::storage::SparseSetIndex; use crate::storage::SparseSetIndex;
use crate::{ use crate::{
archetype::{Archetype, Archetypes}, archetype::{Archetype, Archetypes},
@ -14,6 +13,7 @@ use crate::{
system::{Query, SystemMeta}, system::{Query, SystemMeta},
world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FromWorld, World}, world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FromWorld, World},
}; };
use crate::{query::AccessConflicts, storage::ResourceData};
use bevy_ecs_macros::impl_param_set; use bevy_ecs_macros::impl_param_set;
pub use bevy_ecs_macros::Resource; pub use bevy_ecs_macros::Resource;
pub use bevy_ecs_macros::SystemParam; pub use bevy_ecs_macros::SystemParam;
@ -184,17 +184,17 @@ pub unsafe trait SystemParam: Sized {
/// The item type returned when constructing this system param. /// The item type returned when constructing this system param.
/// The value of this associated type should be `Self`, instantiated with new lifetimes. /// The value of this associated type should be `Self`, instantiated with new lifetimes.
/// ///
/// You could think of `SystemParam::Item<'w, 's>` as being an *operation* that changes the lifetimes bound to `Self`. /// You could think of [`SystemParam::Item<'w, 's>`] as being an *operation* that changes the lifetimes bound to `Self`.
type Item<'world, 'state>: SystemParam<State = Self::State>; type Item<'world, 'state>: SystemParam<State = Self::State>;
/// Registers any [`World`] access used by this [`SystemParam`] /// Registers any [`World`] access used by this [`SystemParam`]
/// and creates a new instance of this param's [`State`](Self::State). /// and creates a new instance of this param's [`State`](SystemParam::State).
fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State;
/// For the specified [`Archetype`], registers the components accessed by this [`SystemParam`] (if applicable).a /// For the specified [`Archetype`], registers the components accessed by this [`SystemParam`] (if applicable).a
/// ///
/// # Safety /// # Safety
/// `archetype` must be from the [`World`] used to initialize `state` in `init_state`. /// `archetype` must be from the [`World`] used to initialize `state` in [`SystemParam::init_state`].
#[inline] #[inline]
#[allow(unused_variables)] #[allow(unused_variables)]
unsafe fn new_archetype( unsafe fn new_archetype(
@ -217,15 +217,42 @@ pub unsafe trait SystemParam: Sized {
#[allow(unused_variables)] #[allow(unused_variables)]
fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) {} fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) {}
/// Creates a parameter to be passed into a [`SystemParamFunction`]. /// Validates that the param can be acquired by the [`get_param`](SystemParam::get_param).
/// Built-in executors use this to prevent systems with invalid params from running.
/// For nested [`SystemParam`]s validation will fail if any
/// delegated validation fails.
/// ///
/// [`SystemParamFunction`]: super::SystemParamFunction /// However calling and respecting [`SystemParam::validate_param`]
/// is not a strict requirement, [`SystemParam::get_param`] should
/// provide it's own safety mechanism to prevent undefined behavior.
///
/// The [`world`](UnsafeWorldCell) can only be used to read param's data
/// and world metadata. No data can be written.
///
/// # Safety
///
/// - The passed [`UnsafeWorldCell`] must have read-only access to world data
/// registered in [`init_state`](SystemParam::init_state).
/// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state).
/// - all `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype).
unsafe fn validate_param(
_state: &Self::State,
_system_meta: &SystemMeta,
_world: UnsafeWorldCell,
) -> bool {
// By default we allow panics in [`SystemParam::get_param`] and return `true`.
// Preventing panics is an optional feature.
true
}
/// Creates a parameter to be passed into a [`SystemParamFunction`](super::SystemParamFunction).
/// ///
/// # Safety /// # Safety
/// ///
/// - The passed [`UnsafeWorldCell`] must have access to any world data /// - The passed [`UnsafeWorldCell`] must have access to any world data
/// registered in [`init_state`](SystemParam::init_state). /// registered in [`init_state`](SystemParam::init_state).
/// - `world` must be the same `World` that was used to initialize [`state`](SystemParam::init_state). /// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state).
/// - all `world`'s archetypes have been processed by [`new_archetype`](SystemParam::new_archetype).
unsafe fn get_param<'world, 'state>( unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State, state: &'state mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,
@ -587,6 +614,19 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> {
component_id component_id
} }
#[inline]
unsafe fn validate_param(
&component_id: &Self::State,
_system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
// SAFETY: Read-only access to resource metadata.
unsafe { world.storages() }
.resources
.get(component_id)
.is_some_and(ResourceData::is_present)
}
#[inline] #[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State, &mut component_id: &'s mut Self::State,
@ -684,6 +724,19 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
component_id component_id
} }
#[inline]
unsafe fn validate_param(
&component_id: &Self::State,
_system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
// SAFETY: Read-only access to resource metadata.
unsafe { world.storages() }
.resources
.get(component_id)
.is_some_and(ResourceData::is_present)
}
#[inline] #[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State, &mut component_id: &'s mut Self::State,
@ -778,6 +831,7 @@ unsafe impl SystemParam for &'_ World {
system_meta.component_access_set.add(filtered_access); system_meta.component_access_set.add(filtered_access);
} }
#[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State, _state: &'s mut Self::State,
_system_meta: &SystemMeta, _system_meta: &SystemMeta,
@ -1102,6 +1156,7 @@ unsafe impl<T: SystemBuffer> SystemParam for Deferred<'_, T> {
state.get().queue(system_meta, world); state.get().queue(system_meta, world);
} }
#[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
state: &'s mut Self::State, state: &'s mut Self::State,
_system_meta: &SystemMeta, _system_meta: &SystemMeta,
@ -1216,6 +1271,19 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> {
component_id component_id
} }
#[inline]
unsafe fn validate_param(
&component_id: &Self::State,
_system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
// SAFETY: Read-only access to resource metadata.
unsafe { world.storages() }
.non_send_resources
.get(component_id)
.is_some_and(ResourceData::is_present)
}
#[inline] #[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State, &mut component_id: &'s mut Self::State,
@ -1310,6 +1378,19 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> {
component_id component_id
} }
#[inline]
unsafe fn validate_param(
&component_id: &Self::State,
_system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
// SAFETY: Read-only access to resource metadata.
unsafe { world.storages() }
.non_send_resources
.get(component_id)
.is_some_and(ResourceData::is_present)
}
#[inline] #[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State, &mut component_id: &'s mut Self::State,
@ -1486,6 +1567,7 @@ unsafe impl SystemParam for SystemChangeTick {
fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {} fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {}
#[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State, _state: &'s mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,
@ -1511,6 +1593,18 @@ unsafe impl<T: SystemParam> SystemParam for Vec<T> {
Vec::new() Vec::new()
} }
#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
state
.iter()
.all(|state| T::validate_param(state, system_meta, world))
}
#[inline]
unsafe fn get_param<'world, 'state>( unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State, state: &'state mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,
@ -1562,6 +1656,7 @@ unsafe impl<T: SystemParam> SystemParam for ParamSet<'_, '_, Vec<T>> {
Vec::new() Vec::new()
} }
#[inline]
unsafe fn get_param<'world, 'state>( unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State, state: &'state mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,
@ -1668,6 +1763,16 @@ macro_rules! impl_system_param_tuple {
$($param::queue($param, _system_meta, _world.reborrow());)* $($param::queue($param, _system_meta, _world.reborrow());)*
} }
#[inline]
unsafe fn validate_param(
state: &Self::State,
_system_meta: &SystemMeta,
_world: UnsafeWorldCell,
) -> bool {
let ($($param,)*) = state;
$($param::validate_param($param, _system_meta, _world)&&)* true
}
#[inline] #[inline]
#[allow(clippy::unused_unit)] #[allow(clippy::unused_unit)]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
@ -1676,7 +1781,6 @@ macro_rules! impl_system_param_tuple {
_world: UnsafeWorldCell<'w>, _world: UnsafeWorldCell<'w>,
_change_tick: Tick, _change_tick: Tick,
) -> Self::Item<'w, 's> { ) -> Self::Item<'w, 's> {
let ($($param,)*) = state; let ($($param,)*) = state;
($($param::get_param($param, _system_meta, _world, _change_tick),)*) ($($param::get_param($param, _system_meta, _world, _change_tick),)*)
} }
@ -1826,6 +1930,16 @@ unsafe impl<P: SystemParam + 'static> SystemParam for StaticSystemParam<'_, '_,
P::queue(state, system_meta, world); P::queue(state, system_meta, world);
} }
#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
P::validate_param(state, system_meta, world)
}
#[inline]
unsafe fn get_param<'world, 'state>( unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State, state: &'state mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,
@ -1844,6 +1958,7 @@ unsafe impl<T: ?Sized> SystemParam for PhantomData<T> {
fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {} fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {}
#[inline]
unsafe fn get_param<'world, 'state>( unsafe fn get_param<'world, 'state>(
_state: &'state mut Self::State, _state: &'state mut Self::State,
_system_meta: &SystemMeta, _system_meta: &SystemMeta,
@ -2049,7 +2164,7 @@ trait DynParamState: Sync + Send {
/// For the specified [`Archetype`], registers the components accessed by this [`SystemParam`] (if applicable).a /// For the specified [`Archetype`], registers the components accessed by this [`SystemParam`] (if applicable).a
/// ///
/// # Safety /// # Safety
/// `archetype` must be from the [`World`] used to initialize `state` in `init_state`. /// `archetype` must be from the [`World`] used to initialize `state` in [`SystemParam::init_state`].
unsafe fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta); unsafe fn new_archetype(&mut self, archetype: &Archetype, system_meta: &mut SystemMeta);
/// Applies any deferred mutations stored in this [`SystemParam`]'s state. /// Applies any deferred mutations stored in this [`SystemParam`]'s state.
@ -2060,6 +2175,12 @@ trait DynParamState: Sync + Send {
/// Queues any deferred mutations to be applied at the next [`apply_deferred`](crate::prelude::apply_deferred). /// Queues any deferred mutations to be applied at the next [`apply_deferred`](crate::prelude::apply_deferred).
fn queue(&mut self, system_meta: &SystemMeta, world: DeferredWorld); fn queue(&mut self, system_meta: &SystemMeta, world: DeferredWorld);
/// Refer to [`SystemParam::validate_param`].
///
/// # Safety
/// Refer to [`SystemParam::validate_param`].
unsafe fn validate_param(&self, system_meta: &SystemMeta, world: UnsafeWorldCell) -> bool;
} }
/// A wrapper around a [`SystemParam::State`] that can be used as a trait object in a [`DynSystemParam`]. /// A wrapper around a [`SystemParam::State`] that can be used as a trait object in a [`DynSystemParam`].
@ -2082,6 +2203,10 @@ impl<T: SystemParam + 'static> DynParamState for ParamState<T> {
fn queue(&mut self, system_meta: &SystemMeta, world: DeferredWorld) { fn queue(&mut self, system_meta: &SystemMeta, world: DeferredWorld) {
T::queue(&mut self.0, system_meta, world); T::queue(&mut self.0, system_meta, world);
} }
unsafe fn validate_param(&self, system_meta: &SystemMeta, world: UnsafeWorldCell) -> bool {
T::validate_param(&self.0, system_meta, world)
}
} }
// SAFETY: `init_state` creates a state of (), which performs no access. The interesting safety checks are on the `SystemParamBuilder`. // SAFETY: `init_state` creates a state of (), which performs no access. The interesting safety checks are on the `SystemParamBuilder`.
@ -2094,6 +2219,16 @@ unsafe impl SystemParam for DynSystemParam<'_, '_> {
DynSystemParamState::new::<()>(()) DynSystemParamState::new::<()>(())
} }
#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
state.0.validate_param(system_meta, world)
}
#[inline]
unsafe fn get_param<'world, 'state>( unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State, state: &'state mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,

View file

@ -56,6 +56,7 @@ unsafe impl SystemParam for WorldId {
fn init_state(_: &mut World, _: &mut SystemMeta) -> Self::State {} fn init_state(_: &mut World, _: &mut SystemMeta) -> Self::State {}
#[inline]
unsafe fn get_param<'world, 'state>( unsafe fn get_param<'world, 'state>(
_: &'state mut Self::State, _: &'state mut Self::State,
_: &SystemMeta, _: &SystemMeta,

View file

@ -1477,8 +1477,16 @@ impl World {
self.components self.components
.get_resource_id(TypeId::of::<R>()) .get_resource_id(TypeId::of::<R>())
.and_then(|component_id| self.storages.resources.get(component_id)) .and_then(|component_id| self.storages.resources.get(component_id))
.map(ResourceData::is_present) .is_some_and(ResourceData::is_present)
.unwrap_or(false) }
/// Returns `true` if a resource with provided `component_id` exists. Otherwise returns `false`.
#[inline]
pub fn contains_resource_by_id(&self, component_id: ComponentId) -> bool {
self.storages
.resources
.get(component_id)
.is_some_and(ResourceData::is_present)
} }
/// Returns `true` if a resource of type `R` exists. Otherwise returns `false`. /// Returns `true` if a resource of type `R` exists. Otherwise returns `false`.
@ -1487,8 +1495,16 @@ impl World {
self.components self.components
.get_resource_id(TypeId::of::<R>()) .get_resource_id(TypeId::of::<R>())
.and_then(|component_id| self.storages.non_send_resources.get(component_id)) .and_then(|component_id| self.storages.non_send_resources.get(component_id))
.map(ResourceData::is_present) .is_some_and(ResourceData::is_present)
.unwrap_or(false) }
/// Returns `true` if a resource with provided `component_id` exists. Otherwise returns `false`.
#[inline]
pub fn contains_non_send_by_id(&self, component_id: ComponentId) -> bool {
self.storages
.non_send_resources
.get(component_id)
.is_some_and(ResourceData::is_present)
} }
/// Returns `true` if a resource of type `R` exists and was added since the world's /// Returns `true` if a resource of type `R` exists and was added since the world's
@ -1501,8 +1517,7 @@ impl World {
pub fn is_resource_added<R: Resource>(&self) -> bool { pub fn is_resource_added<R: Resource>(&self) -> bool {
self.components self.components
.get_resource_id(TypeId::of::<R>()) .get_resource_id(TypeId::of::<R>())
.map(|component_id| self.is_resource_added_by_id(component_id)) .is_some_and(|component_id| self.is_resource_added_by_id(component_id))
.unwrap_or(false)
} }
/// Returns `true` if a resource with id `component_id` exists and was added since the world's /// Returns `true` if a resource with id `component_id` exists and was added since the world's

View file

@ -187,13 +187,24 @@ where
GizmosState::<Config, Clear>::apply(&mut state.state, system_meta, world); GizmosState::<Config, Clear>::apply(&mut state.state, system_meta, world);
} }
#[inline]
unsafe fn validate_param(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
) -> bool {
// SAFETY: Delegated to existing `SystemParam` implementations.
unsafe { GizmosState::<Config, Clear>::validate_param(&state.state, system_meta, world) }
}
#[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
state: &'s mut Self::State, state: &'s mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,
world: UnsafeWorldCell<'w>, world: UnsafeWorldCell<'w>,
change_tick: Tick, change_tick: Tick,
) -> Self::Item<'w, 's> { ) -> Self::Item<'w, 's> {
// SAFETY: Delegated to existing `SystemParam` implementations // SAFETY: Delegated to existing `SystemParam` implementations.
let (f0, f1) = unsafe { let (f0, f1) = unsafe {
GizmosState::<Config, Clear>::get_param( GizmosState::<Config, Clear>::get_param(
&mut state.state, &mut state.state,

View file

@ -74,6 +74,29 @@ where
} }
} }
#[inline]
unsafe fn validate_param(
state: &Self::State,
_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 {
return false;
};
// SAFETY: Type is guaranteed by `SystemState`.
let main_world: &World = unsafe { main_world.deref() };
// SAFETY: We provide the main world on which this system state was initialized on.
unsafe {
SystemState::<P>::validate_param(
&state.state,
main_world.as_unsafe_world_cell_readonly(),
)
}
}
#[inline]
unsafe fn get_param<'w, 's>( unsafe fn get_param<'w, 's>(
state: &'s mut Self::State, state: &'s mut Self::State,
system_meta: &SystemMeta, system_meta: &SystemMeta,