diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 074a750b43..4a2035bfd3 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -1702,7 +1702,11 @@ impl<'w> BundleSpawner<'w> { } /// # Safety - /// `entity` must be allocated and have no location. `T` must match this [`BundleInfo`]'s type + /// `T` must match this [`BundleInfo`]'s type + /// + /// # Panics + /// + /// Panics if the entity has already been constructed. #[inline] #[track_caller] pub unsafe fn construct( @@ -1736,7 +1740,16 @@ impl<'w> BundleSpawner<'w> { InsertMode::Replace, caller, ); - entities.declare(entity.row(), Some(location)); + + let was_at = entities.declare(entity.row(), Some(location)); + // We need to assert here. + // Even if we can ensure that this entity is fresh from an allocator, + // it does not prevent users constructing arbitrary rows, which may overlap with the allocator. + // One alternative would be making `Entity` creation unsafe, but this is a good safety net anyway. + assert!( + was_at.is_none(), + "Constructing an {entity} that was already constructed is not allowed." + ); entities.mark_construct_or_destruct(entity.row(), caller, self.change_tick); (location, after_effect) }; diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 51424bd282..007e13e310 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -827,30 +827,40 @@ impl Entities { /// Updates the location of an [`EntityRow`]. /// This must be called when moving the components of the existing entity around in storage. + /// Returns the previous location of the row. /// /// # Safety /// - The current location of the `row` must already be set. If not, try [`declare`](Self::declare). /// - `location` must be valid for the entity at `row` or immediately made valid afterwards /// before handing control to unknown code. #[inline] - pub(crate) unsafe fn update(&mut self, row: EntityRow, location: EntityIdLocation) { + pub(crate) unsafe fn update( + &mut self, + row: EntityRow, + location: EntityIdLocation, + ) -> EntityIdLocation { // SAFETY: Caller guarantees that `row` already had a location, so `declare` must have made the index valid already. let meta = unsafe { self.meta.get_unchecked_mut(row.index() as usize) }; - meta.location = location; + mem::replace(&mut meta.location, location) } /// Declares the location of an [`EntityRow`]. /// This must be called when constructing/spawning entities. + /// Returns the previous location of the row. /// /// # Safety /// - `location` must be valid for the entity at `index` or immediately made valid afterwards /// before handing control to unknown code. #[inline] - pub(crate) unsafe fn declare(&mut self, row: EntityRow, location: EntityIdLocation) { + pub(crate) unsafe fn declare( + &mut self, + row: EntityRow, + location: EntityIdLocation, + ) -> EntityIdLocation { self.ensure_row(row); // SAFETY: We just did `ensure_row` let meta = unsafe { self.meta.get_unchecked_mut(row.index() as usize) }; - meta.location = location; + mem::replace(&mut meta.location, location) } /// Ensures row is valid. diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 564c7b02cc..e21c432216 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -2440,7 +2440,8 @@ impl<'w> EntityWorldMut<'w> { } // SAFETY: Since we had a location, and it was valid, this is safe. unsafe { - self.world.entities.update(self.entity.row(), None); + let was_at = self.world.entities.update(self.entity.row(), None); + debug_assert_eq!(was_at, Some(location)); self.world .entities .mark_construct_or_destruct(self.entity.row(), caller, change_tick); diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 7ad931743d..0fbee2cc9f 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1126,7 +1126,12 @@ impl World { /// If the entity can not be constructed for any reason, returns an error. /// /// If it succeeds, this declares the entity to have this bundle. - /// It is possible to construct an `entity` that has not been allocated yet. + /// + /// Note: It is possible to construct an `entity` that has not been allocated yet; + /// however, doing so is currently a bad idea as the allocator may hand out this entity row in the future, assuming it to be not constructed. + /// This would cause a panic. + /// + /// Manual construction is a powerful tool, but must be used carefully. #[track_caller] pub fn construct( &mut self, @@ -1143,16 +1148,15 @@ impl World { caller: MaybeLocation, ) -> Result, ConstructionError> { self.entities.validate_construction(entity)?; - // SAFETY: We just ensured it was valid. - Ok(unsafe { self.construct_unchecked(entity, bundle, caller) }) + Ok(self.construct_unchecked(entity, bundle, caller)) } /// Constructs `bundle` on `entity`. /// - /// # Safety + /// # Panics /// - /// `entity` must be valid and have no location. - pub(crate) unsafe fn construct_unchecked( + /// Panics if the entity row is already constructed + pub(crate) fn construct_unchecked( &mut self, entity: Entity, bundle: B, @@ -1193,32 +1197,38 @@ impl World { caller: MaybeLocation, ) -> Result, ConstructionError> { self.entities.validate_construction(entity)?; - // SAFETY: We just ensured it was valid. - Ok(unsafe { self.construct_empty_unchecked(entity, caller) }) + Ok(self.construct_empty_unchecked(entity, caller)) } /// Constructs `bundle` on `entity`. /// - /// # Safety + /// # Panics /// - /// `entity` must be valid and have no location. - pub(crate) unsafe fn construct_empty_unchecked( + /// Panics if the entity row is already constructed + pub(crate) fn construct_empty_unchecked( &mut self, entity: Entity, caller: MaybeLocation, ) -> EntityWorldMut<'_> { - let archetype = self.archetypes.empty_mut(); - // PERF: consider avoiding allocating entities in the empty archetype unless needed - let table_row = self.storages.tables[archetype.table_id()].allocate(entity); - // SAFETY: no components are allocated by archetype.allocate() because the archetype is - // empty - let location = unsafe { archetype.allocate(entity, table_row) }; - let change_tick = self.change_tick(); - self.entities.declare(entity.row(), Some(location)); - self.entities - .mark_construct_or_destruct(entity.row(), caller, change_tick); + // SAFETY: Locations are immediately made valid + unsafe { + let archetype = self.archetypes.empty_mut(); + // PERF: consider avoiding allocating entities in the empty archetype unless needed + let table_row = self.storages.tables[archetype.table_id()].allocate(entity); + // SAFETY: no components are allocated by archetype.allocate() because the archetype is + // empty + let location = archetype.allocate(entity, table_row); + let change_tick = self.change_tick(); + let was_at = self.entities.declare(entity.row(), Some(location)); + assert!( + was_at.is_none(), + "Attempting to construct an empty entity, but it was already constructed." + ); + self.entities + .mark_construct_or_destruct(entity.row(), caller, change_tick); - EntityWorldMut::new(self, entity, Some(location)) + EntityWorldMut::new(self, entity, Some(location)) + } } /// Spawns a new [`Entity`] with a given [`Bundle`] of [components](`Component`) and returns @@ -1292,8 +1302,8 @@ impl World { caller: MaybeLocation, ) -> EntityWorldMut { let entity = self.spawn_null(); - // SAFETY: This was just spawned from null. - unsafe { self.construct_unchecked(entity, bundle, caller) } + // This was just spawned from null, so it shouldn't panic. + self.construct_unchecked(entity, bundle, caller) } /// Spawns a new [`Entity`] and returns a corresponding [`EntityWorldMut`], which can be used @@ -1327,9 +1337,9 @@ impl World { } pub(crate) fn spawn_empty_with_caller(&mut self, caller: MaybeLocation) -> EntityWorldMut { - let entity = self.allocator.alloc(); - // SAFETY: entity was just allocated - unsafe { self.construct_empty_unchecked(entity, caller) } + let entity = self.spawn_null(); + // This was just spawned from null, so it shouldn't panic. + self.construct_empty_unchecked(entity, caller) } /// Spawns a batch of entities with the same component [`Bundle`] type. Takes a given