fix(ecs): don't resurrect removed required components on insert_if_new

_release-content/migration-guides/insert_if_new_required_components.md

@@ -0,0 +1,24 @@
+---
+title: "`insert_if_new` no longer re-adds required components of already-present components"
+pull_requests: [24593]
+---
+
+The `InsertMode::Keep` insert APIs (`EntityWorldMut::insert_if_new`, `EntityCommands::insert_if_new`, `World::insert_batch_if_new`, `World::try_insert_batch_if_new`) now only add a required component when the component requiring it is actually being inserted.
+
+Previously, inserting a component that was already present could still (re-)insert its required components. So if you removed a required component and then re-inserted its requirer with `insert_if_new`, the required component was silently brought back, even though the requirer itself was unchanged. Now inserting an already-present component is a true no-op, including for its required components.
+
+```rust
+#[derive(Component)]
+#[require(B)]
+struct A;
+#[derive(Component, Default)]
+struct B;
+
+let id = world.spawn(A).id(); // inserts A and its required B
+world.entity_mut(id).remove::<B>();
+world.entity_mut(id).insert_if_new(A);
+// Before: B is present again.
+// Now:    B stays absent, because A was already present so the insert did nothing.
+```
+
+If you relied on the old behavior to restore a required component, insert it explicitly (`insert(B)`), or use `insert` (`InsertMode::Replace`) instead of `insert_if_new`.

crates/bevy_ecs/src/archetype.rs

@@ -20,7 +20,7 @@
 //! [`World::archetypes`]: crate::world::World::archetypes
 
 use crate::{
-    bundle::BundleId,
+    bundle::{BundleId, InsertMode},
     component::{ComponentId, Components, RequiredComponentConstructor, StorageType},
     entity::{Entity, EntityLocation},
     event::Event,
@@ -205,6 +205,11 @@ impl BundleComponentStatus for SpawnBundleStatus {
 #[derive(Default)]
 pub struct Edges {
     insert_bundle: SparseArray<BundleId, ArchetypeAfterBundleInsert>,
+    // Separate edge for `InsertMode::Keep` (`insert_if_new`). It differs from `insert_bundle` only
+    // in that required components pulled in solely by components already present in the source
+    // archetype are excluded from the target, so `insert_if_new` on an already-present component
+    // does not resurrect a required component the caller removed.
+    insert_bundle_if_new: SparseArray<BundleId, ArchetypeAfterBundleInsert>,
     remove_bundle: SparseArray<BundleId, Option<ArchetypeId>>,
     take_bundle: SparseArray<BundleId, Option<ArchetypeId>>,
 }
@@ -217,25 +222,33 @@ impl Edges {
     /// the source archetype via the provided bundle.
     #[inline]
     pub fn get_archetype_after_bundle_insert(&self, bundle_id: BundleId) -> Option<ArchetypeId> {
-        self.get_archetype_after_bundle_insert_internal(bundle_id)
+        self.get_archetype_after_bundle_insert_internal(bundle_id, InsertMode::Replace)
             .map(|bundle| bundle.archetype_id)
     }
 
     /// Internal version of `get_archetype_after_bundle_insert` that
     /// fetches the full `ArchetypeAfterBundleInsert`.
+    ///
+    /// `InsertMode::Keep` (used by `insert_if_new`) selects an edge that excludes required
+    /// components pulled in only by already-present components.
     #[inline]
     pub(crate) fn get_archetype_after_bundle_insert_internal(
         &self,
         bundle_id: BundleId,
+        insert_mode: InsertMode,
     ) -> Option<&ArchetypeAfterBundleInsert> {
-        self.insert_bundle.get(bundle_id)
+        match insert_mode {
+            InsertMode::Keep => self.insert_bundle_if_new.get(bundle_id),
+            InsertMode::Replace => self.insert_bundle.get(bundle_id),
+        }
     }
 
     /// Caches the target archetype when inserting a bundle into the source archetype.
     #[inline]
     pub(crate) fn cache_archetype_after_bundle_insert(
         &mut self,
         bundle_id: BundleId,
+        insert_mode: InsertMode,
         archetype_id: ArchetypeId,
         bundle_status: impl Into<Box<[ComponentStatus]>>,
         required_components: impl Into<Box<[RequiredComponentConstructor]>>,
@@ -246,16 +259,17 @@ impl Edges {
         // Make sure `extend` doesn't over-reserve, since the conversion to `Box<[_]>` would reallocate to shrink.
         added.reserve_exact(existing.len());
         added.extend(existing);
-        self.insert_bundle.insert(
-            bundle_id,
-            ArchetypeAfterBundleInsert {
-                archetype_id,
-                bundle_status: bundle_status.into(),
-                required_components: required_components.into(),
-                added_len,
-                inserted: added.into(),
-            },
-        );
+        let edge = ArchetypeAfterBundleInsert {
+            archetype_id,
+            bundle_status: bundle_status.into(),
+            required_components: required_components.into(),
+            added_len,
+            inserted: added.into(),
+        };
+        match insert_mode {
+            InsertMode::Keep => self.insert_bundle_if_new.insert(bundle_id, edge),
+            InsertMode::Replace => self.insert_bundle.insert(bundle_id, edge),
+        };
     }
 
     /// Checks the cache for the target archetype when removing a bundle from the

crates/bevy_ecs/src/bundle/insert.rs

@@ -1,4 +1,5 @@
 use alloc::vec::Vec;
+use bevy_platform::collections::HashMap;
 use bevy_ptr::{ConstNonNull, MovingPtr};
 use core::ptr::NonNull;
 
@@ -9,7 +10,7 @@ use crate::{
     },
     bundle::{ArchetypeMoveType, Bundle, BundleId, BundleInfo, DynamicBundle, InsertMode},
     change_detection::{MaybeLocation, Tick},
-    component::{Components, StorageType},
+    component::{ComponentId, Components, RequiredComponentConstructor, StorageType},
     entity::{Entities, Entity, EntityLocation},
     event::EntityComponentsTrigger,
     lifecycle::{Add, Discard, Insert, ADD, DISCARD, INSERT},
@@ -39,11 +40,12 @@ impl<'w> BundleInserter<'w> {
         world: &'w mut World,
         archetype_id: ArchetypeId,
         change_tick: Tick,
+        insert_mode: InsertMode,
     ) -> Self {
         let bundle_id = world.register_bundle_info::<T>();
 
         // SAFETY: We just ensured this bundle exists
-        unsafe { Self::new_with_id(world, archetype_id, bundle_id, change_tick) }
+        unsafe { Self::new_with_id(world, archetype_id, bundle_id, change_tick, insert_mode) }
     }
 
     /// Creates a new [`BundleInserter`].
@@ -57,6 +59,7 @@ impl<'w> BundleInserter<'w> {
         archetype_id: ArchetypeId,
         bundle_id: BundleId,
         change_tick: Tick,
+        insert_mode: InsertMode,
     ) -> Self {
         // SAFETY: bundle exists per precondition
         let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) };
@@ -68,6 +71,7 @@ impl<'w> BundleInserter<'w> {
                 &world.components,
                 &world.observers,
                 archetype_id,
+                insert_mode,
             )
         };
 
@@ -84,7 +88,7 @@ impl<'w> BundleInserter<'w> {
         let archetype_after_insert = unsafe {
             archetype
                 .edges()
-                .get_archetype_after_bundle_insert_internal(bundle_id)
+                .get_archetype_after_bundle_insert_internal(bundle_id, insert_mode)
                 .debug_checked_unwrap()
                 .into()
         };
@@ -538,17 +542,27 @@ impl BundleInfo {
         components: &Components,
         observers: &Observers,
         archetype_id: ArchetypeId,
+        insert_mode: InsertMode,
     ) -> (ArchetypeId, bool) {
-        if let Some(archetype_after_insert_id) = archetypes[archetype_id]
+        // `Keep` (`insert_if_new`) builds a distinct edge that excludes required components pulled
+        // in only by components already present in the source archetype.
+        let keep = insert_mode == InsertMode::Keep;
+        if let Some(edge) = archetypes[archetype_id]
             .edges()
-            .get_archetype_after_bundle_insert(self.id)
+            .get_archetype_after_bundle_insert_internal(self.id, insert_mode)
         {
-            return (archetype_after_insert_id, false);
+            return (edge.archetype_id, false);
         }
         let mut new_table_components = Vec::new();
         let mut new_sparse_set_components = Vec::new();
         let mut bundle_status = Vec::with_capacity(self.explicit_components_len());
         let mut added_required_components = Vec::new();
+        // Under `Keep`, required components reachable from the explicit components this insert
+        // actually adds, each mapped to the constructor from an added requiree. Required components
+        // reachable only from already-present components are absent (so they are skipped), and a
+        // shared required component takes an added requiree's constructor rather than the bundle's.
+        let mut required_by_added: HashMap<ComponentId, RequiredComponentConstructor> =
+            HashMap::default();
         let mut added = Vec::new();
         let mut existing = Vec::new();
 
@@ -562,6 +576,15 @@ impl BundleInfo {
                 added.push(component_id);
                 // SAFETY: component_id exists
                 let component_info = unsafe { components.get_info_unchecked(component_id) };
+                if keep {
+                    // Newly added, so its required components are genuinely new under `Keep`. First
+                    // requiree wins, matching the bundle's own required-component precedence.
+                    for (required_id, required) in &component_info.required_components().all {
+                        required_by_added
+                            .entry(*required_id)
+                            .or_insert_with(|| required.constructor.clone());
+                    }
+                }
                 match component_info.storage_type() {
                     StorageType::Table => new_table_components.push(component_id),
                     StorageType::SparseSet => new_sparse_set_components.push(component_id),
@@ -571,7 +594,17 @@ impl BundleInfo {
 
         for (index, component_id) in self.iter_required_components().enumerate() {
             if !current_archetype.contains(component_id) {
-                added_required_components.push(self.required_component_constructors[index].clone());
+                let constructor = if keep {
+                    // Skip required components reachable only from already-present components; for the
+                    // rest, use the constructor from a requiree that is actually being added.
+                    match required_by_added.get(&component_id) {
+                        Some(constructor) => constructor.clone(),
+                        None => continue,
+                    }
+                } else {
+                    self.required_component_constructors[index].clone()
+                };
+                added_required_components.push(constructor);
                 added.push(component_id);
                 // SAFETY: component_id exists
                 let component_info = unsafe { components.get_info_unchecked(component_id) };
@@ -591,6 +624,7 @@ impl BundleInfo {
             // The archetype does not change when we insert this bundle.
             edges.cache_archetype_after_bundle_insert(
                 self.id,
+                insert_mode,
                 archetype_id,
                 bundle_status,
                 added_required_components,
@@ -648,6 +682,7 @@ impl BundleInfo {
                 .edges_mut()
                 .cache_archetype_after_bundle_insert(
                     self.id,
+                    insert_mode,
                     new_archetype_id,
                     bundle_status,
                     added_required_components,

crates/bevy_ecs/src/bundle/spawner.rs

@@ -53,6 +53,7 @@ impl<'w> BundleSpawner<'w> {
                 &world.components,
                 &world.observers,
                 ArchetypeId::EMPTY,
+                InsertMode::Replace,
             )
         };
 

crates/bevy_ecs/src/component/mod.rs

@@ -290,6 +290,34 @@ use core::{fmt::Debug, marker::PhantomData, ops::Deref};
 /// 1. Specifying a required component constructor for Foo directly on a spawned component Bar will result in that constructor being used (and overriding existing constructors lower in the inheritance tree). This is the classic "inheritance override" behavior people expect.
 /// 2. For cases where "multiple inheritance" results in constructor clashes, Components should be listed in "importance order". List a component earlier in the requirement list to initialize its inheritance tree earlier.
 ///
+/// ## Required components and `insert_if_new`
+///
+/// [`insert_if_new`](crate::world::EntityWorldMut::insert_if_new) only adds components that are not
+/// already present, and the same applies to required components: a required component is added only
+/// for a component the call actually inserts. Inserting an already-present component with
+/// `insert_if_new` is a no-op and does not re-add its required components, so it will not bring back
+/// a required component you previously removed:
+///
+/// ```
+/// # use bevy_ecs::prelude::*;
+/// #[derive(Component)]
+/// #[require(B)]
+/// struct A;
+/// #[derive(Component, Default)]
+/// struct B;
+///
+/// # let mut world = World::default();
+/// let id = world.spawn(A).id();
+/// world.entity_mut(id).remove::<B>();
+/// // `A` is already present, so this is a no-op and does not re-add `B`.
+/// world.entity_mut(id).insert_if_new(A);
+/// assert!(!world.entity(id).contains::<B>());
+/// ```
+///
+/// This is decided per explicitly-inserted component, not recursively: `insert_if_new` of a
+/// newly-added component still adds that component's required components transitively, even where an
+/// intermediate requirer already exists.
+///
 /// ## Registering required components at runtime
 ///
 /// In most cases, required components should be registered using the `require` attribute as shown above.