diff --git a/benches/benches/bevy_ecs/fragmentation.rs b/benches/benches/bevy_ecs/fragmentation.rs index 97864990a37e5..e7a2f4df78520 100644 --- a/benches/benches/bevy_ecs/fragmentation.rs +++ b/benches/benches/bevy_ecs/fragmentation.rs @@ -25,7 +25,7 @@ fn iter_frag_empty(c: &mut Criterion) { spawn_empty_frag_archetype::(&mut world); let mut q: SystemState> = SystemState::)>>::new(&mut world); - let query = q.get(&world); + let query = q.get(&world).unwrap(); b.iter(move || { let mut res = 0; query.iter().for_each(|(e, t)| { @@ -39,7 +39,7 @@ fn iter_frag_empty(c: &mut Criterion) { spawn_empty_frag_archetype::(&mut world); let mut q: SystemState> = SystemState::)>>::new(&mut world); - let query = q.get(&world); + let query = q.get(&world).unwrap(); b.iter(move || { let mut res = 0; query.iter().for_each(|(e, t)| { diff --git a/benches/benches/bevy_ecs/world/world_get.rs b/benches/benches/bevy_ecs/world/world_get.rs index bd7507b3047a0..19aa015b91afc 100644 --- a/benches/benches/bevy_ecs/world/world_get.rs +++ b/benches/benches/bevy_ecs/world/world_get.rs @@ -269,7 +269,7 @@ pub fn query_get(criterion: &mut Criterion) { .collect(); entities.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; @@ -288,7 +288,7 @@ pub fn query_get(criterion: &mut Criterion) { .collect(); entities.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; @@ -319,7 +319,7 @@ pub fn query_get_many(criterion: &mut Criterion) { entity_groups.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; @@ -342,7 +342,7 @@ pub fn query_get_many(criterion: &mut Criterion) { entity_groups.shuffle(&mut deterministic_rand()); let mut query = SystemState::>::new(&mut world); - let query = query.get(&world); + let query = query.get(&world).unwrap(); bencher.iter(|| { let mut count = 0; diff --git a/crates/bevy_ecs/compile_fail/tests/ui/query_lens_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/query_lens_lifetime_safety.rs index d3cdb0607842b..31a4f6713ea22 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/query_lens_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/query_lens_lifetime_safety.rs @@ -13,7 +13,7 @@ fn main() { let mut system_state = SystemState::<(Query<&mut Foo>, Query<&mut Bar>)>::new(&mut world); { - let (mut foo_query, mut bar_query) = system_state.get_mut(&mut world); + let (mut foo_query, mut bar_query) = system_state.get_mut(&mut world).unwrap(); dbg!("hi"); { let mut lens = foo_query.as_query_lens(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs index 62d76bec1695b..7dafff0d6c3a6 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs @@ -10,7 +10,7 @@ fn main() { let mut system_state = SystemState::>::new(&mut world); { - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); dbg!("hi"); { let data: &Foo = query.get(e).unwrap(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs index 7518511feee26..97b2e1aa21266 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs @@ -12,7 +12,7 @@ fn main() { world.spawn(Foo(10)); let mut system_state = SystemState::>::new(&mut world); - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); { let mut lens_a = query.transmute_lens::<&mut Foo>(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs index e1912372b61fd..82680d44acbcf 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs @@ -14,10 +14,10 @@ struct State { impl State { fn get_component(&mut self, world: &mut World, entity: Entity) { - let q1 = self.state_r.get(&world); + let q1 = self.state_r.get(&world).unwrap(); let a1 = q1.get(entity).unwrap(); - let mut q2 = self.state_w.get_mut(world); + let mut q2 = self.state_w.get_mut(world).unwrap(); //~^ E0502 let _ = q2.get_mut(entity).unwrap(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs index 88325b2a5f2db..6582cf63d0ff3 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs @@ -14,10 +14,10 @@ struct State { impl State { fn get_components(&mut self, world: &mut World) { - let q1 = self.state_r.get(&world); + let q1 = self.state_r.get(&world).unwrap(); let a1 = q1.iter().next().unwrap(); - let mut q2 = self.state_w.get_mut(world); + let mut q2 = self.state_w.get_mut(world).unwrap(); //~^ E0502 let _ = q2.iter_mut().next().unwrap(); diff --git a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs index 3c767b91be919..bae4d55d9c49c 100644 --- a/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs +++ b/crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs @@ -11,7 +11,7 @@ fn main() { let mut system_state = SystemState::>::new(&mut world); { - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); let mut_vec = query.iter_mut().collect::>>(); assert_eq!( // this should fail to compile due to the later use of mut_vec diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index eafd8f4ad0129..619f442a71444 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -449,33 +449,22 @@ fn derive_system_param_impl( <#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 mut Self::State, - _system_meta: &#path::system::SystemMeta, - _world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, - ) -> Result<(), #path::system::SystemParamValidationError> { - let #state_struct_name { state: (#(#tuple_patterns,)*) } = state; - #( - <#field_types as #path::system::SystemParam>::validate_param(#field_locals, _system_meta, _world) - .map_err(|err| #path::system::SystemParamValidationError::new::(err.skipped, #field_validation_messages, #field_validation_names))?; - )* - Result::Ok(()) - } - #[inline] unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &#path::system::SystemMeta, world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, change_tick: #path::change_detection::Tick, - ) -> Self::Item<'w, 's> { - let (#(#tuple_patterns,)*) = < - (#(#tuple_types,)*) as #path::system::SystemParam - >::get_param(&mut state.state, system_meta, world, change_tick); - #struct_name { + ) -> Result, #path::system::SystemParamValidationError> { + let (#(#tuple_patterns,)*) = &mut state.state; + #( + let #field_locals = unsafe { + <#field_types as #path::system::SystemParam>::get_param(#field_locals, system_meta, world, change_tick) + }.map_err(|err| #path::system::SystemParamValidationError::new::(err.skipped, #field_validation_messages, #field_validation_names))?; + )* + Result::Ok(#struct_name { #(#field_members: #field_locals,)* - } + }) } } diff --git a/crates/bevy_ecs/src/lifecycle.rs b/crates/bevy_ecs/src/lifecycle.rs index ace0dfd87abda..13685cf9ce933 100644 --- a/crates/bevy_ecs/src/lifecycle.rs +++ b/crates/bevy_ecs/src/lifecycle.rs @@ -60,7 +60,7 @@ use crate::{ query::FilteredAccessSet, relationship::RelationshipHookMode, storage::SparseSet, - system::{Local, ReadOnlySystemParam, SystemMeta, SystemParam}, + system::{Local, ReadOnlySystemParam, SystemMeta, SystemParam, SystemParamValidationError}, world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World}, }; @@ -639,7 +639,7 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentMessages { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.removed_components() + ) -> Result, SystemParamValidationError> { + Ok(world.removed_components()) } } diff --git a/crates/bevy_ecs/src/message/message_reader.rs b/crates/bevy_ecs/src/message/message_reader.rs index c53e044fcb000..110ffb9f96116 100644 --- a/crates/bevy_ecs/src/message/message_reader.rs +++ b/crates/bevy_ecs/src/message/message_reader.rs @@ -162,35 +162,15 @@ unsafe impl<'w, 's, M: Message> SystemParam for PopulatedMessageReader<'w, 's, M system_meta: &crate::system::SystemMeta, world: crate::world::unsafe_world_cell::UnsafeWorldCell<'world>, change_tick: crate::change_detection::Tick, - ) -> Self::Item<'world, 'state> { + ) -> Result, SystemParamValidationError> { // SAFETY: requirements are upheld by MessageReader's implementation - unsafe { - PopulatedMessageReader(MessageReader::get_param( - state, - system_meta, - world, - change_tick, - )) - } - } - - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &crate::system::SystemMeta, - world: crate::world::unsafe_world_cell::UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: requirements are upheld by MessageReader's implementation - unsafe { MessageReader::::validate_param(state, system_meta, world) }?; - - // SAFETY: requirements are upheld by MessageReader's implementation - let reader = - unsafe { MessageReader::get_param(state, system_meta, world, world.change_tick()) }; + let reader = unsafe { MessageReader::get_param(state, system_meta, world, change_tick)? }; if reader.is_empty() { Err(SystemParamValidationError::skipped::( "message queue is empty", )) } else { - Ok(()) + Ok(PopulatedMessageReader(reader)) } } } diff --git a/crates/bevy_ecs/src/observer/runner.rs b/crates/bevy_ecs/src/observer/runner.rs index 79369dd4cd498..9180a377edc4b 100644 --- a/crates/bevy_ecs/src/observer/runner.rs +++ b/crates/bevy_ecs/src/observer/runner.rs @@ -107,11 +107,7 @@ pub(super) unsafe fn observer_system_runner>::new(&mut world); - let q = q.get_mut(&mut world); + let q = q.get_mut(&mut world).unwrap(); let _: Option<&Foo> = q.iter().next(); let _: Option<[&Foo; 2]> = q.iter_combinations::<2>().next(); let _: Option<&Foo> = q.iter_many([e]).next(); diff --git a/crates/bevy_ecs/src/reflect/entity_commands.rs b/crates/bevy_ecs/src/reflect/entity_commands.rs index 140c96eba440a..c80c464635f25 100644 --- a/crates/bevy_ecs/src/reflect/entity_commands.rs +++ b/crates/bevy_ecs/src/reflect/entity_commands.rs @@ -462,7 +462,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); let entity2 = commands.spawn_empty().id(); @@ -508,7 +508,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); @@ -538,7 +538,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn(ComponentA(0)).id(); @@ -567,7 +567,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn(ComponentA(0)).id(); @@ -596,7 +596,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); let bundle = Box::new(BundleA { @@ -626,7 +626,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands.spawn_empty().id(); let bundle = Box::new(BundleA { @@ -656,7 +656,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands .spawn(BundleA { @@ -694,7 +694,7 @@ mod tests { world.insert_resource(type_registry); let mut system_state: SystemState = SystemState::new(&mut world); - let mut commands = system_state.get_mut(&mut world); + let mut commands = system_state.get_mut(&mut world).unwrap(); let entity = commands .spawn(BundleA { diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index df297637a6037..551bb0fae30ff 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -22,7 +22,7 @@ use crate::{ ConditionWithAccess, InternedSystemSet, SystemKey, SystemSetKey, SystemTypeSet, SystemWithAccess, }, - system::{RunSystemError, System, SystemIn, SystemParamValidationError, SystemStateFlags}, + system::{RunSystemError, System, SystemIn, SystemStateFlags}, world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World}, }; @@ -197,15 +197,6 @@ impl System for ApplyDeferred { fn queue_deferred(&mut self, _world: DeferredWorld) {} - unsafe fn validate_param_unsafe( - &mut self, - _world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // This system is always valid to run because it doesn't do anything, - // and only used as a marker for the executor. - Ok(()) - } - fn initialize(&mut self, _world: &mut World) -> FilteredAccessSet { FilteredAccessSet::new() } @@ -561,3 +552,330 @@ mod tests { assert_eq!(counter.0, 0); } } + +#[cfg(test)] +mod validation_tests { + use crate::{ + prelude::{Component, In, IntoSystem, Resource, Schedule}, + schedule::ExecutorKind, + system::{ + DynParamBuilder, DynSystemParam, ExclusiveSystemParam, Local, ParamBuilder, ParamSet, + Query, Res, ResMut, RunSystemError, RunSystemOnce, Single, SystemMeta, + SystemParamBuilder, SystemParamValidationError, + }, + world::World, + }; + + #[derive(Component)] + struct TestComponent; + + const EXECUTORS: [ExecutorKind; 2] = + [ExecutorKind::SingleThreaded, ExecutorKind::MultiThreaded]; + + #[derive(Resource, Default)] + struct Counter(u8); + + // A resource that won't be inserted, causing validation to fail. + #[derive(Resource)] + struct MissingResource; + + /// An [`ExclusiveSystemParam`] that always fails validation. + struct AlwaysInvalid; + + impl ExclusiveSystemParam for AlwaysInvalid { + type State = (); + type Item<'s> = AlwaysInvalid; + + fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {} + + fn get_param<'s>( + _state: &'s mut Self::State, + _system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Err(SystemParamValidationError::invalid::( + "always invalid", + )) + } + } + + #[test] + fn function_system_validation_failure_is_error() { + fn system(_res: Res) {} + + let mut world = World::new(); + let result = world.run_system_once(system); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed, got {result:?}" + ); + } + + #[test] + fn function_system_validation_skip() { + fn system(_single: Single<&TestComponent>) {} + + let mut world = World::new(); + let result = world.run_system_once(system); + assert!( + matches!(result, Err(RunSystemError::Skipped(_))), + "Expected Skipped, got {result:?}" + ); + } + + #[test] + fn function_system_validation_success() { + fn system(_res: Res) {} + + let mut world = World::new(); + world.init_resource::(); + let result = world.run_system_once(system); + assert!(result.is_ok(), "Expected Ok, got {result:?}"); + } + + #[test] + fn adapter_system_validation_failure() { + fn system(_res: Res) -> u32 { + 42 + } + + let mut world = World::new(); + let result = world.run_system_once(system.map(|_x| {})); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed from adapter system, got {result:?}" + ); + } + + #[test] + fn adapter_system_validation_skip() { + fn system(_single: Single<&TestComponent>) -> u32 { + 42 + } + + let mut world = World::new(); + let result = world.run_system_once(system.map(|_x| {})); + assert!( + matches!(result, Err(RunSystemError::Skipped(_))), + "Expected Skipped from adapter system, got {result:?}" + ); + } + + #[test] + fn pipe_system_validation_failure_in_first() { + fn first(_res: Res) -> u32 { + 42 + } + fn second(_input: In) {} + + let mut world = World::new(); + let result = world.run_system_once(first.pipe(second)); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed from pipe first system, got {result:?}" + ); + } + + #[test] + fn pipe_system_validation_failure_in_second() { + fn first() -> u32 { + 42 + } + fn second(_input: In, _res: Res) {} + + let mut world = World::new(); + let result = world.run_system_once(first.pipe(second)); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed from pipe second system, got {result:?}" + ); + } + + #[test] + fn pipe_system_validation_skip_in_first() { + fn first(_single: Single<&TestComponent>) -> u32 { + 42 + } + fn second(_input: In) {} + + let mut world = World::new(); + let result = world.run_system_once(first.pipe(second)); + assert!( + matches!(result, Err(RunSystemError::Skipped(_))), + "Expected Skipped from pipe first system, got {result:?}" + ); + } + + #[test] + fn pipe_system_validation_skip_in_second() { + fn first() -> u32 { + 42 + } + + fn second(_input: In, _single: Single<&TestComponent>) {} + + let mut world = World::new(); + let result = world.run_system_once(first.pipe(second)); + assert!( + matches!(result, Err(RunSystemError::Skipped(_))), + "Expected Skipped from pipe second system, got {result:?}" + ); + } + + #[test] + fn builder_system_validation_failure() { + fn system(_res: Res) {} + + let mut world = World::new(); + let result = world.run_system_once(ParamBuilder.build_system(system)); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed from builder system, got {result:?}" + ); + } + + #[test] + fn builder_system_validation_skip() { + fn system(_single: Single<&TestComponent>) {} + + let mut world = World::new(); + let result = world.run_system_once(ParamBuilder.build_system(system)); + assert!( + matches!(result, Err(RunSystemError::Skipped(_))), + "Expected Skipped from builder system, got {result:?}" + ); + } + + #[test] + fn dyn_system_param_validation_failure() { + let mut world = World::new(); + let system = (DynParamBuilder::new::>(ParamBuilder),) + .build_state(&mut world) + .build_system(|_param: DynSystemParam| {}); + let result = world.run_system_once(system); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed from DynSystemParam system, got {result:?}" + ); + } + + #[test] + fn dyn_system_param_validation_skip() { + let mut world = World::new(); + let system = (DynParamBuilder::new::>(ParamBuilder),) + .build_state(&mut world) + .build_system(|_param: DynSystemParam| {}); + let result = world.run_system_once(system); + assert!( + matches!(result, Err(RunSystemError::Skipped(_))), + "Expected Skipped from DynSystemParam system, got {result:?}" + ); + } + + #[test] + fn dyn_system_param_validation_success() { + let mut world = World::new(); + world.init_resource::(); + let system = (DynParamBuilder::new::>(ParamBuilder),) + .build_state(&mut world) + .build_system(|_param: DynSystemParam| {}); + let result = world.run_system_once(system); + assert!( + result.is_ok(), + "Expected Ok from DynSystemParam system, got {result:?}" + ); + } + + #[test] + fn exclusive_system_validation_failure() { + fn system(_world: &mut World, _param: AlwaysInvalid) {} + + let mut world = World::new(); + let result = world.run_system_once(system); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed from exclusive system, got {result:?}" + ); + } + + #[test] + fn exclusive_system_validation_success() { + fn system(_world: &mut World, mut _local: Local) {} + + let mut world = World::new(); + let result = world.run_system_once(system); + assert!( + result.is_ok(), + "Expected Ok from exclusive system, got {result:?}" + ); + } + + #[test] + fn validation_skips_system_in_schedule() { + // Ensure the executor properly handles validation failures by skipping + // and not running the system body. + fn skippable_system(_single: Single<&TestComponent>, mut counter: ResMut) { + counter.0 += 1; + } + + for executor in EXECUTORS { + let mut world = World::new(); + world.init_resource::(); + + let mut schedule = Schedule::default(); + schedule.set_executor_kind(executor); + schedule.add_systems(skippable_system); + + // No TestComponent entity exists, so the system should be skipped. + schedule.run(&mut world); + assert_eq!( + world.resource::().0, + 0, + "System should have been skipped with {executor:?}" + ); + } + } + + #[test] + fn param_set_validation_skip() { + // A system using ParamSet with a Single sub-param should be skipped + // when the Single has no matching entities, rather than panicking. + fn system(mut _set: ParamSet<(Single<&TestComponent>,)>) {} + + let mut world = World::new(); + let result = world.run_system_once(system); + assert!( + matches!(result, Err(RunSystemError::Skipped(_))), + "Expected Skipped from ParamSet with invalid Single, got {result:?}" + ); + } + + #[test] + fn param_set_validation_failure() { + // A system using ParamSet with a Res sub-param should fail validation + // when the resource does not exist. + fn system(mut _set: ParamSet<(Query<&TestComponent>, Res)>) {} + + let mut world = World::new(); + let result = world.run_system_once(system); + assert!( + matches!(result, Err(RunSystemError::Failed(_))), + "Expected Failed from ParamSet with missing resource, got {result:?}" + ); + } + + #[test] + fn param_set_validation_success() { + // A system using ParamSet with valid sub-params should succeed. + fn system(mut set: ParamSet<(Query<&TestComponent>, Res)>) { + let _q = set.p0(); + } + + let mut world = World::new(); + world.init_resource::(); + let result = world.run_system_once(system); + assert!( + result.is_ok(), + "Expected Ok from ParamSet with valid params, got {result:?}" + ); + } +} diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 33bc8d0b80079..a9343f89e0c0c 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -644,32 +644,6 @@ impl ExecutorState { should_run &= system_conditions_met; - if should_run { - // SAFETY: - // - The caller ensures that `world` has permission to read any data - // required by the system. - let valid_params = match unsafe { system.validate_param_unsafe(world) } { - Ok(()) => true, - Err(e) => { - if !e.skipped { - error_handler( - e.into(), - ErrorContext::System { - name: system.name(), - last_run: system.get_last_run(), - }, - ); - } - false - } - }; - if !valid_params { - self.skipped_systems.insert(system_index); - } - - should_run &= valid_params; - } - should_run } @@ -852,16 +826,7 @@ unsafe fn evaluate_and_fold_conditions( // SAFETY: // - The caller ensures that `world` has permission to read any data // required by the condition. - unsafe { condition.validate_param_unsafe(world) } - .map_err(From::from) - .and_then(|()| { - // SAFETY: - // - The caller ensures that `world` has permission to read any data - // required by the condition. - unsafe { - __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) - } - }) + unsafe { __rust_begin_short_backtrace::readonly_run_unsafe(&mut **condition, world) } .unwrap_or_else(|err| { if let RunSystemError::Failed(err) = err { error_handler( diff --git a/crates/bevy_ecs/src/schedule/mod.rs b/crates/bevy_ecs/src/schedule/mod.rs index 70a261f47b1fa..1c6f32deb1cb2 100644 --- a/crates/bevy_ecs/src/schedule/mod.rs +++ b/crates/bevy_ecs/src/schedule/mod.rs @@ -1304,7 +1304,7 @@ mod tests { // start a new frame by running ihe begin_frame() system let mut system_state: SystemState>> = SystemState::new(&mut world); - let res = system_state.get_mut(&mut world); + let res = system_state.get_mut(&mut world).unwrap(); Stepping::begin_frame(res); // now run the schedule; this will panic if the executor doesn't diff --git a/crates/bevy_ecs/src/schedule/node.rs b/crates/bevy_ecs/src/schedule/node.rs index f73d8f9d49c0f..f8c29869db482 100644 --- a/crates/bevy_ecs/src/schedule/node.rs +++ b/crates/bevy_ecs/src/schedule/node.rs @@ -24,10 +24,7 @@ use crate::{ BoxedCondition, InternedSystemSet, ScheduleGraph, }, storage::SparseSetIndex, - system::{ - ReadOnlySystem, RunSystemError, ScheduleSystem, System, SystemParamValidationError, - SystemStateFlags, - }, + system::{ReadOnlySystem, RunSystemError, ScheduleSystem, System, SystemStateFlags}, world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World}, }; @@ -101,15 +98,6 @@ impl System for SystemWithAccess { self.system.queue_deferred(world); } - #[inline] - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Caller ensures the same safety requirements. - unsafe { self.system.validate_param_unsafe(world) } - } - #[inline] fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { self.system.initialize(world) @@ -201,15 +189,6 @@ impl System for ConditionWithAccess { self.condition.queue_deferred(world); } - #[inline] - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Caller ensures the same safety requirements. - unsafe { self.condition.validate_param_unsafe(world) } - } - #[inline] fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { self.condition.initialize(world) diff --git a/crates/bevy_ecs/src/system/adapter_system.rs b/crates/bevy_ecs/src/system/adapter_system.rs index 8d3d35e04c8c2..5d3a70161bf6e 100644 --- a/crates/bevy_ecs/src/system/adapter_system.rs +++ b/crates/bevy_ecs/src/system/adapter_system.rs @@ -1,7 +1,7 @@ use alloc::vec::Vec; use bevy_utils::prelude::DebugName; -use super::{IntoSystem, ReadOnlySystem, RunSystemError, System, SystemParamValidationError}; +use super::{IntoSystem, ReadOnlySystem, RunSystemError, System}; use crate::{ schedule::InternedSystemSet, system::{input::SystemInput, SystemIn}, @@ -162,15 +162,6 @@ where self.system.queue_deferred(world); } - #[inline] - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Delegate to other `System` implementations. - unsafe { self.system.validate_param_unsafe(world) } - } - fn initialize(&mut self, world: &mut crate::prelude::World) -> crate::query::FilteredAccessSet { self.system.initialize(world) } diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index e1b19c14403b7..a84563ea8a01b 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -420,24 +420,6 @@ where } } - #[inline] - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - match &mut self.inner { - // SAFETY: requirements upheld by the caller. - BuilderSystemInner::Initialized { system, .. } => unsafe { - system.validate_param_unsafe(world) - }, - BuilderSystemInner::Uninitialized { .. } => panic!( - "BuilderSystem {} was not initialized before calling validate_param_unsafe.", - self.name() - ), - BuilderSystemInner::Invalid => unreachable!(), - } - } - #[inline] fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { let inner = mem::replace(&mut self.inner, BuilderSystemInner::Invalid); diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 73dbb33d834d5..69d1b563ad232 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -8,7 +8,7 @@ use crate::{ prelude::World, query::FilteredAccessSet, schedule::InternedSystemSet, - system::{input::SystemInput, SystemIn, SystemParamValidationError}, + system::{input::SystemInput, SystemIn}, world::unsafe_world_cell::UnsafeWorldCell, }; @@ -168,10 +168,7 @@ where world: &mut PrivateUnsafeWorldCell, ) -> Result { // SAFETY: see comment on `Func::combine` call - match (|| unsafe { - system.validate_param_unsafe(world.0)?; - system.run_unsafe(input, world.0) - })() { + match unsafe { system.run_unsafe(input, world.0) } { // let the world's default error handler handle the error if `Failed(_)` Err(RunSystemError::Failed(err)) => { // SAFETY: We registered access to DefaultErrorHandler in `initialize`. @@ -233,17 +230,6 @@ where self.b.queue_deferred(world); } - #[inline] - unsafe fn validate_param_unsafe( - &mut self, - _world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // Both systems are validated in `Self::run_unsafe`, so that we get the - // chance to run the second system even if the first one fails to - // validate. - Ok(()) - } - fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { let mut a_access = self.a.initialize(world); let b_access = self.b.initialize(world); @@ -415,9 +401,6 @@ where // SAFETY: Upheld by caller unsafe { let value = self.a.run_unsafe(input, world)?; - // `Self::validate_param_unsafe` already validated the first system, - // but we still need to validate the second system once the first one runs. - self.b.validate_param_unsafe(world)?; self.b.run_unsafe(value, world) } } @@ -439,18 +422,6 @@ where self.b.queue_deferred(world); } - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // We only validate parameters for the first system, - // since it may make changes to the world that affect - // whether the second system has valid parameters. - // The second system will be validated in `Self::run_unsafe`. - // SAFETY: Delegate to the `System` implementation for `a`. - unsafe { self.a.validate_param_unsafe(world) } - } - fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { let mut a_access = self.a.initialize(world); let b_access = self.b.initialize(world); diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index cc2cd12969579..db10dd3bab4e1 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -176,22 +176,6 @@ const _: () = { ); } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &bevy_ecs::system::SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller - unsafe { - <__StructFieldsAlias as bevy_ecs::system::SystemParam>::validate_param( - &mut state.state, - system_meta, - world, - ) - } - } - #[inline] #[track_caller] unsafe fn get_param<'w, 's>( @@ -199,7 +183,7 @@ const _: () = { system_meta: &bevy_ecs::system::SystemMeta, world: UnsafeWorldCell<'w>, change_tick: bevy_ecs::change_detection::Tick, - ) -> Self::Item<'w, 's> { + ) -> Result, SystemParamValidationError> { // SAFETY: Upheld by caller let params = unsafe { <__StructFieldsAlias as bevy_ecs::system::SystemParam>::get_param( @@ -207,13 +191,13 @@ const _: () = { system_meta, world, change_tick, - ) + )? }; - Commands { + Ok(Commands { queue: InternalQueue::CommandQueue(params.0), allocator: params.1, entities: params.2, - } + }) } } // SAFETY: Only reads Entities diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index b1e9de0b11ea5..7948ef197908c 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -15,7 +15,7 @@ use bevy_utils::prelude::DebugName; use core::marker::PhantomData; use variadics_please::all_tuples; -use super::{RunSystemError, SystemParamValidationError, SystemStateFlags}; +use super::{RunSystemError, SystemStateFlags}; /// A function system that runs with exclusive [`World`] access. /// @@ -117,7 +117,7 @@ where let params = F::Param::get_param( self.param_state.as_mut().expect(PARAM_MESSAGE), &self.system_meta, - ); + )?; #[cfg(feature = "hotpatching")] let out = { @@ -166,15 +166,6 @@ where // might have buffers to apply, but this is handled by `PipeSystem`. } - #[inline] - unsafe fn validate_param_unsafe( - &mut self, - _world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // All exclusive system params are always available. - Ok(()) - } - #[inline] fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX); diff --git a/crates/bevy_ecs/src/system/exclusive_system_param.rs b/crates/bevy_ecs/src/system/exclusive_system_param.rs index 89bd1b0d8bcfe..7950c311bd6e7 100644 --- a/crates/bevy_ecs/src/system/exclusive_system_param.rs +++ b/crates/bevy_ecs/src/system/exclusive_system_param.rs @@ -1,7 +1,7 @@ use crate::{ prelude::{FromWorld, QueryState}, query::{QueryData, QueryFilter}, - system::{Local, SystemMeta, SystemParam, SystemState}, + system::{Local, SystemMeta, SystemParam, SystemParamValidationError, SystemState}, world::World, }; use bevy_platform::cell::SyncCell; @@ -27,7 +27,10 @@ pub trait ExclusiveSystemParam: Sized { /// Creates a parameter to be passed into an [`ExclusiveSystemParamFunction`]. /// /// [`ExclusiveSystemParamFunction`]: super::ExclusiveSystemParamFunction - fn get_param<'s>(state: &'s mut Self::State, system_meta: &SystemMeta) -> Self::Item<'s>; + fn get_param<'s>( + state: &'s mut Self::State, + system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError>; } /// Shorthand way of accessing the associated type [`ExclusiveSystemParam::Item`] @@ -44,8 +47,11 @@ impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> ExclusiveSystemParam QueryState::new(world) } - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - state + fn get_param<'s>( + state: &'s mut Self::State, + _system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Ok(state) } } @@ -57,8 +63,11 @@ impl<'a, P: SystemParam + 'static> ExclusiveSystemParam for &'a mut SystemState< SystemState::new(world) } - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - state + fn get_param<'s>( + state: &'s mut Self::State, + _system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Ok(state) } } @@ -70,8 +79,11 @@ impl<'_s, T: FromWorld + Send + 'static> ExclusiveSystemParam for Local<'_s, T> SyncCell::new(T::from_world(world)) } - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - Local(state.get()) + fn get_param<'s>( + state: &'s mut Self::State, + _system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Ok(Local(state.get())) } } @@ -81,8 +93,11 @@ impl ExclusiveSystemParam for PhantomData { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {} - fn get_param<'s>(_state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - PhantomData + fn get_param<'s>( + _state: &'s mut Self::State, + _system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Ok(PhantomData) } } @@ -115,13 +130,13 @@ macro_rules! impl_exclusive_system_param_tuple { fn get_param<'s>( state: &'s mut Self::State, system_meta: &SystemMeta, - ) -> Self::Item<'s> { + ) -> Result, SystemParamValidationError> { let ($($param,)*) = state; #[allow( clippy::unused_unit, reason = "Zero-length tuples won't have any params to get." )] - ($($param::get_param($param, system_meta),)*) + Ok(($($param::get_param($param, system_meta)?,)*)) } } }; diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 1341224fc906f..c5f070f309764 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -183,7 +183,7 @@ impl SystemMeta { /// /// // Use system_state.get_mut(&mut world) and unpack your system parameters into variables! /// // system_state.get(&world) provides read-only versions of your system parameters instead. -/// let (message_writer, maybe_resource, query) = system_state.get_mut(&mut world); +/// let (message_writer, maybe_resource, query) = system_state.get_mut(&mut world).unwrap(); /// /// // If you are using `Commands`, you can choose when you want to apply them to the world. /// // You need to manually call `.apply(world)` on the `SystemState` to apply them. @@ -213,7 +213,7 @@ impl SystemMeta { /// /// // Later, fetch the cached system state, saving on overhead /// world.resource_scope(|world, mut cached_state: Mut| { -/// let mut message_reader = cached_state.message_state.get_mut(world); +/// let mut message_reader = cached_state.message_state.get_mut(world).unwrap(); /// /// for message in message_reader.read() { /// println!("Hello World!"); @@ -229,7 +229,7 @@ impl SystemMeta { /// # struct MyMessage; /// # /// fn exclusive_system(world: &mut World, system_state: &mut SystemState>) { -/// let mut message_reader = system_state.get_mut(world); +/// let mut message_reader = system_state.get_mut(world).unwrap(); /// /// for message in message_reader.read() { /// println!("Hello World!"); @@ -367,8 +367,13 @@ impl SystemState { } /// Retrieve the [`SystemParam`] values. This can only be called when all parameters are read-only. + /// + /// Returns an error if system parameter validation fails. #[inline] - pub fn get<'w, 's>(&'s mut self, world: &'w World) -> SystemParamItem<'w, 's, Param> + pub fn get<'w, 's>( + &'s mut self, + world: &'w World, + ) -> Result, SystemParamValidationError> where Param: ReadOnlySystemParam, { @@ -379,9 +384,14 @@ impl SystemState { } /// Retrieve the mutable [`SystemParam`] values. + /// + /// Returns an error if system parameter validation fails. #[inline] #[track_caller] - pub fn get_mut<'w, 's>(&'s mut self, world: &'w mut World) -> SystemParamItem<'w, 's, Param> { + pub fn get_mut<'w, 's>( + &'s mut self, + world: &'w mut World, + ) -> Result, SystemParamValidationError> { self.validate_world(world.id()); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. unsafe { self.get_unchecked(world.as_unsafe_world_cell()) } @@ -395,21 +405,6 @@ impl SystemState { 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 `component_access_set`. - /// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state). - pub unsafe fn validate_param( - state: &mut Self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Delegated to existing `SystemParam` implementations. - unsafe { Param::validate_param(&mut state.param_state, &state.meta, world) } - } - /// Returns `true` if `world_id` matches the [`World`] that was used to call [`SystemState::new`]. /// Otherwise, this returns false. #[inline] @@ -435,6 +430,8 @@ impl SystemState { /// Retrieve the [`SystemParam`] values. /// + /// Returns an error if system parameter validation fails. + /// /// # Safety /// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data /// access is safe in the context of global [`World`] access. The passed-in [`World`] _must_ be the [`World`] the [`SystemState`] was @@ -444,7 +441,7 @@ impl SystemState { pub unsafe fn get_unchecked<'w, 's>( &'s mut self, world: UnsafeWorldCell<'w>, - ) -> SystemParamItem<'w, 's, Param> { + ) -> Result, SystemParamValidationError> { let change_tick = world.increment_change_tick(); // SAFETY: The invariants are upheld by the caller. unsafe { self.fetch(world, change_tick) } @@ -460,12 +457,12 @@ impl SystemState { &'s mut self, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> SystemParamItem<'w, 's, Param> { + ) -> Result, SystemParamValidationError> { // SAFETY: The invariants are upheld by the caller. let param = - unsafe { Param::get_param(&mut self.param_state, &self.meta, world, change_tick) }; + unsafe { Param::get_param(&mut self.param_state, &self.meta, world, change_tick) }?; self.meta.last_run = change_tick; - param + Ok(param) } /// Returns a reference to the current system param states. @@ -523,7 +520,7 @@ struct FunctionSystemState { /// The cached state of the system's [`SystemParam`]s. param: P::State, /// The id of the [`World`] this system was initialized with. If the world - /// passed to [`System::run_unsafe`] or [`System::validate_param_unsafe`] does not match + /// passed to [`System::run_unsafe`] does not match /// this id, a panic will occur. world_id: WorldId, } @@ -680,8 +677,9 @@ where // - The above assert ensures the world matches. // - All world accesses used by `F::Param` have been registered, so the caller // will ensure that there are no data access conflicts. - let params = - unsafe { F::Param::get_param(&mut state.param, &self.system_meta, world, change_tick) }; + let params = unsafe { + F::Param::get_param(&mut state.param, &self.system_meta, world, change_tick) + }?; #[cfg(feature = "hotpatching")] let out = { @@ -723,20 +721,6 @@ where F::Param::queue(param_state, &self.system_meta, world); } - #[inline] - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - let state = self.state.as_mut().expect(Self::ERROR_UNINITIALIZED); - assert_eq!(state.world_id, world.id(), "Encountered a mismatched World. A System cannot be used with Worlds other than the one it was initialized with."); - // SAFETY: - // - The above assert ensures the world matches. - // - All world accesses used by `F::Param` have been registered, so the caller - // will ensure that there are no data access conflicts. - unsafe { F::Param::validate_param(&mut state.param, &self.system_meta, world) } - } - #[inline] fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { if let Some(state) = &self.state { diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 79fbdbd05c4a1..91f7ab26329f5 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1318,7 +1318,7 @@ mod tests { Option>, ParamSet<(Query<&C>, Query<&D>)>, )> = SystemState::new(&mut world); - let (a, query, _) = system_state.get(&world); + let (a, query, _) = system_state.get(&world).unwrap(); assert_eq!(*a, A(42), "returned resource matches initial value"); assert_eq!( **query.unwrap(), @@ -1345,7 +1345,7 @@ mod tests { // The following line shouldn't compile because the parameters used are not ReadOnlySystemParam // let (a, query) = system_state.get(&world); - let (a, query) = system_state.get_mut(&mut world); + let (a, query) = system_state.get_mut(&mut world).unwrap(); assert_eq!(*a, A(42), "returned resource matches initial value"); assert_eq!( **query.unwrap(), @@ -1365,18 +1365,18 @@ mod tests { let mut system_state: SystemState>>> = SystemState::new(&mut world); { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!(**query.unwrap(), A(1)); } { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert!(query.is_none()); } world.entity_mut(entity).get_mut::().unwrap().0 = 2; { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!(**query.unwrap(), A(2)); } } @@ -1390,12 +1390,12 @@ mod tests { let mut system_state: SystemState>> = SystemState::new(&mut world); { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!(query.unwrap().1.spawn_tick(), spawn_tick); } { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert!(query.is_none()); } } @@ -1406,7 +1406,7 @@ mod tests { let mut world = World::default(); let mut system_state = SystemState::>::new(&mut world); let mismatched_world = World::default(); - system_state.get(&mismatched_world); + system_state.get(&mismatched_world).unwrap(); } #[test] @@ -1422,7 +1422,7 @@ mod tests { let mut system_state = SystemState::>::new(&mut world); { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!( query.iter().collect::>(), vec![&A(1)], @@ -1432,7 +1432,7 @@ mod tests { world.spawn((A(2), B(2))); { - let query = system_state.get(&world); + let query = system_state.get(&world).unwrap(); assert_eq!( query.iter().collect::>(), vec![&A(1), &A(2)], @@ -1462,19 +1462,19 @@ mod tests { impl State { fn hold_res<'w>(&mut self, world: &'w World) -> ResourceHolder<'w> { - let a = self.state.get(world); + let a = self.state.get(world).unwrap(); ResourceHolder { value: a.into_inner(), } } fn hold_component<'w>(&mut self, world: &'w World, entity: Entity) -> Holder<'w> { - let q = self.state_q.get(world); + let q = self.state_q.get(world).unwrap(); let a = q.get_inner(entity).unwrap(); Holder { value: a } } fn hold_components<'w>(&mut self, world: &'w World) -> Vec> { let mut components = Vec::new(); - let q = self.state_q.get(world); + let q = self.state_q.get(world).unwrap(); for a in q.iter_inner() { components.push(Holder { value: a }); } @@ -1494,7 +1494,7 @@ mod tests { let mut system_state = SystemState::>::new(&mut world); { - let mut query = system_state.get_mut(&mut world); + let mut query = system_state.get_mut(&mut world).unwrap(); assert_eq!( query.iter_mut().map(|m| *m).collect::>(), vec![A(1), A(2)], diff --git a/crates/bevy_ecs/src/system/schedule_system.rs b/crates/bevy_ecs/src/system/schedule_system.rs index 8116f6c06b400..4b555072f677e 100644 --- a/crates/bevy_ecs/src/system/schedule_system.rs +++ b/crates/bevy_ecs/src/system/schedule_system.rs @@ -8,7 +8,7 @@ use crate::{ world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FromWorld, World}, }; -use super::{IntoSystem, SystemParamValidationError, SystemStateFlags}; +use super::{IntoSystem, SystemStateFlags}; /// See [`IntoSystem::with_input`] for details. pub struct WithInputWrapper @@ -84,14 +84,6 @@ where self.system.queue_deferred(world); } - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller - unsafe { self.system.validate_param_unsafe(world) } - } - fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { self.system.initialize(world) } @@ -183,14 +175,6 @@ where self.system.queue_deferred(world); } - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller - unsafe { self.system.validate_param_unsafe(world) } - } - fn initialize(&mut self, world: &mut World) -> FilteredAccessSet { if self.value.is_none() { self.value = Some(T::from_world(world)); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index aff628fa322e9..cf48d0d9c8843 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -132,9 +132,6 @@ pub trait System: Send + Sync + 'static { let world_cell = world.as_unsafe_world_cell(); // SAFETY: // - We have exclusive access to the entire world. - unsafe { self.validate_param_unsafe(world_cell) }?; - // SAFETY: - // - We have exclusive access to the entire world. unsafe { self.run_unsafe(input, world_cell) } } @@ -147,36 +144,6 @@ pub trait System: Send + Sync + 'static { /// of this system into the world's command buffer. 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 its safe variant - /// is not a strict requirement, both [`System::run`] and [`System::run_unsafe`] - /// should provide their own safety mechanism to prevent undefined behavior. - /// - /// This method has to be called directly before [`System::run_unsafe`] with no other (relevant) - /// world mutations in between. Otherwise, while it won't lead to any undefined behavior, - /// the validity of the param may change. - /// - /// # Safety - /// - /// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data - /// registered in the access returned from [`System::initialize`]. There must be no conflicting - /// simultaneous accesses while the system is running. - unsafe fn validate_param_unsafe( - &mut self, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError>; - - /// Safe version of [`System::validate_param_unsafe`]. - /// that runs on exclusive, single-threaded `world` pointer. - fn validate_param(&mut self, world: &World) -> Result<(), SystemParamValidationError> { - let world_cell = world.as_unsafe_world_cell_readonly(); - // SAFETY: - // - We have exclusive access to the entire world. - unsafe { self.validate_param_unsafe(world_cell) } - } - /// Initialize the system. /// /// Returns a [`FilteredAccessSet`] with the access required to run the system. @@ -237,9 +204,6 @@ pub unsafe trait ReadOnlySystem: System { let world = world.as_unsafe_world_cell_readonly(); // SAFETY: // - We have read-only access to the entire world. - unsafe { self.validate_param_unsafe(world) }?; - // SAFETY: - // - We have read-only access to the entire world. unsafe { self.run_unsafe(input, world) } } } diff --git a/crates/bevy_ecs/src/system/system_name.rs b/crates/bevy_ecs/src/system/system_name.rs index af4fc91f3c9a2..ea74ff1558931 100644 --- a/crates/bevy_ecs/src/system/system_name.rs +++ b/crates/bevy_ecs/src/system/system_name.rs @@ -2,7 +2,10 @@ use crate::{ change_detection::Tick, prelude::World, query::FilteredAccessSet, - system::{ExclusiveSystemParam, ReadOnlySystemParam, SystemMeta, SystemParam}, + system::{ + ExclusiveSystemParam, ReadOnlySystemParam, SystemMeta, SystemParam, + SystemParamValidationError, + }, world::unsafe_world_cell::UnsafeWorldCell, }; use bevy_utils::prelude::DebugName; @@ -65,8 +68,8 @@ unsafe impl SystemParam for SystemName { system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - SystemName(system_meta.name.clone()) + ) -> Result, SystemParamValidationError> { + Ok(SystemName(system_meta.name.clone())) } } @@ -79,8 +82,11 @@ impl ExclusiveSystemParam for SystemName { fn init(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {} - fn get_param<'s>(_state: &'s mut Self::State, system_meta: &SystemMeta) -> Self::Item<'s> { - SystemName(system_meta.name.clone()) + fn get_param<'s>( + _state: &'s mut Self::State, + system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Ok(SystemName(system_meta.name.clone())) } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 976ee8afff9ca..53a7c56b98e16 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -15,7 +15,6 @@ use crate::{ QuerySingleError, QueryState, ReadOnlyQueryData, }, resource::{Resource, IS_RESOURCE}, - storage::NonSendData, system::{Query, Single, SystemMeta}, world::{ unsafe_world_cell::UnsafeWorldCell, DeferredWorld, FilteredResources, FilteredResourcesMut, @@ -257,53 +256,17 @@ pub unsafe trait SystemParam: Sized { )] fn queue(state: &mut Self::State, system_meta: &SystemMeta, world: DeferredWorld) {} - /// Validates that the param can be acquired by the [`get_param`](SystemParam::get_param). + /// Creates a parameter to be passed into a [`SystemParamFunction`](super::SystemParamFunction). /// - /// Built-in executors use this to prevent systems with invalid params from running, - /// and any failures here will be bubbled up to the default error handler defined in [`bevy_ecs::error`], - /// with a value of type [`SystemParamValidationError`]. + /// This method also validates that the param can be acquired. If validation fails, + /// an appropriate [`SystemParamValidationError`] should be returned. + /// Systems will convert this to a [`RunSystemError`](super::RunSystemError), + /// and the built-in executors will ignore any "skipped" validation results, + /// but pass any "invalid" results to the default error handler defined in [`bevy_ecs::error`]. /// /// For nested [`SystemParam`]s validation will fail if any /// delegated validation fails. /// - /// 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. - /// - /// When using system parameters that require `change_tick` you can use - /// [`UnsafeWorldCell::change_tick()`]. Even if this isn't the exact - /// same tick used for [`SystemParam::get_param`], the world access - /// ensures that the queried data will be the same in both calls. - /// - /// This method has to be called directly before [`SystemParam::get_param`] with no other (relevant) - /// world mutations inbetween. Otherwise, while it won't lead to any undefined behavior, - /// the validity of the param may change. - /// - /// [`System::validate_param`](super::system::System::validate_param), - /// calls this method for each supplied system param. - /// - /// # Safety - /// - /// - The passed [`UnsafeWorldCell`] must have read-only access to world data - /// registered in [`init_access`](SystemParam::init_access). - /// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state). - #[expect( - unused_variables, - reason = "The parameters here are intentionally unused by the default implementation; however, putting underscores here will result in the underscores being copied by rust-analyzer's tab completion." - )] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - Ok(()) - } - - /// Creates a parameter to be passed into a [`SystemParamFunction`](super::SystemParamFunction). - /// /// # Safety /// /// - The passed [`UnsafeWorldCell`] must have access to any world data registered @@ -317,7 +280,7 @@ pub unsafe trait SystemParam: Sized { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state>; + ) -> Result, SystemParamValidationError>; } /// A [`SystemParam`] that only reads a given [`World`]. @@ -363,12 +326,12 @@ unsafe impl SystemParam for Qu system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Result, SystemParamValidationError> { // SAFETY: We have registered all of the query's world accesses, // so the caller ensures that `world` has permission to access any // world data that the query needs. // The caller ensures the world matches the one used in init_state. - unsafe { state.query_unchecked_with_ticks(world, system_meta.last_run, change_tick) } + Ok(unsafe { state.query_unchecked_with_ticks(world, system_meta.last_run, change_tick) }) } } @@ -399,34 +362,16 @@ unsafe impl<'a, 'b, D: IterQueryData + 'static, F: QueryFilter + 'static> System system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Result, SystemParamValidationError> { // SAFETY: State ensures that the components it accesses are not accessible somewhere elsewhere. // The caller ensures the world matches the one used in init_state. let query = unsafe { state.query_unchecked_with_ticks(world, system_meta.last_run, change_tick) }; - let single = query - .single_inner() - .expect("The query was expected to contain exactly one matching entity."); - Single { - item: single, - _filter: PhantomData, - } - } - - #[inline] - unsafe fn validate_param( - state: &mut 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_with_ticks(world, system_meta.last_run, world.change_tick()) - }; match query.single_inner() { - Ok(_) => Ok(()), + Ok(single) => Ok(Single { + item: single, + _filter: PhantomData, + }), Err(QuerySingleError::NoEntities(_)) => Err( SystemParamValidationError::skipped::("No matching entities"), ), @@ -470,30 +415,15 @@ unsafe impl SystemParam system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Result, SystemParamValidationError> { // SAFETY: Delegate to existing `SystemParam` implementations. - let query = unsafe { Query::get_param(state, system_meta, world, change_tick) }; - Populated(query) - } - - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: - // - We have read-only access to the components accessed by query. - // - The caller ensures the world matches the one used in init_state. - let query = unsafe { - state.query_unchecked_with_ticks(world, system_meta.last_run, world.change_tick()) - }; + let query = unsafe { Query::get_param(state, system_meta, world, change_tick) }?; if query.is_empty() { Err(SystemParamValidationError::skipped::( "No matching entities", )) } else { - Ok(()) + Ok(Populated(query)) } } } @@ -682,31 +612,29 @@ macro_rules! impl_param_set { <($($param,)*) as SystemParam>::queue(state, system_meta, world.reborrow()); } - #[inline] - unsafe fn validate_param<'w, 's>( - state: &'s mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell<'w>, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller - unsafe { - <($($param,)*) as SystemParam>::validate_param(state, system_meta, world) - } - } - #[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> { - ParamSet { + ) -> Result, SystemParamValidationError> { + // Validate each sub-param eagerly so that the system is correctly + // skipped by the executor when any sub-param is unavailable. + // PERF: the sub-params will be fetched again lazily when accessed through + // the ParamSet, but this is no worse than the previous + // validate_param + get_param pattern. + $( + // SAFETY: Upheld by caller. + drop(unsafe { $param::get_param(&mut state.$index, system_meta, world, change_tick) }?); + )* + + Ok(ParamSet { param_states: state, system_meta: system_meta.clone(), world, change_tick, - } + }) } } @@ -724,6 +652,7 @@ macro_rules! impl_param_set { unsafe { $param::get_param(&mut self.param_states.$index, &self.system_meta, self.world, self.change_tick) } + .unwrap_or_else(|err| panic!("ParamSet parameter validation failed: {err}")) } )* } @@ -769,42 +698,17 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { panic!("error[B0002]: Res<{}> in system {} conflicts with a previous system parameter. Consider removing the duplicate access using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevy.org/learn/errors/b0002", DebugName::type_name::(), system_meta.name); } - #[inline] - unsafe fn validate_param( - &mut component_id: &mut Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Read-only access to the resource - if let Some(entity) = unsafe { world.resource_entities() }.get(component_id) - && let Ok(entity_ref) = world.get_entity(*entity) - && entity_ref.contains_id(component_id) - { - Ok(()) - } else { - Err(SystemParamValidationError::invalid::( - "Resource does not exist", - )) - } - } - #[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> { - let (ptr, ticks) = world - .get_resource_with_ticks(component_id) - .unwrap_or_else(|| { - panic!( - "Resource requested by {} does not exist: {}", - system_meta.name, - DebugName::type_name::() - ); - }); - Res { + ) -> Result, SystemParamValidationError> { + let (ptr, ticks) = world.get_resource_with_ticks(component_id).ok_or_else(|| { + SystemParamValidationError::invalid::("Resource does not exist") + })?; + Ok(Res { value: ptr.deref(), ticks: ComponentTicksRef { added: ticks.added.deref(), @@ -813,7 +717,7 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { last_run: system_meta.last_run, this_run: change_tick, }, - } + }) } } @@ -851,42 +755,17 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { panic!("error[B0002]: ResMut<{}> in system {} conflicts with a previous system parameter. Consider removing the duplicate access or using `Without` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevy.org/learn/errors/b0002", DebugName::type_name::(), system_meta.name); } - #[inline] - unsafe fn validate_param( - &mut component_id: &mut Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Read-only access to the resource. - if let Some(entity) = unsafe { world.resource_entities() }.get(component_id) - && let Ok(entity_ref) = world.get_entity(*entity) - && entity_ref.contains_id(component_id) - { - Ok(()) - } else { - Err(SystemParamValidationError::invalid::( - "Resource does not exist", - )) - } - } - #[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> { - let value = world - .get_resource_mut_by_id(component_id) - .unwrap_or_else(|| { - panic!( - "Resource requested by {} does not exist: {}", - system_meta.name, - DebugName::type_name::() - ); - }); - ResMut { + ) -> Result, SystemParamValidationError> { + let value = world.get_resource_mut_by_id(component_id).ok_or_else(|| { + SystemParamValidationError::invalid::("Resource does not exist") + })?; + Ok(ResMut { value: value.value.deref_mut::(), ticks: ComponentTicksMut { added: value.ticks.added, @@ -895,7 +774,7 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { last_run: system_meta.last_run, this_run: change_tick, }, - } + }) } } @@ -933,9 +812,9 @@ unsafe impl SystemParam for &'_ World { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Result, SystemParamValidationError> { // SAFETY: Read-only access to the entire world was registered in `init_state`. - unsafe { world.world() } + Ok(unsafe { world.world() }) } } @@ -965,9 +844,9 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> { _system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, _change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Result, SystemParamValidationError> { // SAFETY: Upheld by caller - unsafe { world.into_deferred() } + Ok(unsafe { world.into_deferred() }) } } @@ -1156,8 +1035,8 @@ unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - Local(state.get()) + ) -> Result, SystemParamValidationError> { + Ok(Local(state.get())) } } @@ -1360,8 +1239,8 @@ unsafe impl SystemParam for Deferred<'_, T> { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - Deferred(state.get()) + ) -> Result, SystemParamValidationError> { + Ok(Deferred(state.get())) } } @@ -1391,8 +1270,8 @@ unsafe impl SystemParam for ExclusiveMarker { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'world>, _change_tick: Tick, - ) -> Self::Item<'world, 'state> { - Self(PhantomData) + ) -> Result, SystemParamValidationError> { + Ok(Self(PhantomData)) } } @@ -1425,8 +1304,8 @@ unsafe impl SystemParam for NonSendMarker { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'world>, _change_tick: Tick, - ) -> Self::Item<'world, 'state> { - Self(PhantomData) + ) -> Result, SystemParamValidationError> { + Ok(Self(PhantomData)) } } @@ -1464,46 +1343,20 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { component_access_set.add_unfiltered_component_read(component_id); } - #[inline] - unsafe fn validate_param( - &mut component_id: &mut Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Read-only access to non-send metadata. - if unsafe { world.storages() } - .non_sends - .get(component_id) - .is_some_and(NonSendData::is_present) - { - Ok(()) - } else { - Err(SystemParamValidationError::invalid::( - "Non-send resource does not exist", - )) - } - } - #[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> { - let (ptr, ticks) = world - .get_non_send_with_ticks(component_id) - .unwrap_or_else(|| { - panic!( - "Non-send resource requested by {} does not exist: {}", - system_meta.name, - DebugName::type_name::() - ); - }); - NonSend { + ) -> Result, SystemParamValidationError> { + let (ptr, ticks) = world.get_non_send_with_ticks(component_id).ok_or_else(|| { + SystemParamValidationError::invalid::("Non-send data not found") + })?; + Ok(NonSend { value: ptr.deref(), ticks: ComponentTicksRef::from_tick_cells(ticks, system_meta.last_run, change_tick), - } + }) } } @@ -1538,46 +1391,20 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { component_access_set.add_unfiltered_component_write(component_id); } - #[inline] - unsafe fn validate_param( - &mut component_id: &mut Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Read-only access to non-send metadata. - if unsafe { world.storages() } - .non_sends - .get(component_id) - .is_some_and(NonSendData::is_present) - { - Ok(()) - } else { - Err(SystemParamValidationError::invalid::( - "Non-send resource does not exist", - )) - } - } - #[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> { - let (ptr, ticks) = world - .get_non_send_with_ticks(component_id) - .unwrap_or_else(|| { - panic!( - "Non-send resource requested by {} does not exist: {}", - system_meta.name, - DebugName::type_name::() - ); - }); - NonSendMut { + ) -> Result, SystemParamValidationError> { + let (ptr, ticks) = world.get_non_send_with_ticks(component_id).ok_or_else(|| { + SystemParamValidationError::invalid::("Non-send data not found") + })?; + Ok(NonSendMut { value: ptr.assert_unique().deref_mut(), ticks: ComponentTicksMut::from_tick_cells(ticks, system_meta.last_run, change_tick), - } + }) } } @@ -1605,8 +1432,8 @@ unsafe impl<'a> SystemParam for &'a Archetypes { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.archetypes() + ) -> Result, SystemParamValidationError> { + Ok(world.archetypes()) } } @@ -1634,8 +1461,8 @@ unsafe impl<'a> SystemParam for &'a Components { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.components() + ) -> Result, SystemParamValidationError> { + Ok(world.components()) } } @@ -1663,8 +1490,8 @@ unsafe impl<'a> SystemParam for &'a Entities { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.entities() + ) -> Result, SystemParamValidationError> { + Ok(world.entities()) } } @@ -1692,8 +1519,8 @@ unsafe impl<'a> SystemParam for &'a EntityAllocator { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.entity_allocator() + ) -> Result, SystemParamValidationError> { + Ok(world.entity_allocator()) } } @@ -1721,8 +1548,8 @@ unsafe impl<'a> SystemParam for &'a Bundles { _system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, _change_tick: Tick, - ) -> Self::Item<'w, 's> { - world.bundles() + ) -> Result, SystemParamValidationError> { + Ok(world.bundles()) } } @@ -1779,11 +1606,11 @@ unsafe impl SystemParam for SystemChangeTick { system_meta: &SystemMeta, _world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { - SystemChangeTick { + ) -> Result, SystemParamValidationError> { + Ok(SystemChangeTick { last_run: system_meta.last_run, this_run: change_tick, - } + }) } } @@ -1812,13 +1639,9 @@ unsafe impl SystemParam for Option { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Result, SystemParamValidationError> { // SAFETY: Upheld by caller - unsafe { - T::validate_param(state, system_meta, world) - .ok() - .map(|()| T::get_param(state, system_meta, world, change_tick)) - } + Ok(unsafe { T::get_param(state, system_meta, world, change_tick) }.ok()) } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -1858,12 +1681,9 @@ unsafe impl SystemParam for Result, change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Result, SystemParamValidationError> { // SAFETY: Upheld by caller - unsafe { - T::validate_param(state, system_meta, world) - .map(|()| T::get_param(state, system_meta, world, change_tick)) - } + Ok(unsafe { T::get_param(state, system_meta, world, change_tick) }) } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -1950,28 +1770,20 @@ unsafe impl SystemParam for If { T::init_access(state, system_meta, component_access_set, world); } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller - unsafe { T::validate_param(state, system_meta, world) }.map_err(|mut e| { - e.skipped = true; - e - }) - } - #[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> { + ) -> Result, SystemParamValidationError> { // SAFETY: Upheld by caller. - If(unsafe { T::get_param(state, system_meta, world, change_tick) }) + unsafe { T::get_param(state, system_meta, world, change_tick) } + .map(If) + .map_err(|mut e| { + e.skipped = true; + e + }) } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -2008,26 +1820,13 @@ unsafe impl SystemParam for Vec { } } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - for state in state { - // SAFETY: Upheld by caller - unsafe { T::validate_param(state, system_meta, world)? }; - } - Ok(()) - } - #[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> { + ) -> Result, SystemParamValidationError> { state .iter_mut() // SAFETY: @@ -2088,13 +1887,23 @@ unsafe impl SystemParam for ParamSet<'_, '_, Vec> { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { - ParamSet { + ) -> Result, SystemParamValidationError> { + // Validate each sub-param eagerly so that the system is correctly + // skipped by the executor when any sub-param is unavailable. + // PERF: the sub-params will be fetched again lazily when accessed through + // the ParamSet, but this is no worse than the previous + // validate_param + get_param pattern. + for s in state.iter_mut() { + // SAFETY: Upheld by caller. + drop(unsafe { T::get_param(s, system_meta, world, change_tick) }?); + } + + Ok(ParamSet { param_states: state, system_meta: system_meta.clone(), world, change_tick, - } + }) } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -2125,6 +1934,7 @@ impl ParamSet<'_, '_, Vec> { self.world, self.change_tick, ) + .unwrap() } } @@ -2136,7 +1946,8 @@ impl ParamSet<'_, '_, Vec> { // - We initialized the access for each parameter, so the caller ensures we have access to any world data needed by any param. // We have mutable access to the ParamSet, so no other params in the set are active. // - The caller of `get_param` ensured that this was the world used to initialize our state, and we used that world to initialize parameter states - unsafe { T::get_param(state, &self.system_meta, self.world, self.change_tick) }, + unsafe { T::get_param(state, &self.system_meta, self.world, self.change_tick) } + .unwrap_or_else(|err| panic!("ParamSet parameter validation failed: {err}")), ); }); } @@ -2192,27 +2003,6 @@ macro_rules! impl_system_param_tuple { $($param::queue($param, system_meta, world.reborrow());)* } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - let ($($param,)*) = state; - - #[allow( - unused_unsafe, - reason = "Zero-length tuples won't have any params to validate." - )] - // SAFETY: Upheld by caller - unsafe { - $( - $param::validate_param($param, system_meta, world)?; - )* - } - Ok(()) - } - #[inline] #[track_caller] unsafe fn get_param<'w, 's>( @@ -2220,7 +2010,7 @@ macro_rules! impl_system_param_tuple { system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, - ) -> Self::Item<'w, 's> { + ) -> Result, SystemParamValidationError> { let ($($param,)*) = state; #[allow( @@ -2233,7 +2023,7 @@ macro_rules! impl_system_param_tuple { clippy::unused_unit, reason = "Zero-length tuples won't have any params to get." )] - ($($param::get_param($param, system_meta, world, change_tick),)*) + Ok(($($param::get_param($param, system_meta, world, change_tick)?,)*)) } } } @@ -2381,25 +2171,15 @@ unsafe impl SystemParam for StaticSystemParam<'_, '_, P::queue(state, system_meta, world); } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller - unsafe { P::validate_param(state, system_meta, world) } - } - #[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> { + ) -> Result, SystemParamValidationError> { // SAFETY: Defer to the safety of P::SystemParam - StaticSystemParam(unsafe { P::get_param(state, system_meta, world, change_tick) }) + unsafe { P::get_param(state, system_meta, world, change_tick) }.map(StaticSystemParam) } } @@ -2424,8 +2204,8 @@ unsafe impl SystemParam for PhantomData { _system_meta: &SystemMeta, _world: UnsafeWorldCell<'world>, _change_tick: Tick, - ) -> Self::Item<'world, 'state> { - PhantomData + ) -> Result, SystemParamValidationError> { + Ok(PhantomData) } } @@ -2599,12 +2379,12 @@ where { state .downcast_mut::>>() - .map(|state| { + .and_then(|state| { // SAFETY: // - The caller ensures the world has access for the underlying system param, // and since the downcast succeeded, the underlying system param is T. // - The caller ensures the `world` matches. - unsafe { T::Item::get_param(&mut state.0, system_meta, world, change_tick) } + unsafe { T::Item::get_param(&mut state.0, system_meta, world, change_tick) }.ok() }) } @@ -2636,14 +2416,15 @@ trait DynParamState: Sync + Send + Any { world: &mut World, ); - /// Refer to [`SystemParam::validate_param`]. + /// Validates the inner parameter by calling [`SystemParam::get_param`] and discarding the value. /// /// # Safety - /// Refer to [`SystemParam::validate_param`]. - unsafe fn validate_param( + /// Refer to [`SystemParam::get_param`]. + unsafe fn validate( &mut self, system_meta: &SystemMeta, world: UnsafeWorldCell, + change_tick: Tick, ) -> Result<(), SystemParamValidationError>; } @@ -2668,13 +2449,14 @@ impl DynParamState for ParamState { T::init_access(&self.0, system_meta, component_access_set, world); } - unsafe fn validate_param( + unsafe fn validate( &mut self, system_meta: &SystemMeta, world: UnsafeWorldCell, + change_tick: Tick, ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller - unsafe { T::validate_param(&mut self.0, system_meta, world) } + // SAFETY: Upheld by caller. + unsafe { T::get_param(&mut self.0, system_meta, world, change_tick) }.map(drop) } } @@ -2699,29 +2481,25 @@ unsafe impl SystemParam for DynSystemParam<'_, '_> { .init_access(system_meta, component_access_set, world); } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Upheld by caller. - unsafe { state.0.validate_param(system_meta, world) } - } - #[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> { + ) -> Result, SystemParamValidationError> { + // Validate the inner parameter eagerly so that systems using DynSystemParam + // are correctly skipped by the executor when the inner param is unavailable. + // SAFETY: Upheld by caller. + unsafe { state.0.validate(system_meta, world, change_tick) }?; // SAFETY: // - `state.0` is a boxed `ParamState`. // - `init_access` calls `DynParamState::init_access`, which calls `init_access` on the inner parameter, // so the caller ensures the world has the necessary access. // - The caller ensures that the provided world is the same and has the required access. - unsafe { DynSystemParam::new(state.0.as_mut(), world, system_meta.clone(), change_tick) } + Ok(unsafe { + DynSystemParam::new(state.0.as_mut(), world, system_meta.clone(), change_tick) + }) } fn apply(state: &mut Self::State, system_meta: &SystemMeta, world: &mut World) { @@ -2769,10 +2547,10 @@ unsafe impl SystemParam for FilteredResources<'_, '_> { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Result, SystemParamValidationError> { // SAFETY: The caller ensures that `world` has access to anything registered in `init_access`, // and we registered all resource access in `state``. - unsafe { FilteredResources::new(world, state, system_meta.last_run, change_tick) } + Ok(unsafe { FilteredResources::new(world, state, system_meta.last_run, change_tick) }) } } @@ -2815,17 +2593,17 @@ unsafe impl SystemParam for FilteredResourcesMut<'_, '_> { system_meta: &SystemMeta, world: UnsafeWorldCell<'world>, change_tick: Tick, - ) -> Self::Item<'world, 'state> { + ) -> Result, SystemParamValidationError> { // SAFETY: The caller ensures that `world` has access to anything registered in `init_access`, // and we registered all resource access in `state``. - unsafe { FilteredResourcesMut::new(world, state, system_meta.last_run, change_tick) } + Ok(unsafe { FilteredResourcesMut::new(world, state, system_meta.last_run, change_tick) }) } } /// An error that occurs when a system parameter is not valid, /// used by system executors to determine what to do with a system. /// -/// Returned as an error from [`SystemParam::validate_param`], +/// Returned as an error from [`SystemParam::get_param`], /// and handled using the unified error handling mechanisms defined in [`bevy_ecs::error`]. #[derive(Debug, PartialEq, Eq, Clone, Error)] pub struct SystemParamValidationError { diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs index 3a097dd592c42..819f78c8c9c44 100644 --- a/crates/bevy_ecs/src/system/system_registry.rs +++ b/crates/bevy_ecs/src/system/system_registry.rs @@ -1015,7 +1015,7 @@ mod tests { let result = world.run_system(id); assert!(matches!(result, Err(RegisteredSystemError::Failed { .. }))); - let expected = "Resource does not exist"; + let expected = "does not exist"; let actual = result.unwrap_err().to_string(); assert!( diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index ea4a2b20bc74b..63b1a9ed6c11b 100644 --- a/crates/bevy_ecs/src/world/identifier.rs +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -2,7 +2,10 @@ use crate::{ change_detection::Tick, query::FilteredAccessSet, storage::SparseSetIndex, - system::{ExclusiveSystemParam, ReadOnlySystemParam, SystemMeta, SystemParam}, + system::{ + ExclusiveSystemParam, ReadOnlySystemParam, SystemMeta, SystemParam, + SystemParamValidationError, + }, world::{FromWorld, World}, }; use bevy_platform::sync::atomic::{AtomicUsize, Ordering}; @@ -70,8 +73,8 @@ unsafe impl SystemParam for WorldId { _: &SystemMeta, world: UnsafeWorldCell<'world>, _: Tick, - ) -> Self::Item<'world, 'state> { - world.id() + ) -> Result, SystemParamValidationError> { + Ok(world.id()) } } @@ -83,8 +86,11 @@ impl ExclusiveSystemParam for WorldId { world.id() } - fn get_param<'s>(state: &'s mut Self::State, _system_meta: &SystemMeta) -> Self::Item<'s> { - *state + fn get_param<'s>( + state: &'s mut Self::State, + _system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Ok(*state) } } diff --git a/crates/bevy_gizmos/src/gizmos.rs b/crates/bevy_gizmos/src/gizmos.rs index efe24bdfa35bd..02f13609e3470 100644 --- a/crates/bevy_gizmos/src/gizmos.rs +++ b/crates/bevy_gizmos/src/gizmos.rs @@ -228,44 +228,13 @@ where GizmosState::::queue(&mut state.state, system_meta, world); } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // SAFETY: Delegated to existing `SystemParam` implementation. - unsafe { - GizmosState::::validate_param(&mut state.state, system_meta, world)?; - } - - // SAFETY: Delegated to existing `SystemParam` implementation. - let (_, f1) = unsafe { - GizmosState::::get_param( - &mut state.state, - system_meta, - world, - world.change_tick(), - ) - }; - // This if-block is to accommodate an Option SystemParam. - // The user may decide not to initialize a gizmo group, so its config will not exist. - if f1.get_config::().is_none() { - Err(SystemParamValidationError::invalid::( - format!("Requested config {} does not exist in `GizmoConfigStore`! Did you forget to add it using `app.init_gizmo_group()`?", - Config::type_path()))) - } else { - Ok(()) - } - } - #[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> { + ) -> Result, SystemParamValidationError> { // SAFETY: Delegated to existing `SystemParam` implementations. let (mut f0, f1) = unsafe { GizmosState::::get_param( @@ -273,20 +242,27 @@ where system_meta, world, change_tick, - ) + )? }; // Accessing the GizmoConfigStore in every API call reduces performance significantly. // Implementing SystemParam manually allows us to cache whether the config is currently enabled. // Having this available allows for cheap early returns when gizmos are disabled. - let (config, config_ext) = f1.into_inner().config::(); + // + // We use `get_config` instead of `config` to accommodate `Option`: + // the user may decide not to initialize a gizmo group, so its config will not exist. + let (config, config_ext) = f1.into_inner().get_config::().ok_or_else(|| { + SystemParamValidationError::invalid::( + format!("Requested config {} does not exist in `GizmoConfigStore`! Did you forget to add it using `app.init_gizmo_group()`?", + Config::type_path())) + })?; f0.enabled = config.enabled; - Gizmos { + Ok(Gizmos { buffer: f0, config, config_ext, - } + }) } } diff --git a/crates/bevy_input_focus/src/tab_navigation.rs b/crates/bevy_input_focus/src/tab_navigation.rs index 26dfc613b7d4e..4ee868efdda38 100644 --- a/crates/bevy_input_focus/src/tab_navigation.rs +++ b/crates/bevy_input_focus/src/tab_navigation.rs @@ -478,7 +478,7 @@ mod tests { let tab_entity_2 = world.spawn((TabIndex(1), ChildOf(tab_group_entity))).id(); let mut system_state: SystemState = SystemState::new(world); - let tab_navigation = system_state.get(world); + let tab_navigation = system_state.get(world).unwrap(); assert_eq!(tab_navigation.tabgroup_query.iter().count(), 1); assert!(tab_navigation.tabindex_query.iter().count() >= 2); @@ -511,7 +511,7 @@ mod tests { let tab_entity_4 = world.spawn((TabIndex(1), ChildOf(tab_group_2))).id(); let mut system_state: SystemState = SystemState::new(world); - let tab_navigation = system_state.get(world); + let tab_navigation = system_state.get(world).unwrap(); assert_eq!(tab_navigation.tabgroup_query.iter().count(), 2); assert!(tab_navigation.tabindex_query.iter().count() >= 4); diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index 7c5f008f54541..7cbbd4a314ab1 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -929,7 +929,7 @@ pub(crate) fn specialize_material_meshes( mut specialized_material_pipeline_cache, mut pending_mesh_material_queues, dirty_specializations, - } = state.get_mut(world); + } = state.get_mut(world).unwrap(); for (view, visible_entities) in &views { all_views.insert(view.retained_view_entity); diff --git a/crates/bevy_pbr/src/meshlet/instance_manager.rs b/crates/bevy_pbr/src/meshlet/instance_manager.rs index a7f0e6c0c2820..9ae2b8237ac0a 100644 --- a/crates/bevy_pbr/src/meshlet/instance_manager.rs +++ b/crates/bevy_pbr/src/meshlet/instance_manager.rs @@ -220,7 +220,7 @@ pub fn extract_meshlet_mesh_entities( } let system_state = system_state.as_mut().unwrap(); let (instances_query, asset_server, mut assets, mut asset_events) = - system_state.get_mut(&mut main_world); + system_state.get_mut(&mut main_world).unwrap(); // Reset per-frame data instance_manager.reset(render_entities); diff --git a/crates/bevy_pbr/src/prepass/mod.rs b/crates/bevy_pbr/src/prepass/mod.rs index 0df4717eb3572..f41f2c44b15f1 100644 --- a/crates/bevy_pbr/src/prepass/mod.rs +++ b/crates/bevy_pbr/src/prepass/mod.rs @@ -909,7 +909,7 @@ pub(crate) fn specialize_prepass_material_meshes( mut pending_prepass_mesh_material_queues, dirty_specializations, this_run: system_change_tick, - } = state.get_mut(world); + } = state.get_mut(world).unwrap(); this_run = system_change_tick.this_run(); diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index f4157b4a12f05..d1cb7e27c853b 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -1942,7 +1942,7 @@ pub(crate) fn specialize_shadows( mut specialized_shadow_material_pipeline_cache, mut pending_shadow_queues, dirty_specializations, - } = state.get_mut(world); + } = state.get_mut(world).unwrap(); for (entity, view_lights) in &view_lights { for view_light_entity in view_lights.lights.iter().copied() { diff --git a/crates/bevy_render/src/erased_render_asset.rs b/crates/bevy_render/src/erased_render_asset.rs index 05d55ecc23745..8e530bbf63527 100644 --- a/crates/bevy_render/src/erased_render_asset.rs +++ b/crates/bevy_render/src/erased_render_asset.rs @@ -248,7 +248,7 @@ pub(crate) fn extract_erased_render_asset( ) { main_world.resource_scope( |world, mut cached_state: Mut>| { - let (mut events, mut assets) = cached_state.state.get_mut(world); + let (mut events, mut assets) = cached_state.state.get_mut(world).unwrap(); let mut needs_extracting = >::default(); let mut removed = >::default(); diff --git a/crates/bevy_render/src/extract_param.rs b/crates/bevy_render/src/extract_param.rs index 4d81fb9eb1903..0659cec965403 100644 --- a/crates/bevy_render/src/extract_param.rs +++ b/crates/bevy_render/src/extract_param.rs @@ -94,37 +94,13 @@ where ); } - #[inline] - unsafe fn validate_param( - state: &mut Self::State, - _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { - // 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 Err(SystemParamValidationError::invalid::( - "`MainWorld` resource does not exist", - )); - }; - // 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::

::validate_param( - &mut state.state, - main_world.as_unsafe_world_cell_readonly(), - ) - } - } - #[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> { + ) -> Result, SystemParamValidationError> { // SAFETY: // - The caller ensures that `world` is the same one that `init_state` was called with. // - The caller ensures that no other `SystemParam`s will conflict with the accesses we have registered. @@ -134,10 +110,10 @@ where system_meta, world, change_tick, - ) + )? }; - let item = state.state.get(main_world.into_inner()); - Extract { item } + let item = state.state.get(main_world.into_inner())?; + Ok(Extract { item }) } } diff --git a/crates/bevy_render/src/render_asset.rs b/crates/bevy_render/src/render_asset.rs index 8d90e5b5200b4..43ef2bb5807b5 100644 --- a/crates/bevy_render/src/render_asset.rs +++ b/crates/bevy_render/src/render_asset.rs @@ -262,7 +262,7 @@ pub(crate) fn extract_render_asset( ) { main_world.resource_scope( |world, mut cached_state: Mut>| { - let (mut events, mut assets, maybe_render_assets) = cached_state.state.get_mut(world); + let (mut events, mut assets, maybe_render_assets) = cached_state.state.get_mut(world).unwrap(); let mut needs_extracting = >::default(); let mut removed = >::default(); diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 319c907843f68..13d4f67da52eb 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -326,7 +326,7 @@ where view: Entity, item: &P, ) -> Result<(), DrawError> { - let param = self.state.get(world); + let param = self.state.get(world).unwrap(); let view = match self.view.get_manual(world, view) { Ok(view) => view, Err(err) => match err { diff --git a/crates/bevy_render/src/renderer/mod.rs b/crates/bevy_render/src/renderer/mod.rs index d19d29f0eb08a..38bb7f1ccc674 100644 --- a/crates/bevy_render/src/renderer/mod.rs +++ b/crates/bevy_render/src/renderer/mod.rs @@ -89,7 +89,7 @@ pub fn render_system( let _span = info_span!("present_frames").entered(); world.resource_scope(|world, mut windows: Mut| { - let views = state.get(world); + let views = state.get(world).unwrap(); for window in windows.values_mut() { let view_needs_present = views.iter().any(|(view_target, camera)| { matches!( diff --git a/crates/bevy_render/src/renderer/render_context.rs b/crates/bevy_render/src/renderer/render_context.rs index 5a21ada6f1fb2..0fa4ac1eb67f0 100644 --- a/crates/bevy_render/src/renderer/render_context.rs +++ b/crates/bevy_render/src/renderer/render_context.rs @@ -260,11 +260,12 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam } #[inline] - unsafe fn validate_param( - state: &mut Self::State, + unsafe fn get_param<'w, 's>( + state: &'s mut Self::State, _system_meta: &SystemMeta, - world: UnsafeWorldCell, - ) -> Result<(), SystemParamValidationError> { + world: UnsafeWorldCell<'w>, + _change_tick: Tick, + ) -> Result, SystemParamValidationError> { // SAFETY: We have registered resource read access in init_access let current_view = unsafe { world.get_resource::() }; @@ -278,47 +279,15 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam // SAFETY: Query state access is properly registered in init_access. // The caller ensures the world matches the one used in init_state. - let result = unsafe { state.query_state.get_unchecked(world, entity) }; + let item = unsafe { state.query_state.get_unchecked(world, entity) }.map_err(|_| { + SystemParamValidationError::skipped::("Current view entity does not match query") + })?; - if result.is_err() { - return Err(SystemParamValidationError::skipped::( - "Current view entity does not match query", - )); - } - - Ok(()) - } - - #[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> { - // SAFETY: We have registered resource read access and validate_param succeeded - let current_view = unsafe { - world - .get_resource::() - .expect("CurrentView must exist") - }; - - let entity = current_view.entity(); - - // SAFETY: Query state access is properly registered in init_access. - // validate_param verified the entity matches. - let item = unsafe { - state - .query_state - .get_unchecked(world, entity) - .expect("view entity must match query") - }; - - ViewQuery { + Ok(ViewQuery { entity, item, _filter: PhantomData, - } + }) } } diff --git a/crates/bevy_render/src/sync_world.rs b/crates/bevy_render/src/sync_world.rs index c98afc2c57bf7..cf92577e53326 100644 --- a/crates/bevy_render/src/sync_world.rs +++ b/crates/bevy_render/src/sync_world.rs @@ -253,7 +253,7 @@ pub(crate) fn despawn_temporary_render_entities( state: &mut SystemState>>, mut local: Local>, ) { - let query = state.get(world); + let query = state.get(world).unwrap(); local.extend(query.iter()); diff --git a/crates/bevy_render/src/view/window/screenshot.rs b/crates/bevy_render/src/view/window/screenshot.rs index ae2a5f9efaaea..025aac44ac58f 100644 --- a/crates/bevy_render/src/view/window/screenshot.rs +++ b/crates/bevy_render/src/view/window/screenshot.rs @@ -229,7 +229,8 @@ fn extract_screenshots( *system_state = Some(SystemState::new(&mut main_world)); } let system_state = system_state.as_mut().unwrap(); - let (mut commands, primary_window, screenshots) = system_state.get_mut(&mut main_world); + let (mut commands, primary_window, screenshots) = + system_state.get_mut(&mut main_world).unwrap(); targets.clear(); seen_targets.clear(); diff --git a/crates/bevy_transform/src/helper.rs b/crates/bevy_transform/src/helper.rs index 1ab33b4b421bc..f0dcfc58b85bd 100644 --- a/crates/bevy_transform/src/helper.rs +++ b/crates/bevy_transform/src/helper.rs @@ -138,7 +138,7 @@ mod tests { let transform = *app.world().get::(leaf_entity).unwrap(); let mut state = SystemState::::new(app.world_mut()); - let helper = state.get(app.world()); + let helper = state.get(app.world()).unwrap(); let computed_transform = helper.compute_global_transform(leaf_entity).unwrap(); diff --git a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs index 79d081c6bfd4d..2c942cde2ab46 100644 --- a/crates/bevy_ui/src/experimental/ghost_hierarchy.rs +++ b/crates/bevy_ui/src/experimental/ghost_hierarchy.rs @@ -230,7 +230,7 @@ mod tests { }); let mut system_state = SystemState::<(UiRootNodes, Query<&A>)>::new(world); - let (ui_root_nodes, a_query) = system_state.get(world); + let (ui_root_nodes, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query.iter_many(ui_root_nodes.iter()).collect(); @@ -263,7 +263,7 @@ mod tests { world.entity_mut(n9).add_children(&[n10]); let mut system_state = SystemState::<(UiChildren, Query<&A>)>::new(world); - let (ui_children, a_query) = system_state.get(world); + let (ui_children, a_query) = system_state.get(world).unwrap(); let result: Vec<_> = a_query .iter_many(ui_children.iter_ui_children(n1)) diff --git a/crates/bevy_winit/src/cursor/mod.rs b/crates/bevy_winit/src/cursor/mod.rs index 7dcbc6d99375b..6d5a5471967a6 100644 --- a/crates/bevy_winit/src/cursor/mod.rs +++ b/crates/bevy_winit/src/cursor/mod.rs @@ -67,13 +67,13 @@ impl WinitAppRunnerState { Query<(Entity, &mut PendingCursor), Changed>, )> = SystemState::new(self.world_mut()); #[cfg(feature = "custom_cursor")] - let (mut cursor_cache, mut windows) = windows_state.get_mut(self.world_mut()); + let (mut cursor_cache, mut windows) = windows_state.get_mut(self.world_mut()).unwrap(); #[cfg(not(feature = "custom_cursor"))] let mut windows_state: SystemState<( Query<(Entity, &mut PendingCursor), Changed>, )> = SystemState::new(self.world_mut()); #[cfg(not(feature = "custom_cursor"))] - let (mut windows,) = windows_state.get_mut(self.world_mut()); + let (mut windows,) = windows_state.get_mut(self.world_mut()).unwrap(); WINIT_WINDOWS.with_borrow(|winit_windows| { for (entity, mut pending_cursor) in windows.iter_mut() { diff --git a/crates/bevy_winit/src/state.rs b/crates/bevy_winit/src/state.rs index 1abaaaf6764e4..4af560e16045a 100644 --- a/crates/bevy_winit/src/state.rs +++ b/crates/bevy_winit/src/state.rs @@ -181,7 +181,7 @@ impl ApplicationHandler for WinitAppRunnerState { // Create the initial window if needed let mut create_window = SystemState::::from_world(self.world_mut()); - create_windows(event_loop, create_window.get_mut(self.world_mut())); + create_windows(event_loop, create_window.get_mut(self.world_mut()).unwrap()); create_window.apply(self.world_mut()); } @@ -195,7 +195,7 @@ impl ApplicationHandler for WinitAppRunnerState { WinitUserEvent::WindowAdded => { let mut create_window = SystemState::::from_world(self.world_mut()); - create_windows(event_loop, create_window.get_mut(self.world_mut())); + create_windows(event_loop, create_window.get_mut(self.world_mut()).unwrap()); create_window.apply(self.world_mut()); } } @@ -224,7 +224,8 @@ impl ApplicationHandler for WinitAppRunnerState { mut windows, ) = self .message_writer_system_state - .get_mut(self.app.world_mut()); + .get_mut(self.app.world_mut()) + .unwrap(); let Some(window) = winit_windows.get_window_entity(window_id) else { warn!("Skipped event {event:?} for unknown winit Window Id {window_id:?}"); @@ -459,7 +460,10 @@ impl ApplicationHandler for WinitAppRunnerState { fn about_to_wait(&mut self, event_loop: &ActiveEventLoop) { let mut create_monitor = SystemState::::from_world(self.world_mut()); - create_monitors(event_loop, create_monitor.get_mut(self.world_mut())); + create_monitors( + event_loop, + create_monitor.get_mut(self.world_mut()).unwrap(), + ); create_monitor.apply(self.world_mut()); // TODO: This is a workaround for https://github.com/bevyengine/bevy/issues/17488 @@ -516,7 +520,7 @@ impl WinitAppRunnerState { let mut focused_windows_state: SystemState<(Res, Query<(Entity, &Window)>)> = SystemState::new(self.world_mut()); - let (config, windows) = focused_windows_state.get(self.world()); + let (config, windows) = focused_windows_state.get(self.world()).unwrap(); let focused = windows.iter().any(|(_, window)| window.focused); let mut update_mode = config.update_mode(focused); @@ -572,7 +576,7 @@ impl WinitAppRunnerState { SystemState::::from_world(self.world_mut()); let (.., mut handlers, accessibility_requested, monitors) = - create_window.get_mut(self.world_mut()); + create_window.get_mut(self.world_mut()).unwrap(); let winit_window = winit_windows.create_window( event_loop, @@ -605,7 +609,7 @@ impl WinitAppRunnerState { let begin_frame_time = Instant::now(); if should_update { - let (_, windows) = focused_windows_state.get(self.world()); + let (_, windows) = focused_windows_state.get(self.world()).unwrap(); // If no windows exist, this will evaluate to `true`. let all_invisible = windows.iter().all(|w| !w.1.visible); @@ -648,7 +652,7 @@ impl WinitAppRunnerState { } // Running the app may have changed the WinitSettings resource, so we have to re-extract it. - let (config, windows) = focused_windows_state.get(self.world()); + let (config, windows) = focused_windows_state.get(self.world()).unwrap(); let focused = windows.iter().any(|(_, window)| window.focused); update_mode = config.update_mode(focused); } diff --git a/examples/async_tasks/async_compute.rs b/examples/async_tasks/async_compute.rs index 1018a13c37548..7fab1c7e136bf 100644 --- a/examples/async_tasks/async_compute.rs +++ b/examples/async_tasks/async_compute.rs @@ -91,7 +91,7 @@ fn spawn_tasks(mut commands: Commands) { Res, )>::new(world); let (box_mesh_handle, box_material_handle) = - system_state.get_mut(world); + system_state.get_mut(world).unwrap(); (box_mesh_handle.clone(), box_material_handle.clone()) }; diff --git a/examples/ecs/fallible_params.rs b/examples/ecs/fallible_params.rs index d70ae13bfe191..65008403b51b9 100644 --- a/examples/ecs/fallible_params.rs +++ b/examples/ecs/fallible_params.rs @@ -10,14 +10,14 @@ //! Other system parameters, such as [`Query`], will never fail validation: returning a query with no matching entities is valid. //! //! The result of failed system parameter validation is determined by the [`SystemParamValidationError`] returned -//! by [`SystemParam::validate_param`] for each system parameter. +//! by [`SystemParam::get_param`] for each system parameter. //! Each system will pass if all of its parameters are valid, or else return [`SystemParamValidationError`] for the first failing parameter. //! //! To learn more about setting the fallback behavior for [`SystemParamValidationError`] failures, //! please see the `error_handling.rs` example. //! //! [`SystemParamValidationError`]: bevy::ecs::system::SystemParamValidationError -//! [`SystemParam::validate_param`]: bevy::ecs::system::SystemParam::validate_param +//! [`SystemParam::get_param`]: bevy::ecs::system::SystemParam::get_param use bevy::ecs::error::warn; use bevy::prelude::*; diff --git a/release-content/migration-guides/validation_merging.md b/release-content/migration-guides/validation_merging.md new file mode 100644 index 0000000000000..ad4cfa8099002 --- /dev/null +++ b/release-content/migration-guides/validation_merging.md @@ -0,0 +1,129 @@ +--- +title: "`SystemParam` validation is now done when fetching the data" +pull_requests: [23225] +--- + +In an effort to improve performance by reducing redundant data fetches and simplify internals, +system parameter validation is now done as part of fetching the data for those system parameters. +To be more precise: + +- `SystemParam::get_param` now returns a `Result, SystemParamValidationError>`, instead of simply a `Self::Item<'world, 'state>` + - If validation fails, an appropriate `SystemParamValidationError` should be returned + - If validation passes, the item should be returned wrapped in `Ok` +- `SystemParam::validate_param` has been removed + - All logic that was done in this method should be moved to the `get_param` method of that type +- `SystemState::validate_param` has been removed + - Validation now happens automatically when calling `get`, `get_mut`, or `get_unchecked` +- `SystemState::fetch`, `get_unchecked`, `get` and `get_mut` now return a `Result<..., SystemParamValidationError>`. Callers that previously destructured the result directly will need to add `.unwrap()` or handle the `Result`: + +```rust +// Before +let (res, query) = system_state.get(&world); + +// After +let (res, query) = system_state.get(&world).unwrap(); +``` + +When executing systems, we no longer check for system validation before running the systems. +As a result of these changes, `System::validate_param` and `System::validate_param_unsafe` have been removed. +Instead, validation has been moved to be part of the trait implementation for `System::run_unsafe`. +All implementations of the `System` trait should validate that their parameters are valid during this method, +bubbling up any errors originating in `SystemParam::get_param`. + +## Custom `SystemParam` implementations + +If you have a custom `SystemParam` implementation, you need to: + +1. Remove the `validate_param` method. +2. Move any validation logic into `get_param`. +3. Change `get_param` to return `Result, SystemParamValidationError>`. + +```rust +// Before +unsafe impl SystemParam for MyParam<'_> { + // ... + unsafe fn validate_param( + state: &Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell, + ) -> Result<(), SystemParamValidationError> { + // validation logic + if !is_valid(state, world) { + return Err(SystemParamValidationError::invalid::("not valid")); + } + Ok(()) + } + + unsafe fn get_param<'w, 's>( + state: &'s mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'w>, + change_tick: Tick, + ) -> Self::Item<'w, 's> { + // fetch logic + MyParam { /* ... */ } + } +} + +// After +unsafe impl SystemParam for MyParam<'_> { + // ... + unsafe fn get_param<'w, 's>( + state: &'s mut Self::State, + system_meta: &SystemMeta, + world: UnsafeWorldCell<'w>, + change_tick: Tick, + ) -> Result, SystemParamValidationError> { + // validation logic merged into get_param + if !is_valid(state, world) { + return Err(SystemParamValidationError::invalid::("not valid")); + } + // fetch logic + Ok(MyParam { /* ... */ }) + } +} +``` + +## Custom `ExclusiveSystemParam` implementations + +Similarly, `ExclusiveSystemParam::get_param` now returns a `Result, SystemParamValidationError>` instead of `Self::Item<'s>`. +Existing implementations should wrap their return value in `Ok(...)` and return an appropriate `SystemParamValidationError` if validation fails. + +```rust +// Before +impl ExclusiveSystemParam for MyExclusiveParam { + // ... + fn get_param<'s>( + state: &'s mut Self::State, + system_meta: &SystemMeta, + ) -> Self::Item<'s> { + MyExclusiveParam { /* ... */ } + } +} + +// After +impl ExclusiveSystemParam for MyExclusiveParam { + // ... + fn get_param<'s>( + state: &'s mut Self::State, + system_meta: &SystemMeta, + ) -> Result, SystemParamValidationError> { + Ok(MyExclusiveParam { /* ... */ }) + } +} +``` + +## Custom `System` implementations + +If you have a custom `System` implementation, remove the `validate_param_unsafe` method. Parameter validation should now occur inside `run_unsafe` by propagating errors from `SystemParam::get_param`. + +## `MultithreadedExecutor` performance changes + +For the parallel `MultithreadedExecutor`, validation was previously done as a cheap pre-validation step, +while checking run conditions. +Now, tasks will be spawned for systems which would fail or are skipped during validation. + +In most cases, avoiding the extra overhead of looking up the required data twice should dominate. +However, this change may negatively affect systems which are frequently skipped (e.g. due to `Single`). +If you find that this is a significant performance overhead for your use case, +the previous behavior can be recovered by adding run conditions.