From 9e2bd8ac1850ed922073d47384785769ff60a058 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Wed, 7 May 2025 14:20:08 -0400 Subject: [PATCH] Generic `SystemParam` impls for `Option` and `Result` (#18766) # Objective Provide a generic `impl SystemParam for Option

` that uses system parameter validation. This immediately gives useful impls for params like `EventReader` and `GizmosState` that are defined in terms of `Res`. It also allows third-party system parameters to be usable with `Option`, which was previously impossible due to orphan rules. Note that this is a behavior change for `Option`. It currently fails validation if there are multiple matching entities, but with this change it will pass validation and produce `None`. Also provide an impl for `Result`. This allows systems to inspect the error if necessary, either for bubbling it up or for checking the `skipped` flag. Fixes #12634 Fixes #14949 Related to #18516 ## Solution Add generic `SystemParam` impls for `Option` and `Result`, and remove the impls for specific types. Update documentation and `fallible_params` example with the new semantics for `Option`. --- crates/bevy_ecs/src/system/builder.rs | 33 +- crates/bevy_ecs/src/system/system_param.rs | 282 ++++++------------ examples/ecs/fallible_params.rs | 7 +- .../generic-option-parameter.md | 28 ++ 4 files changed, 148 insertions(+), 202 deletions(-) create mode 100644 release-content/migration-guides/generic-option-parameter.md diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index 3f15e9ef17..441d4d42fc 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -7,7 +7,8 @@ use crate::{ query::{QueryData, QueryFilter, QueryState}, resource::Resource, system::{ - DynSystemParam, DynSystemParamState, Local, ParamSet, Query, SystemMeta, SystemParam, When, + DynSystemParam, DynSystemParamState, Local, ParamSet, Query, SystemMeta, SystemParam, + SystemParamValidationError, When, }, world::{ FilteredResources, FilteredResourcesBuilder, FilteredResourcesMut, @@ -710,6 +711,36 @@ unsafe impl<'w, 's, T: FnOnce(&mut FilteredResourcesMutBuilder)> } } +/// A [`SystemParamBuilder`] for an [`Option`]. +#[derive(Clone)] +pub struct OptionBuilder(T); + +// SAFETY: `OptionBuilder` builds a state that is valid for `P`, and any state valid for `P` is valid for `Option

` +unsafe impl> SystemParamBuilder> + for OptionBuilder +{ + fn build(self, world: &mut World, meta: &mut SystemMeta) -> as SystemParam>::State { + self.0.build(world, meta) + } +} + +/// A [`SystemParamBuilder`] for a [`Result`] of [`SystemParamValidationError`]. +#[derive(Clone)] +pub struct ResultBuilder(T); + +// SAFETY: `ResultBuilder` builds a state that is valid for `P`, and any state valid for `P` is valid for `Result` +unsafe impl> + SystemParamBuilder> for ResultBuilder +{ + fn build( + self, + world: &mut World, + meta: &mut SystemMeta, + ) -> as SystemParam>::State { + self.0.build(world, meta) + } +} + /// A [`SystemParamBuilder`] for a [`When`]. #[derive(Clone)] pub struct WhenBuilder(T); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index ff9420cd52..6ee7e08fb5 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -477,87 +477,12 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo } } -// SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If -// this Query conflicts with any prior access, a panic will occur. -unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam - for Option> -{ - type State = QueryState; - type Item<'w, 's> = Option>; - - fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - Single::init_state(world, system_meta) - } - - unsafe fn new_archetype( - state: &mut Self::State, - archetype: &Archetype, - system_meta: &mut SystemMeta, - ) { - // SAFETY: Delegate to existing `SystemParam` implementations. - unsafe { Single::new_archetype(state, archetype, system_meta) }; - } - - #[inline] - unsafe fn get_param<'w, 's>( - state: &'s mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - change_tick: Tick, - ) -> Self::Item<'w, 's> { - state.validate_world(world.id()); - // SAFETY: State ensures that the components it accesses are not accessible elsewhere. - // The caller ensures the world matches the one used in init_state. - let query = unsafe { - state.query_unchecked_manual_with_ticks(world, system_meta.last_run, change_tick) - }; - match query.single_inner() { - Ok(single) => Some(Single { - item: single, - _filter: PhantomData, - }), - Err(QuerySingleError::NoEntities(_)) => None, - Err(QuerySingleError::MultipleEntities(e)) => panic!("{}", e), - } - } - - #[inline] - unsafe fn validate_param( - state: &Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: State ensures that the components it accesses are not mutably accessible elsewhere - // and the query is read only. - // The caller ensures the world matches the one used in init_state. - let query = unsafe { - state.query_unchecked_manual_with_ticks( - world, - system_meta.last_run, - world.change_tick(), - ) - }; - match query.single_inner() { - Ok(_) | Err(QuerySingleError::NoEntities(_)) => Ok(()), - Err(QuerySingleError::MultipleEntities(_)) => Err( - SystemParamValidationError::skipped::("Multiple matching entities"), - ), - } - } -} - // SAFETY: QueryState is constrained to read-only fetches, so it only reads World. unsafe impl<'a, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOnlySystemParam for Single<'a, D, F> { } -// SAFETY: QueryState is constrained to read-only fetches, so it only reads World. -unsafe impl<'a, D: ReadOnlyQueryData + 'static, F: QueryFilter + 'static> ReadOnlySystemParam - for Option> -{ -} - // SAFETY: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this Query conflicts with any prior access, a panic will occur. unsafe impl SystemParam @@ -931,40 +856,6 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { } } -// SAFETY: Only reads a single World resource -unsafe impl<'a, T: Resource> ReadOnlySystemParam for Option> {} - -// SAFETY: this impl defers to `Res`, which initializes and validates the correct world access. -unsafe impl<'a, T: Resource> SystemParam for Option> { - type State = ComponentId; - type Item<'w, 's> = Option>; - - fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - Res::::init_state(world, system_meta) - } - - #[inline] - unsafe fn get_param<'w, 's>( - &mut component_id: &'s mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - change_tick: Tick, - ) -> Self::Item<'w, 's> { - world - .get_resource_with_ticks(component_id) - .map(|(ptr, ticks, caller)| Res { - value: ptr.deref(), - ticks: Ticks { - added: ticks.added.deref(), - changed: ticks.changed.deref(), - last_run: system_meta.last_run, - this_run: change_tick, - }, - changed_by: caller.map(|caller| caller.deref()), - }) - } -} - // SAFETY: Res ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this Res // conflicts with any prior access, a panic will occur. unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { @@ -1045,37 +936,6 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { } } -// SAFETY: this impl defers to `ResMut`, which initializes and validates the correct world access. -unsafe impl<'a, T: Resource> SystemParam for Option> { - type State = ComponentId; - type Item<'w, 's> = Option>; - - fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - ResMut::::init_state(world, system_meta) - } - - #[inline] - unsafe fn get_param<'w, 's>( - &mut component_id: &'s mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - change_tick: Tick, - ) -> Self::Item<'w, 's> { - world - .get_resource_mut_by_id(component_id) - .map(|value| ResMut { - value: value.value.deref_mut::(), - ticks: TicksMut { - added: value.ticks.added, - changed: value.ticks.changed, - last_run: system_meta.last_run, - this_run: change_tick, - }, - changed_by: value.changed_by, - }) - } -} - /// SAFETY: only reads world unsafe impl<'w> ReadOnlySystemParam for &'w World {} @@ -1644,37 +1504,6 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { } } -// SAFETY: Only reads a single World non-send resource -unsafe impl ReadOnlySystemParam for Option> {} - -// SAFETY: this impl defers to `NonSend`, which initializes and validates the correct world access. -unsafe impl SystemParam for Option> { - type State = ComponentId; - type Item<'w, 's> = Option>; - - fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - NonSend::::init_state(world, system_meta) - } - - #[inline] - unsafe fn get_param<'w, 's>( - &mut component_id: &'s mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - change_tick: Tick, - ) -> Self::Item<'w, 's> { - world - .get_non_send_with_ticks(component_id) - .map(|(ptr, ticks, caller)| NonSend { - value: ptr.deref(), - ticks: ticks.read(), - last_run: system_meta.last_run, - this_run: change_tick, - changed_by: caller.map(|caller| caller.deref()), - }) - } -} - // SAFETY: NonSendMut ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this // NonSendMut conflicts with any prior access, a panic will occur. unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { @@ -1753,32 +1582,6 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { } } -// SAFETY: this impl defers to `NonSendMut`, which initializes and validates the correct world access. -unsafe impl<'a, T: 'static> SystemParam for Option> { - type State = ComponentId; - type Item<'w, 's> = Option>; - - fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - NonSendMut::::init_state(world, system_meta) - } - - #[inline] - unsafe fn get_param<'w, 's>( - &mut component_id: &'s mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - change_tick: Tick, - ) -> Self::Item<'w, 's> { - world - .get_non_send_with_ticks(component_id) - .map(|(ptr, ticks, caller)| NonSendMut { - value: ptr.assert_unique().deref_mut(), - ticks: TicksMut::from_tick_cells(ticks, system_meta.last_run, change_tick), - changed_by: caller.map(|caller| caller.deref_mut()), - }) - } -} - // SAFETY: Only reads World archetypes unsafe impl<'a> ReadOnlySystemParam for &'a Archetypes {} @@ -1916,6 +1719,91 @@ unsafe impl SystemParam for SystemChangeTick { } } +// SAFETY: Delegates to `T`, which ensures the safety requirements are met +unsafe impl SystemParam for Option { + type State = T::State; + + type Item<'world, 'state> = Option>; + + fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { + T::init_state(world, system_meta) + } + + #[inline] + unsafe fn get_param<'world, 'state>( + state: &'state mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'world>, + change_tick: Tick, + ) -> Self::Item<'world, 'state> { + T::validate_param(state, system_meta, world) + .ok() + .map(|()| T::get_param(state, system_meta, world, change_tick)) + } + + unsafe fn new_archetype( + state: &mut Self::State, + archetype: &Archetype, + system_meta: &mut SystemMeta, + ) { + // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. + unsafe { T::new_archetype(state, archetype, system_meta) }; + } + + fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { + T::apply(state, system_meta, world); + } + + fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) { + T::queue(state, system_meta, world); + } +} + +// SAFETY: Delegates to `T`, which ensures the safety requirements are met +unsafe impl ReadOnlySystemParam for Option {} + +// SAFETY: Delegates to `T`, which ensures the safety requirements are met +unsafe impl SystemParam for Result { + type State = T::State; + + type Item<'world, 'state> = Result, SystemParamValidationError>; + + fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { + T::init_state(world, system_meta) + } + + #[inline] + unsafe fn get_param<'world, 'state>( + state: &'state mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'world>, + change_tick: Tick, + ) -> Self::Item<'world, 'state> { + T::validate_param(state, system_meta, world) + .map(|()| T::get_param(state, system_meta, world, change_tick)) + } + + unsafe fn new_archetype( + state: &mut Self::State, + archetype: &Archetype, + system_meta: &mut SystemMeta, + ) { + // SAFETY: The caller ensures that `archetype` is from the World the state was initialized from in `init_state`. + unsafe { T::new_archetype(state, archetype, system_meta) }; + } + + fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { + T::apply(state, system_meta, world); + } + + fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) { + T::queue(state, system_meta, world); + } +} + +// SAFETY: Delegates to `T`, which ensures the safety requirements are met +unsafe impl ReadOnlySystemParam for Result {} + /// A [`SystemParam`] that wraps another parameter and causes its system to skip instead of failing when the parameter is invalid. /// /// # Example diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index 99eaedf10d..1ba510cda9 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -131,13 +131,12 @@ fn move_targets(mut enemies: Populated<(&mut Transform, &mut Enemy)>, time: Res< } /// System that moves the player, causing them to track a single enemy. -/// The player will search for enemies if there are none. -/// If there is one, player will track it. -/// If there are too many enemies, the player will cease all action (the system will not run). +/// If there is exactly one, player will track it. +/// Otherwise, the player will search for enemies. fn track_targets( // `Single` ensures the system runs ONLY when exactly one matching entity exists. mut player: Single<(&mut Transform, &Player)>, - // `Option` ensures that the system runs ONLY when zero or one matching entity exists. + // `Option` never prevents the system from running, but will be `None` if there is not exactly one matching entity. enemy: Option, Without)>>, time: Res