From 1b78c0ef9885b5faa5e1a75707f6d714a8b98d56 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 22 Mar 2023 22:54:46 -0400 Subject: [PATCH 1/5] use `UnsafeWorldCell` for `SystemParam`s --- crates/bevy_ecs/macros/src/lib.rs | 4 +- crates/bevy_ecs/src/removal_detection.rs | 6 +- crates/bevy_ecs/src/system/function_system.rs | 9 ++- crates/bevy_ecs/src/system/system_param.rs | 70 +++++++++---------- crates/bevy_ecs/src/world/identifier.rs | 6 +- crates/bevy_render/src/extract_param.rs | 3 +- 6 files changed, 52 insertions(+), 46 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 3866cea31595f..5642a14629c95 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -230,7 +230,7 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream { unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { ParamSet { @@ -436,7 +436,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { unsafe fn get_param<'w2, 's2>( state: &'s2 mut Self::State, system_meta: &#path::system::SystemMeta, - world: &'w2 #path::world::World, + world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w2>, change_tick: #path::component::Tick, ) -> Self::Item<'w2, 's2> { let (#(#tuple_patterns,)*) = < diff --git a/crates/bevy_ecs/src/removal_detection.rs b/crates/bevy_ecs/src/removal_detection.rs index 139c092bb8b50..f2c91d97ddfe6 100644 --- a/crates/bevy_ecs/src/removal_detection.rs +++ b/crates/bevy_ecs/src/removal_detection.rs @@ -8,7 +8,7 @@ use crate::{ prelude::Local, storage::SparseSet, system::{ReadOnlySystemParam, SystemMeta, SystemParam}, - world::World, + world::{unsafe_world_cell::UnsafeWorldCell, World}, }; use std::{ @@ -264,9 +264,9 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentEvents { unsafe fn get_param<'w, 's>( _state: &'s mut Self::State, _system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { - world.removed_components() + world.world_metadata().removed_components() } } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index d8bf01392ff4a..06bc47875bcfc 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -289,7 +289,12 @@ impl SystemState { world: &'w World, change_tick: Tick, ) -> SystemParamItem<'w, 's, Param> { - let param = Param::get_param(&mut self.param_state, &self.meta, world, change_tick); + let param = Param::get_param( + &mut self.param_state, + &self.meta, + world.as_unsafe_world_cell_migration_internal(), + change_tick, + ); self.meta.last_run = change_tick; param } @@ -479,7 +484,7 @@ where let params = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, - world, + world.as_unsafe_world_cell_migration_internal(), change_tick, ); let out = self.func.run(input, params); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index d0668025c7029..3502e5be842ae 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -9,7 +9,7 @@ use crate::{ Access, FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyWorldQuery, WorldQuery, }, system::{Query, SystemMeta}, - world::{FromWorld, World}, + world::{unsafe_world_cell::UnsafeWorldCell, FromWorld, World}, }; use bevy_ecs_macros::impl_param_set; pub use bevy_ecs_macros::Resource; @@ -129,14 +129,13 @@ pub unsafe trait SystemParam: Sized { /// # Safety /// - /// This call might use any of the [`World`] accesses that were registered in [`Self::init_state`]. - /// - None of those accesses may conflict with any other [`SystemParam`]s - /// that exist at the same time, including those on other threads. + /// - The passed [`UnsafeWorldCell`] must be allowed to use any accesses registered + /// in [`init_state`](SystemParam::init_state). /// - `world` must be the same `World` that was used to initialize [`state`](SystemParam::init_state). unsafe fn get_param<'world, 'state>( state: &'state mut Self::State, system_meta: &SystemMeta, - world: &'world World, + world: UnsafeWorldCell<'world>, change_tick: Tick, ) -> Self::Item<'world, 'state>; } @@ -194,10 +193,16 @@ unsafe impl SystemPara unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { - Query::new(world, state, system_meta.last_run, change_tick, false) + Query::new( + world.world(), + state, + system_meta.last_run, + change_tick, + false, + ) } } @@ -331,7 +336,7 @@ fn assert_component_access_compatibility( /// ``` pub struct ParamSet<'w, 's, T: SystemParam> { param_states: &'s mut T::State, - world: &'w World, + world: UnsafeWorldCell<'w>, system_meta: SystemMeta, change_tick: Tick, } @@ -437,11 +442,10 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { let (ptr, ticks) = world - .as_unsafe_world_cell_migration_internal() .get_resource_with_ticks(component_id) .unwrap_or_else(|| { panic!( @@ -478,11 +482,10 @@ unsafe impl<'a, T: Resource> SystemParam for Option> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { world - .as_unsafe_world_cell_migration_internal() .get_resource_with_ticks(component_id) .map(|(ptr, ticks)| Res { value: ptr.deref(), @@ -532,11 +535,10 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { let value = world - .as_unsafe_world_cell_migration_internal() .get_resource_mut_by_id(component_id) .unwrap_or_else(|| { panic!( @@ -570,11 +572,10 @@ unsafe impl<'a, T: Resource> SystemParam for Option> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { world - .as_unsafe_world_cell_migration_internal() .get_resource_mut_by_id(component_id) .map(|value| ResMut { value: value.value.deref_mut::(), @@ -623,10 +624,11 @@ unsafe impl SystemParam for &'_ World { unsafe fn get_param<'w, 's>( _state: &'s mut Self::State, _system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { - world + // SAFETY: Read-only access to the entire world was registerd in `init_state`. + world.world() } } @@ -744,7 +746,7 @@ unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> { unsafe fn get_param<'w, 's>( state: &'s mut Self::State, _system_meta: &SystemMeta, - _world: &'w World, + _world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { Local(state.get()) @@ -917,7 +919,7 @@ unsafe impl SystemParam for Deferred<'_, T> { unsafe fn get_param<'w, 's>( state: &'s mut Self::State, _system_meta: &SystemMeta, - _world: &'w World, + _world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { Deferred(state.get()) @@ -1023,11 +1025,10 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { let (ptr, ticks) = world - .as_unsafe_world_cell_migration_internal() .get_non_send_with_ticks(component_id) .unwrap_or_else(|| { panic!( @@ -1062,11 +1063,10 @@ unsafe impl SystemParam for Option> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { world - .as_unsafe_world_cell_migration_internal() .get_non_send_with_ticks(component_id) .map(|(ptr, ticks)| NonSend { value: ptr.deref(), @@ -1115,11 +1115,10 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { let (ptr, ticks) = world - .as_unsafe_world_cell_migration_internal() .get_non_send_with_ticks(component_id) .unwrap_or_else(|| { panic!( @@ -1148,11 +1147,10 @@ unsafe impl<'a, T: 'static> SystemParam for Option> { unsafe fn get_param<'w, 's>( &mut component_id: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { world - .as_unsafe_world_cell_migration_internal() .get_non_send_with_ticks(component_id) .map(|(ptr, ticks)| NonSendMut { value: ptr.assert_unique().deref_mut(), @@ -1175,7 +1173,7 @@ unsafe impl<'a> SystemParam for &'a Archetypes { unsafe fn get_param<'w, 's>( _state: &'s mut Self::State, _system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { world.archetypes() @@ -1196,7 +1194,7 @@ unsafe impl<'a> SystemParam for &'a Components { unsafe fn get_param<'w, 's>( _state: &'s mut Self::State, _system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { world.components() @@ -1217,7 +1215,7 @@ unsafe impl<'a> SystemParam for &'a Entities { unsafe fn get_param<'w, 's>( _state: &'s mut Self::State, _system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { world.entities() @@ -1238,7 +1236,7 @@ unsafe impl<'a> SystemParam for &'a Bundles { unsafe fn get_param<'w, 's>( _state: &'s mut Self::State, _system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { world.bundles() @@ -1287,7 +1285,7 @@ unsafe impl SystemParam for SystemChangeTick { unsafe fn get_param<'w, 's>( _state: &'s mut Self::State, system_meta: &SystemMeta, - _world: &'w World, + _world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { SystemChangeTick { @@ -1357,7 +1355,7 @@ unsafe impl SystemParam for SystemName<'_> { unsafe fn get_param<'w, 's>( name: &'s mut Self::State, _system_meta: &SystemMeta, - _world: &'w World, + _world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { SystemName { name } @@ -1399,7 +1397,7 @@ macro_rules! impl_system_param_tuple { unsafe fn get_param<'w, 's>( state: &'s mut Self::State, _system_meta: &SystemMeta, - _world: &'w World, + _world: UnsafeWorldCell<'w>, _change_tick: Tick, ) -> Self::Item<'w, 's> { @@ -1523,7 +1521,7 @@ unsafe impl SystemParam for StaticSystemParam<'_, '_, unsafe fn get_param<'world, 'state>( state: &'state mut Self::State, system_meta: &SystemMeta, - world: &'world World, + world: UnsafeWorldCell<'world>, change_tick: Tick, ) -> Self::Item<'world, 'state> { // SAFETY: Defer to the safety of P::SystemParam diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index 55e2b6a5fa350..952f65e6be932 100644 --- a/crates/bevy_ecs/src/world/identifier.rs +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -6,6 +6,8 @@ use crate::{ }; use std::sync::atomic::{AtomicUsize, Ordering}; +use super::unsafe_world_cell::UnsafeWorldCell; + #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] // We use usize here because that is the largest `Atomic` we want to require /// A unique identifier for a [`World`]. @@ -56,10 +58,10 @@ unsafe impl SystemParam for WorldId { unsafe fn get_param<'world, 'state>( _: &'state mut Self::State, _: &crate::system::SystemMeta, - world: &'world super::World, + world: UnsafeWorldCell<'world>, _: Tick, ) -> Self::Item<'world, 'state> { - world.id + world.world_metadata().id() } } diff --git a/crates/bevy_render/src/extract_param.rs b/crates/bevy_render/src/extract_param.rs index 898dfe0e28ae0..57a1267ce5953 100644 --- a/crates/bevy_render/src/extract_param.rs +++ b/crates/bevy_render/src/extract_param.rs @@ -3,6 +3,7 @@ use bevy_ecs::{ component::Tick, prelude::*, system::{ReadOnlySystemParam, SystemMeta, SystemParam, SystemParamItem, SystemState}, + world::unsafe_world_cell::UnsafeWorldCell, }; use std::ops::{Deref, DerefMut}; @@ -76,7 +77,7 @@ where unsafe fn get_param<'w, 's>( state: &'s mut Self::State, system_meta: &SystemMeta, - world: &'w World, + world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Self::Item<'w, 's> { // SAFETY: From 0d50c367835ee43b88f3121ee337e1378c4a8d1f Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 22 Mar 2023 23:05:40 -0400 Subject: [PATCH 2/5] make a safety invariant explicit --- crates/bevy_ecs/src/system/system_param.rs | 5 ++++- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 3502e5be842ae..522190f773e36 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -197,7 +197,10 @@ unsafe impl SystemPara change_tick: Tick, ) -> Self::Item<'w, 's> { Query::new( - world.world(), + // SAFETY: We have registered all of the query's world accesses, + // so the caller ensures that `world` has permission to access any + // world datata that the query needs. + world.unsafe_world(), state, system_meta.last_run, change_tick, diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 99231fe3f32ef..54afd95f7323a 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -144,7 +144,7 @@ impl<'w> UnsafeWorldCell<'w> { /// # Safety /// - must not be used in a way that would conflict with any /// live exclusive borrows on world data - unsafe fn unsafe_world(self) -> &'w World { + pub(crate) unsafe fn unsafe_world(self) -> &'w World { // SAFETY: // - caller ensures that the returned `&World` is not used in a way that would conflict // with any existing mutable borrows of world data From eb0ab0e2fa6f49fc2f1b427a46b6366dc30ef15f Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 22 Mar 2023 23:09:46 -0400 Subject: [PATCH 3/5] fix a typo --- crates/bevy_ecs/src/system/system_param.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 522190f773e36..f5578e6d3729a 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -199,7 +199,7 @@ unsafe impl SystemPara Query::new( // SAFETY: We have registered all of the query's world accesses, // so the caller ensures that `world` has permission to access any - // world datata that the query needs. + // world data that the query needs. world.unsafe_world(), state, system_meta.last_run, From 1799d4109b14044f014f39a156d8331ecd0a9de2 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 22 Mar 2023 23:20:02 -0400 Subject: [PATCH 4/5] tweak phrasing for a safety invariant --- crates/bevy_ecs/src/system/system_param.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index f5578e6d3729a..a9953a43f2510 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -129,8 +129,8 @@ pub unsafe trait SystemParam: Sized { /// # Safety /// - /// - The passed [`UnsafeWorldCell`] must be allowed to use any accesses registered - /// in [`init_state`](SystemParam::init_state). + /// - The passed [`UnsafeWorldCell`] must have access to any world data + /// registered in [`init_state`](SystemParam::init_state). /// - `world` must be the same `World` that was used to initialize [`state`](SystemParam::init_state). unsafe fn get_param<'world, 'state>( state: &'state mut Self::State, From bebd929ae21e116f3a10dbb29b5e2acede2fa60b Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 30 Mar 2023 15:15:17 -0400 Subject: [PATCH 5/5] fix a merge conflict --- crates/bevy_ecs/src/system/system_param.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 056db36f23db3..c13445b63854d 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1548,7 +1548,7 @@ unsafe impl SystemParam for PhantomData { unsafe fn get_param<'world, 'state>( _state: &'state mut Self::State, _system_meta: &SystemMeta, - _world: &'world World, + _world: UnsafeWorldCell<'world>, _change_tick: Tick, ) -> Self::Item<'world, 'state> { PhantomData