Allow disjoint mutable world access via EntityMut (bevyengine#9419)

# Objective

Fix bevyengine#4278
Fix bevyengine#5504
Fix bevyengine#9422

Provide safe ways to borrow an entire entity, while allowing disjoint
mutable access. `EntityRef` and `EntityMut` are not suitable for this,
since they provide access to the entire world -- they are just helper
types for working with `&World`/`&mut World`.

This has potential uses for reflection and serialization

## Solution

Remove `EntityRef::world`, which allows it to soundly be used within
queries.

`EntityMut` no longer supports structural world mutations, which allows
multiple instances of it to exist for different entities at once.
Structural world mutations are performed using the new type
`EntityWorldMut`.

```rust
fn disjoint_system(
     q2: Query<&mut A>,
     q1: Query<EntityMut, Without<A>>,
) { ... }

let [entity1, entity2] = world.many_entities_mut([id1, id2]);
*entity1.get_mut::<T>().unwrap() = *entity2.get().unwrap();

for entity in world.iter_entities_mut() {
    ...
}
```

---

## Changelog

- Removed `EntityRef::world`, to fix a soundness issue with queries.
+ Removed the ability to structurally mutate the world using
`EntityMut`, which allows it to be used in queries.
+ Added `EntityWorldMut`, which is used to perform structural mutations
that are no longer allowed using `EntityMut`.

## Migration Guide

**Note for maintainers: ensure that the guide for bevyengine#9604 is updated
accordingly.**

Removed the method `EntityRef::world`, to fix a soundness issue with
queries. If you need access to `&World` while using an `EntityRef`,
consider passing the world as a separate parameter.

`EntityMut` can no longer perform 'structural' world mutations, such as
adding or removing components, or despawning the entity. Additionally,
`EntityMut::world`, `EntityMut::world_mut` , and
`EntityMut::world_scope` have been removed.
Instead, use the newly-added type `EntityWorldMut`, which is a helper
type for working with `&mut World`.

---------

Co-authored-by: Alice Cecile <[email protected]>

crates/bevy_ecs/src/archetype.rs

@@ -158,12 +158,12 @@ pub struct Edges {
 
 impl Edges {
     /// Checks the cache for the target archetype when adding a bundle to the
-    /// source archetype. For more information, see [`EntityMut::insert`].
+    /// source archetype. For more information, see [`EntityWorldMut::insert`].
     ///
     /// If this returns `None`, it means there has not been a transition from
     /// the source archetype via the provided bundle.
     ///
-    /// [`EntityMut::insert`]: crate::world::EntityMut::insert
+    /// [`EntityWorldMut::insert`]: crate::world::EntityWorldMut::insert
     #[inline]
     pub fn get_add_bundle(&self, bundle_id: BundleId) -> Option<ArchetypeId> {
         self.get_add_bundle_internal(bundle_id)
@@ -177,9 +177,9 @@ impl Edges {
     }
 
     /// Caches the target archetype when adding a bundle to the source archetype.
-    /// For more information, see [`EntityMut::insert`].
+    /// For more information, see [`EntityWorldMut::insert`].
     ///
-    /// [`EntityMut::insert`]: crate::world::EntityMut::insert
+    /// [`EntityWorldMut::insert`]: crate::world::EntityWorldMut::insert
     #[inline]
     pub(crate) fn insert_add_bundle(
         &mut self,
@@ -197,24 +197,24 @@ impl Edges {
     }
 
     /// Checks the cache for the target archetype when removing a bundle to the
-    /// source archetype. For more information, see [`EntityMut::remove`].
+    /// source archetype. For more information, see [`EntityWorldMut::remove`].
     ///
     /// If this returns `None`, it means there has not been a transition from
     /// the source archetype via the provided bundle.
     ///
     /// If this returns `Some(None)`, it means that the bundle cannot be removed
     /// from the source archetype.
     ///
-    /// [`EntityMut::remove`]: crate::world::EntityMut::remove
+    /// [`EntityWorldMut::remove`]: crate::world::EntityWorldMut::remove
     #[inline]
     pub fn get_remove_bundle(&self, bundle_id: BundleId) -> Option<Option<ArchetypeId>> {
         self.remove_bundle.get(bundle_id).cloned()
     }
 
     /// Caches the target archetype when removing a bundle to the source archetype.
-    /// For more information, see [`EntityMut::remove`].
+    /// For more information, see [`EntityWorldMut::remove`].
     ///
-    /// [`EntityMut::remove`]: crate::world::EntityMut::remove
+    /// [`EntityWorldMut::remove`]: crate::world::EntityWorldMut::remove
     #[inline]
     pub(crate) fn insert_remove_bundle(
         &mut self,
@@ -225,21 +225,21 @@ impl Edges {
     }
 
     /// Checks the cache for the target archetype when removing a bundle to the
-    /// source archetype. For more information, see [`EntityMut::remove`].
+    /// source archetype. For more information, see [`EntityWorldMut::remove`].
     ///
     /// If this returns `None`, it means there has not been a transition from
     /// the source archetype via the provided bundle.
     ///
-    /// [`EntityMut::remove`]: crate::world::EntityMut::remove
+    /// [`EntityWorldMut::remove`]: crate::world::EntityWorldMut::remove
     #[inline]
     pub fn get_take_bundle(&self, bundle_id: BundleId) -> Option<Option<ArchetypeId>> {
         self.take_bundle.get(bundle_id).cloned()
     }
 
     /// Caches the target archetype when removing a bundle to the source archetype.
-    /// For more information, see [`EntityMut::take`].
+    /// For more information, see [`EntityWorldMut::take`].
     ///
-    /// [`EntityMut::take`]: crate::world::EntityMut::take
+    /// [`EntityWorldMut::take`]: crate::world::EntityWorldMut::take
     #[inline]
     pub(crate) fn insert_take_bundle(
         &mut self,

crates/bevy_ecs/src/entity/mod.rs

@@ -21,8 +21,8 @@
 //! |Spawn an entity with components|[`Commands::spawn`]|[`World::spawn`]|
 //! |Spawn an entity without components|[`Commands::spawn_empty`]|[`World::spawn_empty`]|
 //! |Despawn an entity|[`EntityCommands::despawn`]|[`World::despawn`]|
-//! |Insert a component, bundle, or tuple of components and bundles to an entity|[`EntityCommands::insert`]|[`EntityMut::insert`]|
-//! |Remove a component, bundle, or tuple of components and bundles from an entity|[`EntityCommands::remove`]|[`EntityMut::remove`]|
+//! |Insert a component, bundle, or tuple of components and bundles to an entity|[`EntityCommands::insert`]|[`EntityWorldMut::insert`]|
+//! |Remove a component, bundle, or tuple of components and bundles from an entity|[`EntityCommands::remove`]|[`EntityWorldMut::remove`]|
 //!
 //! [`World`]: crate::world::World
 //! [`Commands::spawn`]: crate::system::Commands::spawn
@@ -33,8 +33,8 @@
 //! [`World::spawn`]: crate::world::World::spawn
 //! [`World::spawn_empty`]: crate::world::World::spawn_empty
 //! [`World::despawn`]: crate::world::World::despawn
-//! [`EntityMut::insert`]: crate::world::EntityMut::insert
-//! [`EntityMut::remove`]: crate::world::EntityMut::remove
+//! [`EntityWorldMut::insert`]: crate::world::EntityWorldMut::insert
+//! [`EntityWorldMut::remove`]: crate::world::EntityWorldMut::remove
 mod map_entities;
 
 pub use map_entities::*;
@@ -72,7 +72,7 @@ type IdCursor = isize;
 /// # Usage
 ///
 /// This data type is returned by iterating a `Query` that has `Entity` as part of its query fetch type parameter ([learn more]).
-/// It can also be obtained by calling [`EntityCommands::id`] or [`EntityMut::id`].
+/// It can also be obtained by calling [`EntityCommands::id`] or [`EntityWorldMut::id`].
 ///
 /// ```
 /// # use bevy_ecs::prelude::*;
@@ -84,7 +84,7 @@ type IdCursor = isize;
 /// }
 ///
 /// fn exclusive_system(world: &mut World) {
-///     // Calling `spawn` returns `EntityMut`.
+///     // Calling `spawn` returns `EntityWorldMut`.
 ///     let entity = world.spawn(SomeComponent).id();
 /// }
 /// #
@@ -111,7 +111,7 @@ type IdCursor = isize;
 ///
 /// [learn more]: crate::system::Query#entity-id-access
 /// [`EntityCommands::id`]: crate::system::EntityCommands::id
-/// [`EntityMut::id`]: crate::world::EntityMut::id
+/// [`EntityWorldMut::id`]: crate::world::EntityWorldMut::id
 /// [`EntityCommands`]: crate::system::EntityCommands
 /// [`Query::get`]: crate::system::Query::get
 /// [`World`]: crate::world::World

crates/bevy_ecs/src/lib.rs

@@ -52,7 +52,7 @@ pub mod prelude {
             Commands, Deferred, In, IntoSystem, Local, NonSend, NonSendMut, ParallelCommands,
             ParamSet, Query, ReadOnlySystem, Res, ResMut, Resource, System, SystemParamFunction,
         },
-        world::{EntityRef, FromWorld, World},
+        world::{EntityMut, EntityRef, EntityWorldMut, FromWorld, World},
     };
 }
 

crates/bevy_ecs/src/query/access.rs

@@ -53,9 +53,12 @@ pub struct Access<T: SparseSetIndex> {
     reads_and_writes: FixedBitSet,
     /// The exclusively-accessed elements.
     writes: FixedBitSet,
-    /// Is `true` if this has access to all elements in the collection?
+    /// Is `true` if this has access to all elements in the collection.
     /// This field is a performance optimization for `&World` (also harder to mess up for soundness).
     reads_all: bool,
+    /// Is `true` if this has mutable access to all elements in the collection.
+    /// If this is true, then `reads_all` must also be true.
+    writes_all: bool,
     marker: PhantomData<T>,
 }
 
@@ -68,6 +71,7 @@ impl<T: SparseSetIndex + fmt::Debug> fmt::Debug for Access<T> {
             )
             .field("writes", &FormattedBitSet::<T>::new(&self.writes))
             .field("reads_all", &self.reads_all)
+            .field("writes_all", &self.writes_all)
             .finish()
     }
 }
@@ -83,6 +87,7 @@ impl<T: SparseSetIndex> Access<T> {
     pub const fn new() -> Self {
         Self {
             reads_all: false,
+            writes_all: false,
             reads_and_writes: FixedBitSet::new(),
             writes: FixedBitSet::new(),
             marker: PhantomData,
@@ -116,36 +121,54 @@ impl<T: SparseSetIndex> Access<T> {
         self.reads_all || self.reads_and_writes.contains(index.sparse_set_index())
     }
 
+    /// Returns `true` if this can access anything.
+    pub fn has_any_read(&self) -> bool {
+        self.reads_all || !self.reads_and_writes.is_clear()
+    }
+
     /// Returns `true` if this can exclusively access the element given by `index`.
     pub fn has_write(&self, index: T) -> bool {
-        self.writes.contains(index.sparse_set_index())
+        self.writes_all || self.writes.contains(index.sparse_set_index())
     }
 
     /// Returns `true` if this accesses anything mutably.
     pub fn has_any_write(&self) -> bool {
-        !self.writes.is_clear()
+        self.writes_all || !self.writes.is_clear()
     }
 
     /// Sets this as having access to all indexed elements (i.e. `&World`).
     pub fn read_all(&mut self) {
         self.reads_all = true;
     }
 
+    /// Sets this as having mutable access to all indexed elements (i.e. `EntityMut`).
+    pub fn write_all(&mut self) {
+        self.reads_all = true;
+        self.writes_all = true;
+    }
+
     /// Returns `true` if this has access to all indexed elements (i.e. `&World`).
     pub fn has_read_all(&self) -> bool {
         self.reads_all
     }
 
+    /// Returns `true` if this has write access to all indexed elements (i.e. `EntityMut`).
+    pub fn has_write_all(&self) -> bool {
+        self.writes_all
+    }
+
     /// Removes all accesses.
     pub fn clear(&mut self) {
         self.reads_all = false;
+        self.writes_all = false;
         self.reads_and_writes.clear();
         self.writes.clear();
     }
 
     /// Adds all access from `other`.
     pub fn extend(&mut self, other: &Access<T>) {
         self.reads_all = self.reads_all || other.reads_all;
+        self.writes_all = self.writes_all || other.writes_all;
         self.reads_and_writes.union_with(&other.reads_and_writes);
         self.writes.union_with(&other.writes);
     }
@@ -155,13 +178,20 @@ impl<T: SparseSetIndex> Access<T> {
     /// [`Access`] instances are incompatible if one can write
     /// an element that the other can read or write.
     pub fn is_compatible(&self, other: &Access<T>) -> bool {
-        // Only systems that do not write data are compatible with systems that operate on `&World`.
+        if self.writes_all {
+            return !other.has_any_read();
+        }
+
+        if other.writes_all {
+            return !self.has_any_read();
+        }
+
         if self.reads_all {
-            return other.writes.count_ones(..) == 0;
+            return !other.has_any_write();
         }
 
         if other.reads_all {
-            return self.writes.count_ones(..) == 0;
+            return !self.has_any_write();
         }
 
         self.writes.is_disjoint(&other.reads_and_writes)
@@ -172,12 +202,23 @@ impl<T: SparseSetIndex> Access<T> {
     pub fn get_conflicts(&self, other: &Access<T>) -> Vec<T> {
         let mut conflicts = FixedBitSet::default();
         if self.reads_all {
+            // QUESTION: How to handle `other.writes_all`?
             conflicts.extend(other.writes.ones());
         }
 
         if other.reads_all {
+            // QUESTION: How to handle `self.writes_all`.
             conflicts.extend(self.writes.ones());
         }
+
+        if self.writes_all {
+            conflicts.extend(other.reads_and_writes.ones());
+        }
+
+        if other.writes_all {
+            conflicts.extend(self.reads_and_writes.ones());
+        }
+
         conflicts.extend(self.writes.intersection(&other.reads_and_writes));
         conflicts.extend(self.reads_and_writes.intersection(&other.writes));
         conflicts
@@ -377,6 +418,11 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
     pub fn read_all(&mut self) {
         self.access.read_all();
     }
+
+    /// Sets the underlying unfiltered access as having mutable access to all indexed elements.
+    pub fn write_all(&mut self) {
+        self.access.write_all();
+    }
 }
 
 #[derive(Clone, Eq, PartialEq)]