get `unchecked_mut` variants to `by_id` methods #5922

jakobhellermann · 2022-09-09T09:01:56Z

Objective

The World stores its components and resources in UnsafeCells, which means that (with the necessary care) you can get a &mut T from a &World soundly. Having two &mut Worlds however is unsound.

To support use cases like bevy's systems where multiple callers share a &World but mutably access different parts of it, there are _unchecked versions of many functions:

EntityRef::get_unchecked_mut
World::get_resource_unchecked_mut
Query:.get_unchecked
... (https://docs.rs/bevy_ecs/latest/bevy_ecs/?search=unchecked_mut)

I have a use case where I have two &Worlds where one may access resources and the other one may access components, and I would like to use untyped by_id methods there.

Solution

add World::get_resource_unchecked_mut_by_id
add EntityRef::get_unchecked_mut_by_id

both with safety comments like the by_id variant.

Changelog

add World::get_resource_unchecked_mut_by_id and EntityRef::get_unchecked_mut_by_id for manually synchronized mutable world access with untyped pointers

BoxyUwU · 2022-09-09T13:06:22Z

We should be moving in the opposite direction here, removing EntityRef::get_unchecked_mut not adding a _by_id variant

jakobhellermann · 2022-09-09T13:08:48Z

Isn't this pretty central to the design of the bevy world?
The world contains UnsafeCells in the storages, so multiple systems can share a &World while carefully making sure that their access patterns are disjoint.

How would that work without &World -> &mut T methods?

BoxyUwU · 2022-09-09T13:10:13Z

&World means shared access to components, we may use &World for interior mutability in places but this is a terrible idea as we also use &World to actually mean "shared access to all components".

jakobhellermann · 2022-10-29T15:17:33Z

I opened #6404 implementing my thoughts in #5956 on how to split our usages of &World to mean "shared access to the world" and &World to mean "manually synchronized through a mechanism like systems, but can actually mutably use specific things".

I rebased this PR anyways in hopes that if #6404 does not get merged in time for 0.9, that we can merge this PR instead as a stop gap solution.

I agree that we should be moving into a direction where get_unchecked_mut does not exist, but until then I would much prefer

fn World::get_resource_unchecked_mut
fn World::get_resource_unchecked_mut_by_id
fn EntityRef::get_unchecked_mut
fn EntityRef::get_unchecked_mut_by_id

over

fn World::get_resource_unchecked_mut
fn EntityRef::get_unchecked_mut

which already exists but is incomplete and makes certain things unimplementable as third-party crate authors.

tim-blackbird · 2022-11-04T10:33:29Z

crates/bevy_ecs/src/world/mod.rs

@@ -1462,14 +1481,14 @@ impl World {
            // 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
-            component_ticks: unsafe { ticks.deref_mut() },
+            component_ticks: ticks.deref_mut(),
            last_change_tick: self.last_change_tick(),
            change_tick: self.read_change_tick(),
        };

        Some(MutUntyped {
            // SAFETY: This function has exclusive access to the world so nothing aliases `ptr`.


This comment is no longer valid. Same for one above.

jakobhellermann · 2022-11-04T10:48:02Z

crates/bevy_ecs/src/world/entity_ref.rs

+    /// Unlike [`EntityRef::get`], this returns a raw pointer to the component,
+    /// which is only valid while the `'w` borrow of the lifetime is active.


This needs to be reworded. It is correct for get_by_id because there lifetimes are checked, but for get_unchecked_mut_by_id the caller has to manage their lifetimes themselves.

TheRawMeatball

Also currently need this.

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>

james7132 · 2023-01-27T00:19:56Z

Closing in favor of #6404, which resolves the same issues.

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>

jakobhellermann force-pushed the by-id-unchecked branch from 9df8cbd to 87ef333 Compare September 9, 2022 09:02

jakobhellermann added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Sep 9, 2022

jakobhellermann force-pushed the by-id-unchecked branch from 87ef333 to e0a6c71 Compare September 9, 2022 09:03

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 9, 2022

jakobhellermann mentioned this pull request Oct 29, 2022

[Merged by Bors] - add UnsafeWorldCell abstraction #6404

Closed

jakobhellermann force-pushed the by-id-unchecked branch 2 times, most recently from 73f2b98 to 5b90e52 Compare October 29, 2022 15:17

alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Oct 29, 2022

jakobhellermann force-pushed the by-id-unchecked branch from 5b90e52 to 6ed25de Compare November 4, 2022 10:21

tim-blackbird reviewed Nov 4, 2022

View reviewed changes

BoxyUwU self-requested a review November 4, 2022 10:34

jakobhellermann commented Nov 4, 2022

View reviewed changes

jakobhellermann force-pushed the by-id-unchecked branch from 9daeae7 to 6cf5c66 Compare November 8, 2022 18:13

TheRawMeatball approved these changes Dec 22, 2022

View reviewed changes

jakobhellermann added 4 commits December 31, 2022 21:41

add get_resource_unchecked_mut_by_id

f5bdeb9

add EntityRef::get_unchecked_mut_by_id

ef85d54

clean up safety comments

9df2ac0

remove note about validity copied from non unchecked_mut variant

47bada4

jakobhellermann force-pushed the by-id-unchecked branch from 6cf5c66 to 47bada4 Compare December 31, 2022 20:43

james7132 closed this Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get `unchecked_mut` variants to `by_id` methods #5922

get `unchecked_mut` variants to `by_id` methods #5922

jakobhellermann commented Sep 9, 2022 •

edited

Loading

BoxyUwU commented Sep 9, 2022

jakobhellermann commented Sep 9, 2022

BoxyUwU commented Sep 9, 2022

jakobhellermann commented Oct 29, 2022

tim-blackbird Nov 4, 2022

jakobhellermann Nov 4, 2022

TheRawMeatball left a comment

james7132 commented Jan 27, 2023

		/// Unlike [`EntityRef::get`], this returns a raw pointer to the component,
		/// which is only valid while the `'w` borrow of the lifetime is active.

get unchecked_mut variants to by_id methods #5922

get unchecked_mut variants to by_id methods #5922

Conversation

jakobhellermann commented Sep 9, 2022 • edited Loading

Objective

Solution

Changelog

BoxyUwU commented Sep 9, 2022

jakobhellermann commented Sep 9, 2022

BoxyUwU commented Sep 9, 2022

jakobhellermann commented Oct 29, 2022

tim-blackbird Nov 4, 2022

Choose a reason for hiding this comment

jakobhellermann Nov 4, 2022

Choose a reason for hiding this comment

TheRawMeatball left a comment

Choose a reason for hiding this comment

james7132 commented Jan 27, 2023

get `unchecked_mut` variants to `by_id` methods #5922

get `unchecked_mut` variants to `by_id` methods #5922

jakobhellermann commented Sep 9, 2022 •

edited

Loading