guard against arbitrary constructions

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<T: DynamicBundle>(
@@ -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)
         };

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.

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);

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<B: Bundle>(
         &mut self,
@@ -1143,16 +1148,15 @@ impl World {
         caller: MaybeLocation,
     ) -> Result<EntityWorldMut<'_>, ConstructionError> {
         self.entities.validate_construction(entity)?;
-        // SAFETY: We just ensured it was valid.
+        Ok(self.construct_unchecked(entity, bundle, caller))
-        Ok(unsafe { self.construct_unchecked(entity, bundle, caller) })
     }
 
     /// Constructs `bundle` on `entity`.
     ///
-    /// # Safety
+    /// # Panics
     ///
-    /// `entity` must be valid and have no location.
+    /// Panics if the entity row is already constructed
-    pub(crate) unsafe fn construct_unchecked<B: Bundle>(
+    pub(crate) fn construct_unchecked<B: Bundle>(
         &mut self,
         entity: Entity,
         bundle: B,
@@ -1193,33 +1197,39 @@ impl World {
         caller: MaybeLocation,
     ) -> Result<EntityWorldMut<'_>, ConstructionError> {
         self.entities.validate_construction(entity)?;
-        // SAFETY: We just ensured it was valid.
+        Ok(self.construct_empty_unchecked(entity, caller))
-        Ok(unsafe { self.construct_empty_unchecked(entity, caller) })
     }
 
     /// Constructs `bundle` on `entity`.
     ///
-    /// # Safety
+    /// # Panics
     ///
-    /// `entity` must be valid and have no location.
+    /// Panics if the entity row is already constructed
-    pub(crate) unsafe fn construct_empty_unchecked(
+    pub(crate) fn construct_empty_unchecked(
         &mut self,
         entity: Entity,
         caller: MaybeLocation,
     ) -> EntityWorldMut<'_> {
+        // 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 = unsafe { archetype.allocate(entity, table_row) };
+            let location = archetype.allocate(entity, table_row);
             let change_tick = self.change_tick();
-        self.entities.declare(entity.row(), Some(location));
+            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))
         }
+    }
 
     /// Spawns a new [`Entity`] with a given [`Bundle`] of [components](`Component`) and returns
     /// a corresponding [`EntityWorldMut`], which can be used to add components to the entity or
@@ -1292,8 +1302,8 @@ impl World {
         caller: MaybeLocation,
     ) -> EntityWorldMut {
         let entity = self.spawn_null();
-        // SAFETY: This was just spawned from null.
+        // This was just spawned from null, so it shouldn't panic.
-        unsafe { self.construct_unchecked(entity, bundle, caller) }
+        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();
+        let entity = self.spawn_null();
-        // SAFETY: entity was just allocated
+        // This was just spawned from null, so it shouldn't panic.
-        unsafe { self.construct_empty_unchecked(entity, caller) }
+        self.construct_empty_unchecked(entity, caller)
     }
 
     /// Spawns a batch of entities with the same component [`Bundle`] type. Takes a given