[Merged by Bors] - add UnsafeWorldCell abstraction

[jakobhellermann]

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]

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]

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.

2. 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.

3. 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]

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]

@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

bevy/crates/bevy_ecs/src/system/function_system.rs

Lines 163 to 174 in 688f13c

	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]

&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]

As another example,

bevy/crates/bevy_ecs/src/query/fetch.rs

Lines 333 to 338 in 6763b31

	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]

&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]

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]

Yeah that looks good to me

[DJMcNab]

Would something like PartialWorld be a better name?

[BoxyUwU]

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]

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]

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]

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]

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.

[BoxyUwU]

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]

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]

yes

[james7132]

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]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• build-without-default-features (bevy_reflect)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• msrv
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12