From 3fa358a1b774ea494cc190f5714354eac4ebca87 Mon Sep 17 00:00:00 2001 From: Mira Morgana Date: Fri, 22 May 2026 19:12:04 +0200 Subject: [PATCH 1/5] more unsafe_op_in_unsafe_fn linting --- crates/bevy_ecs/src/archetype.rs | 4 +- crates/bevy_ecs/src/bundle/info.rs | 113 ++++++---- crates/bevy_ecs/src/bundle/insert.rs | 206 +++++++++++------- crates/bevy_ecs/src/bundle/mod.rs | 5 - crates/bevy_ecs/src/bundle/remove.rs | 113 ++++++---- crates/bevy_ecs/src/bundle/spawner.rs | 94 ++++---- crates/bevy_ecs/src/bundle/writer.rs | 7 +- crates/bevy_ecs/src/entity/clone_entities.rs | 16 +- crates/bevy_ecs/src/storage/table/column.rs | 2 +- crates/bevy_ecs/src/world/command_queue.rs | 80 +++---- .../src/world/entity_access/world_mut.rs | 97 ++++++--- crates/bevy_ecs/src/world/mod.rs | 30 ++- crates/bevy_ecs/src/world/spawn_batch.rs | 8 +- .../bevy_ecs/src/world/unsafe_world_cell.rs | 112 ++++++---- crates/bevy_ptr/src/lib.rs | 4 +- 15 files changed, 532 insertions(+), 359 deletions(-) diff --git a/crates/bevy_ecs/src/archetype.rs b/crates/bevy_ecs/src/archetype.rs index 64ca9740567e6..e4b75653aa57b 100644 --- a/crates/bevy_ecs/src/archetype.rs +++ b/crates/bevy_ecs/src/archetype.rs @@ -589,8 +589,8 @@ impl Archetype { /// Allocates an entity to the archetype. /// /// # Safety - /// valid component values must be immediately written to the relevant storages - /// `table_row` must be valid + /// - valid component values must have been or be immediately written to the relevant storages + /// - `table_row` must be valid #[inline] pub(crate) unsafe fn allocate( &mut self, diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs index 7a278e3c4d627..bf0af590dd5e5 100644 --- a/crates/bevy_ecs/src/bundle/info.rs +++ b/crates/bevy_ecs/src/bundle/info.rs @@ -227,8 +227,8 @@ impl BundleInfo { /// which removes the need to look up the [`ArchetypeAfterBundleInsert`](crate::archetype::ArchetypeAfterBundleInsert) /// in the archetype graph, which requires ownership of the entity's current archetype. /// - /// Regardless of how this is used, [`apply_effect`] must be called at most once on `bundle` after this function is - /// called if `T::Effect: !NoBundleEffect` before returning to user-space safe code before returning to user-space safe code. + /// Regardless of how this is used, if `T::Effect: !NoBundleEffect` then [`apply_effect`] must be called + /// exactly once on `bundle` after this function is called before returning to user-space safe code. /// This is currently only doable via use of [`MovingPtr::partial_move`]. /// /// `table` must be the "new" table for `entity`. `table_row` must have space allocated for the @@ -252,11 +252,13 @@ impl BundleInfo { // NOTE: get_components calls this closure on each component in "bundle order". // bundle_info.component_ids are also in "bundle order" let mut bundle_component = 0; - T::get_components(bundle, &mut |storage_type, component_ptr| { - let component_id = *self - .contributed_component_ids - .get_unchecked(bundle_component); - // SAFETY: bundle_component is a valid index for this bundle + let mut write_component = bind_lifetime(|storage_type, component_ptr| { + // SAFETY: bundle_component is a valid index per unsafe bundle impl + let component_id = *unsafe { + self.contributed_component_ids + .get_unchecked(bundle_component) + }; + // SAFETY: bundle_component is a valid index per unsafe bundle impl let status = unsafe { bundle_component_status.get_status(bundle_component) }; match storage_type { StorageType::Table => { @@ -265,15 +267,23 @@ impl BundleInfo { // the target table contains the component. unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; match (status, insert_mode) { - (ComponentStatus::Added, _) => { + // SAFETY: + // - table_row valid per preconditions + // - component_ptr valid per unsafe bundle impl + (ComponentStatus::Added, _) => unsafe { column.initialize(table_row, component_ptr, change_tick, caller); - } - (ComponentStatus::Existing, InsertMode::Replace) => { + }, + // SAFETY: + // - table_row valid per preconditions + // - component_ptr valid per unsafe bundle impl + // - component exists per bundle_component_status requirements + (ComponentStatus::Existing, InsertMode::Replace) => unsafe { column.replace(table_row, component_ptr, change_tick, caller); - } + }, (ComponentStatus::Existing, InsertMode::Keep) => { if let Some(drop_fn) = table.get_drop_for(component_id) { - drop_fn(component_ptr); + // SAFETY: component_ptr valid per unsafe bundle impl + unsafe { drop_fn(component_ptr) }; } } } @@ -284,12 +294,14 @@ impl BundleInfo { // a sparse set exists for the component. unsafe { sparse_sets.get_mut(component_id).debug_checked_unwrap() }; match (status, insert_mode) { - (ComponentStatus::Added, _) | (_, InsertMode::Replace) => { + // SAFETY: component_ptr valid per unsafe bundle impl + (ComponentStatus::Added, _) | (_, InsertMode::Replace) => unsafe { sparse_set.insert(entity, component_ptr, change_tick, caller); - } + }, (ComponentStatus::Existing, InsertMode::Keep) => { if let Some(drop_fn) = sparse_set.get_drop() { - drop_fn(component_ptr); + // SAFETY: component_ptr valid per unsafe bundle impl + unsafe { drop_fn(component_ptr) }; } } } @@ -297,16 +309,27 @@ impl BundleInfo { } bundle_component += 1; }); + // Remove this once closure_lifetime_binder is stable + fn bind_lifetime)>(func: F) -> F { + func + } + // SAFETY: + // - storage type correct per precondition + // - `apply_effect` called if required per precondition + unsafe { T::get_components(bundle, &mut write_component) }; for required_component in required_components { - required_component.initialize( - table, - sparse_sets, - change_tick, - table_row, - entity, - caller, - ); + // SAFETY: we're in write_components + unsafe { + required_component.initialize( + table, + sparse_sets, + change_tick, + table_row, + entity, + caller, + ); + } } } @@ -333,20 +356,17 @@ impl BundleInfo { component_ptr: OwningPtr, caller: MaybeLocation, ) { - { + // SAFETY: + // - if component_id is in required_components, `BundleInfo::new` ensured that the storage exists + // - caller ensures row and component_ptr are valid + unsafe { match storage_type { StorageType::Table => { - let column = - // SAFETY: If component_id is in required_components, BundleInfo::new requires that - // the target table contains the component. - unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; + let column = table.get_column_mut(component_id).debug_checked_unwrap(); column.initialize(table_row, component_ptr, change_tick, caller); } StorageType::SparseSet => { - let sparse_set = - // SAFETY: If component_id is in required_components, BundleInfo::new requires that - // a sparse set exists for the component. - unsafe { sparse_sets.get_mut(component_id).debug_checked_unwrap() }; + let sparse_set = sparse_sets.get_mut(component_id).debug_checked_unwrap(); sparse_set.insert(entity, component_ptr, change_tick, caller); } } @@ -424,7 +444,6 @@ impl Bundles { /// `components` and `storages` must be from the same [`World`] as `self`. /// /// [`World`]: crate::world::World - #[deny(unsafe_op_in_unsafe_fn)] pub(crate) unsafe fn register_info( &mut self, components: &mut ComponentsRegistrator, @@ -454,7 +473,6 @@ impl Bundles { /// `components` and `storages` must be from the same [`World`] as `self`. /// /// [`World`]: crate::world::World - #[deny(unsafe_op_in_unsafe_fn)] pub(crate) unsafe fn register_contributed_bundle_info( &mut self, components: &mut ComponentsRegistrator, @@ -488,26 +506,31 @@ impl Bundles { /// # Safety /// A [`BundleInfo`] with the given [`BundleId`] must have been initialized for this instance of `Bundles`. pub(crate) unsafe fn get_unchecked(&self, id: BundleId) -> &BundleInfo { - self.bundle_infos.get_unchecked(id.0) + // SAFETY: Bundle exists per precondition + unsafe { self.bundle_infos.get_unchecked(id.0) } } /// # Safety /// This [`BundleId`] must have been initialized with a single [`Component`](crate::component::Component) - /// (via [`init_component_info`](Self::init_dynamic_info)) + /// via [`init_component_info`](Self::init_dynamic_info) pub(crate) unsafe fn get_storage_unchecked(&self, id: BundleId) -> StorageType { - *self - .dynamic_component_storages - .get(&id) - .debug_checked_unwrap() + // SAFETY: Added in `init_component_info` + *unsafe { + self.dynamic_component_storages + .get(&id) + .debug_checked_unwrap() + } } /// # Safety - /// This [`BundleId`] must have been initialized with multiple [`Component`](crate::component::Component)s - /// (via [`init_dynamic_info`](Self::init_dynamic_info)) + /// This [`BundleId`] must have been initialized via [`init_dynamic_info`](Self::init_dynamic_info) pub(crate) unsafe fn get_storages_unchecked(&mut self, id: BundleId) -> &mut Vec { - self.dynamic_bundle_storages - .get_mut(&id) - .debug_checked_unwrap() + // SAFETY: Added in `init_dynamic_info` + unsafe { + self.dynamic_bundle_storages + .get_mut(&id) + .debug_checked_unwrap() + } } /// Initializes a new [`BundleInfo`] for a dynamic [`Bundle`]. diff --git a/crates/bevy_ecs/src/bundle/insert.rs b/crates/bevy_ecs/src/bundle/insert.rs index 7eb2f64ac9845..a615943c80dbe 100644 --- a/crates/bevy_ecs/src/bundle/insert.rs +++ b/crates/bevy_ecs/src/bundle/insert.rs @@ -22,6 +22,7 @@ use crate::{ // SAFETY: We have exclusive world access so our pointers can't be invalidated externally pub(crate) struct BundleInserter<'w> { + /// Has complete read+write permissions world: UnsafeWorldCell<'w>, bundle_info: ConstNonNull, archetype_after_insert: ConstNonNull, @@ -57,16 +58,18 @@ impl<'w> BundleInserter<'w> { bundle_id: BundleId, change_tick: Tick, ) -> Self { - // SAFETY: We will not make any accesses to the command queue, component or resource data of this world - let bundle_info = world.bundles.get_unchecked(bundle_id); - let bundle_id = bundle_info.id(); - let (new_archetype_id, is_new_created) = bundle_info.insert_bundle_into_archetype( - &mut world.archetypes, - &mut world.storages, - &world.components, - &world.observers, - archetype_id, - ); + // SAFETY: bundle exists per precondition + let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) }; + // SAFETY: same world + let (new_archetype_id, is_new_created) = unsafe { + bundle_info.insert_bundle_into_archetype( + &mut world.archetypes, + &mut world.storages, + &world.components, + &world.observers, + archetype_id, + ) + }; // SAFETY: // - The caller ensures `archetype_id` is valid. @@ -110,10 +113,10 @@ impl<'w> BundleInserter<'w> { }; if is_new_created { - inserter - .world - .into_deferred() - .trigger(ArchetypeCreated(new_archetype_id)); + // SAFETY: + // - we have exclusive ownership of the world and hold no references to command queue or component data + // - as this goes through `DeferredWorld`, our pointers will not be invalidated + unsafe { inserter.world.into_deferred() }.trigger(ArchetypeCreated(new_archetype_id)); } inserter } @@ -177,14 +180,15 @@ impl<'w> BundleInserter<'w> { } // SAFETY: Archetype gets borrowed when running the on_discard observers above, - // so this reference can only be promoted from shared to &mut down here, after they have been ran - let archetype = archetype.as_mut(); + // so this reference can only be promoted from shared to &mut down here, after they have been ran. + // The created reference doesn't escape `BundleInserter::insert` + let archetype = unsafe { archetype.as_mut() }; match archetype_move_type { ArchetypeMoveType::SameArchetype => { - // SAFETY: Mutable references do not alias and will be dropped after this block let (sparse_sets, table) = { - let world = world.world_mut(); + // SAFETY: has read+write perms, we do not use the `&mut World` to access archetypes, world ref will be dropped after this block + let world = unsafe { world.world_mut() }; ( &mut world.storages.sparse_sets, &mut world.storages.tables[archetype.table_id()], @@ -194,11 +198,12 @@ impl<'w> BundleInserter<'w> { (&*archetype, location, sparse_sets, table) } ArchetypeMoveType::NewArchetypeSameTable { new_archetype } => { - let new_archetype = new_archetype.as_mut(); + // SAFETY: Points to valid archetype, doesn't conflict with other references, will not outlive `BundleInserter::insert` + let new_archetype = unsafe { new_archetype.as_mut() }; - // SAFETY: Mutable references do not alias and will be dropped after this block let (sparse_sets, table, entities) = { - let world = world.world_mut(); + // SAFETY: has read+write perms, we do not use the `&mut World` to access archetypes, world ref will be dropped after this block + let world = unsafe { world.world_mut() }; ( &mut world.storages.sparse_sets, &mut world.storages.tables[new_archetype.table_id()], @@ -211,27 +216,34 @@ impl<'w> BundleInserter<'w> { let swapped_location = // SAFETY: If the swap was successful, swapped_entity must be valid. unsafe { entities.get_spawned(swapped_entity).debug_checked_unwrap() }; - entities.update_existing_location( - swapped_entity.index(), - Some(EntityLocation { - archetype_id: swapped_location.archetype_id, - archetype_row: location.archetype_row, - table_id: swapped_location.table_id, - table_row: swapped_location.table_row, - }), - ); + // SAFETY: entity already existed, it's archetype_row just changed + unsafe { + entities.update_existing_location( + swapped_entity.index(), + Some(EntityLocation { + archetype_id: swapped_location.archetype_id, + archetype_row: location.archetype_row, + table_id: swapped_location.table_id, + table_row: swapped_location.table_row, + }), + ) + }; } - let new_location = new_archetype.allocate(entity, result.table_row); - entities.update_existing_location(entity.index(), Some(new_location)); + // SAFETY: Component data already existed there + let new_location = unsafe { new_archetype.allocate(entity, result.table_row) }; + // SAFETY: Entity and therefore index already exists, location was just allocated + unsafe { entities.update_existing_location(entity.index(), Some(new_location)) }; (&*new_archetype, new_location, sparse_sets, table) } ArchetypeMoveType::NewArchetypeNewTable { new_archetype } => { - let new_archetype = new_archetype.as_mut(); + // SAFETY: Points to valid archetype, doesn't conflict with other references, will not outlive `BundleInserter::insert` + let new_archetype = unsafe { new_archetype.as_mut() }; // SAFETY: Mutable references do not alias and will be dropped after this block let (archetypes_ptr, tables, sparse_sets, entities) = { - let world = world.world_mut(); + // SAFETY: has read+write perms, we do not use the `&mut World` to access archetypes, world ref will be dropped after this block + let world = unsafe { world.world_mut() }; let archetype_ptr: *mut Archetype = world.archetypes.archetypes.as_mut_ptr(); ( archetype_ptr, @@ -245,15 +257,18 @@ impl<'w> BundleInserter<'w> { let swapped_location = // SAFETY: If the swap was successful, swapped_entity must be valid. unsafe { entities.get_spawned(swapped_entity).debug_checked_unwrap() }; - entities.update_existing_location( - swapped_entity.index(), - Some(EntityLocation { - archetype_id: swapped_location.archetype_id, - archetype_row: location.archetype_row, - table_id: swapped_location.table_id, - table_row: swapped_location.table_row, - }), - ); + // SAFETY: Existing entity has changed archetype row + unsafe { + entities.update_existing_location( + swapped_entity.index(), + Some(EntityLocation { + archetype_id: swapped_location.archetype_id, + archetype_row: location.archetype_row, + table_id: swapped_location.table_id, + table_row: swapped_location.table_row, + }), + ) + }; } // SAFETY: @@ -273,8 +288,10 @@ impl<'w> BundleInserter<'w> { ) }; - let new_location = new_archetype.allocate(entity, move_result.new_row); - entities.update_existing_location(entity.index(), Some(new_location)); + // SAFETY: Table data has been moved to this table/row, sparse set data was already valid + let new_location = unsafe { new_archetype.allocate(entity, move_result.new_row) }; + // SAFETY: Entity exists & location was just allocated + unsafe { entities.update_existing_location(entity.index(), Some(new_location)) }; // If an entity was moved into this entity's table spot, update its table row. if let Some(swapped_entity) = move_result.swapped_entity { @@ -282,15 +299,18 @@ impl<'w> BundleInserter<'w> { // SAFETY: If the swap was successful, swapped_entity must be valid. unsafe { entities.get_spawned(swapped_entity).debug_checked_unwrap() }; - entities.update_existing_location( - swapped_entity.index(), - Some(EntityLocation { - archetype_id: swapped_location.archetype_id, - archetype_row: swapped_location.archetype_row, - table_id: swapped_location.table_id, - table_row: result.table_row, - }), - ); + // SAFETY: Existing entity has changed archetype row + unsafe { + entities.update_existing_location( + swapped_entity.index(), + Some(EntityLocation { + archetype_id: swapped_location.archetype_id, + archetype_row: swapped_location.archetype_row, + table_id: swapped_location.table_id, + table_row: result.table_row, + }), + ) + }; if archetype.id() == swapped_location.archetype_id { archetype @@ -300,8 +320,13 @@ impl<'w> BundleInserter<'w> { .set_entity_table_row(swapped_location.archetype_row, result.table_row); } else { // SAFETY: the only two borrowed archetypes are above and we just did collision checks - (*archetypes_ptr.add(swapped_location.archetype_id.index())) - .set_entity_table_row(swapped_location.archetype_row, result.table_row); + unsafe { + (*archetypes_ptr.add(swapped_location.archetype_id.index())) + .set_entity_table_row( + swapped_location.archetype_row, + result.table_row, + ); + } } } @@ -319,7 +344,7 @@ impl<'w> BundleInserter<'w> { /// - `entity` must currently exist in the source archetype for this inserter. /// - `location` must be `entity`'s location in the archetype. /// - `T` must match this [`BundleInserter`] type used to create - /// - If `T::Effect: !NoBundleEffect.`, then [`apply_effect`] must be called at most once on + /// - If `T::Effect: !NoBundleEffect.`, then [`apply_effect`] must be called exactly once on /// `bundle` after this function before returning to user-space safe code. /// - The value pointed to by `bundle` must not be accessed for anything other than [`apply_effect`] /// or dropped. @@ -335,34 +360,48 @@ impl<'w> BundleInserter<'w> { caller: MaybeLocation, relationship_hook_mode: RelationshipHookMode, ) -> EntityLocation { - let archetype_after_insert = self.archetype_after_insert.as_ref(); + // SAFETY: Points to valid data; the reference doesn't escape this function. + // This points to data in the world, but + // - We have exclusive ownership of the world + // - we do not construct any other references into world.archetype.edges + // - we run hooks, which can immutably access `Archetypes`, but cannot get a reference to edges through that + let archetype_after_insert = unsafe { self.archetype_after_insert.as_ref() }; let (new_archetype, new_location) = { // Non-generic prelude extracted to improve compile time by minimizing monomorphized code. - let (new_archetype, new_location, sparse_sets, table) = Self::before_insert( - entity, - location, - insert_mode, - caller, - relationship_hook_mode, - self.archetype, - archetype_after_insert, - &self.world, - &mut self.archetype_move_type, - ); + // SAFETY: before_insert is functionally part of this function + let (new_archetype, new_location, sparse_sets, table) = unsafe { + Self::before_insert( + entity, + location, + insert_mode, + caller, + relationship_hook_mode, + self.archetype, + archetype_after_insert, + &self.world, + &mut self.archetype_move_type, + ) + }; - self.bundle_info.as_ref().write_components( - table, - sparse_sets, - archetype_after_insert, - archetype_after_insert.required_components.iter(), - entity, - new_location.table_row, - self.change_tick, - bundle, - insert_mode, - caller, - ); + // SAFETY: + // - bundle component status determined in `insert_bundle_into_archetype` + // - `apply_effect` called if needed per precondition + // - table_row & bundle match + unsafe { + self.bundle_info.as_ref().write_components( + table, + sparse_sets, + archetype_after_insert, + archetype_after_insert.required_components.iter(), + entity, + new_location.table_row, + self.change_tick, + bundle, + insert_mode, + caller, + ); + } (new_archetype, new_location) }; @@ -370,6 +409,9 @@ impl<'w> BundleInserter<'w> { // SAFETY: We have no outstanding mutable references to world as they were dropped let deferred_world = unsafe { self.world.into_deferred() }; + // SAFETY: No existing mutable references to archetypes or world + let old_archetype = unsafe { self.archetype.as_ref() }; + // Non-generic postlude extracted to improve compile time by minimizing monomorphized code. Self::after_insert( entity, @@ -377,7 +419,7 @@ impl<'w> BundleInserter<'w> { caller, relationship_hook_mode, archetype_after_insert, - self.archetype.as_ref(), + old_archetype, new_archetype, deferred_world, ); diff --git a/crates/bevy_ecs/src/bundle/mod.rs b/crates/bevy_ecs/src/bundle/mod.rs index c4d36c568ebba..2ec51ff0b7765 100644 --- a/crates/bevy_ecs/src/bundle/mod.rs +++ b/crates/bevy_ecs/src/bundle/mod.rs @@ -1,8 +1,3 @@ -#![expect( - unsafe_op_in_unsafe_fn, - reason = "See #11590. To be removed once all applicable unsafe code has an unsafe block with a safety comment." -)] - //! Types for handling [`Bundle`]s. //! //! This module contains the [`Bundle`] trait and some other helper types. diff --git a/crates/bevy_ecs/src/bundle/remove.rs b/crates/bevy_ecs/src/bundle/remove.rs index 6b9f59186a835..1634e3b05273a 100644 --- a/crates/bevy_ecs/src/bundle/remove.rs +++ b/crates/bevy_ecs/src/bundle/remove.rs @@ -16,7 +16,9 @@ use crate::{ world::{unsafe_world_cell::UnsafeWorldCell, World}, }; -// SAFETY: We have exclusive world access so our pointers can't be invalidated externally +// SAFETY: +// Pointers are valid for 'w and belong to `world` +// (as we have exclusive world access, our pointers can't be invalidated externally). pub(crate) struct BundleRemover<'w> { world: UnsafeWorldCell<'w>, bundle_info: ConstNonNull, @@ -34,7 +36,6 @@ impl<'w> BundleRemover<'w> { /// # Safety /// Caller must ensure that `archetype_id` is valid #[inline] - #[deny(unsafe_op_in_unsafe_fn)] pub(crate) unsafe fn new( world: &'w mut World, archetype_id: ArchetypeId, @@ -60,7 +61,8 @@ impl<'w> BundleRemover<'w> { bundle_id: BundleId, require_all: bool, ) -> Option { - let bundle_info = world.bundles.get_unchecked(bundle_id); + // SAFETY: Bundle exists per precondition + let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) }; // SAFETY: Caller ensures archetype and bundle ids are correct. let (new_archetype_id, is_new_created) = unsafe { bundle_info.remove_bundle_from_archetype( @@ -96,10 +98,10 @@ impl<'w> BundleRemover<'w> { relationship_hook_mode: RelationshipHookMode::Run, }; if is_new_created { - remover - .world - .into_deferred() - .trigger(ArchetypeCreated(new_archetype_id)); + // SAFETY: + // - we have exclusive ownership of the world and hold no references to command queue or component data + // - as this goes through `DeferredWorld`, our pointers will not be invalidated + unsafe { remover.world.into_deferred() }.trigger(ArchetypeCreated(new_archetype_id)); } Some(remover) } @@ -189,6 +191,7 @@ impl<'w> BundleRemover<'w> { } // SAFETY: We still have the cell, so this is unique, it doesn't conflict with other references, and we drop it shortly. + // We will not use it to invalidate the bundle_info or archetype pointers. let world = unsafe { self.world.world_mut() }; let (needs_drop, pre_remove_result) = pre_remove( @@ -199,19 +202,24 @@ impl<'w> BundleRemover<'w> { world.storages.tables.get_unchecked_mut(old_and_new_table.0) }), &world.components, - self.bundle_info.as_ref().explicit_components(), + // SAFETY: + // bundle_info valid for 'w, returned reference is only live for the pre_remove call + unsafe { self.bundle_info.as_ref() }.explicit_components(), ); // Handle sparse set removes - for component_id in self.bundle_info.as_ref().iter_explicit_components() { - if self.old_archetype.as_ref().contains(component_id) { + // SAFETY: + // bundle_info valid for 'w, returned reference is only live for the loop + for component_id in unsafe { self.bundle_info.as_ref() }.iter_explicit_components() { + // SAFETY: + // old_archetype valid for 'w, returned reference is only live for the loop + let old_archetype = unsafe { self.old_archetype.as_ref() }; + if old_archetype.contains(component_id) { world.removed_components.write(component_id, entity); // Make sure to drop components stored in sparse sets. // Dense components are dropped later in `move_to_and_drop_missing_unchecked`. - if let Some(StorageType::SparseSet) = - self.old_archetype.as_ref().get_storage_type(component_id) - { + if let Some(StorageType::SparseSet) = old_archetype.get_storage_type(component_id) { world .storages .sparse_sets @@ -225,23 +233,25 @@ impl<'w> BundleRemover<'w> { } // Handle archetype change - let remove_result = self - .old_archetype - .as_mut() - .swap_remove(location.archetype_row); + let remove_result = + // SAFETY: Pointer valid for 'w; we have no life references into world.archetypes + unsafe { self.old_archetype.as_mut() }.swap_remove(location.archetype_row); // if an entity was moved into this entity's archetype row, update its archetype row if let Some(swapped_entity) = remove_result.swapped_entity { let swapped_location = world.entities.get_spawned(swapped_entity).unwrap(); - world.entities.update_existing_location( - swapped_entity.index(), - Some(EntityLocation { - archetype_id: swapped_location.archetype_id, - archetype_row: location.archetype_row, - table_id: swapped_location.table_id, - table_row: swapped_location.table_row, - }), - ); + // SAFETY: entity already existed, it's archetype_row just changed + unsafe { + world.entities.update_existing_location( + swapped_entity.index(), + Some(EntityLocation { + archetype_id: swapped_location.archetype_id, + archetype_row: location.archetype_row, + table_id: swapped_location.table_id, + table_row: swapped_location.table_row, + }), + ) + }; } // Handle table change @@ -289,15 +299,18 @@ impl<'w> BundleRemover<'w> { if let Some(swapped_entity) = move_result.swapped_entity { let swapped_location = world.entities.get_spawned(swapped_entity).unwrap(); - world.entities.update_existing_location( - swapped_entity.index(), - Some(EntityLocation { - archetype_id: swapped_location.archetype_id, - archetype_row: swapped_location.archetype_row, - table_id: swapped_location.table_id, - table_row: location.table_row, - }), - ); + // SAFETY: Entity existed but is now at new table row + unsafe { + world.entities.update_existing_location( + swapped_entity.index(), + Some(EntityLocation { + archetype_id: swapped_location.archetype_id, + archetype_row: swapped_location.archetype_row, + table_id: swapped_location.table_id, + table_row: location.table_row, + }), + ) + }; world.archetypes[swapped_location.archetype_id] .set_entity_table_row(swapped_location.archetype_row, location.table_row); } @@ -305,9 +318,14 @@ impl<'w> BundleRemover<'w> { new_location } else { // The tables are the same - self.new_archetype - .as_mut() - .allocate(entity, location.table_row) + // SAFETY: + // - pointer valid for 'w, reference life only for this line, no other references into world.archetypes currently exist + // - component data already exists at the correct location + unsafe { + self.new_archetype + .as_mut() + .allocate(entity, location.table_row) + } }; // SAFETY: The entity is valid and has been moved to the new location already. @@ -411,13 +429,18 @@ impl BundleInfo { }; } - let (new_archetype_id, is_new_created) = archetypes.get_id_or_insert( - components, - observers, - next_table_id, - next_table_components, - next_sparse_set_components, - ); + // SAFETY: + // - table id was created if it doesn't exist + // - components exists as they are already part of the old archetype + let (new_archetype_id, is_new_created) = unsafe { + archetypes.get_id_or_insert( + components, + observers, + next_table_id, + next_table_components, + next_sparse_set_components, + ) + }; (Some(new_archetype_id), is_new_created) }; let current_archetype = &mut archetypes[archetype_id]; diff --git a/crates/bevy_ecs/src/bundle/spawner.rs b/crates/bevy_ecs/src/bundle/spawner.rs index 8a43899bb28a2..3bce44826cd29 100644 --- a/crates/bevy_ecs/src/bundle/spawner.rs +++ b/crates/bevy_ecs/src/bundle/spawner.rs @@ -16,6 +16,7 @@ use crate::{ // SAFETY: We have exclusive world access so our pointers can't be invalidated externally pub(crate) struct BundleSpawner<'w> { + /// Has complete read+write permissions world: UnsafeWorldCell<'w>, bundle_info: ConstNonNull, table: NonNull, @@ -42,14 +43,18 @@ impl<'w> BundleSpawner<'w> { bundle_id: BundleId, change_tick: Tick, ) -> Self { - let bundle_info = world.bundles.get_unchecked(bundle_id); - let (new_archetype_id, is_new_created) = bundle_info.insert_bundle_into_archetype( - &mut world.archetypes, - &mut world.storages, - &world.components, - &world.observers, - ArchetypeId::EMPTY, - ); + // SAFETY: bundle exists per precondition + let bundle_info = unsafe { world.bundles.get_unchecked(bundle_id) }; + // SAFETY: retrieved from same world in previous line + let (new_archetype_id, is_new_created) = unsafe { + bundle_info.insert_bundle_into_archetype( + &mut world.archetypes, + &mut world.storages, + &world.components, + &world.observers, + ArchetypeId::EMPTY, + ) + }; let archetype = &mut world.archetypes[new_archetype_id]; let table = &mut world.storages.tables[archetype.table_id()]; @@ -61,10 +66,10 @@ impl<'w> BundleSpawner<'w> { world: world.as_unsafe_world_cell(), }; if is_new_created { - spawner - .world - .into_deferred() - .trigger(ArchetypeCreated(new_archetype_id)); + // SAFETY: + // - we have exclusive ownership of the world and hold no references to command queue or component data + // - as this goes through `DeferredWorld`, our pointers will not be invalidated + unsafe { spawner.world.into_deferred() }.trigger(ArchetypeCreated(new_archetype_id)); } spawner } @@ -95,39 +100,52 @@ impl<'w> BundleSpawner<'w> { caller: MaybeLocation, ) -> EntityLocation { // SAFETY: We do not make any structural changes to the archetype graph through self.world so these pointers always remain valid - let bundle_info = self.bundle_info.as_ref(); + let bundle_info = unsafe { self.bundle_info.as_ref() }; let location = { - let table = self.table.as_mut(); - let archetype = self.archetype.as_mut(); + // SAFETY: exclusive world access; reference does not outlife this block + let table = unsafe { self.table.as_mut() }; + // SAFETY: exclusive world access; reference does not outlife this block + let archetype = unsafe { self.archetype.as_mut() }; - // SAFETY: Mutable references do not alias and will be dropped after this block let (sparse_sets, entities) = { - let world = self.world.world_mut(); + // SAFETY: has read+write perms, not used to access tables or archetypes, will be dropped after this block + let world = unsafe { self.world.world_mut() }; (&mut world.storages.sparse_sets, &mut world.entities) }; - let table_row = table.allocate(entity); - let location = archetype.allocate(entity, table_row); - bundle_info.write_components( - table, - sparse_sets, - &SpawnBundleStatus, - bundle_info.required_component_constructors.iter(), - entity, - table_row, - self.change_tick, - bundle, - InsertMode::Replace, - caller, - ); - entities.set_location(entity.index(), Some(location)); - entities.mark_spawned_or_despawned(entity.index(), caller, self.change_tick); + // SAFETY: Component data will be written + let table_row = unsafe { table.allocate(entity) }; + // SAFETY: row was just allocated & component data will be written + let location = unsafe { archetype.allocate(entity, table_row) }; + // SAFETY: + // - bundle component status is always added, as entity previously didn't exist per precondition + // - `apply_effect` called if needed per precondition + // - table_row was just allocated, bundle matches + unsafe { + bundle_info.write_components( + table, + sparse_sets, + &SpawnBundleStatus, + bundle_info.required_component_constructors.iter(), + entity, + table_row, + self.change_tick, + bundle, + InsertMode::Replace, + caller, + ); + } + // SAFETY: Entity was just spawned at this location + unsafe { + entities.set_location(entity.index(), Some(location)); + entities.mark_spawned_or_despawned(entity.index(), caller, self.change_tick); + } location }; // SAFETY: We have no outstanding mutable references to world as they were dropped let mut deferred_world = unsafe { self.world.into_deferred() }; // SAFETY: `DeferredWorld` cannot provide mutable access to `Archetypes`. - let archetype = self.archetype.as_ref(); + let archetype = unsafe { self.archetype.as_ref() }; // SAFETY: All components in the bundle are guaranteed to exist in the World // as they must be initialized before creating the BundleInfo. unsafe { @@ -202,10 +220,12 @@ impl<'w> BundleSpawner<'w> { } /// # Safety - /// - `Self` must be dropped after running this function as it may invalidate internal pointers. + /// - `Self` must be dropped immediately after running this function as it may invalidate internal pointers. #[inline] pub(crate) unsafe fn flush_commands(&mut self) { - // SAFETY: pointers on self can be invalidated, - self.world.world_mut().flush(); + // SAFETY: + // - we have complete read+write access + // - pointers on self can be invalidated, but will not be used again per precondition + unsafe { self.world.world_mut().flush() }; } } diff --git a/crates/bevy_ecs/src/bundle/writer.rs b/crates/bevy_ecs/src/bundle/writer.rs index 7b7d9635d7033..03d384a31a493 100644 --- a/crates/bevy_ecs/src/bundle/writer.rs +++ b/crates/bevy_ecs/src/bundle/writer.rs @@ -98,7 +98,7 @@ impl<'a> BundleWriter<'a> { OwningPtr::make(component, |ptr| { // SAFETY: ptr points to a C component value which matches the `id` looked up above. // Layout matches C. - self.push_component_by_id(id, ptr, Layout::new::()); + unsafe { self.push_component_by_id(id, ptr, Layout::new::()) }; }); } @@ -116,7 +116,10 @@ impl<'a> BundleWriter<'a> { layout: Layout, ) { let ptr = self.0.alloc.alloc_layout(layout); - core::ptr::copy(component.as_ptr(), ptr.as_ptr(), layout.size()); + // SAFETY: + // - `component` points to a valid value with this layout per precondition + // - `ptr` was just allocated (so cannot overlap) and has the same layout + unsafe { core::ptr::copy_nonoverlapping(component.as_ptr(), ptr.as_ptr(), layout.size()) }; self.0.component_ids.push(id); self.0.component_ptrs.push(ptr); } diff --git a/crates/bevy_ecs/src/entity/clone_entities.rs b/crates/bevy_ecs/src/entity/clone_entities.rs index aa3bdd52237a4..b03a457353703 100644 --- a/crates/bevy_ecs/src/entity/clone_entities.rs +++ b/crates/bevy_ecs/src/entity/clone_entities.rs @@ -1,8 +1,3 @@ -#![expect( - unsafe_op_in_unsafe_fn, - reason = "See #11590. To be removed once all applicable unsafe code has an unsafe block with a safety comment." -)] - use crate::{ archetype::Archetype, bundle::{Bundle, BundleRemover, InsertMode}, @@ -219,9 +214,14 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> { } let layout = self.component_info.layout(); let target_ptr = self.bundle_scratch_allocator.alloc_layout(layout); - core::ptr::copy_nonoverlapping(ptr.as_ptr(), target_ptr.as_ptr(), layout.size()); - self.bundle_scratch - .push_ptr(self.component_id, PtrMut::new(target_ptr)); + // SAFETY: + // - `ptr` points to a readable value matching self.component type + // - `target_ptr` was just allocated (and therefore does not overlap) with the correct layout + unsafe { + core::ptr::copy_nonoverlapping(ptr.as_ptr(), target_ptr.as_ptr(), layout.size()); + self.bundle_scratch + .push_ptr(self.component_id, PtrMut::new(target_ptr)); + } self.target_component_written = true; } diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index 98727e862400f..7d435987705f5 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -168,7 +168,7 @@ impl Column { /// /// # Safety /// - `row.as_usize()` must be in bounds. - /// - `comp_ptr` holds a component that matches the `component_id` + /// - `data` holds a component that matches the `component_id` #[inline] pub(crate) unsafe fn initialize( &mut self, diff --git a/crates/bevy_ecs/src/world/command_queue.rs b/crates/bevy_ecs/src/world/command_queue.rs index 55942fe36a2fc..2624fa118d1ed 100644 --- a/crates/bevy_ecs/src/world/command_queue.rs +++ b/crates/bevy_ecs/src/world/command_queue.rs @@ -32,13 +32,16 @@ struct CommandMeta { // due to mutable [`World`] access, maximizing performance for [`CommandQueue`] is // preferred to simplicity of implementation. pub struct CommandQueue { - // This buffer densely stores all queued commands. - // - // For each command, one `CommandMeta` is stored, followed by zero or more bytes - // to store the command itself. To interpret these bytes, a pointer must - // be passed to the corresponding `CommandMeta.apply_command_and_get_size` fn pointer. + //// This buffer densely stores all queued commands. + //// + //// For each command, one `CommandMeta` is stored, followed by zero or more bytes + //// to store the command itself. To interpret these bytes, a pointer must + //// be passed to the corresponding `CommandMeta.apply_command_and_get_size` fn pointer. pub(crate) bytes: Vec>, + /// Index into `bytes` at which unapplied commands start. pub(crate) cursor: usize, + /// Commands that have not yet been applied because a previous command has panicked. + /// Only contains data during a panic. pub(crate) panic_recovery: Vec>, pub(crate) caller: MaybeLocation, } @@ -147,8 +150,8 @@ impl RawCommandQueue { /// Returns true if the queue is empty. /// /// # Safety - /// - /// * Caller ensures that `bytes` and `cursor` point to valid memory + /// - Caller ensures that `bytes` and `cursor` point to valid memory + /// - there is no other unsynchonized access to the same underlying queue pub unsafe fn is_empty(&self) -> bool { // SAFETY: Pointers are guaranteed to be valid by requirements on `.clone_unsafe` (unsafe { *self.cursor.as_ref() }) >= (unsafe { self.bytes.as_ref() }).len() @@ -157,8 +160,8 @@ impl RawCommandQueue { /// Push a [`Command`] onto the queue. /// /// # Safety - /// - /// * Caller ensures that `self` has not outlived the underlying queue + /// - Caller ensures that `self` has not outlived the underlying queue + /// - there is no other unsynchonized access to the same underlying queue #[inline] pub unsafe fn push>(&mut self, command: C) { // Stores a command alongside its metadata. @@ -229,18 +232,18 @@ impl RawCommandQueue { /// This clears the queue. /// /// # Safety - /// - /// * Caller ensures that `self` has not outlived the underlying queue + /// - `self` has not outlived the underlying queue + /// - there is no other unsynchonized access to the same underlying queue #[inline] pub(crate) unsafe fn apply_or_drop_queued(&mut self, world: Option>) { - // SAFETY: If this is the command queue on world, world will not be dropped as we have a mutable reference - // If this is not the command queue on world we have exclusive ownership and self will not be mutated - let start = *self.cursor.as_ref(); - let stop = self.bytes.as_ref().len(); + // SAFETY: Queue is life & there is no other unsynchronized access + let (start, stop) = unsafe { (self.cursor.read(), self.bytes.as_ref().len()) }; let mut local_cursor = start; // SAFETY: we are setting the global cursor to the current length to prevent the executing commands from applying // the remaining commands currently in this list. This is safe. - *self.cursor.as_mut() = stop; + unsafe { + self.cursor.write(stop); + } while local_cursor < stop { // SAFETY: The cursor is either at the start of the buffer, or just after the previous command. @@ -280,27 +283,30 @@ impl RawCommandQueue { let result = std::panic::catch_unwind(f); if let Err(payload) = result { - // local_cursor now points to the location _after_ the panicked command. - // Add the remaining commands that _would have_ been applied to the - // panic_recovery queue. - // - // This uses `current_stop` instead of `stop` to account for any commands - // that were queued _during_ this panic. - // - // This is implemented in such a way that if apply_or_drop_queued() are nested recursively in, - // an applied Command, the correct command order will be retained. - let panic_recovery = self.panic_recovery.as_mut(); - let bytes = self.bytes.as_mut(); - let current_stop = bytes.len(); - panic_recovery.extend_from_slice(&bytes[local_cursor..current_stop]); - bytes.set_len(start); - *self.cursor.as_mut() = start; - - // This was the "top of the apply stack". If we are _not_ at the top of the apply stack, - // when we call`resume_unwind" the caller "closer to the top" will catch the unwind and do this check, - // until we reach the top. - if start == 0 { - bytes.append(panic_recovery); + // SAFETY: Queue is life & there is no other unsynchronized access + unsafe { + // local_cursor now points to the location _after_ the panicked command. + // Add the remaining commands that _would have_ been applied to the + // panic_recovery queue. + // + // This uses `current_stop` instead of `stop` to account for any commands + // that were queued _during_ this panic. + // + // This is implemented in such a way that if apply_or_drop_queued() are nested recursively in, + // an applied Command, the correct command order will be retained. + let panic_recovery = self.panic_recovery.as_mut(); + let bytes = self.bytes.as_mut(); + let current_stop = bytes.len(); + panic_recovery.extend_from_slice(&bytes[local_cursor..current_stop]); + bytes.set_len(start); + self.cursor.write(start); + + // This was the "top of the apply stack". If we are _not_ at the top of the apply stack, + // when we call`resume_unwind" the caller "closer to the top" will catch the unwind and do this check, + // until we reach the top. + if start == 0 { + bytes.append(panic_recovery); + } } std::panic::resume_unwind(payload); } diff --git a/crates/bevy_ecs/src/world/entity_access/world_mut.rs b/crates/bevy_ecs/src/world/entity_access/world_mut.rs index 658cc161482da..3c82b344358cf 100644 --- a/crates/bevy_ecs/src/world/entity_access/world_mut.rs +++ b/crates/bevy_ecs/src/world/entity_access/world_mut.rs @@ -617,7 +617,9 @@ impl<'w> EntityWorldMut<'w> { /// - `T` must be a mutable component #[inline] pub unsafe fn get_mut_assume_mutable(&mut self) -> Option> { - self.as_mutable().into_mut_assume_mutable() + let entity_mut = self.as_mutable(); + // SAFETY: Same preconditions + unsafe { entity_mut.into_mut_assume_mutable() } } /// Consumes `self` and gets mutable access to the component of type `T` @@ -1135,8 +1137,6 @@ impl<'w> EntityWorldMut<'w> { } /// # Safety - /// - /// - [`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( @@ -1154,21 +1154,32 @@ impl<'w> EntityWorldMut<'w> { &self.world.components, component_id, ); - let storage_type = self.world.bundles.get_storage_unchecked(bundle_id); + // SAFETY: + // init done above via init_component_info + let storage_type = unsafe { self.world.bundles.get_storage_unchecked(bundle_id) }; - let bundle_inserter = - BundleInserter::new_with_id(self.world, location.archetype_id, bundle_id, change_tick); + // SAFETY: + // - bundle initialized above + // - archetype id taken from existing entity + let bundle_inserter = unsafe { + BundleInserter::new_with_id(self.world, location.archetype_id, bundle_id, change_tick) + }; - self.location = Some(insert_dynamic_bundle( - bundle_inserter, - self.entity, - location, - Some(component).into_iter(), - Some(storage_type).iter().cloned(), - mode, - caller, - relationship_hook_insert_mode, - )); + // SAFETY: + // - only one component, with its component & storage type retrieved above + // - entity & location both belong to self + self.location = Some(unsafe { + insert_dynamic_bundle( + bundle_inserter, + self.entity, + location, + Some(component).into_iter(), + Some(storage_type).iter().cloned(), + mode, + caller, + relationship_hook_insert_mode, + ) + }); self.world.flush(); self.update_location(); self @@ -1196,9 +1207,15 @@ impl<'w> EntityWorldMut<'w> { component_ids: &[ComponentId], iter_components: I, ) -> &mut Self { - self.insert_by_ids_internal(component_ids, iter_components, RelationshipHookMode::Run) + // SAFETY: + // same preconditions + unsafe { + self.insert_by_ids_internal(component_ids, iter_components, RelationshipHookMode::Run) + } } + /// # Safety + /// see [`insert_by_ids`] #[track_caller] pub(crate) unsafe fn insert_by_ids_internal<'a, I: Iterator>>( &mut self, @@ -1213,22 +1230,38 @@ impl<'w> EntityWorldMut<'w> { &self.world.components, component_ids, ); + + // SAFETY: + // init done above via init_dynamic_info let mut storage_types = - core::mem::take(self.world.bundles.get_storages_unchecked(bundle_id)); - let bundle_inserter = - BundleInserter::new_with_id(self.world, location.archetype_id, bundle_id, change_tick); + core::mem::take(unsafe { self.world.bundles.get_storages_unchecked(bundle_id) }); + // SAFETY: + // - bundle initialized above + // - archetype id taken from existing entity + let bundle_inserter = unsafe { + BundleInserter::new_with_id(self.world, location.archetype_id, bundle_id, change_tick) + }; - self.location = Some(insert_dynamic_bundle( - bundle_inserter, - self.entity, - location, - iter_components, - (*storage_types).iter().cloned(), - InsertMode::Replace, - MaybeLocation::caller(), - relationship_hook_insert_mode, - )); - *self.world.bundles.get_storages_unchecked(bundle_id) = core::mem::take(&mut storage_types); + // SAFETY: + // - owning pointers are of the component's types per precondition + // - storage types retrieved above + // - entity & location both belong to self + self.location = Some(unsafe { + insert_dynamic_bundle( + bundle_inserter, + self.entity, + location, + iter_components, + (*storage_types).iter().cloned(), + InsertMode::Replace, + MaybeLocation::caller(), + relationship_hook_insert_mode, + ) + }); + // SAFETY: + // same as above + *unsafe { self.world.bundles.get_storages_unchecked(bundle_id) } = + core::mem::take(&mut storage_types); self.world.flush(); self.update_location(); self @@ -2409,7 +2442,7 @@ unsafe fn insert_dynamic_bundle< // - `location` matches `entity`. and thus must currently exist in the source // archetype for this inserter and its location within the archetype. // - The caller must ensure that the iterators and storage types match up with the `BundleInserter` - // - `apply_effect` is never called on this bundle. + // - `DynamicInsertBundle::Effect: NoBundleEffect` // - `bundle` is not used or dropped after this point. unsafe { bundle_inserter.insert( diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index ca1236a867c28..08f18eb42b22f 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1,8 +1,3 @@ -#![expect( - unsafe_op_in_unsafe_fn, - reason = "See #11590. To be removed once all applicable unsafe code has an unsafe block with a safety comment." -)] - //! Defines the [`World`] and APIs for accessing it directly. pub(crate) mod command_queue; @@ -2592,7 +2587,7 @@ impl World { archetype_id: first_location.archetype_id, }; move_as_ptr!(first_bundle); - // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter + // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter, B::Effect: NoBundleEffect unsafe { cache.inserter.insert( first_entity, @@ -2622,7 +2617,7 @@ impl World { } } move_as_ptr!(bundle); - // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter + // SAFETY: `entity` is valid, `location` matches entity, bundle matches inserter, B::Effect: NoBundleEffect unsafe { cache.inserter.insert( entity, @@ -2743,7 +2738,7 @@ impl World { move_as_ptr!(first_bundle); // SAFETY: // - `entity` is valid, `location` matches entity, bundle matches inserter - // - `apply_effect` is never called on this bundle. + // - B::Effect: NoBundleEffect` // - `first_bundle` is not be accessed or dropped after this. unsafe { cache.inserter.insert( @@ -2785,7 +2780,7 @@ impl World { move_as_ptr!(bundle); // SAFETY: // - `entity` is valid, `location` matches entity, bundle matches inserter - // - `apply_effect` is never called on this bundle. + // - `B::Effect: NoBundleEffect` // - `bundle` is not be accessed or dropped after this. unsafe { cache.inserter.insert( @@ -3064,13 +3059,16 @@ impl World { } else { self.spawn_empty() }; - entity_mut.insert_by_id_with_caller( - component_id, - value, - InsertMode::Replace, - caller, - RelationshipHookMode::Run, - ); + // SAFETY: pointer valid for this component id per precondition + unsafe { + entity_mut.insert_by_id_with_caller( + component_id, + value, + InsertMode::Replace, + caller, + RelationshipHookMode::Run, + ) + }; } /// Inserts new `!Send` data with the given `value`. Will replace the value if it already diff --git a/crates/bevy_ecs/src/world/spawn_batch.rs b/crates/bevy_ecs/src/world/spawn_batch.rs index 80424a731eabb..5d35db6d46d7e 100644 --- a/crates/bevy_ecs/src/world/spawn_batch.rs +++ b/crates/bevy_ecs/src/world/spawn_batch.rs @@ -78,13 +78,17 @@ where let bundle = self.inner.next()?; move_as_ptr!(bundle); Some(if let Some(bulk) = self.allocator.next() { - // SAFETY: bundle matches spawner type and we just allocated it + // SAFETY: + // - bundle matches spawner type and we just allocated it + // - I::Item::Effect: NoBundleEffect unsafe { self.spawner.spawn_at(bulk, bundle, self.caller); } bulk } else { - // SAFETY: bundle matches spawner type + // SAFETY: + // - bundle matches spawner type + // - I::Item::Effect: NoBundleEffect unsafe { self.spawner.spawn(bundle, self.caller) } }) } diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index feac4d98d3461..ec05cd3ce724c 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -161,9 +161,12 @@ impl<'w> UnsafeWorldCell<'w> { /// - The returned `&mut World` *must* be unique: it must never be allowed to exist /// at the same time as any other borrows of the world or any accesses to its data. /// This includes safe ways of accessing world data, such as [`UnsafeWorldCell::archetypes`]. - /// - Note that the `&mut World` *may* exist at the same time as instances of `UnsafeWorldCell`, + /// - The `&mut World` *may* exist at the same time as instances of `UnsafeWorldCell`, /// so long as none of those instances are used to access world data in any way /// while the mutable borrow is active. + /// - When called from within `bevy_ecs`: The `&mut World` *may* exist at the same time as borrows of + /// any data the world holds behind a pointer (e.g. into an archetype), as long as the `&mut World` + /// is never used to dereference that pointer. /// /// [//]: # (This test fails miri.) /// ```no_run @@ -468,7 +471,8 @@ impl<'w> UnsafeWorldCell<'w> { // SAFETY: We have permission to access the resource of `component_id`. let entity = unsafe { self.resource_entities() }.get(component_id)?; let entity_cell = self.get_entity(entity).ok()?; - entity_cell.get_by_id(component_id) + // SAFETY: Exclusive access per preconditions + unsafe { entity_cell.get_by_id(component_id) } } /// Gets a reference to a non-send resource of the given type if it exists. @@ -479,7 +483,8 @@ impl<'w> UnsafeWorldCell<'w> { /// - no mutable reference to the data exists at the same time #[deprecated(since = "0.19.0", note = "use UnsafeWorldCell::get_non_send")] pub unsafe fn get_non_send_resource(self) -> Option<&'w R> { - self.get_non_send::() + // SAFETY: same preconditions + unsafe { self.get_non_send::() } } /// Gets a reference to non-send data of the given type if it exists @@ -510,7 +515,8 @@ impl<'w> UnsafeWorldCell<'w> { /// - no mutable reference to the data exists at the same time #[deprecated(since = "0.19.0", note = "use UnsafeWorldCell::get_non_send_by_id")] pub unsafe fn get_non_send_resource_by_id(self, component_id: ComponentId) -> Option> { - self.get_non_send_by_id(component_id) + // SAFETY: Access permissions and uniqueness per preconditions + unsafe { self.get_non_send_by_id(component_id) } } /// Gets a pointer to `!Send` data with the id [`ComponentId`] if it exists. @@ -577,7 +583,8 @@ impl<'w> UnsafeWorldCell<'w> { // SAFETY: We have permission to access the resource of `component_id`. let entity = unsafe { self.resource_entities() }.get(component_id)?; let entity_cell = self.get_entity(entity).ok()?; - entity_cell.get_mut_by_id(component_id).ok() + // SAFETY: Access permissions and uniqueness per preconditions + unsafe { entity_cell.get_mut_by_id(component_id).ok() } } /// # Safety @@ -607,7 +614,8 @@ impl<'w> UnsafeWorldCell<'w> { /// - no other references to the data exist at the same time #[deprecated(since = "0.19.0", note = "use UnsafeWorldCell::get_non_send_mut")] pub unsafe fn get_non_send_resource_mut(self) -> Option> { - self.get_non_send_mut::() + // SAFETY: Access permissions and uniqueness per preconditions + unsafe { self.get_non_send_mut::() } } /// Gets a mutable reference to the non-send data of the given type if it exists @@ -643,7 +651,8 @@ impl<'w> UnsafeWorldCell<'w> { self, component_id: ComponentId, ) -> Option> { - self.get_non_send_mut_by_id(component_id) + // SAFETY: Access permissions and uniqueness per preconditions + unsafe { self.get_non_send_mut_by_id(component_id) } } /// Gets mutable access to `!Send` data with the id [`ComponentId`] if it exists. @@ -706,7 +715,7 @@ impl<'w> UnsafeWorldCell<'w> { // - caller ensures there are no mutable borrows of this resource // - caller ensures that we have permission to access this resource // - storage_type and location are valid - get_component_and_ticks(self, component_id, storage_type, entity, location) + unsafe { get_component_and_ticks(self, component_id, storage_type, entity, location) } } // Shorthand helper function for getting the data and change ticks for a resource. @@ -1306,14 +1315,16 @@ unsafe fn get_component( entity: Entity, location: EntityLocation, ) -> Option> { - // SAFETY: component_id exists and is therefore valid - match storage_type { - StorageType::Table => { - let table = world.fetch_table(location)?; - // SAFETY: archetypes only store valid table_rows and caller ensure aliasing rules - table.get_component(component_id, location.table_row) + // SAFETY: + // - caller ensure aliasing rules + // - archetypes only store valid table_rows + unsafe { + match storage_type { + StorageType::Table => world + .fetch_table(location)? + .get_component(component_id, location.table_row), + StorageType::SparseSet => world.fetch_sparse_set(component_id)?.get(entity), } - StorageType::SparseSet => world.fetch_sparse_set(component_id)?.get(entity), } } @@ -1334,25 +1345,31 @@ unsafe fn get_component_and_ticks( ) -> Option<(Ptr<'_>, ComponentTickCells<'_>)> { match storage_type { StorageType::Table => { - let table = world.fetch_table(location)?; + // SAFETY: caller upholds aliasing rules + let table = unsafe { world.fetch_table(location)? }; // SAFETY: archetypes only store valid table_rows and caller ensure aliasing rules - Some(( - table.get_component(component_id, location.table_row)?, - ComponentTickCells { - added: table - .get_added_tick(component_id, location.table_row) - .debug_checked_unwrap(), - changed: table - .get_changed_tick(component_id, location.table_row) - .debug_checked_unwrap(), - changed_by: table - .get_changed_by(component_id, location.table_row) - .map(|changed_by| changed_by.debug_checked_unwrap()), - }, - )) + Some(unsafe { + ( + table.get_component(component_id, location.table_row)?, + ComponentTickCells { + added: table + .get_added_tick(component_id, location.table_row) + .debug_checked_unwrap(), + changed: table + .get_changed_tick(component_id, location.table_row) + .debug_checked_unwrap(), + changed_by: table + .get_changed_by(component_id, location.table_row) + .map(|changed_by| changed_by.debug_checked_unwrap()), + }, + ) + }) + } + StorageType::SparseSet => { + // SAFETY: caller upholds aliasing rules + unsafe { world.fetch_sparse_set(component_id) }?.get_with_ticks(entity) } - StorageType::SparseSet => world.fetch_sparse_set(component_id)?.get_with_ticks(entity), } } @@ -1372,13 +1389,17 @@ unsafe fn get_ticks( entity: Entity, location: EntityLocation, ) -> Option { - match storage_type { - StorageType::Table => { - let table = world.fetch_table(location)?; - // SAFETY: archetypes only store valid table_rows and caller ensure aliasing rules - table.get_ticks_unchecked(component_id, location.table_row) + // SAFETY: + // - caller ensure aliasing rules + // - archetypes only store valid table_rows + unsafe { + match storage_type { + StorageType::Table => { + let table = world.fetch_table(location)?; + table.get_ticks_unchecked(component_id, location.table_row) + } + StorageType::SparseSet => world.fetch_sparse_set(component_id)?.get_ticks(entity), } - StorageType::SparseSet => world.fetch_sparse_set(component_id)?.get_ticks(entity), } } @@ -1399,16 +1420,21 @@ unsafe fn get_changed_by( entity: Entity, location: EntityLocation, ) -> Option { - let caller = match storage_type { - StorageType::Table => world - .fetch_table(location)? - .get_changed_by(component_id, location.table_row), - StorageType::SparseSet => world.fetch_sparse_set(component_id)?.get_changed_by(entity), + // SAFETY: + // - caller ensure aliasing rules + // - archetypes only store valid table_rows + let caller = unsafe { + match storage_type { + StorageType::Table => world + .fetch_table(location)? + .get_changed_by(component_id, location.table_row), + StorageType::SparseSet => world.fetch_sparse_set(component_id)?.get_changed_by(entity), + } }; Some( caller .transpose()? - // SAFETY: This function is being called through an exclusive mutable reference to Self + // SAFETY: Caller ensures there are no mutable aliases .map(|changed_by| unsafe { *changed_by.deref() }), ) } diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 9025f71aca0d1..a881340dedcaf 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -221,7 +221,7 @@ impl ConstNonNull { /// /// * The pointer must be [properly aligned]. /// - /// * It must be "dereferenceable" in the sense defined in [the module documentation]. + /// * It must be "dereferenceable" in the sense defined in [the `core::ptr` documentation]. /// /// * The pointer must point to an initialized instance of `T`. /// @@ -246,7 +246,7 @@ impl ConstNonNull { /// println!("{ref_x}"); /// ``` /// - /// [the module documentation]: core::ptr#safety + /// [the `core::ptr` documentation]: core::ptr#safety /// [properly aligned]: https://doc.rust-lang.org/std/ptr/index.html#alignment #[inline] pub unsafe fn as_ref<'a>(&self) -> &'a T { From 700943c40b1424d76d9b486cda18029a61acb601 Mon Sep 17 00:00:00 2001 From: Mira Morgana Date: Sat, 30 May 2026 00:13:50 +0200 Subject: [PATCH 2/5] doc link --- crates/bevy_ecs/src/world/entity_access/world_mut.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/entity_access/world_mut.rs b/crates/bevy_ecs/src/world/entity_access/world_mut.rs index 3c82b344358cf..ed4a1198ce935 100644 --- a/crates/bevy_ecs/src/world/entity_access/world_mut.rs +++ b/crates/bevy_ecs/src/world/entity_access/world_mut.rs @@ -1215,7 +1215,7 @@ impl<'w> EntityWorldMut<'w> { } /// # Safety - /// see [`insert_by_ids`] + /// see [`EntityWorldMut::insert_by_ids`] #[track_caller] pub(crate) unsafe fn insert_by_ids_internal<'a, I: Iterator>>( &mut self, From 04840f69f44795ffd63179ed7c328f2f0589c015 Mon Sep 17 00:00:00 2001 From: Mira Date: Tue, 2 Jun 2026 08:58:32 +0200 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Trashtalk217 --- crates/bevy_ecs/src/bundle/info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle/info.rs b/crates/bevy_ecs/src/bundle/info.rs index bf0af590dd5e5..c48832439d4eb 100644 --- a/crates/bevy_ecs/src/bundle/info.rs +++ b/crates/bevy_ecs/src/bundle/info.rs @@ -249,7 +249,7 @@ impl BundleInfo { insert_mode: InsertMode, caller: MaybeLocation, ) { - // NOTE: get_components calls this closure on each component in "bundle order". + // NOTE: get_components calls `write_component` on each component in "bundle order". // bundle_info.component_ids are also in "bundle order" let mut bundle_component = 0; let mut write_component = bind_lifetime(|storage_type, component_ptr| { From ba5a740d6dc43ab45249c3bb1d7e8228960073ee Mon Sep 17 00:00:00 2001 From: Mira Morgana Date: Tue, 2 Jun 2026 09:01:04 +0200 Subject: [PATCH 4/5] suggestions --- crates/bevy_ecs/src/bundle/insert.rs | 2 +- crates/bevy_ecs/src/world/command_queue.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/bundle/insert.rs b/crates/bevy_ecs/src/bundle/insert.rs index a615943c80dbe..e745f7ddbeb5f 100644 --- a/crates/bevy_ecs/src/bundle/insert.rs +++ b/crates/bevy_ecs/src/bundle/insert.rs @@ -216,7 +216,7 @@ impl<'w> BundleInserter<'w> { let swapped_location = // SAFETY: If the swap was successful, swapped_entity must be valid. unsafe { entities.get_spawned(swapped_entity).debug_checked_unwrap() }; - // SAFETY: entity already existed, it's archetype_row just changed + // SAFETY: entity already existed, its archetype_row just changed unsafe { entities.update_existing_location( swapped_entity.index(), diff --git a/crates/bevy_ecs/src/world/command_queue.rs b/crates/bevy_ecs/src/world/command_queue.rs index 2624fa118d1ed..b258b9af58fd6 100644 --- a/crates/bevy_ecs/src/world/command_queue.rs +++ b/crates/bevy_ecs/src/world/command_queue.rs @@ -32,11 +32,11 @@ struct CommandMeta { // due to mutable [`World`] access, maximizing performance for [`CommandQueue`] is // preferred to simplicity of implementation. pub struct CommandQueue { - //// This buffer densely stores all queued commands. - //// - //// For each command, one `CommandMeta` is stored, followed by zero or more bytes - //// to store the command itself. To interpret these bytes, a pointer must - //// be passed to the corresponding `CommandMeta.apply_command_and_get_size` fn pointer. + /// This buffer densely stores all queued commands. + /// + /// For each command, one `CommandMeta` is stored, followed by zero or more bytes + /// to store the command itself. To interpret these bytes, a pointer must + /// be passed to the corresponding `CommandMeta.apply_command_and_get_size` fn pointer. pub(crate) bytes: Vec>, /// Index into `bytes` at which unapplied commands start. pub(crate) cursor: usize, From 4d303f7e37eb8a74a1ee4fc6afb85b44f7f7308a Mon Sep 17 00:00:00 2001 From: Mira Morgana Date: Tue, 2 Jun 2026 09:25:49 +0200 Subject: [PATCH 5/5] unsafe block required by changes from main --- crates/bevy_ecs/src/bundle/writer.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle/writer.rs b/crates/bevy_ecs/src/bundle/writer.rs index af9fad293aea0..5131dec7c1715 100644 --- a/crates/bevy_ecs/src/bundle/writer.rs +++ b/crates/bevy_ecs/src/bundle/writer.rs @@ -135,7 +135,8 @@ impl<'a> BundleWriter<'a> { /// [`Self::write`] or [`Self::write_with_relationship_hook_insert_mode`] were called with. #[track_caller] pub unsafe fn write(self, entity: &mut EntityWorldMut) { - self.write_with_relationship_hook_insert_mode(entity, RelationshipHookMode::Run); + // SAFETY: Same preconditions + unsafe { self.write_with_relationship_hook_insert_mode(entity, RelationshipHookMode::Run) }; } /// Writes the current contents of the bundle to the given `entity` and clears the scratch space.