Skip to content
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

[Merged by Bors] - add UnsafeWorldCell abstraction #6404

Closed

Conversation

jakobhellermann
Copy link
Contributor

@jakobhellermann jakobhellermann commented Oct 29, 2022

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:

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
}
UnsafeWorldCell docs

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
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) in UnsafeCells, and carefully validates disjoint access patterns using
APIs like System::component_access.

A system then can be executed using 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.

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)
}

@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change P-Unsound A bug that results in undefined compiler behavior and removed X-Controversial There is active debate or serious implications around merging this PR labels Oct 29, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very well-made and documented. Overall, I like this much better than unsafe accessor functions for every usage of the World.

A couple of questions to get a better sense of the architectural direction:

  1. What external use cases do you see this being useful for?
  2. In the long-term, how does this fit in with WorldCell (which also allows split mutable access, but via reference counting)? Should we expand WorldCell to match / use this API? Should we remove it entirely in favor of InteriorMutableWorld? Should we unify them and have an option to turn on ref-counting?
  3. In the long-term, what would you like to use this for internally?

@jakobhellermann
Copy link
Contributor Author

jakobhellermann commented Oct 31, 2022

Good questions.

  1. What external use cases do you see this being useful for?

The use case I had is for bevy-inspector-egui. There I want to get a mutable reference to one value (e.g. a component Handle<StandardMaterial>) pass store the remaining &mut World without that particular component in a context. Code walking a reflect object can then use the remaining context to look at the Assets<StandardMaterial> resource and get a &mut StandardMaterial. This recursively continues, so &mut StandardMaterial is then displayed but the context has a &mut World except for the handle component and the Assets<StandardMaterial> resource.
I packaged this into the RestrictedWorldView abstraction.
This basically builds a permission tree in the world:

&mut World
├── Handle@Entity1
└── everything -Handle@Entity1
    ├── Assets<StandardMaterial>
    └── everything -Handle -Assets<StandardMaterial>

Currently this stores a &World and works with _get_unchecked_mut methods and #5922, but a InteriorMutableWorld would document its purpose more clearly.

Another use case is running scripts in parallel (basically the same use case as bevy has with systems) where the signature

fn run_script(world: InteriorMutableWorld<'_>) {}

is much more self-documenting than the &World variant with unchecked methods.


  1. In the long-term, how does this fit in with WorldCell

WorldCell is an API that contains a &mut World and hands out multiple &mut Rs to resources.
It never actually uses &mut World methods, it only calls &world.get_resource_unchecked_mut internally.
With InteriorMutableWorld it calls world.as_interior_mutable().get_resource_mut() instead and works exactly the same otherwise.

InteriorMutableWorld relates to WorldCell similar to how UnsafeCell relates to RefCell. It is used as an unsafe building block which needs additional structure and information to get a safe API on top of it.


  1. In the long-term, what would you like to use this for internally?

In this PR: Change the signature of the unsafe methods in ReflectResource, ReflectComponent and ReflectAsset.
Long-term: It could be used in System::run_unsafe, SystemParamFetch::get_param and QueryState::iter_unchecked & friends.

@BoxyUwU BoxyUwU self-requested a review November 4, 2022 10:34
@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 15, 2022

It needs to be &mut World -> InteriorMutableWorld<'_> otherwise this is relatively pointless. The goal behind InteriorMutableWorld should be to preserve the semantics that &World means shared/readonly, and &mut World means exclusive/mutable. We also dont want the unsafe methods for getting mutable borrows out of &World to have to care about what safe code might be doing with another &World which is only accomplished via &mut World -> InteriorMutableWorld<'_>

@jakobhellermann
Copy link
Contributor Author

@BoxyUwU

It needs to be &mut World -> InteriorMutableWorld<'_> otherwise this is relatively pointless.

Right, the default can be a safe &mut World -> InteriorMutableWorld<'w> function which can be used most of the time.

We still need a function to go from &World to InteriorMutableWorld function, the question is who exactly has to uphold which invariants.
Consider

pub fn get<'w, 's>(
&'s mut self,
world: &'w World,
) -> <Param::Fetch as SystemParamFetch<'w, 's>>::Item
where
Param::Fetch: ReadOnlySystemParamFetch,
{
self.validate_world_and_update_archetypes(world);
// SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with.
unsafe { self.get_unchecked_manual(world) }
}

In the future, get_unchecked_manual will take a InteriorMutableWorld. But get should take a &World, that is correct.
What is the safety contract of get_unchecked_manual?
Something like "You can only call this with a InteriorMutableWorld that may be used to access everything defined in this state's component_access_set".

And the new safety comment here would be

- `Param::Fetch` is `ReadOnlySystemParamFetch` so our component_access will be a set of only reads
- we have a `&World` which allows read access to everything
- so we can call `get_unchecked_manual` with an InteriorMutableWorld created from the World

Note that not the creation of the InteriorMutableWorld was unsafe, but passing that world to get_unchecked_manual.


A InteriorMutableWorld by default can be used for absolutely no resource/component access.
I you have a

fn some_safe_function(world: InteriorMutableWorld<'_>) {
 // you can access *no* resources here, neither mutable nor immutable. What would you write as the Safety comment? You have no permission.
}

The way that the InteriorMutableWorld will be used is more like this:

/// SAFETY: may only be called with a interior mutable world that may be used to access MyResource mutably
unsafe fn some_unsafe_function(world: InteriorMutableWorld<'_>) {
  // SAFETY: we're allowed to access it
  let res = unsafe { world.get_resource_mut::<MyResource>() };
}

So what should be the safety comment of a function &World -> InteriorMutableWorld<'_> be?
In my mental model, this is completely safe, because without additional context, you can do nothing with the InteriorMutableWorld. All its spicy methods are unsafe and need explicit permission through safety comments.
Everyone function that accepts a InteriorMutableWorld needs to specify exactly what needs to be able to do with it, and every caller of such functions has to make sure that it can provide these guarantees. If you have a &mut World that's easy, and if you have a &World it is still possible sometimes (see SystemState::get where Param::Fetch: ReadyOnlySystemParamFetch).

@jakobhellermann
Copy link
Contributor Author

&mut World -> InteriorMutableWorld creates a view into the world that may be used for anything.
&World -> InteriorMutableWorld creates a view into the world that my be used for any read-only access.

Anything else (e.g. a world that can only access according to a FilteredAccessSet<ComponentId> or a world that may only access one specific resource) must be guaranteed by some additional requirements, such as where Param::Fetch: ReadOnlySystemParamFetch.

@jakobhellermann
Copy link
Contributor Author

As another example,

unsafe fn init_fetch<'w>(
world: &'w World,
state: &Self::State,
last_change_tick: u32,
change_tick: u32,
) -> Self::Fetch<'w>;

WorldQuery::init_fetch would take a InteriorMutableWorld that comes with no extra promises.
As such, it can not be used to access any resources or components, but you can e.g. prepare a WriteFetch by storing the pointer to a particular ComponentSparseSet.

Doing this is completely safe, both inside the function (interior_mutable_world.storages().sparse_sets.get(component_id) is safe) and as the caller, since you can pass in any InteriorMutableWorld, even from a system with absolutely no access.

@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 30, 2022

&World -> InteriorMutableWorld makes sense to me if we have a /// Can only be used for read only access on the function. My objection would be to allowing people to pull mutable references out of an InteriorMutableWorld made from an &World (which is what this PR currently does)

@jakobhellermann
Copy link
Contributor Author

Agreed.
What do you think of this API?

impl World {
  // Grants read-write access to the entire world
  fn as_interior_mutable(&mut self) -> InteriorMutableWorld<'_> {}
  // Grants read-only access to the entire world
  fn as_interior_mutable_readonly(&self) -> InteriorMutableWorld<'_> {}
}

?

@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 30, 2022

Yeah that looks good to me

@DJMcNab
Copy link
Member

DJMcNab commented Nov 30, 2022

Would something like PartialWorld be a better name?

@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 30, 2022

I don't think so, it doesnt have anything making it seem like interior mutability is going on (InteriorMutableWorld is maybe a bit heavy handed lol but WorldCell is already taken... i guess UnsafeWorldCell would also work and kinda mirrors UnsafeCell's completely unchecked nature and mirroring of Cell..)

@jakobhellermann jakobhellermann force-pushed the interior-mutable-world branch 3 times, most recently from 46fb354 to 31caf80 Compare December 17, 2022 12:32
@jakobhellermann
Copy link
Contributor Author

I added the methods

    /// Creates a new [`InteriorMutableWorld`] view with complete read+write access
    pub fn as_interior_mutable(&mut self) -> InteriorMutableWorld<'_>:
   
    /// Creates a new [`InteriorMutableWorld`] view only read access to everything
    pub fn as_interior_mutable_readonly(&self) -> InteriorMutableWorld<'_>:

    /// Creates a new [`InteriorMutableWorld`] with read+write access from a [&World](World).
    /// This is only a temporary measure until every `&World` that is semantically a [InteriorMutableWorld]
    /// has been replaced.
    pub(crate) fn as_interior_mutable_migration_internal(&self) -> InteriorMutableWorld<'_>;

The last one will be removed in a PR I will put up soon.

There is one new change here I'd like feedback on, regarding WorldCell:
WorldCell previously contained a &mut World. It has &self methods that perform interior mutability.
The obvious solution is to store a InteriorMutableWorld that gets created with full access in WorldCell::new(&mut World).
This is great for all the methods, except the Drop impl which swaps the archetype_component_access field on the &mut World which is only used as a cache. This is obviously not possible through a &World/InteriorMutableWorld.
My solution here was to put that field in an UnsafeCell that is only access in the Drop impl.

The alternative would be to keep storing &mut World and turning that into a InteriorMutableWorld just when we need it in get_resource etc., but that doesn't work because those methods take &self and as such can't use InteriorMutableWorld::new(&mut World).

@JoJoJet
Copy link
Member

JoJoJet commented Dec 22, 2022

impl World {
  // Grants read-write access to the entire world
  fn as_interior_mutable(&mut self) -> InteriorMutableWorld<'_> {}
  // Grants read-only access to the entire world
  fn as_interior_mutable_readonly(&self) -> InteriorMutableWorld<'_> {}
}

What's the motivation for a read-only interior mutable world? Couldn't you just use &World for that?

@jakobhellermann
Copy link
Contributor Author

What's the motivation for a read-only interior mutable world? Couldn't you just use &World for that?

If you have an unsafe method that takes a InteriorMutableWorld and want to call that from a function with access to a &World where you know that the unsafe method won't write to anything it is useful:
4f04a57#diff-aa2678d5c3fe9adbe529e57567595039a862b26cc787cfea208678b4d323c2dbR172

@JoJoJet
Copy link
Member

JoJoJet commented Dec 22, 2022

If the function doesn't write to anything, why not just have it take &World instead of InteriorMutableWorld?

Edit: Nevermind, I see. In the example you linked, get_unchecked_manual sometimes gives mutable access.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 24, 2023
@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 24, 2023

I think that UnsafeWorldCell should have a way to pull &mut World out of it. Would be useful for the actual WorldCell impl and also the scheduler seems to want to go from &UnsafeCell<World> -> &mut World and it seems a bit sus to have the "most flexible unsafe world access" type be unable to express two of the interior mutability abstractions in bevy_ecs.

I think that we should add a unsafe fn(self) -> &'w mut World and make all the safe world metadata accesses unsafe with a safety invariant like "must be no active &mut World" 🤔

@james7132
Copy link
Member

I think that UnsafeWorldCell should have a way to pull &mut World out of it. Would be useful for the actual WorldCell impl and also the scheduler seems to want to go from &UnsafeCell<World> -> &mut World and it seems a bit sus to have the "most flexible unsafe world access" type be unable to express two of the interior mutability abstractions in bevy_ecs.

Isn't this unsound unless we put a &UnsafeCell<World> in it?

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 24, 2023

yes

@james7132
Copy link
Member

james7132 commented Jan 27, 2023

Discussed with @BoxyUwU that their comments above are not blocking and can be done in a followup PR. Counting that as 2 SME approvals.

bors r+

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>
@bors bors bot changed the title add UnsafeWorldCell abstraction [Merged by Bors] - add UnsafeWorldCell abstraction Jan 27, 2023
@bors bors bot closed this Jan 27, 2023
@jakobhellermann jakobhellermann deleted the interior-mutable-world branch January 27, 2023 10:49
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 11, 2023
# Objective

Make the name less verbose without sacrificing clarity.

---

## Migration Guide

*Note for maintainers:* This PR has no breaking changes relative to bevy 0.9. Instead of this PR having its own migration guide, we should just edit the changelog for #6404.

The type `UnsafeWorldCellEntityRef` has been renamed to `UnsafeEntityCell`.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 14, 2023
)

# Objective

Make the name less verbose without sacrificing clarity.

---

## Migration Guide

*Note for maintainers:* This PR has no breaking changes relative to bevy 0.9. Instead of this PR having its own migration guide, we should just edit the changelog for bevyengine#6404.

The type `UnsafeWorldCellEntityRef` has been renamed to `UnsafeEntityCell`.
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Feb 15, 2023
)

# Objective

Make the name less verbose without sacrificing clarity.

---

## Migration Guide

*Note for maintainers:* This PR has no breaking changes relative to bevy 0.9. Instead of this PR having its own migration guide, we should just edit the changelog for bevyengine#6404.

The type `UnsafeWorldCellEntityRef` has been renamed to `UnsafeEntityCell`.
alice-i-cecile pushed a commit that referenced this pull request Apr 1, 2023
# Objective

The type `&World` is currently in an awkward place, since it has two
meanings:
1. Read-only access to the entire world.
2. Interior mutable access to the world; immutable and/or mutable access
to certain portions of world data.

This makes `&World` difficult to reason about, and surprising to see in
function signatures if one does not know about the interior mutable
property.

The type `UnsafeWorldCell` was added in #6404, which is meant to
alleviate this confusion by adding a dedicated type for interior mutable
world access. However, much of the engine still treats `&World` as an
interior mutable-ish type. One of those places is `SystemParam`.

## Solution

Modify `SystemParam::get_param` to accept `UnsafeWorldCell` instead of
`&World`. Simplify the safety invariants, since the `UnsafeWorldCell`
type encapsulates the concept of constrained world access.

---

## Changelog

`SystemParam::get_param` now accepts an `UnsafeWorldCell` instead of
`&World`. This type provides a high-level API for unsafe interior
mutable world access.

## Migration Guide

For manual implementers of `SystemParam`: the function `get_item` now
takes `UnsafeWorldCell` instead of `&World`. To access world data, use:

* `.get_entity()`, which returns an `UnsafeEntityCell` which can be used
to access component data.
* `get_resource()` and its variants, to access resource data.
alice-i-cecile pushed a commit that referenced this pull request Jun 15, 2023
# Objective

Follow-up to #6404 and #8292.

Mutating the world through a shared reference is surprising, and it
makes the meaning of `&World` unclear: sometimes it gives read-only
access to the entire world, and sometimes it gives interior mutable
access to only part of it.

This is an up-to-date version of #6972.

## Solution

Use `UnsafeWorldCell` for all interior mutability. Now, `&World`
*always* gives you read-only access to the entire world.

---

## Changelog

TODO - do we still care about changelogs?

## Migration Guide

Mutating any world data using `&World` is now considered unsound -- the
type `UnsafeWorldCell` must be used to achieve interior mutability. The
following methods now accept `UnsafeWorldCell` instead of `&World`:

- `QueryState`: `get_unchecked`, `iter_unchecked`,
`iter_combinations_unchecked`, `for_each_unchecked`,
`get_single_unchecked`, `get_single_unchecked_manual`.
- `SystemState`: `get_unchecked_manual`

```rust
let mut world = World::new();
let mut query = world.query::<&mut T>();

// Before:
let t1 = query.get_unchecked(&world, entity_1);
let t2 = query.get_unchecked(&world, entity_2);

// After:
let world_cell = world.as_unsafe_world_cell();
let t1 = query.get_unchecked(world_cell, entity_1);
let t2 = query.get_unchecked(world_cell, entity_2);
```

The methods `QueryState::validate_world` and
`SystemState::matches_world` now take a `WorldId` instead of `&World`:

```rust
// Before:
query_state.validate_world(&world);

// After:
query_state.validate_world(world.id());
```

The methods `QueryState::update_archetypes` and
`SystemState::update_archetypes` now take `UnsafeWorldCell` instead of
`&World`:

```rust
// Before:
query_state.update_archetypes(&world);

// After:
query_state.update_archetypes(world.as_unsafe_world_cell_readonly());
```
github-merge-queue bot pushed a commit that referenced this pull request Oct 2, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (#6404, #8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants