[Merged by Bors] - refactor: move internals from entity_ref to World, add SAFETY comments
#6402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakobhellermann
added
A-ECS
|
CI will get fixed in 4 days with 1.65: rust-lang/rust#100081 |
|
I agree with the code reorg. Lemme take a look at the safety comments... |
alice-i-cecile
requested changes
Oct 29, 2022
There was a problem hiding this comment.
Okay, so the safety docs have some problems 😂 They're pretty much all repeated, and I didn't call them out in each location to reduce spam.
Not your fault by any means (these docs are much better than we had), but eh, I'd like to clean them up while we're here.
crates/bevy_ecs/src/storage/mod.rs
Outdated
| /// Get a raw pointer to a particular [`Component`](crate::component::Component) and its [`ComponentTicks`] identified by their [`TypeId`] | ||
| /// | ||
| /// # Safety | ||
| /// - `entity_location` must be within bounds of the given archetype and `entity` must exist inside |
There was a problem hiding this comment.
A specific archetype isn't given, so this comment is unclear.
crates/bevy_ecs/src/storage/mod.rs
Outdated
| /// | ||
| /// # Safety | ||
| /// - `entity_location` must be within bounds of the given archetype and `entity` must exist inside | ||
| /// - `component_id` must be valid |
There was a problem hiding this comment.
I'd like a bit more advice here on what it means for a component id to be valid. Probably best suited to living on the ComponentId docs though.
james7132
reviewed
Nov 14, 2022
james7132
reviewed
Nov 14, 2022
jakobhellermann
force-pushed
the
move-internals-to-storages
branch
from
311d2a3 to
62da22d
Compare
|
Rebased and pushed a few commits cleaning up the comments. |
jakobhellermann
force-pushed
the
move-internals-to-storages
branch
from
62da22d to
9673cbf
Compare
jakobhellermann
changed the title
entity_ref to storages, add SAFETY commentsentity_ref to World, add SAFETY comments
james7132
requested changes
Dec 9, 2022
james7132
approved these changes
Dec 17, 2022
crates/bevy_ecs/src/world/mod.rs
Outdated
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn fetch_table( |
There was a problem hiding this comment.
I don't understand why this method is unsafe: it needs safety comments and a unsafe block highlighting the unsafe function call.
alice-i-cecile
requested changes
Jan 11, 2023
| /// - `components`, `archetypes` and `storages` must come from the same world | ||
| /// - The relevant table row **must be removed** by the caller once all components are taken | ||
| #[inline] | ||
| pub(crate) unsafe fn take_component<'a>( |
There was a problem hiding this comment.
The PR description suggests that this is a method on Storages (which I like better), but here it's a freestanding function.
There was a problem hiding this comment.
I first moved all the functions to Storages, but then moved them to World instead.
take_component can't be moved, because of how it is called: #6402 (comment)
I can move it back to Storages as the only method there, or leave it here. I personally don't care either way.
There was a problem hiding this comment.
No strong feelings, I just want the PR description to be accurate :)
…and_ticks[_with_type] to Storages
…private imports, add safety comments
jakobhellermann
force-pushed
the
move-internals-to-storages
branch
from
cd69b55 to
49e33a5
Compare
|
Rebased and addressed the comments (only #6402 (comment) pending) |
alice-i-cecile
approved these changes
Jan 13, 2023
|
I updated the PR description |
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 13, 2023
…omments (#6402) # Objective There are some utility functions for actually working with `Storages` inside `entity_ref.rs` that are used both for `EntityRef/EntityMut` and `World`, with a `// TODO: move to Storages`. This PR moves them to private methods on `World`, because that's the safest API boundary. On `Storages` you would need to ensure that you pass `Components` from the same world. ## Solution - move get_component[_with_type], get_ticks[_with_type], get_component_and_ticks[_with_type] to `World` (still pub(crate)) - replace `pub use entity_ref::*;` with `pub use entity_ref::{EntityRef, EntityMut}` and qualified `entity_ref::get_mut[_by_id]` in `world.rs` - add safety comments to a bunch of methods
bors
bot
changed the title
entity_ref to World, add SAFETY commentsentity_ref to World, add SAFETY comments
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
…omments (bevyengine#6402) # Objective There are some utility functions for actually working with `Storages` inside `entity_ref.rs` that are used both for `EntityRef/EntityMut` and `World`, with a `// TODO: move to Storages`. This PR moves them to private methods on `World`, because that's the safest API boundary. On `Storages` you would need to ensure that you pass `Components` from the same world. ## Solution - move get_component[_with_type], get_ticks[_with_type], get_component_and_ticks[_with_type] to `World` (still pub(crate)) - replace `pub use entity_ref::*;` with `pub use entity_ref::{EntityRef, EntityMut}` and qualified `entity_ref::get_mut[_by_id]` in `world.rs` - add safety comments to a bunch of methods
bors bot
pushed a commit
that referenced
this pull request
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>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…omments (bevyengine#6402) # Objective There are some utility functions for actually working with `Storages` inside `entity_ref.rs` that are used both for `EntityRef/EntityMut` and `World`, with a `// TODO: move to Storages`. This PR moves them to private methods on `World`, because that's the safest API boundary. On `Storages` you would need to ensure that you pass `Components` from the same world. ## Solution - move get_component[_with_type], get_ticks[_with_type], get_component_and_ticks[_with_type] to `World` (still pub(crate)) - replace `pub use entity_ref::*;` with `pub use entity_ref::{EntityRef, EntityMut}` and qualified `entity_ref::get_mut[_by_id]` in `world.rs` - add safety comments to a bunch of methods
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
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>
bors bot
pushed a commit
that referenced
this pull request
Feb 15, 2023
…7665) # Objective - #6402 changed `World::fetch_table` (now `UnsafeWorldCell::fetch_table`) to access the archetype in order to get the `table_id` and `table_row` of the entity involved. However this is useless since those were already present in the `EntityLocation` - Moreover it's useless for `UnsafeWorldCell::fetch_table` to return the `TableRow` too, since the caller must already have access to the `EntityLocation` which contains the `TableRow`. - The result is that `UnsafeWorldCell::fetch_table` now only does 2 memory fetches instead of 4. ## Solution - Revert the changes to the implementation of `UnsafeWorldCell::fetch_table` made in #6402
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this pull request
Feb 15, 2023
…evyengine#7665) # Objective - bevyengine#6402 changed `World::fetch_table` (now `UnsafeWorldCell::fetch_table`) to access the archetype in order to get the `table_id` and `table_row` of the entity involved. However this is useless since those were already present in the `EntityLocation` - Moreover it's useless for `UnsafeWorldCell::fetch_table` to return the `TableRow` too, since the caller must already have access to the `EntityLocation` which contains the `TableRow`. - The result is that `UnsafeWorldCell::fetch_table` now only does 2 memory fetches instead of 4. ## Solution - Revert the changes to the implementation of `UnsafeWorldCell::fetch_table` made in bevyengine#6402
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this pull request
Feb 15, 2023
…evyengine#7665) # Objective - bevyengine#6402 changed `World::fetch_table` (now `UnsafeWorldCell::fetch_table`) to access the archetype in order to get the `table_id` and `table_row` of the entity involved. However this is useless since those were already present in the `EntityLocation` - Moreover it's useless for `UnsafeWorldCell::fetch_table` to return the `TableRow` too, since the caller must already have access to the `EntityLocation` which contains the `TableRow`. - The result is that `UnsafeWorldCell::fetch_table` now only does 2 memory fetches instead of 4. ## Solution - Revert the changes to the implementation of `UnsafeWorldCell::fetch_table` made in bevyengine#6402
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this pull request
Feb 15, 2023
…evyengine#7665) # Objective - bevyengine#6402 changed `World::fetch_table` (now `UnsafeWorldCell::fetch_table`) to access the archetype in order to get the `table_id` and `table_row` of the entity involved. However this is useless since those were already present in the `EntityLocation` - Moreover it's useless for `UnsafeWorldCell::fetch_table` to return the `TableRow` too, since the caller must already have access to the `EntityLocation` which contains the `TableRow`. - The result is that `UnsafeWorldCell::fetch_table` now only does 2 memory fetches instead of 4. ## Solution - Revert the changes to the implementation of `UnsafeWorldCell::fetch_table` made in bevyengine#6402
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
There are some utility functions for actually working with
Storagesinsideentity_ref.rsthat are used both forEntityRef/EntityMutandWorld, with a// TODO: move to Storages.This PR moves them to private methods on
World, because that's the safest API boundary. OnStoragesyou would need to ensure that you passComponentsfrom the same world.Solution
World(still pub(crate))pub use entity_ref::*;withpub use entity_ref::{EntityRef, EntityMut}and qualifiedentity_ref::get_mut[_by_id]inworld.rs