From 7524df1ec4f2cf4ded7aa18046f3f24fdc305cee Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 12 Mar 2023 06:26:46 -0700 Subject: [PATCH 1/8] Less panicking bundles --- crates/bevy_ecs/src/bundle.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 66fe6cdbcfb8b..38d1d1fa71d08 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -13,6 +13,7 @@ use crate::{ component::{Component, ComponentId, ComponentStorage, Components, StorageType, Tick}, entity::{Entities, Entity, EntityLocation}, storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow}, + query::DebugCheckedUnwrap, TypeIdMap, }; use bevy_ptr::OwningPtr; @@ -393,7 +394,9 @@ impl BundleInfo { let component_id = *self.component_ids.get_unchecked(bundle_component); match storage_type { StorageType::Table => { - let column = table.get_column_mut(component_id).unwrap(); + // SAFETY: If component_id is in self.component_is, the caller guarentees that + // the target table contains the ID. + let column = unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; // SAFETY: bundle_component is a valid index for this bundle match bundle_component_status.get_status(bundle_component) { ComponentStatus::Added => { @@ -405,7 +408,7 @@ impl BundleInfo { } } StorageType::SparseSet => { - sparse_sets.get_mut(component_id).unwrap().insert( + sparse_sets.get_mut(component_id).debug_checked_unwrap().insert( entity, component_ptr, change_tick, @@ -540,7 +543,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> { .archetype .edges() .get_add_bundle_internal(self.bundle_info.id) - .unwrap(); + .debug_checked_unwrap(); self.bundle_info.write_components( self.table, self.sparse_sets, @@ -555,7 +558,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { InsertBundleResult::NewArchetypeSameTable { new_archetype } => { let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { - let swapped_location = self.entities.get(swapped_entity).unwrap(); + // SAFETY: If the swap was successful, swapped_entity must be valid. + let swapped_location = unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; self.entities.set( swapped_entity.index(), EntityLocation { @@ -574,7 +578,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> { .archetype .edges() .get_add_bundle_internal(self.bundle_info.id) - .unwrap(); + .debug_checked_unwrap(); self.bundle_info.write_components( self.table, self.sparse_sets, @@ -592,7 +596,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { } => { let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { - let swapped_location = self.entities.get(swapped_entity).unwrap(); + // SAFETY: If the swap was successful, swapped_entity must be valid. + let swapped_location = unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; self.entities.set( swapped_entity.index(), EntityLocation { @@ -613,7 +618,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { // if an entity was moved into this entity's table spot, update its table row if let Some(swapped_entity) = move_result.swapped_entity { - let swapped_location = self.entities.get(swapped_entity).unwrap(); + // SAFETY: If the swap was successful, swapped_entity must be valid. + let swapped_location = unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; let swapped_archetype = if self.archetype.id() == swapped_location.archetype_id { &mut *self.archetype @@ -644,7 +650,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> { .archetype .edges() .get_add_bundle_internal(self.bundle_info.id) - .unwrap(); + .debug_checked_unwrap(); self.bundle_info.write_components( new_table, self.sparse_sets, From 78aa6dffdf683223af6b3c9af50c72b1f7d35477 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 12 Mar 2023 06:41:51 -0700 Subject: [PATCH 2/8] Add safety comments --- crates/bevy_ecs/src/bundle.rs | 67 ++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 38d1d1fa71d08..7a1fea477cfa7 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -12,8 +12,8 @@ use crate::{ }, component::{Component, ComponentId, ComponentStorage, Components, StorageType, Tick}, entity::{Entities, Entity, EntityLocation}, - storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow}, query::DebugCheckedUnwrap, + storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow}, TypeIdMap, }; use bevy_ptr::OwningPtr; @@ -394,9 +394,10 @@ impl BundleInfo { let component_id = *self.component_ids.get_unchecked(bundle_component); match storage_type { StorageType::Table => { - // SAFETY: If component_id is in self.component_is, the caller guarentees that - // the target table contains the ID. - let column = unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; + // SAFETY: If component_id is in self.component_ids, the caller guarentees that + // the target table contains the component. + let column = + unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; // SAFETY: bundle_component is a valid index for this bundle match bundle_component_status.get_status(bundle_component) { ComponentStatus::Added => { @@ -408,11 +409,11 @@ impl BundleInfo { } } StorageType::SparseSet => { - sparse_sets.get_mut(component_id).debug_checked_unwrap().insert( - entity, - component_ptr, - change_tick, - ); + // SAFETY: If component_id is in self.component_ids, the caller guarentees that + // a sparse set exists for the component. + let sparse_set = + unsafe { sparse_sets.get_mut(component_id).debug_checked_unwrap() }; + sparse_set.insert(entity, component_ptr, change_tick); } } bundle_component += 1; @@ -528,7 +529,8 @@ pub(crate) enum InsertBundleResult<'a> { impl<'a, 'b> BundleInserter<'a, 'b> { /// # Safety /// `entity` must currently exist in the source archetype for this inserter. `archetype_row` - /// must be `entity`'s location in the archetype. `T` must match this [`BundleInfo`]'s type + /// must be `entity`'s location in the archetype. `T` must match this [`BundleInfo`]'s type. + /// The `add_bundle` edge on `self.archetype` #[inline] pub unsafe fn insert( &mut self, @@ -539,11 +541,13 @@ impl<'a, 'b> BundleInserter<'a, 'b> { match &mut self.result { InsertBundleResult::SameArchetype => { // PERF: this could be looked up during Inserter construction and stored (but borrowing makes this nasty) - let add_bundle = self - .archetype - .edges() - .get_add_bundle_internal(self.bundle_info.id) - .debug_checked_unwrap(); + // SAFETY: The edge is assured to be initialized when creating the BundleInserter + let add_bundle = unsafe { + self.archetype + .edges() + .get_add_bundle_internal(self.bundle_info.id) + .debug_checked_unwrap() + }; self.bundle_info.write_components( self.table, self.sparse_sets, @@ -559,7 +563,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { // SAFETY: If the swap was successful, swapped_entity must be valid. - let swapped_location = unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; + let swapped_location = + unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; self.entities.set( swapped_entity.index(), EntityLocation { @@ -574,11 +579,13 @@ impl<'a, 'b> BundleInserter<'a, 'b> { self.entities.set(entity.index(), new_location); // PERF: this could be looked up during Inserter construction and stored (but borrowing makes this nasty) - let add_bundle = self - .archetype - .edges() - .get_add_bundle_internal(self.bundle_info.id) - .debug_checked_unwrap(); + // SAFETY: The edge is assured to be initialized when creating the BundleInserter + let add_bundle = unsafe { + self.archetype + .edges() + .get_add_bundle_internal(self.bundle_info.id) + .debug_checked_unwrap() + }; self.bundle_info.write_components( self.table, self.sparse_sets, @@ -597,7 +604,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { // SAFETY: If the swap was successful, swapped_entity must be valid. - let swapped_location = unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; + let swapped_location = + unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; self.entities.set( swapped_entity.index(), EntityLocation { @@ -619,7 +627,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { // if an entity was moved into this entity's table spot, update its table row if let Some(swapped_entity) = move_result.swapped_entity { // SAFETY: If the swap was successful, swapped_entity must be valid. - let swapped_location = unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; + let swapped_location = + unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; let swapped_archetype = if self.archetype.id() == swapped_location.archetype_id { &mut *self.archetype @@ -646,11 +655,13 @@ impl<'a, 'b> BundleInserter<'a, 'b> { } // PERF: this could be looked up during Inserter construction and stored (but borrowing makes this nasty) - let add_bundle = self - .archetype - .edges() - .get_add_bundle_internal(self.bundle_info.id) - .debug_checked_unwrap(); + // SAFETY: The edge is assured to be initialized when creating the BundleInserter + let add_bundle = unsafe { + self.archetype + .edges() + .get_add_bundle_internal(self.bundle_info.id) + .debug_checked_unwrap() + }; self.bundle_info.write_components( new_table, self.sparse_sets, From d0351dcf23c0c2b107b92dffd1ff00bac6db9c34 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 12 Mar 2023 07:33:13 -0700 Subject: [PATCH 3/8] Shut up clippy --- crates/bevy_ecs/src/bundle.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 7a1fea477cfa7..9cb774d7c5a13 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -394,9 +394,9 @@ impl BundleInfo { let component_id = *self.component_ids.get_unchecked(bundle_component); match storage_type { StorageType::Table => { - // SAFETY: If component_id is in self.component_ids, the caller guarentees that - // the target table contains the component. let column = + // SAFETY: If component_id is in self.component_ids, the caller guarentees that + // the target table contains the component. unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; // SAFETY: bundle_component is a valid index for this bundle match bundle_component_status.get_status(bundle_component) { @@ -409,9 +409,9 @@ impl BundleInfo { } } StorageType::SparseSet => { - // SAFETY: If component_id is in self.component_ids, the caller guarentees that - // a sparse set exists for the component. let sparse_set = + // SAFETY: If component_id is in self.component_ids, the caller guarentees that + // a sparse set exists for the component. unsafe { sparse_sets.get_mut(component_id).debug_checked_unwrap() }; sparse_set.insert(entity, component_ptr, change_tick); } @@ -562,8 +562,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { InsertBundleResult::NewArchetypeSameTable { new_archetype } => { let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { - // SAFETY: If the swap was successful, swapped_entity must be valid. let swapped_location = + // SAFETY: If the swap was successful, swapped_entity must be valid. unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; self.entities.set( swapped_entity.index(), @@ -603,8 +603,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { } => { let result = self.archetype.swap_remove(location.archetype_row); if let Some(swapped_entity) = result.swapped_entity { - // SAFETY: If the swap was successful, swapped_entity must be valid. let swapped_location = + // SAFETY: If the swap was successful, swapped_entity must be valid. unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; self.entities.set( swapped_entity.index(), @@ -626,8 +626,8 @@ impl<'a, 'b> BundleInserter<'a, 'b> { // if an entity was moved into this entity's table spot, update its table row if let Some(swapped_entity) = move_result.swapped_entity { - // SAFETY: If the swap was successful, swapped_entity must be valid. let swapped_location = + // SAFETY: If the swap was successful, swapped_entity must be valid. unsafe { self.entities.get(swapped_entity).debug_checked_unwrap() }; let swapped_archetype = if self.archetype.id() == swapped_location.archetype_id { From 0bdcd5280b4f323be34df249e93bb6e458c693b3 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 12 Mar 2023 19:55:40 -0700 Subject: [PATCH 4/8] Move the safety invariant to BundleInfo::new --- crates/bevy_ecs/src/bundle.rs | 89 ++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 43 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 9cb774d7c5a13..40316ca065bc0 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -262,11 +262,53 @@ impl SparseSetIndex for BundleId { } pub struct BundleInfo { + // TODO: Make these not pub(crate) pub(crate) id: BundleId, pub(crate) component_ids: Vec, } impl BundleInfo { + /// Create a new [`BundleInfo`]. + /// + /// # Safety + /// + /// `component_id` must be valid [`ComponentId`]'s in the same World and their associated storages must be + /// properly initialized. + unsafe fn new( + bundle_type_name: &'static str, + components: &Components, + component_ids: Vec, + id: BundleId, + ) -> BundleInfo { + let mut deduped = component_ids.clone(); + deduped.sort(); + deduped.dedup(); + + if deduped.len() != component_ids.len() { + // TODO: Replace with `Vec::partition_dedup` once https://github.com/rust-lang/rust/issues/54279 is stabilized + let mut seen = HashSet::new(); + let mut dups = Vec::new(); + for id in component_ids { + if !seen.insert(id) { + dups.push(id); + } + } + + let names = dups + .into_iter() + .map(|id| { + // SAFETY: component_id exists and is therefore valid + unsafe { components.get_info_unchecked(id).name() } + }) + .collect::>() + .join(", "); + + panic!("Bundle {bundle_type_name} has duplicate components: {names}"); + } + + BundleInfo { id, component_ids } + } + #[inline] pub fn id(&self) -> BundleId { self.id @@ -395,7 +437,7 @@ impl BundleInfo { match storage_type { StorageType::Table => { let column = - // SAFETY: If component_id is in self.component_ids, the caller guarentees that + // SAFETY: If component_id is in self.component_ids, BundleInfo::new requires that // the target table contains the component. unsafe { table.get_column_mut(component_id).debug_checked_unwrap() }; // SAFETY: bundle_component is a valid index for this bundle @@ -410,7 +452,7 @@ impl BundleInfo { } StorageType::SparseSet => { let sparse_set = - // SAFETY: If component_id is in self.component_ids, the caller guarentees that + // SAFETY: If component_id is in self.component_ids, BundleInfo::new requires that // a sparse set exists for the component. unsafe { sparse_sets.get_mut(component_id).debug_checked_unwrap() }; sparse_set.insert(entity, component_ptr, change_tick); @@ -529,8 +571,7 @@ pub(crate) enum InsertBundleResult<'a> { impl<'a, 'b> BundleInserter<'a, 'b> { /// # Safety /// `entity` must currently exist in the source archetype for this inserter. `archetype_row` - /// must be `entity`'s location in the archetype. `T` must match this [`BundleInfo`]'s type. - /// The `add_bundle` edge on `self.archetype` + /// must be `entity`'s location in the archetype. `T` must match this [`BundleInfo`]'s type #[inline] pub unsafe fn insert( &mut self, @@ -755,7 +796,7 @@ impl Bundles { let id = BundleId(bundle_infos.len()); let bundle_info = // SAFETY: T::component_id ensures info was created - unsafe { initialize_bundle(std::any::type_name::(), components, component_ids, id) }; + unsafe { BundleInfo::new(std::any::type_name::(), components, component_ids, id) }; bundle_infos.push(bundle_info); id }); @@ -763,41 +804,3 @@ impl Bundles { unsafe { self.bundle_infos.get_unchecked(id.0) } } } - -/// # Safety -/// -/// `component_id` must be valid [`ComponentId`]'s -unsafe fn initialize_bundle( - bundle_type_name: &'static str, - components: &Components, - component_ids: Vec, - id: BundleId, -) -> BundleInfo { - let mut deduped = component_ids.clone(); - deduped.sort(); - deduped.dedup(); - - if deduped.len() != component_ids.len() { - // TODO: Replace with `Vec::partition_dedup` once https://github.com/rust-lang/rust/issues/54279 is stabilized - let mut seen = HashSet::new(); - let mut dups = Vec::new(); - for id in component_ids { - if !seen.insert(id) { - dups.push(id); - } - } - - let names = dups - .into_iter() - .map(|id| { - // SAFETY: component_id exists and is therefore valid - unsafe { components.get_info_unchecked(id).name() } - }) - .collect::>() - .join(", "); - - panic!("Bundle {bundle_type_name} has duplicate components: {names}"); - } - - BundleInfo { id, component_ids } -} From 4c18677b611c3b53a51207a66feaf4ce8104711e Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 12 Mar 2023 19:58:55 -0700 Subject: [PATCH 5/8] Formatting --- crates/bevy_ecs/src/bundle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 40316ca065bc0..d8e7588e67e1e 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -272,7 +272,7 @@ impl BundleInfo { /// /// # Safety /// - /// `component_id` must be valid [`ComponentId`]'s in the same World and their associated storages must be + /// `component_id` must be valid [`ComponentId`]'s in the same World and their associated storages must be /// properly initialized. unsafe fn new( bundle_type_name: &'static str, From 44e16309c3f367a2d71b8328571704c102334baf Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 12 Mar 2023 19:59:39 -0700 Subject: [PATCH 6/8] Update safety comment for BundleInfo::new --- crates/bevy_ecs/src/bundle.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index d8e7588e67e1e..11d0d7ddcd87e 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -795,7 +795,8 @@ impl Bundles { T::component_ids(components, storages, &mut |id| component_ids.push(id)); let id = BundleId(bundle_infos.len()); let bundle_info = - // SAFETY: T::component_id ensures info was created + // SAFETY: T::component_id ensures info was created and the appropriate storage for it + // has been initialized. unsafe { BundleInfo::new(std::any::type_name::(), components, component_ids, id) }; bundle_infos.push(bundle_info); id From 11f7ff9485ed6df0da1e5d7d87eb5f42895713b7 Mon Sep 17 00:00:00 2001 From: James Liu Date: Mon, 13 Mar 2023 19:53:24 -0700 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com> --- crates/bevy_ecs/src/bundle.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index ae8e412450507..999424f6ced80 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -271,7 +271,7 @@ impl BundleInfo { /// /// # Safety /// - /// `component_id` must be valid [`ComponentId`]'s in the same World and their associated storages must be + /// `component_ids` must be valid [`ComponentId`]'s belonging to the same World and their associated storages must be /// properly initialized. unsafe fn new( bundle_type_name: &'static str, @@ -296,7 +296,7 @@ impl BundleInfo { let names = dups .into_iter() .map(|id| { - // SAFETY: component_id exists and is therefore valid + // SAFETY: the caller ensures component_id is valid. unsafe { components.get_info_unchecked(id).name() } }) .collect::>() From a6b1db8f0b12b4b8bfc43aa8ab59b6c1ebf92416 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 13 Mar 2023 21:09:17 -0700 Subject: [PATCH 8/8] Move safety invariant onto BundleInfo::component_ids --- crates/bevy_ecs/src/bundle.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index 999424f6ced80..5d8563776080b 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -263,6 +263,9 @@ impl SparseSetIndex for BundleId { pub struct BundleInfo { id: BundleId, + // SAFETY: Every ID in this list must be valid within the World that owns the BundleInfo, + // must have its storage initialized (i.e. columns created in tables, sparse set created), + // and must be in the same order as the source bundle type writes its components in. component_ids: Vec, } @@ -271,8 +274,9 @@ impl BundleInfo { /// /// # Safety /// - /// `component_ids` must be valid [`ComponentId`]'s belonging to the same World and their associated storages must be - /// properly initialized. + // Every ID in `component_ids` must be valid within the World that owns the BundleInfo, + // must have its storage initialized (i.e. columns created in tables, sparse set created), + // and must be in the same order as the source bundle type writes its components in. unsafe fn new( bundle_type_name: &'static str, components: &Components, @@ -305,6 +309,10 @@ impl BundleInfo { panic!("Bundle {bundle_type_name} has duplicate components: {names}"); } + // SAFETY: The caller ensures that component_ids: + // - is valid for the associated world + // - has had its storage initialized + // - is in the same order as the source bundle type BundleInfo { id, component_ids } } @@ -794,8 +802,10 @@ impl Bundles { T::component_ids(components, storages, &mut |id| component_ids.push(id)); let id = BundleId(bundle_infos.len()); let bundle_info = - // SAFETY: T::component_id ensures info was created and the appropriate storage for it - // has been initialized. + // SAFETY: T::component_id ensures its: + // - info was created + // - appropriate storage for it has been initialized. + // - was created in the same order as the components in T unsafe { BundleInfo::new(std::any::type_name::(), components, component_ids, id) }; bundle_infos.push(bundle_info); id