Add missing unsafe to entity_command::insert_by_id and make it mo…

…re configurable (#18052)

## Objective

`insert_by_id` is unsafe, but I forgot to add that to the
manually-queueable version in `entity_command`.

It also can only insert using `InsertMode::Replace`, when it could
easily be configurable by threading an `InsertMode` parameter to the
final `BundleInserter::insert` call.

## Solution

- Add `unsafe` and safety comment.
- Add `InsertMode` parameter to `entity_command::insert_by_id`,
`EntityWorldMut::insert_by_id_with_caller`, and
`EntityWorldMut::insert_dynamic_bundle`.
- Add `InsertMode` parameter to `entity_command::insert` and remove
`entity_command::insert_if_new`, for consistency with the other
manually-queued insertion commands.

crates/bevy_ecs/src/system/commands/entity_command.rs

@@ -151,36 +151,34 @@ where
     }
 }
 
-/// An [`EntityCommand`] that adds the components in a [`Bundle`] to an entity,
-/// replacing any that were already present.
+/// An [`EntityCommand`] that adds the components in a [`Bundle`] to an entity.
 #[track_caller]
-pub fn insert(bundle: impl Bundle) -> impl EntityCommand {
+pub fn insert(bundle: impl Bundle, mode: InsertMode) -> impl EntityCommand {
     let caller = MaybeLocation::caller();
     move |mut entity: EntityWorldMut| {
-        entity.insert_with_caller(bundle, InsertMode::Replace, caller);
-    }
-}
-
-/// An [`EntityCommand`] that adds the components in a [`Bundle`] to an entity,
-/// except for any that were already present.
-#[track_caller]
-pub fn insert_if_new(bundle: impl Bundle) -> impl EntityCommand {
-    let caller = MaybeLocation::caller();
-    move |mut entity: EntityWorldMut| {
-        entity.insert_with_caller(bundle, InsertMode::Keep, caller);
+        entity.insert_with_caller(bundle, mode, caller);
     }
 }
 
 /// An [`EntityCommand`] that adds a dynamic component to an entity.
+///
+/// # Safety
+///
+/// - [`ComponentId`] must be from the same world as the target entity.
+/// - `T` must have the same layout as the one passed during `component_id` creation.
 #[track_caller]
-pub fn insert_by_id<T: Send + 'static>(component_id: ComponentId, value: T) -> impl EntityCommand {
+pub unsafe fn insert_by_id<T: Send + 'static>(
+    component_id: ComponentId,
+    value: T,
+    mode: InsertMode,
+) -> impl EntityCommand {
     let caller = MaybeLocation::caller();
     move |mut entity: EntityWorldMut| {
         // SAFETY:
         // - `component_id` safety is ensured by the caller
         // - `ptr` is valid within the `make` block
         OwningPtr::make(value, |ptr| unsafe {
-            entity.insert_by_id_with_caller(component_id, ptr, caller);
+            entity.insert_by_id_with_caller(component_id, ptr, mode, caller);
         });
     }
 }

crates/bevy_ecs/src/system/commands/mod.rs

@@ -1290,7 +1290,7 @@ impl<'a> EntityCommands<'a> {
     /// ```
     #[track_caller]
     pub fn insert(&mut self, bundle: impl Bundle) -> &mut Self {
-        self.queue(entity_command::insert(bundle))
+        self.queue(entity_command::insert(bundle, InsertMode::Replace))
     }
 
     /// Similar to [`Self::insert`] but will only insert if the predicate returns true.
@@ -1350,7 +1350,7 @@ impl<'a> EntityCommands<'a> {
     /// To avoid a panic in this case, use the command [`Self::try_insert_if_new`] instead.
     #[track_caller]
     pub fn insert_if_new(&mut self, bundle: impl Bundle) -> &mut Self {
-        self.queue(entity_command::insert_if_new(bundle))
+        self.queue(entity_command::insert(bundle, InsertMode::Keep))
     }
 
     /// Adds a [`Bundle`] of components to the entity without overwriting if the
@@ -1399,7 +1399,12 @@ impl<'a> EntityCommands<'a> {
         component_id: ComponentId,
         value: T,
     ) -> &mut Self {
-        self.queue(entity_command::insert_by_id(component_id, value))
+        self.queue(
+            // SAFETY:
+            // - `ComponentId` safety is ensured by the caller.
+            // - `T` safety is ensured by the caller.
+            unsafe { entity_command::insert_by_id(component_id, value, InsertMode::Replace) },
+        )
     }
 
     /// Attempts to add a dynamic component to an entity.
@@ -1417,7 +1422,10 @@ impl<'a> EntityCommands<'a> {
         value: T,
     ) -> &mut Self {
         self.queue_handled(
-            entity_command::insert_by_id(component_id, value),
+            // SAFETY:
+            // - `ComponentId` safety is ensured by the caller.
+            // - `T` safety is ensured by the caller.
+            unsafe { entity_command::insert_by_id(component_id, value, InsertMode::Replace) },
             error_handler::silent(),
         )
     }
@@ -1472,7 +1480,10 @@ impl<'a> EntityCommands<'a> {
     /// ```
     #[track_caller]
     pub fn try_insert(&mut self, bundle: impl Bundle) -> &mut Self {
-        self.queue_handled(entity_command::insert(bundle), error_handler::silent())
+        self.queue_handled(
+            entity_command::insert(bundle, InsertMode::Replace),
+            error_handler::silent(),
+        )
     }
 
     /// Similar to [`Self::try_insert`] but will only try to insert if the predicate returns true.
@@ -1572,7 +1583,7 @@ impl<'a> EntityCommands<'a> {
     #[track_caller]
     pub fn try_insert_if_new(&mut self, bundle: impl Bundle) -> &mut Self {
         self.queue_handled(
-            entity_command::insert_if_new(bundle),
+            entity_command::insert(bundle, InsertMode::Keep),
             error_handler::silent(),
         )
     }

crates/bevy_ecs/src/world/entity_ref.rs

@@ -1585,16 +1585,24 @@ impl<'w> EntityWorldMut<'w> {
         component_id: ComponentId,
         component: OwningPtr<'_>,
     ) -> &mut Self {
-        self.insert_by_id_with_caller(component_id, component, MaybeLocation::caller())
+        self.insert_by_id_with_caller(
+            component_id,
+            component,
+            InsertMode::Replace,
+            MaybeLocation::caller(),
+        )
     }
 
     /// # Safety
-    /// See [`EntityWorldMut::insert_by_id`]
+    ///
+    /// - [`ComponentId`] must be from the same world as [`EntityWorldMut`]
+    /// - [`OwningPtr`] must be a valid reference to the type represented by [`ComponentId`]
     #[inline]
     pub(crate) unsafe fn insert_by_id_with_caller(
         &mut self,
         component_id: ComponentId,
         component: OwningPtr<'_>,
+        mode: InsertMode,
         caller: MaybeLocation,
     ) -> &mut Self {
         self.assert_not_despawned();
@@ -1619,6 +1627,7 @@ impl<'w> EntityWorldMut<'w> {
             self.location,
             Some(component).into_iter(),
             Some(storage_type).iter().cloned(),
+            mode,
             caller,
         );
         self.world.flush();
@@ -1670,6 +1679,7 @@ impl<'w> EntityWorldMut<'w> {
             self.location,
             iter_components,
             (*storage_types).iter().cloned(),
+            InsertMode::Replace,
             MaybeLocation::caller(),
         );
         *self.world.bundles.get_storages_unchecked(bundle_id) = core::mem::take(&mut storage_types);
@@ -4153,6 +4163,7 @@ unsafe fn insert_dynamic_bundle<
     location: EntityLocation,
     components: I,
     storage_types: S,
+    mode: InsertMode,
     caller: MaybeLocation,
 ) -> EntityLocation {
     struct DynamicInsertBundle<'a, I: Iterator<Item = (StorageType, OwningPtr<'a>)>> {
@@ -4175,7 +4186,7 @@ unsafe fn insert_dynamic_bundle<
     // SAFETY: location matches current entity.
     unsafe {
         bundle_inserter
-            .insert(entity, location, bundle, InsertMode::Replace, caller)
+            .insert(entity, location, bundle, mode, caller)
             .0
     }
 }