Skip to content

Commit

Permalink
Fix unsound EntityMut::remove_children (#6464)
Browse files Browse the repository at this point in the history
`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.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
  • Loading branch information
tim-blackbird and tim-blackbird committed Nov 4, 2022
1 parent 3a14ae4 commit 8468298
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 31 deletions.
8 changes: 7 additions & 1 deletion crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
35 changes: 11 additions & 24 deletions crates/bevy_hierarchy/src/child_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
}
}
Expand Down
8 changes: 2 additions & 6 deletions crates/bevy_hierarchy/src/hierarchy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand All @@ -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) {
Expand Down

0 comments on commit 8468298

Please sign in to comment.