Fix unsound EntityMut::remove_children. Add EntityMut::world_scope (#6464)

`EntityMut::remove_children` does not call `self.update_location()` which is unsound.
Verified by adding the following assertion, which fails when running the tests.
```rust
let before = self.location();
self.update_location();
assert_eq!(before, self.location());
```

I also removed incorrect messages like "parent entity is not modified" and the unhelpful "Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype" which might lead people to think that's the *only* thing that can change the entity's location.

# Changelog
Added `EntityMut::world_scope`.

Co-authored-by: devil-ira <[email protected]>

Author: tim-blackbird

crates/bevy_ecs/src/world/entity_ref.rs

@@ -526,7 +526,7 @@ impl<'w> EntityMut<'w> {
 
     /// Returns this `EntityMut`'s world.
     ///
-    /// See [`EntityMut::into_world_mut`] for a safe alternative.
+    /// See [`EntityMut::world_scope`] or [`EntityMut::into_world_mut`] for a safe alternative.
     ///
     /// # Safety
     /// Caller must not modify the world in a way that changes the current entity's location
@@ -543,6 +543,12 @@ impl<'w> EntityMut<'w> {
         self.world
     }
 
+    /// Gives mutable access to this `EntityMut`'s [`World`] in a temporary scope.
+    pub fn world_scope(&mut self, f: impl FnOnce(&mut World)) {
+        f(self.world);
+        self.update_location();
+    }
+
     /// Updates the internal entity location to match the current location in the internal
     /// [`World`]. This is only needed if the user called [`EntityMut::world`], which enables the
     /// location to change.

crates/bevy_hierarchy/src/child_builder.rs

@@ -456,31 +456,23 @@ pub trait BuildWorldChildren {
 
 impl<'w> BuildWorldChildren for EntityMut<'w> {
     fn with_children(&mut self, spawn_children: impl FnOnce(&mut WorldChildBuilder)) -> &mut Self {
-        {
-            let entity = self.id();
+        let entity = self.id();
+        self.world_scope(|world| {
             let mut builder = WorldChildBuilder {
                 current_entity: None,
                 parent_entities: vec![entity],
-                // SAFETY: self.update_location() is called below. It is impossible to make EntityMut
-                // function calls on `self` within the scope defined here
-                world: unsafe { self.world_mut() },
+                world,
             };
-
             spawn_children(&mut builder);
-        }
-        self.update_location();
+        });
         self
     }
 
     fn push_children(&mut self, children: &[Entity]) -> &mut Self {
         let parent = self.id();
-        {
-            // SAFETY: parent entity is not modified and its location is updated manually
-            let world = unsafe { self.world_mut() };
+        self.world_scope(|world| {
             update_old_parents(world, parent, children);
-            // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype
-            self.update_location();
-        }
+        });
         if let Some(mut children_component) = self.get_mut::<Children>() {
             children_component
                 .0
@@ -494,14 +486,9 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {
 
     fn insert_children(&mut self, index: usize, children: &[Entity]) -> &mut Self {
         let parent = self.id();
-        {
-            // SAFETY: parent entity is not modified and its location is updated manually
-            let world = unsafe { self.world_mut() };
+        self.world_scope(|world| {
             update_old_parents(world, parent, children);
-            // Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype
-            self.update_location();
-        }
-
+        });
         if let Some(mut children_component) = self.get_mut::<Children>() {
             children_component
                 .0
@@ -515,9 +502,9 @@ impl<'w> BuildWorldChildren for EntityMut<'w> {
 
     fn remove_children(&mut self, children: &[Entity]) -> &mut Self {
         let parent = self.id();
-        // SAFETY: This doesn't change the parent's location
-        let world = unsafe { self.world_mut() };
-        remove_children(parent, children, world);
+        self.world_scope(|world| {
+            remove_children(parent, children, world);
+        });
         self
     }
 }

crates/bevy_hierarchy/src/hierarchy.rs

@@ -104,7 +104,7 @@ impl<'w, 's, 'a> DespawnRecursiveExt for EntityCommands<'w, 's, 'a> {
 
 impl<'w> DespawnRecursiveExt for EntityMut<'w> {
     /// Despawns the provided entity and its children.
-    fn despawn_recursive(mut self) {
+    fn despawn_recursive(self) {
         let entity = self.id();
 
         #[cfg(feature = "trace")]
@@ -114,11 +114,7 @@ impl<'w> DespawnRecursiveExt for EntityMut<'w> {
         )
         .entered();
 
-        // SAFETY: EntityMut is consumed so even though the location is no longer
-        // valid, it cannot be accessed again with the invalid location.
-        unsafe {
-            despawn_with_children_recursive(self.world_mut(), entity);
-        }
+        despawn_with_children_recursive(self.into_world_mut(), entity);
     }
 
     fn despawn_descendants(&mut self) {