-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abstraction for working with a &World
and interior mutability
#5956
Labels
A-ECS
Entities, components, systems, and events
C-Code-Quality
A section of code that is hard to understand or change
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
P-Unsound
A bug that results in undefined compiler behavior
Comments
jakobhellermann
added
C-Feature
A new feature, making something new possible
A-ECS
Entities, components, systems, and events
S-Needs-Triage
This issue needs to be labelled
labels
Sep 12, 2022
alice-i-cecile
added
P-Unsound
A bug that results in undefined compiler behavior
C-Code-Quality
A section of code that is hard to understand or change
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
and removed
S-Needs-Triage
This issue needs to be labelled
C-Feature
A new feature, making something new possible
labels
Sep 12, 2022
This was referenced Oct 29, 2022
bors bot
pushed a commit
that referenced
this issue
Jan 27, 2023
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>
This was done in #6404 |
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this issue
Feb 1, 2023
alternative to bevyengine#5922, implements bevyengine#5956 builds on top of bevyengine#6402 # Objective bevyengine#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: bevyengine#5922 (comment) Also, this is something that **cannot be implemented outside of bevy**, so having either this PR or bevyengine#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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-ECS
Entities, components, systems, and events
C-Code-Quality
A section of code that is hard to understand or change
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
P-Unsound
A bug that results in undefined compiler behavior
Motivation
The essential rule for aliasing in rust is that you can have either multiple shared
&
references to a location, or a unique&mut
reference.This is sometimes too restrictive, e.g. when you have two systems that both share a reference to a
World
but you know that they don't access the same data. As an escape hatch rust provides theUnsafeCell<T>
type (and safe wrappers around it), which is special in that it lets you get a&mut T
from a&UnsafeCell<T>
, provided that you manually uphold the safety requirements. Notable, this doesn't change anything about the fact that you can't have two aliasing&mut
s. The only thing that this allows, and that we make use of, is building abstractions like these:where you can pass a
&World
around and expose someunsafe fn World::get_unchecked_mut<T>(&World, Entity) -> Option<&mut T>
.Why is this not enough?
This works if you carefully document the
unchecked_mut
safety contracts and take care to not violate them. It is still error prone, because when you have a&World
that only lets you access resourceFoo
, safe code can do bad things.This is because all the methods on
World
likeWorld::get_resource
are safe, which is usually corrent when treating&World
as an immutable world reference. This isn't the case anymore if it is used as a access restricted kinda-mutable world.Solution
Add a new wrapper type around
&World
(hereInteriorMutableWorld
, name up for discussion). Using this type documents intent better and makes writing unsafe code easier than using a&World
directly.Proposed API
After this abstraction has been built, we should update our internal use of
&World
toInteriorMutableWorld
, for examplePrior Art
#5588 builds something similar, but custom built for the
QueryState
use case.Implementation Questions
When we have
World::get_resource_mut
andInteriorMutableWorld::get_resource_mut
we don't want code duplication.Do we
get_resource_mut_inner(&self)
onWorld
and use it in both orInteriorMutableWorld
and callself.as_interior_mutable().get_resource_mut(self)
inWorld
Same question for
get_resource
, but here we could also define it inWorld
and letInteriorMutableWorld
just callself.0.get_resource
.The text was updated successfully, but these errors were encountered: