From f5bdeb9ab5c3b99b0e6a273e27e6b19eed7f6638 Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Fri, 9 Sep 2022 09:21:38 +0200 Subject: [PATCH 1/4] add get_resource_unchecked_mut_by_id --- crates/bevy_ecs/src/world/mod.rs | 33 +++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 0e8009916715c..020a4f26f151e 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1499,23 +1499,42 @@ impl World { /// use this in cases where the actual types are not known at compile time.** #[inline] pub fn get_resource_mut_by_id(&mut self, component_id: ComponentId) -> Option> { + // SAFETY: unique world access + unsafe { self.get_resource_unchecked_mut_by_id(component_id) } + } + + /// Gets a resource to the resource with the id [`ComponentId`] if it exists. + /// The returned pointer may be used to modify the resource, as long as the mutable borrow + /// of the [`World`] is still valid. + /// + /// **You should prefer to use the typed API [`World::get_resource_mut`] where possible and only + /// use this in cases where the actual types are not known at compile time.** + /// + /// # Safety + /// This will allow aliased mutable access to the given resource type. The caller must ensure + /// that there is either only one mutable access or multiple immutable accesses at a time. + #[inline] + pub unsafe fn get_resource_unchecked_mut_by_id( + &self, + component_id: ComponentId, + ) -> Option> { let info = self.components.get_info(component_id)?; if !info.is_send_and_sync() { self.validate_non_send_access_untyped(info.name()); } - let change_tick = self.change_tick(); - let (ptr, ticks) = self.get_resource_with_ticks(component_id)?; - // SAFETY: This function has exclusive access to the world so nothing aliases `ticks`. + // SAFETY: // - index is in-bounds because the column is initialized and non-empty - // - no other reference to the ticks of the same row can exist at the same time - let ticks = unsafe { Ticks::from_tick_cells(ticks, self.last_change_tick(), change_tick) }; + // - no other reference to the ticks, because the caller promises it + let ticks = unsafe { + Ticks::from_tick_cells(ticks, self.last_change_tick(), self.read_change_tick()) + }; Some(MutUntyped { // SAFETY: This function has exclusive access to the world so nothing aliases `ptr`. - value: unsafe { ptr.assert_unique() }, + value: ptr.assert_unique(), ticks, }) } @@ -1574,7 +1593,7 @@ impl World { component_id: ComponentId, ) -> Option> { self.components().get_info(component_id)?; - // SAFETY: entity_location is valid, component_id is valid as checked by the line above + // SAFETY: entity_location is valid, component_id is valid as checked by the line above, world access is unique unsafe { get_mut_by_id( self, From ef85d548dc616f0c00bc4ac258c87939a7eee7f2 Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Fri, 9 Sep 2022 11:03:21 +0200 Subject: [PATCH 2/4] add EntityRef::get_unchecked_mut_by_id --- crates/bevy_ecs/src/world/entity_ref.rs | 42 +++++++++++++++++++++---- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 458564beb4cfe..6a6ed90604591 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -189,6 +189,31 @@ impl<'w> EntityRef<'w> { ) } } + + /// Gets the component of the given [`ComponentId`] from the entity. + /// + /// **You should prefer to use the typed API where possible and only + /// use this in cases where the actual component types are not known at + /// compile time.** + /// + /// Unlike [`EntityRef::get`], this returns a raw pointer to the component, + /// which is only valid while the `'w` borrow of the lifetime is active. + /// + /// # Safety + /// + /// - The returned reference must never alias a mutable borrow of this component. + /// - The returned reference must not be used after this component is moved which + /// may happen from **any** `insert_component`, `remove_component` or `despawn` + /// operation on this world (non-exhaustive list). + #[inline] + pub unsafe fn get_unchecked_mut_by_id( + &self, + component_id: ComponentId, + ) -> Option> { + self.world.components().get_info(component_id)?; + // SAFETY: entity_location is valid, component_id is valid as checked by the line above, world access is promised by the caller + get_mut_by_id(self.world, self.entity, self.location, component_id) + } } impl<'w> From> for EntityRef<'w> { @@ -692,7 +717,7 @@ impl<'w> EntityMut<'w> { #[inline] pub fn get_mut_by_id(&mut self, component_id: ComponentId) -> Option> { self.world.components().get_info(component_id)?; - // SAFETY: entity_location is valid, component_id is valid as checked by the line above + // SAFETY: entity_location is valid, component_id is valid as checked by the line above, world access is unique unsafe { get_mut_by_id(self.world, self.entity, self.location, component_id) } } } @@ -1064,21 +1089,26 @@ pub(crate) unsafe fn get_mut( }) } -// SAFETY: EntityLocation must be valid, component_id must be valid +// SAFETY: +// - EntityLocation must be valid, component_id must be valid +// - world access to the component must be valid, either because the caller has a `&mut` world or it synchronizes access like systems do #[inline] pub(crate) unsafe fn get_mut_by_id( - world: &mut World, + world: &World, entity: Entity, location: EntityLocation, component_id: ComponentId, ) -> Option { - let change_tick = world.change_tick(); let info = world.components.get_info_unchecked(component_id); - // SAFETY: world access is unique, entity location and component_id required to be valid + // SAFETY: world access promised by the caller, entity location and component_id required to be valid get_component_and_ticks(world, component_id, info.storage_type(), entity, location).map( |(value, ticks)| MutUntyped { value: value.assert_unique(), - ticks: Ticks::from_tick_cells(ticks, world.last_change_tick(), change_tick), + ticks: Ticks::from_tick_cells( + ticks, + world.last_change_tick(), + world.read_change_tick(), + ), }, ) } From 9df2ac09860cd6b4b29c7d53674ed0717e6a141a Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Fri, 4 Nov 2022 11:44:15 +0100 Subject: [PATCH 3/4] clean up safety comments --- crates/bevy_ecs/src/world/entity_ref.rs | 5 +++-- crates/bevy_ecs/src/world/mod.rs | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 6a6ed90604591..fef6f175a617d 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -201,7 +201,7 @@ impl<'w> EntityRef<'w> { /// /// # Safety /// - /// - The returned reference must never alias a mutable borrow of this component. + /// - The returned reference must never alias another reference to this component /// - The returned reference must not be used after this component is moved which /// may happen from **any** `insert_component`, `remove_component` or `despawn` /// operation on this world (non-exhaustive list). @@ -211,7 +211,8 @@ impl<'w> EntityRef<'w> { component_id: ComponentId, ) -> Option> { self.world.components().get_info(component_id)?; - // SAFETY: entity_location is valid, component_id is valid as checked by the line above, world access is promised by the caller + // SAFETY: entity_location is valid, component_id is valid as checked by the line above, + // the caller promises that they can uniquely access this component get_mut_by_id(self.world, self.entity, self.location, component_id) } } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 020a4f26f151e..56edf477ea1b4 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1533,7 +1533,8 @@ impl World { }; Some(MutUntyped { - // SAFETY: This function has exclusive access to the world so nothing aliases `ptr`. + // SAFETY: the caller of this function has to ensure that they can mutably access this + // component for the duration of the `'_` lifetime value: ptr.assert_unique(), ticks, }) From 47bada41aae56ca5c80782a3a1dc24d98f7344d7 Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Tue, 8 Nov 2022 19:13:32 +0100 Subject: [PATCH 4/4] remove note about validity copied from non unchecked_mut variant --- crates/bevy_ecs/src/world/entity_ref.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index fef6f175a617d..501b548b3ce72 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -197,7 +197,7 @@ impl<'w> EntityRef<'w> { /// compile time.** /// /// Unlike [`EntityRef::get`], this returns a raw pointer to the component, - /// which is only valid while the `'w` borrow of the lifetime is active. + /// instead of a reference. /// /// # Safety ///