Skip to content

Commit 0cb0d8b

Browse files
add UnsafeWorldCell abstraction (#6404)
alternative to #5922, implements #5956 builds on top of #6402 # Objective #5956 goes into more detail, but the TLDR is: - bevy systems ensure disjoint accesses to resources and components, and for that to work there are methods `World::get_resource_unchecked_mut(&self)`, ..., `EntityRef::get_mut_unchecked(&self)` etc. - we don't have these unchecked methods for `by_id` variants, so third-party crate authors cannot build their own safe disjoint-access abstractions with these - having `_unchecked_mut` methods is not great, because in their presence safe code can accidentally violate subtle invariants. Having to go through `world.as_unsafe_world_cell().unsafe_method()` forces you to stop and think about what you want to write in your `// SAFETY` comment. The alternative is to keep exposing `_unchecked_mut` variants for every operation that we want third-party crates to build upon, but we'd prefer to avoid using these methods alltogether: #5922 (comment) Also, this is something that **cannot be implemented outside of bevy**, so having either this PR or #5922 as an escape hatch with lots of discouraging comments would be great. ## Solution - add `UnsafeWorldCell` with `unsafe fn get_resource(&self)`, `unsafe fn get_resource_mut(&self)` - add `fn World::as_unsafe_world_cell(&mut self) -> UnsafeWorldCell<'_>` (and `as_unsafe_world_cell_readonly(&self)`) - add `UnsafeWorldCellEntityRef` with `unsafe fn get`, `unsafe fn get_mut` and the other utilities on `EntityRef` (no methods for spawning, despawning, insertion) - use the `UnsafeWorldCell` abstraction in `ReflectComponent`, `ReflectResource` and `ReflectAsset`, so these APIs are easier to reason about - remove `World::get_resource_mut_unchecked`, `EntityRef::get_mut_unchecked` and use `unsafe { world.as_unsafe_world_cell().get_mut() }` and `unsafe { world.as_unsafe_world_cell().get_entity(entity)?.get_mut() }` instead This PR does **not** make use of `UnsafeWorldCell` for anywhere else in `bevy_ecs` such as `SystemParam` or `Query`. That is a much larger change, and I am convinced that having `UnsafeWorldCell` is already useful for third-party crates. Implemented API: ```rust struct World { .. } impl World { fn as_unsafe_world_cell(&self) -> UnsafeWorldCell<'_>; } struct UnsafeWorldCell<'w>(&'w World); impl<'w> UnsafeWorldCell { unsafe fn world(&self) -> &World; fn get_entity(&self) -> UnsafeWorldCellEntityRef<'w>; // returns 'w which is `'self` of the `World::as_unsafe_world_cell(&'w self)` unsafe fn get_resource<T>(&self) -> Option<&'w T>; unsafe fn get_resource_by_id(&self, ComponentId) -> Option<&'w T>; unsafe fn get_resource_mut<T>(&self) -> Option<Mut<'w, T>>; unsafe fn get_resource_mut_by_id(&self) -> Option<MutUntyped<'w>>; unsafe fn get_non_send_resource<T>(&self) -> Option<&'w T>; unsafe fn get_non_send_resource_mut<T>(&self) -> Option<Mut<'w, T>>>; // not included: remove, remove_resource, despawn, anything that might change archetypes } struct UnsafeWorldCellEntityRef<'w> { .. } impl UnsafeWorldCellEntityRef<'w> { unsafe fn get<T>(&self, Entity) -> Option<&'w T>; unsafe fn get_by_id(&self, Entity, ComponentId) -> Option<Ptr<'w>>; unsafe fn get_mut<T>(&self, Entity) -> Option<Mut<'w, T>>; unsafe fn get_mut_by_id(&self, Entity, ComponentId) -> Option<MutUntyped<'w>>; unsafe fn get_change_ticks<T>(&self, Entity) -> Option<Mut<'w, T>>; // fn id, archetype, contains, contains_id, containts_type_id } ``` <details> <summary>UnsafeWorldCell docs</summary> Variant of the [`World`] where resource and component accesses takes a `&World`, and the responsibility to avoid aliasing violations are given to the caller instead of being checked at compile-time by rust's unique XOR shared rule. ### Rationale In rust, having a `&mut World` means that there are absolutely no other references to the safe world alive at the same time, without exceptions. Not even unsafe code can change this. But there are situations where careful shared mutable access through a type is possible and safe. For this, rust provides the [`UnsafeCell`](std::cell::UnsafeCell) escape hatch, which allows you to get a `*mut T` from a `&UnsafeCell<T>` and around which safe abstractions can be built. Access to resources and components can be done uniquely using [`World::resource_mut`] and [`World::entity_mut`], and shared using [`World::resource`] and [`World::entity`]. These methods use lifetimes to check at compile time that no aliasing rules are being broken. This alone is not enough to implement bevy systems where multiple systems can access *disjoint* parts of the world concurrently. For this, bevy stores all values of resources and components (and [`ComponentTicks`](crate::component::ComponentTicks)) in [`UnsafeCell`](std::cell::UnsafeCell)s, and carefully validates disjoint access patterns using APIs like [`System::component_access`](crate::system::System::component_access). A system then can be executed using [`System::run_unsafe`](crate::system::System::run_unsafe) with a `&World` and use methods with interior mutability to access resource values. access resource values. ### Example Usage [`UnsafeWorldCell`] can be used as a building block for writing APIs that safely allow disjoint access into the world. In the following example, the world is split into a resource access half and a component access half, where each one can safely hand out mutable references. ```rust use bevy_ecs::world::World; use bevy_ecs::change_detection::Mut; use bevy_ecs::system::Resource; use bevy_ecs::world::unsafe_world_cell_world::UnsafeWorldCell; // INVARIANT: existance of this struct means that users of it are the only ones being able to access resources in the world struct OnlyResourceAccessWorld<'w>(UnsafeWorldCell<'w>); // INVARIANT: existance of this struct means that users of it are the only ones being able to access components in the world struct OnlyComponentAccessWorld<'w>(UnsafeWorldCell<'w>); impl<'w> OnlyResourceAccessWorld<'w> { fn get_resource_mut<T: Resource>(&mut self) -> Option<Mut<'w, T>> { // SAFETY: resource access is allowed through this UnsafeWorldCell unsafe { self.0.get_resource_mut::<T>() } } } // impl<'w> OnlyComponentAccessWorld<'w> { // ... // } // the two interior mutable worlds borrow from the `&mut World`, so it cannot be accessed while they are live fn split_world_access(world: &mut World) -> (OnlyResourceAccessWorld<'_>, OnlyComponentAccessWorld<'_>) { let resource_access = OnlyResourceAccessWorld(unsafe { world.as_unsafe_world_cell() }); let component_access = OnlyComponentAccessWorld(unsafe { world.as_unsafe_world_cell() }); (resource_access, component_access) } ``` </details>
1 parent aab518a commit 0cb0d8b

File tree

8 files changed

+774
-335
lines changed

8 files changed

+774
-335
lines changed

crates/bevy_asset/src/reflect.rs

Lines changed: 17 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::any::{Any, TypeId};
22

3-
use bevy_ecs::world::World;
3+
use bevy_ecs::world::{unsafe_world_cell::UnsafeWorldCell, World};
44
use bevy_reflect::{FromReflect, FromType, Reflect, Uuid};
55

66
use crate::{Asset, Assets, Handle, HandleId, HandleUntyped};
@@ -18,8 +18,10 @@ pub struct ReflectAsset {
1818
assets_resource_type_id: TypeId,
1919

2020
get: fn(&World, HandleUntyped) -> Option<&dyn Reflect>,
21-
get_mut: fn(&mut World, HandleUntyped) -> Option<&mut dyn Reflect>,
22-
get_unchecked_mut: unsafe fn(&World, HandleUntyped) -> Option<&mut dyn Reflect>,
21+
// SAFETY:
22+
// - may only be called with a [`IteriorMutableWorld`] which can be used to access the corresponding `Assets<T>` resource mutably
23+
// - may only be used to access **at most one** access at once
24+
get_unchecked_mut: unsafe fn(UnsafeWorldCell<'_>, HandleUntyped) -> Option<&mut dyn Reflect>,
2325
add: fn(&mut World, &dyn Reflect) -> HandleUntyped,
2426
set: fn(&mut World, HandleUntyped, &dyn Reflect) -> HandleUntyped,
2527
len: fn(&World) -> usize,
@@ -54,10 +56,11 @@ impl ReflectAsset {
5456
world: &'w mut World,
5557
handle: HandleUntyped,
5658
) -> Option<&'w mut dyn Reflect> {
57-
(self.get_mut)(world, handle)
59+
// SAFETY: unique world access
60+
unsafe { (self.get_unchecked_mut)(world.as_unsafe_world_cell(), handle) }
5861
}
5962

60-
/// Equivalent of [`Assets::get_mut`], but does not require a mutable reference to the world.
63+
/// Equivalent of [`Assets::get_mut`], but works with an [`UnsafeWorldCell`].
6164
///
6265
/// Only use this method when you have ensured that you are the *only* one with access to the [`Assets`] resource of the asset type.
6366
/// Furthermore, this does *not* allow you to have look up two distinct handles,
@@ -67,11 +70,12 @@ impl ReflectAsset {
6770
/// # use bevy_asset::{ReflectAsset, HandleUntyped};
6871
/// # use bevy_ecs::prelude::World;
6972
/// # let reflect_asset: ReflectAsset = unimplemented!();
70-
/// # let world: World = unimplemented!();
73+
/// # let mut world: World = unimplemented!();
7174
/// # let handle_1: HandleUntyped = unimplemented!();
7275
/// # let handle_2: HandleUntyped = unimplemented!();
73-
/// let a = unsafe { reflect_asset.get_unchecked_mut(&world, handle_1).unwrap() };
74-
/// let b = unsafe { reflect_asset.get_unchecked_mut(&world, handle_2).unwrap() };
76+
/// let unsafe_world_cell = world.as_unsafe_world_cell();
77+
/// let a = unsafe { reflect_asset.get_unchecked_mut(unsafe_world_cell, handle_1).unwrap() };
78+
/// let b = unsafe { reflect_asset.get_unchecked_mut(unsafe_world_cell, handle_2).unwrap() };
7579
/// // ^ not allowed, two mutable references through the same asset resource, even though the
7680
/// // handles are distinct
7781
///
@@ -81,12 +85,11 @@ impl ReflectAsset {
8185
/// # Safety
8286
/// This method does not prevent you from having two mutable pointers to the same data,
8387
/// violating Rust's aliasing rules. To avoid this:
84-
/// * Only call this method when you have exclusive access to the world
85-
/// (or use a scheduler that enforces unique access to the `Assets` resource).
88+
/// * Only call this method if you know that the [`UnsafeWorldCell`] may be used to access the corresponding `Assets<T>`
8689
/// * Don't call this method more than once in the same scope.
8790
pub unsafe fn get_unchecked_mut<'w>(
8891
&self,
89-
world: &'w World,
92+
world: UnsafeWorldCell<'w>,
9093
handle: HandleUntyped,
9194
) -> Option<&'w mut dyn Reflect> {
9295
// SAFETY: requirements are deferred to the caller
@@ -140,18 +143,10 @@ impl<A: Asset + FromReflect> FromType<A> for ReflectAsset {
140143
let asset = assets.get(&handle.typed());
141144
asset.map(|asset| asset as &dyn Reflect)
142145
},
143-
get_mut: |world, handle| {
144-
let assets = world.resource_mut::<Assets<A>>().into_inner();
145-
let asset = assets.get_mut(&handle.typed());
146-
asset.map(|asset| asset as &mut dyn Reflect)
147-
},
148146
get_unchecked_mut: |world, handle| {
149-
let assets = unsafe {
150-
world
151-
.get_resource_unchecked_mut::<Assets<A>>()
152-
.unwrap()
153-
.into_inner()
154-
};
147+
// SAFETY: `get_unchecked_mut` must be callied with `UnsafeWorldCell` having access to `Assets<A>`,
148+
// and must ensure to only have at most one reference to it live at all times.
149+
let assets = unsafe { world.get_resource_mut::<Assets<A>>().unwrap().into_inner() };
155150
let asset = assets.get_mut(&handle.typed());
156151
asset.map(|asset| asset as &mut dyn Reflect)
157152
},

crates/bevy_ecs/src/reflect.rs

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::{
55
component::Component,
66
entity::{Entity, EntityMap, MapEntities, MapEntitiesError},
77
system::Resource,
8-
world::{FromWorld, World},
8+
world::{unsafe_world_cell::UnsafeWorldCell, FromWorld, World},
99
};
1010
use bevy_reflect::{
1111
impl_from_reflect_value, impl_reflect_value, FromType, Reflect, ReflectDeserialize,
@@ -52,7 +52,10 @@ pub struct ReflectComponentFns {
5252
/// Function pointer implementing [`ReflectComponent::reflect()`].
5353
pub reflect: fn(&World, Entity) -> Option<&dyn Reflect>,
5454
/// Function pointer implementing [`ReflectComponent::reflect_mut()`].
55-
pub reflect_mut: unsafe fn(&World, Entity) -> Option<Mut<dyn Reflect>>,
55+
///
56+
/// # Safety
57+
/// The function may only be called with an [`UnsafeWorldCell`] that can be used to mutably access the relevant component on the given entity.
58+
pub reflect_mut: unsafe fn(UnsafeWorldCell<'_>, Entity) -> Option<Mut<'_, dyn Reflect>>,
5659
/// Function pointer implementing [`ReflectComponent::copy()`].
5760
pub copy: fn(&World, &mut World, Entity, Entity),
5861
}
@@ -117,20 +120,20 @@ impl ReflectComponent {
117120
entity: Entity,
118121
) -> Option<Mut<'a, dyn Reflect>> {
119122
// SAFETY: unique world access
120-
unsafe { (self.0.reflect_mut)(world, entity) }
123+
unsafe { (self.0.reflect_mut)(world.as_unsafe_world_cell(), entity) }
121124
}
122125

123126
/// # Safety
124127
/// This method does not prevent you from having two mutable pointers to the same data,
125128
/// violating Rust's aliasing rules. To avoid this:
126-
/// * Only call this method in an exclusive system to avoid sharing across threads (or use a
127-
/// scheduler that enforces safe memory access).
129+
/// * Only call this method with a [`UnsafeWorldCell`] that may be used to mutably access the component on the entity `entity`
128130
/// * Don't call this method more than once in the same scope for a given [`Component`].
129131
pub unsafe fn reflect_unchecked_mut<'a>(
130132
&self,
131-
world: &'a World,
133+
world: UnsafeWorldCell<'a>,
132134
entity: Entity,
133135
) -> Option<Mut<'a, dyn Reflect>> {
136+
// SAFETY: safety requirements deferred to caller
134137
(self.0.reflect_mut)(world, entity)
135138
}
136139

@@ -209,16 +212,14 @@ impl<C: Component + Reflect + FromWorld> FromType<C> for ReflectComponent {
209212
.map(|c| c as &dyn Reflect)
210213
},
211214
reflect_mut: |world, entity| {
212-
// SAFETY: reflect_mut is an unsafe function pointer used by `reflect_unchecked_mut` which promises to never
213-
// produce aliasing mutable references, and reflect_mut, which has mutable world access
215+
// SAFETY: reflect_mut is an unsafe function pointer used by
216+
// 1. `reflect_unchecked_mut` which must be called with an UnsafeWorldCell with access to the the component `C` on the `entity`, and
217+
// 2. `reflect_mut`, which has mutable world access
214218
unsafe {
215-
world
216-
.get_entity(entity)?
217-
.get_unchecked_mut::<C>(world.last_change_tick(), world.read_change_tick())
218-
.map(|c| Mut {
219-
value: c.value as &mut dyn Reflect,
220-
ticks: c.ticks,
221-
})
219+
world.get_entity(entity)?.get_mut::<C>().map(|c| Mut {
220+
value: c.value as &mut dyn Reflect,
221+
ticks: c.ticks,
222+
})
222223
}
223224
},
224225
})
@@ -265,7 +266,10 @@ pub struct ReflectResourceFns {
265266
/// Function pointer implementing [`ReflectResource::reflect()`].
266267
pub reflect: fn(&World) -> Option<&dyn Reflect>,
267268
/// Function pointer implementing [`ReflectResource::reflect_unchecked_mut()`].
268-
pub reflect_unchecked_mut: unsafe fn(&World) -> Option<Mut<dyn Reflect>>,
269+
///
270+
/// # Safety
271+
/// The function may only be called with an [`UnsafeWorldCell`] that can be used to mutably access the relevant resource.
272+
pub reflect_unchecked_mut: unsafe fn(UnsafeWorldCell<'_>) -> Option<Mut<'_, dyn Reflect>>,
269273
/// Function pointer implementing [`ReflectResource::copy()`].
270274
pub copy: fn(&World, &mut World),
271275
}
@@ -314,19 +318,18 @@ impl ReflectResource {
314318
/// Gets the value of this [`Resource`] type from the world as a mutable reflected reference.
315319
pub fn reflect_mut<'a>(&self, world: &'a mut World) -> Option<Mut<'a, dyn Reflect>> {
316320
// SAFETY: unique world access
317-
unsafe { (self.0.reflect_unchecked_mut)(world) }
321+
unsafe { (self.0.reflect_unchecked_mut)(world.as_unsafe_world_cell()) }
318322
}
319323

320324
/// # Safety
321325
/// This method does not prevent you from having two mutable pointers to the same data,
322326
/// violating Rust's aliasing rules. To avoid this:
323-
/// * Only call this method in an exclusive system to avoid sharing across threads (or use a
324-
/// scheduler that enforces safe memory access).
327+
/// * Only call this method with an [`UnsafeWorldCell`] which can be used to mutably access the resource.
325328
/// * Don't call this method more than once in the same scope for a given [`Resource`].
326-
pub unsafe fn reflect_unchecked_mut<'a>(
329+
pub unsafe fn reflect_unchecked_mut<'w>(
327330
&self,
328-
world: &'a World,
329-
) -> Option<Mut<'a, dyn Reflect>> {
331+
world: UnsafeWorldCell<'w>,
332+
) -> Option<Mut<'w, dyn Reflect>> {
330333
// SAFETY: caller promises to uphold uniqueness guarantees
331334
(self.0.reflect_unchecked_mut)(world)
332335
}
@@ -385,7 +388,7 @@ impl<C: Resource + Reflect + FromWorld> FromType<C> for ReflectResource {
385388
// SAFETY: all usages of `reflect_unchecked_mut` guarantee that there is either a single mutable
386389
// reference or multiple immutable ones alive at any given point
387390
unsafe {
388-
world.get_resource_unchecked_mut::<C>().map(|res| Mut {
391+
world.get_resource_mut::<C>().map(|res| Mut {
389392
value: res.value as &mut dyn Reflect,
390393
ticks: res.ticks,
391394
})

crates/bevy_ecs/src/system/query.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1134,6 +1134,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> {
11341134
}
11351135
let world = self.world;
11361136
let entity_ref = world
1137+
.as_unsafe_world_cell_migration_internal()
11371138
.get_entity(entity)
11381139
.ok_or(QueryComponentError::NoSuchEntity)?;
11391140
let component_id = world
@@ -1150,7 +1151,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> {
11501151
.has_write(archetype_component)
11511152
{
11521153
entity_ref
1153-
.get_unchecked_mut::<T>(self.last_change_tick, self.change_tick)
1154+
.get_mut_using_ticks::<T>(self.last_change_tick, self.change_tick)
11541155
.ok_or(QueryComponentError::MissingComponent)
11551156
} else {
11561157
Err(QueryComponentError::MissingWriteAccess)

crates/bevy_ecs/src/system/system_param.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,8 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
541541
change_tick: u32,
542542
) -> Self::Item<'w, 's> {
543543
let value = world
544-
.get_resource_unchecked_mut_with_id(component_id)
544+
.as_unsafe_world_cell_migration_internal()
545+
.get_resource_mut_with_id(component_id)
545546
.unwrap_or_else(|| {
546547
panic!(
547548
"Resource requested by {} does not exist: {}",
@@ -578,7 +579,8 @@ unsafe impl<'a, T: Resource> SystemParam for Option<ResMut<'a, T>> {
578579
change_tick: u32,
579580
) -> Self::Item<'w, 's> {
580581
world
581-
.get_resource_unchecked_mut_with_id(component_id)
582+
.as_unsafe_world_cell_migration_internal()
583+
.get_resource_mut_with_id(component_id)
582584
.map(|value| ResMut {
583585
value: value.value,
584586
ticks: TicksMut {

0 commit comments

Comments
 (0)