From 1605ecc188ceea82dc551176041b0f25e35e072d Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 13 Mar 2022 14:21:07 -0700 Subject: [PATCH 01/71] Beginnings of a safety audit. --- src/archetype/identifier/mod.rs | 69 ++++- src/archetype/mod.rs | 2 +- src/lib.rs | 6 +- src/registry/seal/storage.rs | 431 +++++++++++++++++++++++++++----- 4 files changed, 443 insertions(+), 65 deletions(-) diff --git a/src/archetype/identifier/mod.rs b/src/archetype/identifier/mod.rs index def35266..cab7e0b0 100644 --- a/src/archetype/identifier/mod.rs +++ b/src/archetype/identifier/mod.rs @@ -16,13 +16,46 @@ use core::{ slice, }; +/// A unique identifier for an [`Archetype`] using a [`Registry`] `R`. +/// +/// This is an allocated buffer of `(R::LEN + 7) / 8` bytes (enough bytes to have a bit for every +/// possible component type within the `Registry`). For each `Archetype`, a single `Identifier` +/// should be allocated, with [`IdentifierRef`]s being used to refer to that `Archetype`s +/// identification elsewhere. Where `Identifier` is essentially an allocated buffer of fixed size, +/// `IdentifierRef` is essentially a pointer to that allocated buffer. +/// +/// In a way, this identifier is a run-time canonical type signature for an entity, where each bit +/// represents a component being present in the entity. +/// +/// Note that several aspects of this identifier system are marked as `unsafe` for a reason. Care +/// must be taken with each of the methods involved to ensure they are used correctly. +/// Specifically, make sure that lifetimes of `Identifier`s are longer than the `IdentifierRef`s or +/// `Iter`s that are obtained from them, or you will end up accessing freed memory. Pay attention +/// to the safety requirements. +/// +/// [`Archetype`]: crate::archetype::Archetype +/// [`IdentifierRef`]: crate::archetype::IdentifierRef +/// [`Registry`]: crate::registry::Registry pub(crate) struct Identifier where R: Registry, { + /// The [`Registry`] defining the set of valid values of this identifier. + /// + /// Each identifier must exist within a set defined by a `Registry`. This defines a space over + /// which each identifier can uniquely define a set of components. Each bit within the + /// identifier corresponds with a component in the registry. + /// + /// The length of the allocated buffer is defined at compile-time as `(R::LEN + 7) / 8`. + /// + /// [`Registry`]: crate::registry::Registry registry: PhantomData, + /// Pointer to the allocated bytes. + /// + /// This allocation is owned by this identifier. pointer: *mut u8, + /// The capacity allotted to this allocation. capacity: usize, } @@ -30,6 +63,10 @@ impl Identifier where R: Registry, { + /// Create a new identifier from an allocated buffer. + /// + /// # Safety + /// `bytes` must be of length `(R::LEN + 7) / 8`. pub(crate) unsafe fn new(bytes: Vec) -> Self { let mut bytes = ManuallyDrop::new(bytes); Self { @@ -40,10 +77,22 @@ where } } + /// Returns a reference to the bytes defining this identifier. + /// + /// # Safety + /// The caller must ensure the `Identifier` outlives the returned slice. pub(crate) unsafe fn as_slice(&self) -> &[u8] { - slice::from_raw_parts(self.pointer, (R::LEN + 7) / 8) + // SAFETY: `pointer` is invariantly guaranteed to point to an allocation of length + // `(R::LEN + 7) / 8`. + unsafe { slice::from_raw_parts(self.pointer, (R::LEN + 7) / 8) } } + /// Returns a reference to this identifier. + /// + /// # Safety + /// The caller must ensure the `Identifier` outlives the returned [`IdentifierRef`]. + /// + /// [`IdentifierRef`]: crate::archetype::IdentifierRef pub(crate) unsafe fn as_ref(&self) -> IdentifierRef { IdentifierRef:: { registry: self.registry, @@ -52,11 +101,29 @@ where } } + /// Returns an iterator over the bits of this identifier. + /// + /// The returned iterator is guaranteed to return exactly `(R::LEN + 7) / 8` values, one for + /// each bit corresponding to the components of the registry. + /// + /// # Safety + /// The caller must ensure the `Identifier` outlives the returned [`Iter`]. + /// + /// [`Iter`]: crate::archetype::identifier::Iter pub(crate) unsafe fn iter(&self) -> Iter { Iter::::new(self.pointer) } + /// Returns the size of the components within the canonical entity represented by this + /// identifier. + /// + /// Every identifier essentially represents a canonical entity signature used by an archetype + /// so that the archetype knows how much space to allocate for the components it will store. + /// This method tells us exactly how large each entity stored in an archetype with this + /// identifier will be. pub(crate) fn size_of_components(&self) -> usize { + // SAFETY: `self.iter()` returns an `Iter`, which is the same `R` that the associated + // method belongs to. unsafe { R::size_of_components_for_identifier(self.iter()) } } } diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 40f0731c..91281ea5 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -59,7 +59,7 @@ where length: usize, ) -> Self { let mut component_map = HashMap::new(); - R::create_component_map_for_key(&mut component_map, 0, identifier_buffer.iter()); + R::create_component_map_for_identifier(&mut component_map, 0, identifier_buffer.iter()); Self { identifier_buffer, diff --git a/src/lib.rs b/src/lib.rs index 53106f4f..7d4bd702 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,10 @@ #![no_std] #![cfg_attr(doc_cfg, feature(doc_cfg, decl_macro))] -#![warn(clippy::pedantic)] +#![warn( + clippy::pedantic, + clippy::undocumented_unsafe_blocks, + unsafe_op_in_unsafe_fn +)] #![allow(clippy::module_name_repetitions)] extern crate alloc; diff --git a/src/registry/seal/storage.rs b/src/registry/seal/storage.rs index 8e8a44da..33baa152 100644 --- a/src/registry/seal/storage.rs +++ b/src/registry/seal/storage.rs @@ -14,28 +14,76 @@ use core::{ use hashbrown::HashMap; pub trait Storage { + /// Populate a map with component [`TypeId`]s and their associated index within the registry. + /// + /// [`TypeId`]: std::any::TypeId fn create_component_map(component_map: &mut HashMap, index: usize); - unsafe fn create_component_map_for_key( + /// Populate a map with component [`TypeId`]s and their associated index within the components + /// identified by the identifier in the order defined by the registry. + /// + /// # Safety + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + // there are components remaining. + /// + /// [`TypeId`]: std::any::TypeId + unsafe fn create_component_map_for_identifier( component_map: &mut HashMap, index: usize, identifier_iter: archetype::identifier::Iter, ) where R: Registry; + /// Populate a [`Vec`] with component columns corresponding to the given identifier, each with + /// the requested capacity. + /// + /// # Safety + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + // there are components remaining. + /// + /// [`Vec`]: std::vec::Vec unsafe fn new_components_with_capacity( components: &mut Vec<(*mut u8, usize)>, - length: usize, + capacity: usize, identifier_iter: archetype::identifier::Iter, ) where R: Registry; + /// Returns the size of the components indicated within the `Registry` by an identifier. + /// + /// # Safety + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + // there are components remaining. unsafe fn size_of_components_for_identifier( identifier_iter: archetype::identifier::Iter, ) -> usize where R: Registry; + /// Remove the component at the given index from each component column. + /// + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + // there are components remaining. unsafe fn remove_component_row( index: usize, components: &[(*mut u8, usize)], @@ -44,6 +92,33 @@ pub trait Storage { ) where R: Registry; + /// Remove the component at the given index from each component column, appending it to the + /// provided bit buffer. + /// + /// Note that the components stored in `buffer` are unaligned, and should be read as such when + /// the components are later read out of the buffer. + /// + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// `buffer` must be [valid](https://doc.rust-lang.org/std/ptr/index.html#safety) for writes. + /// Note that even if the combined size of components being stored is of size zero, this + /// pointer still must be non-null. + /// + /// `buffer` must point to an allocated buffer large enough to hold all components identified + /// by the `identifier_iter`. In other words, it must at least be of size + /// `R::size_of_components_for_identifier(identifier_iter)`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + // there are components remaining. unsafe fn pop_component_row( index: usize, buffer: *mut u8, @@ -53,6 +128,39 @@ pub trait Storage { ) where R: Registry; + /// Push components from a bit buffer, together with an additional component not in the bit + /// buffer, onto the end of their corresponding component columns. + /// + /// Note that the components stored in `buffer` are expected to be unaligned, being packed one + /// immediately after another, and will be read as such. + /// + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// `buffer` must be [valid](https://doc.rust-lang.org/std/ptr/index.html#safety) for reads. + /// Note that even if the combined size of components being stored is of size zero, this + /// pointer still must be non-null. + /// + /// `buffer` must point to an allocated buffer of packed, properly initialized components + /// corresponding with the components identified by `identifier_iter`, in the same order as + /// they are specified by the `Registry` on which this method is being called, with the + /// exception of the component corresponding to the component `C`, which must not be provided + /// in the buffer, but instead by provided using the separate `component` parameter. + /// + /// `component` must be a properly initialized value when called externally. When called + /// internally, `component` must be properly initialized if the bit corresponding to `C` has + /// not yet been read out of the `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + // there are components remaining. unsafe fn push_components_from_buffer_and_component( buffer: *const u8, component: MaybeUninit, @@ -63,6 +171,35 @@ pub trait Storage { C: Component, R: Registry; + /// Push components from a bit buffer, skipping the component `C`, onto the end of their + /// corresponding component columns. + /// + /// Note that the components stored in `buffer` are expected to be unaligned, being packed one + /// immediately after another, and will be read as such. + /// + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// `buffer` must be [valid](https://doc.rust-lang.org/std/ptr/index.html#safety) for reads. + /// Note that even if the combined size of components being stored is of size zero, this + /// pointer still must be non-null. + /// + /// `buffer` must point to an allocated buffer of packed, properly initialized components + /// corresponding with the components identified by `identifier_iter`, in the same order as + /// they are specified by the `Registry` on which this method is being called, with an + /// additional component of type `C` in its proper place in the ordering that is not identified + /// in the `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + // there are components remaining. unsafe fn push_components_from_buffer_skipping_component( buffer: *const u8, component: PhantomData, @@ -97,7 +234,7 @@ pub trait Storage { impl Storage for Null { fn create_component_map(_component_map: &mut HashMap, _index: usize) {} - unsafe fn create_component_map_for_key( + unsafe fn create_component_map_for_identifier( _component_map: &mut HashMap, _index: usize, _identifier_iter: archetype::identifier::Iter, @@ -108,7 +245,7 @@ impl Storage for Null { unsafe fn new_components_with_capacity( _components: &mut Vec<(*mut u8, usize)>, - _length: usize, + _capacity: usize, _identifier_iter: archetype::identifier::Iter, ) where R: Registry, @@ -206,33 +343,44 @@ where R::create_component_map(component_map, index + 1); } - unsafe fn create_component_map_for_key( + unsafe fn create_component_map_for_identifier( component_map: &mut HashMap, mut index: usize, mut identifier_iter: archetype::identifier::Iter, ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { component_map.insert(TypeId::of::(), index); index += 1; } - R::create_component_map_for_key(component_map, index, identifier_iter); + // SAFETY: One bit of `identifier_iter` has been consumed, and since `R` is one component + // smaller than `(C, R)`, `identifier_iter` has the same number of bits remaining as `R` + // has components remaining. + unsafe { R::create_component_map_for_identifier(component_map, index, identifier_iter) }; } unsafe fn new_components_with_capacity( components: &mut Vec<(*mut u8, usize)>, - length: usize, + capacity: usize, mut identifier_iter: archetype::identifier::Iter, ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - let mut v = ManuallyDrop::new(Vec::::with_capacity(length)); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let mut v = ManuallyDrop::new(Vec::::with_capacity(capacity)); components.push((v.as_mut_ptr().cast::(), v.capacity())); } - - R::new_components_with_capacity(components, length, identifier_iter); + // SAFETY: One bit of `identifier_iter` has been consumed, and since `R` is one component + // smaller than `(C, R)`, `identifier_iter` has the same number of bits remaining as `R` + // has components remaining. + unsafe { R::new_components_with_capacity(components, capacity, identifier_iter) }; } unsafe fn size_of_components_for_identifier( @@ -241,8 +389,15 @@ where where R_: Registry, { - (usize::from(identifier_iter.next().unwrap_unchecked()) * size_of::()) - + R::size_of_components_for_identifier(identifier_iter) + (usize::from( + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() }) * size_of::()) + + + // SAFETY: One bit of `identifier_iter` has been consumed, and since `R` is one + // component smaller than `(C, R)`, `identifier_iter` has the same number of bits + // remaining as `R` has components remaining. + unsafe { R::size_of_components_for_identifier(identifier_iter) } } unsafe fn remove_component_row( @@ -253,18 +408,52 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked(0); - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(0) }; + let mut v = ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract + // of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); v.swap_remove(index); - components = components.get_unchecked(1..); + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(1..) }; } - R::remove_component_row(index, components, length, identifier_iter); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { R::remove_component_row(index, components, length, identifier_iter) }; } unsafe fn pop_component_row( @@ -276,20 +465,69 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked(0); - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); - - ptr::write_unaligned(buffer.cast::(), v.swap_remove(index)); - buffer = buffer.add(size_of::()); - - components = components.get_unchecked(1..); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(0) }; + let mut v = ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract + // of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); + + // SAFETY: `buffer` is valid for writes and points within an allocation that is large + // enough to store the component `C`, as guaranteed by the safety contract of this + // method. + unsafe { + ptr::write_unaligned(buffer.cast::(), v.swap_remove(index)); + } + buffer = + // SAFETY: Since `buffer` points within an allocation that is large enough to store + // a component of size `C`, then moving it `size_of::()` bytes means the pointer + // will still be within the same allocation, or one byte past the allocated object + // if there are no more components remaining. + unsafe { buffer.add(size_of::()) }; + + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(1..) }; } - R::pop_component_row(index, buffer, components, length, identifier_iter); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. Also, `buffer` will be offset by the + // size of `C`, which means it will have enough space left in its allocation for all + // remaining components identified by `identifier_iter`, and will therefore be valid for + // writes still. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. Also, + // `buffer` will remain unchanged and still point to an allocation with enough space to + // write all remaining components identified. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { R::pop_component_row(index, buffer, components, length, identifier_iter) }; } unsafe fn push_components_from_buffer_and_component( @@ -302,44 +540,113 @@ where C_: Component, R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked_mut(0); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(0) }; if TypeId::of::() == TypeId::of::() { - // Consume the component. This is sound, since we won't ever read this - // component again. This is because each component type is guaranteed to only - // occur once within an Archetype's identifier. - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); - v.push(component.assume_init()); + let mut v = ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety + // contract of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); + v.push( + // SAFETY: Since each unique component type is guaranteed to only be set once + // within `identifier_iter`, then we can guarantee this value has not been read + // and overwritten previously and will not be read again after this point. + // Therefore, `component` is guaranteed to be properly initialized and valid. + unsafe { component.assume_init() } + ); + // Since the component won't be read again, we can set it to an uninitialized + // value. component = MaybeUninit::uninit(); *component_column = (v.as_mut_ptr().cast::(), v.capacity()); } else { - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); - v.push(buffer.cast::().read_unaligned()); - buffer = buffer.add(size_of::()); + let mut v = ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety + // contract of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); + v.push( + // SAFETY: `buffer` is guaranteed by the safety contract of the method to be + // valid for reads and to point to all components identified by + // `identifier_iter` (except for the `component` parameter) in the order they + // are specified in the `Registry`. Therefore, the pointer must point to a + // valid, properly initialized value of type `C`. + unsafe { buffer.cast::().read_unaligned() } + ); + buffer = + // SAFETY: `buffer` is guaranteed by the safety contract of the method to point + // to a packed buffer of components corresponding to all components identified + // by `identifier_iter` within the registry, except the component `component`. + // Therefore, offsetting the buffer by `size_of::()` will point it to the + // next component within the same allocation, or it will point it to one byte + // past the end of the allocation if no more components are in the buffer. + unsafe { buffer.add(size_of::()) }; *component_column = (v.as_mut_ptr().cast::(), v.capacity()); } - components = components.get_unchecked_mut(1..); + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(1..) }; } - R::push_components_from_buffer_and_component( - buffer, - component, - components, - length, - identifier_iter, - ); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. Also, `buffer` is still guaranteed to + // hold the remaining components identified, save for the `component` parameter, whether or + // not a value was actually read from `buffer`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. Also, + // `buffer` will remain unchanged and still point to an allocation of valid components + // identified by `identier_iter`. + // + // If `components` had not been read yet, it will still be a valid value. If it has been + // read, it will no longer be valid but it will also no longer be read, since + // `identifier_iter` will not identify the same unique component type twice. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { + R::push_components_from_buffer_and_component( + buffer, + component, + components, + length, + identifier_iter, + ) + }; } unsafe fn push_components_from_buffer_skipping_component( From 66c712696488fb159e2c60d1bb97533896372174 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 13 Mar 2022 23:26:28 -0700 Subject: [PATCH 02/71] Safety audit for registry::seal::Storage trait. --- src/registry/seal/storage.rs | 402 ++++++++++++++++++++++++++++------- 1 file changed, 326 insertions(+), 76 deletions(-) diff --git a/src/registry/seal/storage.rs b/src/registry/seal/storage.rs index b6d84f6b..3a3d8847 100644 --- a/src/registry/seal/storage.rs +++ b/src/registry/seal/storage.rs @@ -27,7 +27,7 @@ pub trait Storage { /// `Registry` on which this method is being called. /// /// When called internally, the `identifier_iter` must have the same amount of bits left as - // there are components remaining. + /// there are components remaining. /// /// [`TypeId`]: std::any::TypeId unsafe fn create_component_map_for_identifier( @@ -45,7 +45,7 @@ pub trait Storage { /// `Registry` on which this method is being called. /// /// When called internally, the `identifier_iter` must have the same amount of bits left as - // there are components remaining. + /// there are components remaining. /// /// [`Vec`]: std::vec::Vec unsafe fn new_components_with_capacity( @@ -62,7 +62,7 @@ pub trait Storage { /// `Registry` on which this method is being called. /// /// When called internally, the `identifier_iter` must have the same amount of bits left as - // there are components remaining. + /// there are components remaining. unsafe fn size_of_components_for_identifier( identifier_iter: archetype::identifier::Iter, ) -> usize @@ -83,7 +83,7 @@ pub trait Storage { /// `Registry` on which this method is being called. /// /// When called internally, the `identifier_iter` must have the same amount of bits left as - // there are components remaining. + /// there are components remaining. unsafe fn remove_component_row( index: usize, components: &[(*mut u8, usize)], @@ -118,7 +118,7 @@ pub trait Storage { /// `Registry` on which this method is being called. /// /// When called internally, the `identifier_iter` must have the same amount of bits left as - // there are components remaining. + /// there are components remaining. unsafe fn pop_component_row( index: usize, buffer: *mut u8, @@ -156,11 +156,13 @@ pub trait Storage { /// internally, `component` must be properly initialized if the bit corresponding to `C` has /// not yet been read out of the `identifier_iter`. /// + /// The `Registry` `R` must not contain any duplicate component types. + /// /// When called externally, the `Registry` `R` provided to the method must by the same as the /// `Registry` on which this method is being called. /// /// When called internally, the `identifier_iter` must have the same amount of bits left as - // there are components remaining. + /// there are components remaining. unsafe fn push_components_from_buffer_and_component( buffer: *const u8, component: MaybeUninit, @@ -195,11 +197,13 @@ pub trait Storage { /// additional component of type `C` in its proper place in the ordering that is not identified /// in the `identifier_iter`. /// + /// The `Registry` `R` must not contain any duplicate component types. + /// /// When called externally, the `Registry` `R` provided to the method must by the same as the /// `Registry` on which this method is being called. /// /// When called internally, the `identifier_iter` must have the same amount of bits left as - // there are components remaining. + /// there are components remaining. unsafe fn push_components_from_buffer_skipping_component( buffer: *const u8, component: PhantomData, @@ -210,6 +214,24 @@ pub trait Storage { C: Component, R: Registry; + /// Free the allocated memory for each component column. + /// + /// This converts all component columns back into `Vec` for each component `C`, and then + /// drops them, allowing the memory to be freed. + /// + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn free_components( components: &[(*mut u8, usize)], length: usize, @@ -217,6 +239,28 @@ pub trait Storage { ) where R: Registry; + /// Attempt to free the allocated memory for each component column. + /// + /// By "attempt", this method frees the component columns until there are no more component + /// columns left in `components`. This is necessary for things like deserialization, where some + /// columns may have been created, but an invalid value was attempted to be deserialized and + /// now the whole collection must be deallocated before returning an error. Column length is + /// the only thing that is not expected to be correct here; all other requirements for + /// `free_components` are still expected to be upheld. + /// + /// # Safety + /// `components` must contain up to the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn try_free_components( components: &[(*mut u8, usize)], length: usize, @@ -224,6 +268,24 @@ pub trait Storage { ) where R: Registry; + /// Clear all components from the component columns, saving the heap allocations to be reused. + /// + /// Note that this does not free the component columns. It simply removes all elements from the + /// columns. + /// + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn clear_components( components: &mut [(*mut u8, usize)], length: usize, @@ -231,6 +293,20 @@ pub trait Storage { ) where R: Registry; + /// Populate a [`DebugList`] with string forms of the names of every component type identified + /// by `identifier_iter`. + /// + /// This is meant to be used for debugging purposes. The string names output by this method are + /// not stable. + /// + /// # Safety + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. + /// + /// [`DebugList`]: core::fmt::DebugList unsafe fn debug_identifier( debug_list: &mut DebugList, identifier_iter: archetype::identifier::Iter, @@ -446,14 +522,14 @@ where ); v.swap_remove(index); - components = + components = // SAFETY: `components` is guaranteed to have the same number of values as there // set bits in `identifier_iter`. Since a bit must have been set to enter this // block, there must be at least one component column. unsafe { components.get_unchecked(1..) }; } // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two - // possibilities here: either the bit was set or it was not. + // possibilities here: either the bit was set or it was not. // // If the bit was set, then the `components` slice will no longer include the first value, // which means the slice will still contain the same number of pointer and capacity tuples @@ -515,14 +591,14 @@ where // if there are no more components remaining. unsafe { buffer.add(size_of::()) }; - components = + components = // SAFETY: `components` is guaranteed to have the same number of values as there // set bits in `identifier_iter`. Since a bit must have been set to enter this // block, there must be at least one component column. unsafe { components.get_unchecked(1..) }; } // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two - // possibilities here: either the bit was set or it was not. + // possibilities here: either the bit was set or it was not. // // If the bit was set, then the `components` slice will no longer include the first value, // which means the slice will still contain the same number of pointer and capacity tuples @@ -538,7 +614,7 @@ where // number of elements as there are set bits in `identifier_iter`, which still make valid // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. Also, // `buffer` will remain unchanged and still point to an allocation with enough space to - // write all remaining components identified. + // write all remaining components identified. // // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the @@ -564,7 +640,7 @@ where // SAFETY: `components` is guaranteed to have the same number of values as there // set bits in `identifier_iter`. Since a bit must have been set to enter this // block, there must be at least one component column. - unsafe { components.get_unchecked(0) }; + unsafe { components.get_unchecked_mut(0) }; if TypeId::of::() == TypeId::of::() { let mut v = ManuallyDrop::new( @@ -579,11 +655,10 @@ where }, ); v.push( - // SAFETY: Since each unique component type is guaranteed to only be set once - // within `identifier_iter`, then we can guarantee this value has not been read - // and overwritten previously and will not be read again after this point. - // Therefore, `component` is guaranteed to be properly initialized and valid. - unsafe { component.assume_init() } + // SAFETY: Since each component within a registry must be unique, we can + // guarantee that we will only read `component` one time, at which point it + // be a valid properly initialized value. + unsafe { component.assume_init() }, ); // Since the component won't be read again, we can set it to an uninitialized // value. @@ -608,9 +683,9 @@ where // `identifier_iter` (except for the `component` parameter) in the order they // are specified in the `Registry`. Therefore, the pointer must point to a // valid, properly initialized value of type `C`. - unsafe { buffer.cast::().read_unaligned() } + unsafe { buffer.cast::().read_unaligned() }, ); - buffer = + buffer = // SAFETY: `buffer` is guaranteed by the safety contract of the method to point // to a packed buffer of components corresponding to all components identified // by `identifier_iter` within the registry, except the component `component`. @@ -622,15 +697,15 @@ where *component_column = (v.as_mut_ptr().cast::(), v.capacity()); } - components = + components = // SAFETY: `components` is guaranteed to have the same number of values as there // set bits in `identifier_iter`. Since a bit must have been set to enter this // block, there must be at least one component column. - unsafe { components.get_unchecked(1..) }; + unsafe { components.get_unchecked_mut(1..) }; } // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two - // possibilities here: either the bit was set or it was not. + // possibilities here: either the bit was set or it was not. // // If the bit was set, then the `components` slice will no longer include the first value, // which means the slice will still contain the same number of pointer and capacity tuples @@ -647,13 +722,15 @@ where // `buffer` will remain unchanged and still point to an allocation of valid components // identified by `identier_iter`. // - // If `components` had not been read yet, it will still be a valid value. If it has been + // If `component` had not been read yet, it will still be a valid value. If it has been // read, it will no longer be valid but it will also no longer be read, since // `identifier_iter` will not identify the same unique component type twice. // // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the // same number of bits remaining as `R` has components remaining. + // + // Finally, since `(C, R)` contains no duplicate components, neither does `R`. unsafe { R::push_components_from_buffer_and_component( buffer, @@ -677,31 +754,100 @@ where { if TypeId::of::() == TypeId::of::() { // Skip this component in the buffer. - buffer = buffer.add(size_of::()); + buffer = + // SAFETY: The bit buffer is guaranteed to have a value of type `C` at this point + // because the components within the bit buffer are guaranteed to be ordered in the + // same order as the registry. + unsafe { buffer.add(size_of::()) }; // Pop the next identifier bit. We don't need to look at it, since the component is - // being skipped anyway. + // being skipped anyway. We also know the bit was not set because of the safety + // contract on this method, which is why we know we don't need to pop any component + // pointer-capacity tuples from `components`. identifier_iter.next(); - } else if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked_mut(0); - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); - v.push(buffer.cast::().read_unaligned()); + } else if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked_mut(0) }; + let mut v = ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety + // contract of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); + v.push( + // SAFETY: `buffer` is guaranteed by the safety contract of the method to be + // valid for reads and to point to all components identified by + // `identifier_iter` (except for the `component` parameter) in the order they + // are specified in the `Registry`. Therefore, the pointer must point to a + // valid, properly initialized value of type `C`. + unsafe { buffer.cast::().read_unaligned() }, + ); + buffer = + // SAFETY: `buffer` is guaranteed by the safety contract of the method to point + // to a packed buffer of components corresponding to all components identified + // by `identifier_iter` within the registry, except the component `component`. + // Therefore, offsetting the buffer by `size_of::()` will point it to the + // next component within the same allocation, or it will point it to one byte + // past the end of the allocation if no more components are in the buffer. + unsafe { buffer.add(size_of::()) }; + *component_column = (v.as_mut_ptr().cast::(), v.capacity()); - components = components.get_unchecked_mut(1..); - buffer = buffer.add(size_of::()); + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked_mut(1..) }; } - R::push_components_from_buffer_skipping_component( - buffer, - component, - components, - length, - identifier_iter, - ); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. Also, `buffer` is still guaranteed to + // hold the remaining components identified, save for the `component` parameter, whether or + // not a value was actually read from `buffer`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. Also, + // `buffer` will remain unchanged and still point to an allocation of valid components + // identified by `identier_iter`. + // + // If the bit was not set but the component was of type `C_`, then the bit buffer will have + // been offset by `size_of::()`. This fills the safety contract of the next call, as it + // guarantees the rest of the components in the buffer will be components specified in the + // identifier, which will match the registry since there are no duplicate components in the + // registry. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + // + // Finally, since `(C, R)` contains no duplicate components, neither does `R`. + unsafe { + R::push_components_from_buffer_skipping_component( + buffer, + component, + components, + length, + identifier_iter, + ) + }; } unsafe fn free_components( @@ -711,16 +857,50 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked(0); - drop(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); - components = components.get_unchecked(1..); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(0) }; + drop( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety + // contract of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(1..) }; } - R::free_components(components, length, identifier_iter); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { R::free_components(components, length, identifier_iter) }; } unsafe fn try_free_components( @@ -730,26 +910,52 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { let component_column = match components.get(0) { Some(component_column) => component_column, None => { return; } }; - drop(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); - components = match components.get(1..) { - Some(components) => components, - None => { - return; - } - }; + drop( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety + // contract of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); + components = + // SAFETY: Since we didn't return on the `get(0)` call above, we know `components` + // has at least one element. + unsafe { + components.get_unchecked(1..) + }; } - R::try_free_components(components, length, identifier_iter); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain up to the number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still up to the + // same number of elements as there are set bits in `identifier_iter`, which still make + // valid `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { R::try_free_components(components, length, identifier_iter) }; } unsafe fn clear_components( @@ -759,18 +965,54 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked_mut(0); - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked_mut(0) }; + let mut v = ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety + // contract of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + ); v.clear(); - components = components.get_unchecked_mut(1..); + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked_mut(1..) }; } - R::clear_components(components, length, identifier_iter); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain up to the number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still up to the + // same number of elements as there are set bits in `identifier_iter`, which still make + // valid `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { + R::clear_components(components, length, identifier_iter) + }; } unsafe fn debug_identifier( @@ -779,10 +1021,18 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { debug_list.entry(&type_name::()); } - R::debug_identifier(debug_list, identifier_iter); + // SAFETY: One bit of `identifier_iter` has been consumed, and since `R` is one component + // smaller than `(C, R)`, `identifier_iter` has the same number of bits remaining as `R` + // has components remaining. + unsafe { + R::debug_identifier(debug_list, identifier_iter) + }; } } From 1c99b2e22598db22f6521f9e536daaf4fea187d7 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 17 Mar 2022 08:28:08 -0700 Subject: [PATCH 03/71] Several tests and some safety audits. --- Cargo.toml | 1 + src/archetype/identifier/mod.rs | 15 +- src/registry/seal/length.rs | 32 ++ src/registry/seal/mod.rs | 22 ++ src/registry/seal/storage.rs | 611 +++++++++++++++++++++++++++++++- 5 files changed, 676 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bb54b75b..1884d85a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ rayon = {version = "1.5.1", optional = true} serde = {version = "1.0.133", default-features = false, features = ["alloc"], optional = true} [dev-dependencies] +claim = "0.5.0" rustversion = "1.0.6" serde_test = "1.0.133" trybuild = "1.0.56" diff --git a/src/archetype/identifier/mod.rs b/src/archetype/identifier/mod.rs index cab7e0b0..4a80e897 100644 --- a/src/archetype/identifier/mod.rs +++ b/src/archetype/identifier/mod.rs @@ -124,6 +124,8 @@ where pub(crate) fn size_of_components(&self) -> usize { // SAFETY: `self.iter()` returns an `Iter`, which is the same `R` that the associated // method belongs to. + // + // Additionally, the iterator returned by `self.iter()` will not outlive `self`. unsafe { R::size_of_components_for_identifier(self.iter()) } } } @@ -133,6 +135,8 @@ where R: Registry, { fn eq(&self, other: &Self) -> bool { + // SAFETY: Both `self` and `other` outlive these slices, as the slices are dropped at the + // end of this method. unsafe { self.as_slice() == other.as_slice() } } } @@ -142,7 +146,12 @@ where R: Registry, { fn drop(&mut self) { - drop(unsafe { Vec::from_raw_parts(self.pointer, (R::LEN + 7) / 8, self.capacity) }); + drop( + // SAFETY: `self.pointer` points to an allocated buffer of length `(R::LEN + 7)`. This + // is an invariant upheld by the `Identifier` struct. Additionally, it is guaranteed to + // have a capacity of `self.capacity`. + unsafe { Vec::from_raw_parts(self.pointer, (R::LEN + 7) / 8, self.capacity) }, + ); } } @@ -153,6 +162,10 @@ where fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut debug_list = f.debug_list(); + // SAFETY: `self.iter()` returns an `Iter`, which is the same `R` that the associated + // method belongs to. + // + // Additionally, the iterator returned by `self.iter()` will not outlive `self`. unsafe { R::debug_identifier(&mut debug_list, self.iter()); } diff --git a/src/registry/seal/length.rs b/src/registry/seal/length.rs index 00dcc870..d9e67962 100644 --- a/src/registry/seal/length.rs +++ b/src/registry/seal/length.rs @@ -1,6 +1,14 @@ +//! Defines and implements a trait for length on a [`Registry`]. +//! +//! [`Registry`]: crate::registry::Registry + use crate::{component::Component, registry::Null}; +/// Defines a length for the given heterogeneous list. pub trait Length { + /// The number of components within the heterogeneous list. + /// + /// This is defined recursively at compile time. const LEN: usize; } @@ -15,3 +23,27 @@ where { const LEN: usize = R::LEN + 1; } + +#[cfg(test)] +mod tests { + use super::Length; + use crate::registry; + + #[test] + fn empty() { + type Registry = registry!(); + + assert_eq!(Registry::LEN, 0); + } + + #[test] + fn non_empty() { + struct A; + struct B; + struct C; + + type Registry = registry!(A, B, C); + + assert_eq!(Registry::LEN, 3); + } +} diff --git a/src/registry/seal/mod.rs b/src/registry/seal/mod.rs index 0498b2ec..38e00eb6 100644 --- a/src/registry/seal/mod.rs +++ b/src/registry/seal/mod.rs @@ -1,3 +1,16 @@ +//! Traits functions internal to the library. +//! +//! While [`Registry`] is a public trait, there are some implementation details that should be kept +//! private. This is done using a `Seal` trait, which is technically public, but within a private +//! module and therefore inaccessible to external users of the library. +//! +//! Additionally, making `Registry` a sealed trait blocks any external users from implementing it +//! on their own types that were never intended to be `Registry`s. +//! +//! Nothing within this module should be considered a part of the public API. +//! +//! [`Registry`]: crate::registry::Registry + mod assertions; mod length; mod storage; @@ -7,6 +20,15 @@ use assertions::Assertions; use length::Length; use storage::Storage; +/// A trait that is public but defined within a private module. +/// +/// This effectively hides all function definitions, making them only usable internally within this +/// library. Additionally, no external types can implement this trait, and therefore no external +/// types can implement `Registry`. +/// +/// While this trait specifically does not have any functions implemented, the traits it relies on +/// do. See the modules where they are defined for more details on the internal functionality +/// defined through these sealed traits. pub trait Seal: Assertions + Length + Storage {} impl Seal for Null {} diff --git a/src/registry/seal/storage.rs b/src/registry/seal/storage.rs index 3a3d8847..efe2965f 100644 --- a/src/registry/seal/storage.rs +++ b/src/registry/seal/storage.rs @@ -1,3 +1,17 @@ +//! This module defines and implements [`World`] storage within a [`Registry`]. +//! +//! In order to store components within archetype tables in a `World`, we have to define recursive +//! functions over the components of the heterogeneous lists that are the registries. For most +//! functions an `identifier_iter` is provided which informs the `Registry` which components should +//! be acted upon for the columns of that table. +//! +//! Note that the majority of this code is `unsafe`. This is due to the component columns being +//! type-erased, requiring us to infer the type from the registry and identifier. This is not taken +//! lightly, and great care has been used in auditing the unsafe code blocks here. +//! +//! [`Registry`]: crate::registry::Registry +//! [`World`]: crate::world::World + use crate::{ archetype, component::Component, @@ -1010,9 +1024,7 @@ where // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the // same number of bits remaining as `R` has components remaining. - unsafe { - R::clear_components(components, length, identifier_iter) - }; + unsafe { R::clear_components(components, length, identifier_iter) }; } unsafe fn debug_identifier( @@ -1031,8 +1043,599 @@ where // SAFETY: One bit of `identifier_iter` has been consumed, and since `R` is one component // smaller than `(C, R)`, `identifier_iter` has the same number of bits remaining as `R` // has components remaining. + unsafe { R::debug_identifier(debug_list, identifier_iter) }; + } +} + +#[cfg(test)] +mod tests { + use super::Storage; + use crate::{archetype::Identifier, registry}; + use alloc::{vec, vec::Vec}; + use claim::{assert_none, assert_some_eq}; + use core::{ + any::TypeId, + mem::{size_of, ManuallyDrop}, + }; + use hashbrown::HashMap; + + #[test] + fn create_component_map_for_empty_registry() { + type Registry = registry!(); + + let mut component_map = HashMap::new(); + Registry::create_component_map(&mut component_map, 0); + + assert!(component_map.is_empty()); + } + + #[test] + fn create_component_map_for_registry() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + + let mut component_map = HashMap::new(); + Registry::create_component_map(&mut component_map, 0); + + assert_some_eq!(component_map.get(&TypeId::of::()), &0); + assert_some_eq!(component_map.get(&TypeId::of::()), &1); + assert_some_eq!(component_map.get(&TypeId::of::()), &2); + } + + #[test] + fn create_component_map_for_registry_starting_from_nonzero() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + + let mut component_map = HashMap::new(); + Registry::create_component_map(&mut component_map, 42); + + assert_some_eq!(component_map.get(&TypeId::of::()), &42); + assert_some_eq!(component_map.get(&TypeId::of::()), &43); + assert_some_eq!(component_map.get(&TypeId::of::()), &44); + } + + #[test] + #[should_panic] + fn create_component_map_for_registry_overflow() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + + let mut component_map = HashMap::new(); + Registry::create_component_map(&mut component_map, usize::MAX); + } + + #[test] + fn create_component_map_for_identifier_empty() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + + let mut component_map = HashMap::new(); + unsafe { + Registry::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) + }; + + assert!(component_map.is_empty()); + } + + #[test] + fn create_component_map_for_identifier_all() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + + let mut component_map = HashMap::new(); + unsafe { + Registry::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) + }; + + assert_some_eq!(component_map.get(&TypeId::of::()), &0); + assert_some_eq!(component_map.get(&TypeId::of::()), &1); + assert_some_eq!(component_map.get(&TypeId::of::()), &2); + } + + #[test] + fn create_component_map_for_identifier_subset() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![3]) }; + + let mut component_map = HashMap::new(); + unsafe { + Registry::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) + }; + + assert_some_eq!(component_map.get(&TypeId::of::()), &0); + assert_some_eq!(component_map.get(&TypeId::of::()), &1); + assert_none!(component_map.get(&TypeId::of::())); + } + + #[test] + fn create_component_map_for_empty_identifier() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![0]) }; + + let mut component_map = HashMap::new(); + unsafe { + Registry::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) + }; + + assert_none!(component_map.get(&TypeId::of::())); + assert_none!(component_map.get(&TypeId::of::())); + assert_none!(component_map.get(&TypeId::of::())); + } + + #[test] + fn create_component_map_for_identifier_subset_starting_from_nonzero() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + + let mut component_map = HashMap::new(); + unsafe { + Registry::create_component_map_for_identifier(&mut component_map, 42, identifier.iter()) + }; + + assert_some_eq!(component_map.get(&TypeId::of::()), &42); + assert_none!(component_map.get(&TypeId::of::())); + assert_some_eq!(component_map.get(&TypeId::of::()), &43); + } + + #[test] + #[should_panic] + fn create_component_map_for_identifier_subset_overflow() { + struct A; + struct B; + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + + let mut component_map = HashMap::new(); + unsafe { + Registry::create_component_map_for_identifier( + &mut component_map, + usize::MAX, + identifier.iter(), + ) + }; + } + + #[test] + fn new_components_with_capacity_empty_registry() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + + let mut components = Vec::new(); + unsafe { Registry::new_components_with_capacity(&mut components, 100, identifier.iter()) }; + + assert!(components.is_empty()); + } + + #[test] + fn new_components_with_capacity_all_components() { + struct A(usize); + struct B(usize); + struct C(usize); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + const CAPACITY: usize = 100; + + let mut components = Vec::new(); + unsafe { + Registry::new_components_with_capacity(&mut components, CAPACITY, identifier.iter()) + }; + + assert_eq!(components.get(0).unwrap().1, CAPACITY); + assert_eq!(components.get(1).unwrap().1, CAPACITY); + assert_eq!(components.get(2).unwrap().1, CAPACITY); + } + + #[test] + fn new_components_with_capacity_some_components() { + struct A(usize); + struct B(usize); + struct C(usize); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + const CAPACITY: usize = 100; + + let mut components = Vec::new(); + unsafe { + Registry::new_components_with_capacity(&mut components, CAPACITY, identifier.iter()) + }; + + assert_eq!(components.get(0).unwrap().1, CAPACITY); + assert_eq!(components.get(1).unwrap().1, CAPACITY); + } + + #[test] + fn new_components_with_capacity_no_components() { + struct A(usize); + struct B(usize); + struct C(usize); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![0]) }; + const CAPACITY: usize = 100; + + let mut components = Vec::new(); + unsafe { + Registry::new_components_with_capacity(&mut components, CAPACITY, identifier.iter()) + }; + + assert!(components.is_empty()); + } + + #[test] + fn size_of_components_for_identifier_empty_registry() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + + let size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + + assert_eq!(size, 0); + } + + #[test] + fn size_of_components_for_identifier_all_components() { + struct A; + struct B(f32); + struct C(u8); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + + let size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + + assert_eq!(size, 5); + } + + #[test] + fn size_of_components_for_identifier_some_components() { + struct A; + struct B(f32); + struct C(u8); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + + let size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + + assert_eq!(size, 1); + } + + #[test] + fn size_of_components_for_empty_identifier() { + struct A; + struct B(f32); + struct C(u8); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![0]) }; + + let size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + + assert_eq!(size, 0); + } + + #[test] + fn remove_component_row_empty_registry() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + // `components` must be empty because there are no components in the registry. + let mut components = Vec::new(); + + unsafe { Registry::remove_component_row(0, &mut components, 1, identifier.iter()) }; + + assert!(components.is_empty()); + } + + #[test] + fn remove_component_row_all_components() { + #[derive(Debug, Eq, PartialEq)] + struct A(usize); + #[derive(Debug, Eq, PartialEq)] + struct B(bool); + #[derive(Debug, Eq, PartialEq)] + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut b_column = ManuallyDrop::new(vec![B(false), B(true), B(true)]); + let mut c_column = ManuallyDrop::new(vec![C, C, C]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (b_column.as_mut_ptr().cast::(), b_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + unsafe { Registry::remove_component_row(0, &mut components, 3, identifier.iter()) }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 2, + components.get(0).unwrap().1, + ) + }; + let new_b_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 2, + components.get(1).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(2).unwrap().0.cast::(), + 2, + components.get(2).unwrap().1, + ) + }; + assert_eq!(new_a_column, vec![A(2), A(1)]); + assert_eq!(new_b_column, vec![B(true), B(true)]); + assert_eq!(new_c_column, vec![C, C]); + } + + #[test] + fn remove_component_row_some_components() { + #[derive(Debug, Eq, PartialEq)] + struct A(usize); + #[derive(Debug, Eq, PartialEq)] + struct B(bool); + #[derive(Debug, Eq, PartialEq)] + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut c_column = ManuallyDrop::new(vec![C, C, C]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + unsafe { Registry::remove_component_row(0, &mut components, 3, identifier.iter()) }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 2, + components.get(0).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 2, + components.get(1).unwrap().1, + ) + }; + assert_eq!(new_a_column, vec![A(2), A(1)]); + assert_eq!(new_c_column, vec![C, C]); + } + + #[test] + fn remove_component_row_no_components() { + struct A(usize); + struct B(bool); + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![0]) }; + // `components` must be empty because there are no components in the identifier. + let mut components = Vec::new(); + + unsafe { Registry::remove_component_row(0, &mut components, 1, identifier.iter()) }; + + assert!(components.is_empty()); + } + + #[test] + #[should_panic] + fn remove_component_row_out_of_bounds() { + struct A(usize); + struct B(bool); + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column = vec![A(0), A(1), A(2)]; + let mut b_column = vec![B(false), B(true), B(true)]; + let mut c_column = vec![C, C, C]; + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (b_column.as_mut_ptr().cast::(), b_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + unsafe { Registry::remove_component_row(3, &mut components, 3, identifier.iter()) }; + } + + #[test] + fn pop_component_row_empty_registry() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + // `components` must be empty because there are no components in the registry. + let mut components = Vec::new(); + + let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + let mut buffer = Vec::with_capacity(buffer_size); + unsafe { buffer.set_len(buffer_size) }; + unsafe { + Registry::pop_component_row( + 0, + buffer.as_mut_ptr(), + &mut components, + 1, + identifier.iter(), + ) + }; + + assert!(components.is_empty()); + assert!(buffer.is_empty()); + } + + #[test] + fn pop_component_row_all_components() { + #[derive(Debug, Eq, PartialEq)] + struct A(usize); + #[derive(Debug, Eq, PartialEq)] + struct B(bool); + #[derive(Debug, Eq, PartialEq)] + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut b_column = ManuallyDrop::new(vec![B(false), B(true), B(true)]); + let mut c_column = ManuallyDrop::new(vec![C, C, C]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (b_column.as_mut_ptr().cast::(), b_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + let mut buffer = Vec::with_capacity(buffer_size); + unsafe { buffer.set_len(buffer_size) }; unsafe { - R::debug_identifier(debug_list, identifier_iter) + Registry::pop_component_row( + 0, + buffer.as_mut_ptr(), + &mut components, + 3, + identifier.iter(), + ) + }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 2, + components.get(0).unwrap().1, + ) }; + let new_b_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 2, + components.get(1).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(2).unwrap().0.cast::(), + 2, + components.get(2).unwrap().1, + ) + }; + assert_eq!(new_a_column, vec![A(2), A(1)]); + assert_eq!(new_b_column, vec![B(true), B(true)]); + assert_eq!(new_c_column, vec![C, C]); + let mut buffer_ptr = buffer.as_ptr(); + let popped_a = unsafe { buffer_ptr.cast::().read_unaligned() }; + buffer_ptr = unsafe { buffer_ptr.add(size_of::()) }; + let popped_b = unsafe { buffer_ptr.cast::().read_unaligned() }; + buffer_ptr = unsafe { buffer_ptr.add(size_of::()) }; + let popped_c = unsafe { buffer_ptr.cast::().read_unaligned() }; + assert_eq!(popped_a, A(0)); + assert_eq!(popped_b, B(false)); + assert_eq!(popped_c, C); + } + +#[test] + fn pop_component_row_some_components() { + #[derive(Debug, Eq, PartialEq)] + struct A(usize); + #[derive(Debug, Eq, PartialEq)] + struct B(bool); + #[derive(Debug, Eq, PartialEq)] + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut c_column = ManuallyDrop::new(vec![C, C, C]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + let mut buffer = Vec::with_capacity(buffer_size); + unsafe { buffer.set_len(buffer_size) }; + unsafe { Registry::pop_component_row(0, buffer.as_mut_ptr(), &mut components, 3, identifier.iter()) }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 2, + components.get(0).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 2, + components.get(1).unwrap().1, + ) + }; + assert_eq!(new_a_column, vec![A(2), A(1)]); + assert_eq!(new_c_column, vec![C, C]); + let mut buffer_ptr = buffer.as_ptr(); + let popped_a = unsafe { buffer_ptr.cast::().read_unaligned() }; + buffer_ptr = unsafe { buffer_ptr.add(size_of::()) }; + let popped_c = unsafe { buffer_ptr.cast::().read_unaligned() }; + assert_eq!(popped_a, A(0)); + assert_eq!(popped_c, C); + } + + #[test] + fn pop_component_row_no_components() { + struct A(usize); + struct B(bool); + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![0]) }; + // `components` must be empty because there are no components in the identifier. + let mut components = Vec::new(); + + let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + let mut buffer = Vec::with_capacity(buffer_size); + unsafe { buffer.set_len(buffer_size) }; + unsafe { Registry::pop_component_row(0, buffer.as_mut_ptr(), &mut components, 1, identifier.iter()) }; + + assert!(components.is_empty()); + assert!(buffer.is_empty()); + } + + #[test] + #[should_panic] + fn pop_component_row_out_of_bounds() { + struct A(usize); + struct B(bool); + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column = vec![A(0), A(1), A(2)]; + let mut b_column = vec![B(false), B(true), B(true)]; + let mut c_column = vec![C, C, C]; + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (b_column.as_mut_ptr().cast::(), b_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; + let mut buffer = Vec::with_capacity(buffer_size); + unsafe { buffer.set_len(buffer_size) }; + unsafe { Registry::pop_component_row(3, buffer.as_mut_ptr(), &mut components, 3, identifier.iter()) }; } } From 86a66d5aa90f5aff6d627bfec626996f021b7113 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Fri, 18 Mar 2022 22:50:40 -0700 Subject: [PATCH 04/71] Rest of tests in registry/seal/storage.rs. --- src/registry/seal/storage.rs | 343 ++++++++++++++++++++++++++++++++++- 1 file changed, 335 insertions(+), 8 deletions(-) diff --git a/src/registry/seal/storage.rs b/src/registry/seal/storage.rs index efe2965f..22cf9ede 100644 --- a/src/registry/seal/storage.rs +++ b/src/registry/seal/storage.rs @@ -30,7 +30,7 @@ use hashbrown::HashMap; pub trait Storage { /// Populate a map with component [`TypeId`]s and their associated index within the registry. /// - /// [`TypeId`]: std::any::TypeId + /// [`TypeId`]: core::any::TypeId fn create_component_map(component_map: &mut HashMap, index: usize); /// Populate a map with component [`TypeId`]s and their associated index within the components @@ -43,7 +43,7 @@ pub trait Storage { /// When called internally, the `identifier_iter` must have the same amount of bits left as /// there are components remaining. /// - /// [`TypeId`]: std::any::TypeId + /// [`TypeId`]: core::any::TypeId unsafe fn create_component_map_for_identifier( component_map: &mut HashMap, index: usize, @@ -61,7 +61,7 @@ pub trait Storage { /// When called internally, the `identifier_iter` must have the same amount of bits left as /// there are components remaining. /// - /// [`Vec`]: std::vec::Vec + /// [`Vec`]: alloc::vec::Vec unsafe fn new_components_with_capacity( components: &mut Vec<(*mut u8, usize)>, capacity: usize, @@ -1055,7 +1055,8 @@ mod tests { use claim::{assert_none, assert_some_eq}; use core::{ any::TypeId, - mem::{size_of, ManuallyDrop}, + marker::PhantomData, + mem::{size_of, ManuallyDrop, MaybeUninit}, }; use hashbrown::HashMap; @@ -1551,7 +1552,7 @@ mod tests { assert_eq!(popped_c, C); } -#[test] + #[test] fn pop_component_row_some_components() { #[derive(Debug, Eq, PartialEq)] struct A(usize); @@ -1571,7 +1572,15 @@ mod tests { let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; let mut buffer = Vec::with_capacity(buffer_size); unsafe { buffer.set_len(buffer_size) }; - unsafe { Registry::pop_component_row(0, buffer.as_mut_ptr(), &mut components, 3, identifier.iter()) }; + unsafe { + Registry::pop_component_row( + 0, + buffer.as_mut_ptr(), + &mut components, + 3, + identifier.iter(), + ) + }; let new_a_column = unsafe { Vec::from_raw_parts( @@ -1610,7 +1619,15 @@ mod tests { let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; let mut buffer = Vec::with_capacity(buffer_size); unsafe { buffer.set_len(buffer_size) }; - unsafe { Registry::pop_component_row(0, buffer.as_mut_ptr(), &mut components, 1, identifier.iter()) }; + unsafe { + Registry::pop_component_row( + 0, + buffer.as_mut_ptr(), + &mut components, + 1, + identifier.iter(), + ) + }; assert!(components.is_empty()); assert!(buffer.is_empty()); @@ -1636,6 +1653,316 @@ mod tests { let buffer_size = unsafe { Registry::size_of_components_for_identifier(identifier.iter()) }; let mut buffer = Vec::with_capacity(buffer_size); unsafe { buffer.set_len(buffer_size) }; - unsafe { Registry::pop_component_row(3, buffer.as_mut_ptr(), &mut components, 3, identifier.iter()) }; + unsafe { + Registry::pop_component_row( + 3, + buffer.as_mut_ptr(), + &mut components, + 3, + identifier.iter(), + ) + }; + } + + #[test] + fn push_components_from_buffer_and_component() { + #[derive(Debug, PartialEq)] + struct A(usize); + #[derive(Debug, PartialEq)] + struct B(bool); + #[derive(Debug, PartialEq)] + struct C(f32); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut b_column = ManuallyDrop::new(vec![B(false), B(true), B(true)]); + let mut c_column = ManuallyDrop::new(vec![C(1.0), C(1.1), C(1.2)]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (b_column.as_mut_ptr().cast::(), b_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + // Initialize input buffer. + let input_identifier = unsafe { Identifier::::new(vec![5]) }; + let buffer_size = + unsafe { Registry::size_of_components_for_identifier(input_identifier.iter()) }; + let mut buffer = Vec::::with_capacity(buffer_size); + unsafe { buffer.set_len(buffer_size) }; + let buffer_ptr = buffer.as_mut_ptr(); + unsafe { buffer_ptr.cast::().write_unaligned(A(3)) }; + unsafe { + buffer_ptr + .add(size_of::()) + .cast::() + .write_unaligned(C(1.3)) + }; + + unsafe { + Registry::push_components_from_buffer_and_component( + buffer_ptr, + MaybeUninit::new(B(false)), + &mut components, + 3, + identifier.iter(), + ) + }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 4, + components.get(0).unwrap().1, + ) + }; + let new_b_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 4, + components.get(1).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(2).unwrap().0.cast::(), + 4, + components.get(2).unwrap().1, + ) + }; + assert_eq!(new_a_column, vec![A(0), A(1), A(2), A(3)]); + assert_eq!(new_b_column, vec![B(false), B(true), B(true), B(false)]); + assert_eq!(new_c_column, vec![C(1.0), C(1.1), C(1.2), C(1.3)]); + } + + #[test] + fn push_components_from_buffer_skipping_component() { + #[derive(Debug, PartialEq)] + struct A(usize); + #[derive(Debug, PartialEq)] + struct B(bool); + #[derive(Debug, PartialEq)] + struct C(f32); + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut c_column = ManuallyDrop::new(vec![C(1.0), C(1.1), C(1.2)]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + // Initialize input buffer. + let input_identifier = unsafe { Identifier::::new(vec![7]) }; + let buffer_size = + unsafe { Registry::size_of_components_for_identifier(input_identifier.iter()) }; + let mut buffer = Vec::::with_capacity(buffer_size); + unsafe { buffer.set_len(buffer_size) }; + let buffer_ptr = buffer.as_mut_ptr(); + unsafe { buffer_ptr.cast::().write_unaligned(A(3)) }; + unsafe { + buffer_ptr + .add(size_of::()) + .cast::() + .write_unaligned(B(false)) + }; + unsafe { + buffer_ptr + .add(size_of::()) + .add(size_of::()) + .cast::() + .write_unaligned(C(1.3)) + }; + + unsafe { + Registry::push_components_from_buffer_skipping_component( + buffer_ptr, + PhantomData::, + &mut components, + 3, + identifier.iter(), + ) + }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 4, + components.get(0).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 4, + components.get(1).unwrap().1, + ) + }; + assert_eq!(new_a_column, vec![A(0), A(1), A(2), A(3)]); + assert_eq!(new_c_column, vec![C(1.0), C(1.1), C(1.2), C(1.3)]); + } + + #[test] + fn free_components_empty_registry() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + let mut components = Vec::new(); + + unsafe { Registry::free_components(&mut components, 0, identifier.iter()) }; + } + + #[test] + fn free_components() { + static mut DROP_COUNT: usize = 0; + struct A; + impl Drop for A { + fn drop(&mut self) { + unsafe { DROP_COUNT += 1 }; + } + } + type Registry = registry!(A); + let identifier = unsafe { Identifier::::new(vec![1]) }; + let mut a_column = ManuallyDrop::new(vec![A]); + let mut components = vec![(a_column.as_mut_ptr().cast::(), a_column.capacity())]; + + unsafe { Registry::free_components(&mut components, 1, identifier.iter()) }; + + assert_eq!(unsafe { DROP_COUNT }, 1); + } + + #[test] + fn try_free_components_empty_registry() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + let mut components = Vec::new(); + + unsafe { Registry::try_free_components(&mut components, 0, identifier.iter()) }; + } + + #[test] + fn try_free_components() { + static mut DROP_COUNT: usize = 0; + struct A; + impl Drop for A { + fn drop(&mut self) { + unsafe { DROP_COUNT += 1 }; + } + } + type Registry = registry!(A); + let identifier = unsafe { Identifier::::new(vec![1]) }; + let mut a_column = ManuallyDrop::new(vec![A]); + let mut components = vec![(a_column.as_mut_ptr().cast::(), a_column.capacity())]; + + unsafe { Registry::try_free_components(&mut components, 1, identifier.iter()) }; + + assert_eq!(unsafe { DROP_COUNT }, 1); + } + + #[test] + fn try_free_components_incomplete() { + static mut DROP_COUNT: usize = 0; + struct A; + impl Drop for A { + fn drop(&mut self) { + unsafe { DROP_COUNT += 1 }; + } + } + struct B; + type Registry = registry!(A, B); + let identifier = unsafe { Identifier::::new(vec![3]) }; + let mut a_column = ManuallyDrop::new(vec![A]); + let mut components = vec![(a_column.as_mut_ptr().cast::(), a_column.capacity())]; + + unsafe { Registry::try_free_components(&mut components, 1, identifier.iter()) }; + + assert_eq!(unsafe { DROP_COUNT }, 1); + } + + #[test] + fn clear_components_empty_registry() { + type Registry = registry!(); + let identifier = unsafe { Identifier::::new(Vec::new()) }; + let mut components = Vec::new(); + + unsafe { Registry::clear_components(&mut components, 0, identifier.iter()) }; + + assert!(components.is_empty()); + } + + #[test] + fn clear_components_all() { + struct A(usize); + struct B(bool); + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut b_column = ManuallyDrop::new(vec![B(false), B(true), B(true)]); + let mut c_column = ManuallyDrop::new(vec![C, C, C]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (b_column.as_mut_ptr().cast::(), b_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + unsafe { Registry::clear_components(&mut components, 3, identifier.iter()) }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 0, + components.get(0).unwrap().1, + ) + }; + let new_b_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 0, + components.get(1).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(2).unwrap().0.cast::(), + 0, + components.get(2).unwrap().1, + ) + }; + assert!(new_a_column.is_empty()); + assert!(new_b_column.is_empty()); + assert!(new_c_column.is_empty()); + } + + #[test] + fn clear_components_some() { + struct A(usize); + struct B(bool); + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![5]) }; + let mut a_column = ManuallyDrop::new(vec![A(0), A(1), A(2)]); + let mut c_column = ManuallyDrop::new(vec![C, C, C]); + let mut components = vec![ + (a_column.as_mut_ptr().cast::(), a_column.capacity()), + (c_column.as_mut_ptr().cast::(), c_column.capacity()), + ]; + + unsafe { Registry::clear_components(&mut components, 3, identifier.iter()) }; + + let new_a_column = unsafe { + Vec::from_raw_parts( + components.get(0).unwrap().0.cast::(), + 0, + components.get(0).unwrap().1, + ) + }; + let new_c_column = unsafe { + Vec::from_raw_parts( + components.get(1).unwrap().0.cast::(), + 0, + components.get(1).unwrap().1, + ) + }; + assert!(new_a_column.is_empty()); + assert!(new_c_column.is_empty()); } } From e6d43cda551ff2c54b72bd25cff7ea105e1f604d Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sat, 19 Mar 2022 15:52:27 -0700 Subject: [PATCH 05/71] Safety audit and tests for debug registry functions. --- src/registry/debug.rs | 132 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 124 insertions(+), 8 deletions(-) diff --git a/src/registry/debug.rs b/src/registry/debug.rs index 72fe13ea..ca6b4318 100644 --- a/src/registry/debug.rs +++ b/src/registry/debug.rs @@ -1,3 +1,10 @@ +//! Functions for the `Debug` implementation of `Archetype`. +//! +//! The `RegistryDebug` trait is implemented on any `Registry` where each `Component` implements +//! `Debug`. It is a "public-in-private" trait, so external users can't implement it. These methods +//! should not be considered a part of the public API. The methods are used in the implementation +//! of `Debug` on `Archetype`. + use crate::{ archetype, component::Component, @@ -10,7 +17,29 @@ use core::{ mem::size_of, }; +/// Functions for the `Debug` implementation of `Archetype`. +/// +/// These functions are for performing row-wise debug formatting. pub trait RegistryDebug: Registry { + /// Returns pointers to the components stored at the given index. + /// + /// This function handles the offset arithmetic required to obtain each component and stores a + /// byte pointer for each one in `pointers`. + /// + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` where `C` is the component corresponding to the set bit in `identifier_iter`. + /// + /// `index` must be within the length of each `Vec` defined in `components`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn extract_component_pointers( index: usize, components: &[(*mut u8, usize)], @@ -19,6 +48,28 @@ pub trait RegistryDebug: Registry { ) where R: Registry; + /// Populates a [`DebugMap`] with key-value pairs of component type name and component value + /// for a single row in an archetype table. + /// + /// This function is meant to be called multiple times, once for each row in an archetype + /// table, and is only meant to be used in debugging contexts. The type name used here is not + /// guaranteed to be in any specific form. Therefore, the output is not guaranteed as a part of + /// the public API. + /// + /// # Safety + /// `pointers` must contain the same number of values as there are bits set in the + /// `identifier_iter`. + /// + /// Each pointer in `pointers` must point to a valid properly initialized value of type `C`, + /// where `C` is the component corresponding to the set bit in `identiifer_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. + /// + /// [`DebugMap`]: core::fmt::DebugMap unsafe fn debug_components<'a, 'b, R>( pointers: &[*const u8], debug_map: &mut DebugMap<'a, 'b>, @@ -61,12 +112,46 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - pointers.push(components.get_unchecked(0).0.add(index * size_of::())); - components = components.get_unchecked(1..); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + pointers.push( + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. Additionally, `index` is + // within the bounds of each component's `Vec`, so the offset will still result + // in a valid pointer within the allocation. + unsafe { components.get_unchecked(0).0.add(index * size_of::()) }, + ); + components = + // SAFETY: `components` is guaranteed to have the same number of values as + // there set bits in `identifier_iter`. Since a bit must have been set to enter + // this block, there must be at least one component column. + unsafe { components.get_unchecked(1..) }; } - R::extract_component_pointers(index, components, pointers, identifier_iter); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + // + // Since each `Vec` is still valid for the remaining set components, then `index` is + // still a valid index into those allocations. + unsafe { R::extract_component_pointers(index, components, pointers, identifier_iter) }; } unsafe fn debug_components<'a, 'b, R_>( @@ -76,11 +161,42 @@ where ) where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - debug_map.entry(&type_name::(), &*pointers.get_unchecked(0).cast::()); - pointers = pointers.get_unchecked(1..); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + debug_map.entry( + &type_name::(), + // SAFETY: Since a set bit was found, there must invariantly be at least one valid + // pointer within pointers which points to a properly-initialized value of the + // corresponding component type `C`. + unsafe { &*pointers.get_unchecked(0).cast::() }, + ); + pointers = + // SAFETY: `pointers` is guaranteed to have the same number of values as there are + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one valid pointer remaining. + unsafe {pointers.get_unchecked(1..)}; } - R::debug_components(pointers, debug_map, identifier_iter); + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `pointers` slice will no longer include the first value, + // which means the slice will still contain the same number of values as there are set bits + // in `identifier_iter`. Additionally, since the first value was removed from the slice, + // which corresponded to the component identified by the consumed bit, all remaining + // pointer values will still correspond to valid `C` component types identified by the + // remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `pointers` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still point to + // valid properly-initialized values of type `C` for each remaining `C` identified by the + // remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { R::debug_components(pointers, debug_map, identifier_iter) }; } } From 91498b18fdba1ddd20c4002587fcaeb374fc417e Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 20 Mar 2022 13:32:05 -0700 Subject: [PATCH 06/71] Safety comments for Null system impls. --- src/system/null.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/system/null.rs b/src/system/null.rs index 6d2ac212..617d10ee 100644 --- a/src/system/null.rs +++ b/src/system/null.rs @@ -25,6 +25,7 @@ impl<'a> System<'a> for Null { where R: Registry + 'a, { + // SAFETY: This type can never be instantiated. Therefore, this method can never be called. unsafe { unreachable_unchecked() } } @@ -32,6 +33,7 @@ impl<'a> System<'a> for Null { where R: Registry, { + // SAFETY: This type can never be instantiated. Therefore, this method can never be called. unsafe { unreachable_unchecked() } } } @@ -45,6 +47,7 @@ impl<'a> ParSystem<'a> for Null { where R: Registry + 'a, { + // SAFETY: This type can never be instantiated. Therefore, this method can never be called. unsafe { unreachable_unchecked() } } @@ -52,6 +55,7 @@ impl<'a> ParSystem<'a> for Null { where R: Registry, { + // SAFETY: This type can never be instantiated. Therefore, this method can never be called. unsafe { unreachable_unchecked() } } } From a2f7e04a62b3a3d443a6bf6252d677d3e68e6ab1 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 20 Mar 2022 14:02:04 -0700 Subject: [PATCH 07/71] Safety comments, docs, and tests for RegistryPartialEq. --- src/registry/eq.rs | 175 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 160 insertions(+), 15 deletions(-) diff --git a/src/registry/eq.rs b/src/registry/eq.rs index fb8734a4..ee84ef08 100644 --- a/src/registry/eq.rs +++ b/src/registry/eq.rs @@ -1,3 +1,13 @@ +//! Functions for the `PartialEq` and `Eq` implementation of `Archetype`. +//! +//! These traits are implemented on `Registry`s whose components implement `PartialEq` and `Eq`, +//! allowing `Archetype`s to implement `PartialEq` and `Eq` if and only if the components of the +//! `Registry` do. +//! +//! These are implemented as "public in private" traits, and therefore cannot be implemented by +//! external users of the library. The functions defined here are not considered part of the public +//! API. + use crate::{ archetype, component::Component, @@ -6,7 +16,34 @@ use crate::{ use alloc::vec::Vec; use core::mem::ManuallyDrop; +/// Component-wise implementation for `PartialEq` for a `Registry`. +/// +/// Any `Registry` whose components implement `PartialEq` will implement this trait. +/// +/// This trait is similar to `PartialEq`, but the implementation specifically allows for recursive +/// equality evaluation of each component column within an archetype table. pub trait RegistryPartialEq: Registry { + /// Returns whether the components in `components_a` are equal to the components in + /// `components_b`, where `components_a` and `components_b` are lists of pointer-capacity + /// tuples defining `Vec`s of length `length` for each `C` component type identified by the + /// `identifier_iter`. + /// + /// Note that evaluation of equality of components is ultimately deferred to each component + /// type's `PartialEq` implementation. + /// + /// # Safety + /// `components_a` and `components_b` must both contain the same number of values as there are + /// set bits in the `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components_a` and `components_b` must be the pointer and + /// capacity respectively of a `Vec` of length `length` where `C` is the component + /// corresponding to the set bit in `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn component_eq( components_a: &[(*mut u8, usize)], components_b: &[(*mut u8, usize)], @@ -45,30 +82,71 @@ where where R_: Registry, { - if identifier_iter.next().unwrap_unchecked() { - let component_column_a = components_a.get_unchecked(0); - let component_column_b = components_b.get_unchecked(0); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column_a = + // SAFETY: `components_a` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components_a.get_unchecked(0) }; + let component_column_b = + // SAFETY: `components_b` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components_b.get_unchecked(0) }; - if ManuallyDrop::new(Vec::from_raw_parts( - component_column_a.0, - length, - component_column_a.1, - )) != ManuallyDrop::new(Vec::from_raw_parts( - component_column_b.0, - length, - component_column_b.1, - )) { + if ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract + // of this method to define a valid `Vec`. + unsafe { Vec::from_raw_parts(component_column_a.0.cast::(), length, component_column_a.1) }, + ) != ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract + // of this method to define a valid `Vec`. + unsafe { Vec::from_raw_parts(component_column_b.0.cast::(), length, component_column_b.1) }, + ) { return false; } - components_a = components_a.get_unchecked(1..); - components_b = components_b.get_unchecked(1..); + components_a = + // SAFETY: `components_a` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components_a.get_unchecked(1..) }; + components_b = + // SAFETY: `components_b` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components_b.get_unchecked(1..) }; } - R::component_eq(components_a, components_b, length, identifier_iter) + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components_a` and `components_b` slices will no longer + // include their first values, which means these slices will both still contain the same + // number of pointer and capacity tuples as there are set bits in `identifier_iter`. + // Additionally, since the first value was removed from each slice, which corresponded to + // the component identified by the consumed bit, all remaining component values will still + // correspond to valid `Vec`s identified by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components_a` and `components_b` are unaltered, and there + // are still the same number of elements as there are set bits in `identifier_iter`, which + // still make valid `Vec`s for each `C` identified by the remaining set bits in + // `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + unsafe { R::component_eq(components_a, components_b, length, identifier_iter) } } } +/// This trait indicates that all components within a registry implement `Eq`. +/// +/// This is needed to indicate whether an archetype can implement `Eq`, since it is generic over +/// a heterogeneous list of components. pub trait RegistryEq: RegistryPartialEq {} impl RegistryEq for Null {} @@ -79,3 +157,70 @@ where R: RegistryEq, { } + +#[cfg(test)] +mod tests { + use super::RegistryPartialEq; + use crate::{archetype::Identifier, registry}; + use alloc::vec; + + #[test] + fn components_equal() { + #[derive(PartialEq)] + struct A(usize); + #[derive(PartialEq)] + struct B(bool); + #[derive(PartialEq)] + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column_a = vec![A(0), A(1), A(2)]; + let mut b_column_a = vec![B(false), B(true), B(true)]; + let mut c_column_a = vec![C, C, C]; + let mut components_a = vec![ + (a_column_a.as_mut_ptr().cast::(), a_column_a.capacity()), + (b_column_a.as_mut_ptr().cast::(), b_column_a.capacity()), + (c_column_a.as_mut_ptr().cast::(), c_column_a.capacity()), + ]; + let mut a_column_b = vec![A(0), A(1), A(2)]; + let mut b_column_b = vec![B(false), B(true), B(true)]; + let mut c_column_b = vec![C, C, C]; + let mut components_b = vec![ + (a_column_b.as_mut_ptr().cast::(), a_column_b.capacity()), + (b_column_b.as_mut_ptr().cast::(), b_column_b.capacity()), + (c_column_b.as_mut_ptr().cast::(), c_column_b.capacity()), + ]; + + assert!(unsafe {Registry::component_eq(&components_a, &components_b, 3, identifier.iter())}); + } + + #[test] + fn components_not_equal() { + #[derive(PartialEq)] + struct A(usize); + #[derive(PartialEq)] + struct B(bool); + #[derive(PartialEq)] + struct C; + type Registry = registry!(A, B, C); + let identifier = unsafe { Identifier::::new(vec![7]) }; + let mut a_column_a = vec![A(0), A(1), A(2)]; + let mut b_column_a = vec![B(false), B(true), B(true)]; + let mut c_column_a = vec![C, C, C]; + let mut components_a = vec![ + (a_column_a.as_mut_ptr().cast::(), a_column_a.capacity()), + (b_column_a.as_mut_ptr().cast::(), b_column_a.capacity()), + (c_column_a.as_mut_ptr().cast::(), c_column_a.capacity()), + ]; + let mut a_column_b = vec![A(0), A(1), A(2)]; + let mut b_column_b = vec![B(false), B(false), B(true)]; + let mut c_column_b = vec![C, C, C]; + let mut components_b = vec![ + (a_column_b.as_mut_ptr().cast::(), a_column_b.capacity()), + (b_column_b.as_mut_ptr().cast::(), b_column_b.capacity()), + (c_column_b.as_mut_ptr().cast::(), c_column_b.capacity()), + ]; + + assert!(!unsafe {Registry::component_eq(&components_a, &components_b, 3, identifier.iter())}); + } +} From 3afabe51c4e8082aa5c1cc212f0a0167df436a5e Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 20 Mar 2022 14:11:33 -0700 Subject: [PATCH 08/71] Safety documentation for Slot::activate_unchecked. --- src/entity/allocator/slot.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/entity/allocator/slot.rs b/src/entity/allocator/slot.rs index 3a68640c..7abae8d7 100644 --- a/src/entity/allocator/slot.rs +++ b/src/entity/allocator/slot.rs @@ -13,15 +13,23 @@ impl Slot where R: Registry, { - // TODO: Should this really be considered unsafe? - pub(super) unsafe fn new(location: Location) -> Self { + pub(super) fn new(location: Location) -> Self { Self { generation: 0, location: Some(location), } } - // TODO: Should this really be considered unsafe? + /// Activate this slot without checking whether it is already activated. + /// + /// If the current slot is already active, this will lead to invalidation of + /// `entity::Identifier`s prematurely and make it impossible to remove entities from the + /// `World`. Additionally, it would leave invalid identifiers within archetype tables, causing + /// some improper assumptions. Therefore, this method is considered `unsafe` and must be called + /// with care. + /// + /// # Safety + /// A `Slot` this method is called on must not already be active. pub(super) unsafe fn activate_unchecked(&mut self, location: Location) { self.generation = self.generation.wrapping_add(1); self.location = Some(location); From e5aa69fe2a705689faff2718d68c2fd360802dc8 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 20 Mar 2022 14:44:57 -0700 Subject: [PATCH 09/71] Safety comments for entity allocator. --- src/entity/allocator/mod.rs | 68 +++++++++++++++++++++++++++++-------- src/registry/eq.rs | 24 ++++++++++--- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/src/entity/allocator/mod.rs b/src/entity/allocator/mod.rs index e6b5376e..9088746b 100644 --- a/src/entity/allocator/mod.rs +++ b/src/entity/allocator/mod.rs @@ -34,10 +34,14 @@ where } } - pub(crate) unsafe fn allocate(&mut self, location: Location) -> entity::Identifier { + pub(crate) fn allocate(&mut self, location: Location) -> entity::Identifier { let (index, generation) = if let Some(index) = self.free.pop_front() { - let slot = self.slots.get_unchecked_mut(index); - slot.activate_unchecked(location); + let slot = + // SAFETY: `self.free` is guaranteed to contain valid indices within the bounds of + // `self.slots`. + unsafe {self.slots.get_unchecked_mut(index)}; + // SAFETY: `self.free` is guaranteed to contain indices for slots that are not active. + unsafe { slot.activate_unchecked(location) }; (index, slot.generation) } else { let index = self.slots.len(); @@ -50,7 +54,7 @@ where } #[inline] - pub(crate) unsafe fn allocate_batch( + pub(crate) fn allocate_batch( &mut self, mut locations: Locations, ) -> Vec { @@ -61,8 +65,13 @@ where if locations.is_empty() { break; } - let slot = self.slots.get_unchecked_mut(index); - slot.activate_unchecked(locations.next().unwrap_unchecked()); + let slot = + // SAFETY: indices within `self.free` are guaranteed to be within the bounds of + // `self.slots`. + unsafe { self.slots.get_unchecked_mut(index) }; + // SAFETY: `self.free` is guaranteed to contain indices for slots that are not active. + // Also, `locations` is already checked above to be nonempty. + unsafe { slot.activate_unchecked(locations.next().unwrap_unchecked()) }; identifiers.push(entity::Identifier::new(index, slot.generation)); } @@ -96,31 +105,62 @@ where false } + /// Free the entity allocation identified by `identifier`, skipping checks for whether the + /// allocation exists. + /// + /// # Safety + /// `identifier` must be for a valid, currently allocated entity. pub(crate) unsafe fn free_unchecked(&mut self, identifier: entity::Identifier) { - let slot = self.slots.get_unchecked_mut(identifier.index); + let slot = + // SAFETY: `identifier` is guaranteed by the safety contract of this method to identify + // a valid entity. Therefore, its `index` will correspond to a valid value within + // `self.slots`. + unsafe {self.slots.get_unchecked_mut(identifier.index)}; slot.deactivate(); self.free.push_back(identifier.index); } + /// Update the location of the entity identified by `identifier`, skipping checks for whether + /// the allocation exists. + /// + /// # Safety + /// `identifier` must be for a valid, currently allocated entity. pub(crate) unsafe fn modify_location_unchecked( &mut self, identifier: entity::Identifier, location: Location, ) { - self.slots.get_unchecked_mut(identifier.index).location = Some(location); + // SAFETY: `identifier` is guaranteed by the safety contract of this method to identify a + // valid entity. Therefore, its `index` will correspond to a valid value within + // `self.slots`. + unsafe { self.slots.get_unchecked_mut(identifier.index) }.location = Some(location); } + /// Update the location's index of the entity identified by `identifier`, skipping checks for + /// whether the allocation exists. + /// + /// This should be used when an entity's location within an archetype table has changed. + /// Calling this method ensures the allocator's mapping of where entities currently are stays + /// up to date. + /// + /// # Safety + /// `identifier` must be for a valid, currently allocated entity. pub(crate) unsafe fn modify_location_index_unchecked( &mut self, identifier: entity::Identifier, index: usize, ) { - self.slots - .get_unchecked_mut(identifier.index) - .location - .as_mut() - .unwrap_unchecked() - .index = index; + // SAFETY: `identifier` is guaranteed by the safety contract of this method to identify a + // valid active entity. Therefore, its `index` will correspond to a valid active value + // within `self.slots`. + unsafe { + self.slots + .get_unchecked_mut(identifier.index) + .location + .as_mut() + .unwrap_unchecked() + } + .index = index; } } diff --git a/src/registry/eq.rs b/src/registry/eq.rs index ee84ef08..5d8b99be 100644 --- a/src/registry/eq.rs +++ b/src/registry/eq.rs @@ -100,11 +100,23 @@ where if ManuallyDrop::new( // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract // of this method to define a valid `Vec`. - unsafe { Vec::from_raw_parts(component_column_a.0.cast::(), length, component_column_a.1) }, + unsafe { + Vec::from_raw_parts( + component_column_a.0.cast::(), + length, + component_column_a.1, + ) + }, ) != ManuallyDrop::new( // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract // of this method to define a valid `Vec`. - unsafe { Vec::from_raw_parts(component_column_b.0.cast::(), length, component_column_b.1) }, + unsafe { + Vec::from_raw_parts( + component_column_b.0.cast::(), + length, + component_column_b.1, + ) + }, ) { return false; } @@ -191,7 +203,9 @@ mod tests { (c_column_b.as_mut_ptr().cast::(), c_column_b.capacity()), ]; - assert!(unsafe {Registry::component_eq(&components_a, &components_b, 3, identifier.iter())}); + assert!(unsafe { + Registry::component_eq(&components_a, &components_b, 3, identifier.iter()) + }); } #[test] @@ -221,6 +235,8 @@ mod tests { (c_column_b.as_mut_ptr().cast::(), c_column_b.capacity()), ]; - assert!(!unsafe {Registry::component_eq(&components_a, &components_b, 3, identifier.iter())}); + assert!(!unsafe { + Registry::component_eq(&components_a, &components_b, 3, identifier.iter()) + }); } } From 4fd900baedbd1b4908b39312ee8530667c1510f8 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 00:22:22 -0700 Subject: [PATCH 10/71] Safety comments for entity module. --- src/entities/seal/storage.rs | 10 +++--- src/entity/seal/storage.rs | 64 +++++++++++++++++++++++++++++------- src/world/mod.rs | 12 +++---- 3 files changed, 63 insertions(+), 23 deletions(-) diff --git a/src/entities/seal/storage.rs b/src/entities/seal/storage.rs index 1c1ad5b8..a6030ed5 100644 --- a/src/entities/seal/storage.rs +++ b/src/entities/seal/storage.rs @@ -11,7 +11,7 @@ pub trait Storage { length: usize, ); - unsafe fn to_key(key: &mut [u8], component_map: &HashMap); + unsafe fn to_identifier(identifier: &mut [u8], component_map: &HashMap); } impl Storage for Null { @@ -23,7 +23,7 @@ impl Storage for Null { ) { } - unsafe fn to_key(_key: &mut [u8], _component_map: &HashMap) {} + unsafe fn to_identifier(_identifier: &mut [u8], _component_map: &HashMap) {} } impl Storage for (Vec, E) @@ -54,13 +54,13 @@ where E::extend_components(self.1, component_map, components, length); } - unsafe fn to_key(key: &mut [u8], component_map: &HashMap) { + unsafe fn to_identifier(identifier: &mut [u8], component_map: &HashMap) { let component_index = component_map.get(&TypeId::of::()).unwrap(); let index = component_index / 8; let bit = component_index % 8; - *key.get_unchecked_mut(index) |= 1 << bit; + *identifier.get_unchecked_mut(index) |= 1 << bit; - E::to_key(key, component_map); + E::to_identifier(identifier, component_map); } } diff --git a/src/entity/seal/storage.rs b/src/entity/seal/storage.rs index 21e81d7f..eaf6ecf2 100644 --- a/src/entity/seal/storage.rs +++ b/src/entity/seal/storage.rs @@ -4,6 +4,19 @@ use core::{any::TypeId, mem::ManuallyDrop}; use hashbrown::HashMap; pub trait Storage { + /// Push the components contained in this heterogeneous list into component columns. + /// + /// This consumes the entity, moving the components into their appropriate columns. + /// + /// The components are stored within the `Vec`s defined by `components` and `length`, using + /// the given `component_map` to determine which column each component should be added to. + /// + /// # Safety + /// `component_map` must contain an entry for every type `C` that makes up this entity. Each + /// entry must contain a unique index corresponding with a valid `components` entry. + /// + /// `components`, together with `length`, must define a valid `Vec` for each component for + /// which `component_map` has an entry whose index references it. unsafe fn push_components( self, component_map: &HashMap, @@ -11,7 +24,19 @@ pub trait Storage { length: usize, ); - unsafe fn to_key(key: &mut [u8], component_map: &HashMap); + /// Populate raw identifier bits corresponding to this entity's components. + /// + /// The bits are filled according to the provided `component_map`. This ideally should be a map + /// created using a `Registry` of components. + /// + /// # Safety + /// `identifier` must be a zeroed-out allocation of enough bytes to have bits up to the highest + /// bit index value stored in `component_map`. + /// + /// # Panics + /// This method will panic if this entity contains a component that does not have an entry in + /// the given `component_map`. + unsafe fn to_identifier(identifier: &mut [u8], component_map: &HashMap); } impl Storage for Null { @@ -23,7 +48,7 @@ impl Storage for Null { ) { } - unsafe fn to_key(_key: &mut [u8], _component_map: &HashMap) {} + unsafe fn to_identifier(_identifier: &mut [u8], _component_map: &HashMap) {} } impl Storage for (C, E) @@ -38,24 +63,39 @@ where length: usize, ) { let component_column = - components.get_unchecked_mut(*component_map.get(&TypeId::of::()).unwrap_unchecked()); - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); + // SAFETY: `component_map` is guaranteed to have an entry for `TypeId::of::`, and + // entry is guaranteed to be a valid index into `components`. + unsafe { + components.get_unchecked_mut(*component_map.get(&TypeId::of::()).unwrap_unchecked()) + }; + let mut v = ManuallyDrop::new( + // SAFETY: The `component_column` extracted from `components` is guaranteed to, + // together with `length`, define a valid `Vec` for the current `C`, because the + // `component_column` extracted is guaranteed by the safety contract to correspond to + // the column for component `C`. + unsafe { + Vec::::from_raw_parts(component_column.0.cast::(), length, component_column.1) + }, + ); v.push(self.0); *component_column = (v.as_mut_ptr().cast::(), v.capacity()); - E::push_components(self.1, component_map, components, length); + // SAFETY: Since `component_map`, `components`, and `length` all meet the safety + // requirements for the current method body, they will meet those same requirements for + // this method call. + unsafe { E::push_components(self.1, component_map, components, length) }; } - unsafe fn to_key(key: &mut [u8], component_map: &HashMap) { + unsafe fn to_identifier(identifier: &mut [u8], component_map: &HashMap) { let component_index = component_map.get(&TypeId::of::()).unwrap(); let index = component_index / 8; let bit = component_index % 8; - *key.get_unchecked_mut(index) |= 1 << bit; + // SAFETY: `identifier` is guaranteed by the safety contract of this function to have + // enough bits to be indexed by any value in the given `component_map`. + unsafe { + *identifier.get_unchecked_mut(index) |= 1 << bit; + } - E::to_key(key, component_map); + E::to_identifier(identifier, component_map); } } diff --git a/src/world/mod.rs b/src/world/mod.rs index 96cdd697..7fdab080 100644 --- a/src/world/mod.rs +++ b/src/world/mod.rs @@ -154,11 +154,11 @@ where { self.len += 1; - let mut key = vec![0; (R::LEN + 7) / 8]; + let mut identifier = vec![0; (R::LEN + 7) / 8]; unsafe { - E::to_key(&mut key, &self.component_map); + E::to_identifier(&mut identifier, &self.component_map); } - let identifier_buffer = unsafe { archetype::Identifier::new(key) }; + let identifier_buffer = unsafe { archetype::Identifier::new(identifier) }; unsafe { self.archetypes @@ -193,11 +193,11 @@ where { self.len += entities.len(); - let mut key = vec![0; (R::LEN + 7) / 8]; + let mut identifier = vec![0; (R::LEN + 7) / 8]; unsafe { - E::to_key(&mut key, &self.component_map); + E::to_identifier(&mut identifier, &self.component_map); } - let identifier_buffer = unsafe { archetype::Identifier::new(key) }; + let identifier_buffer = unsafe { archetype::Identifier::new(identifier) }; unsafe { self.archetypes From ad58685437dc5b9e434cb5a10f54bf8ad73beacc Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 17:02:27 -0700 Subject: [PATCH 11/71] Safety comment for recursive call. --- src/entity/seal/storage.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/entity/seal/storage.rs b/src/entity/seal/storage.rs index eaf6ecf2..7da089b8 100644 --- a/src/entity/seal/storage.rs +++ b/src/entity/seal/storage.rs @@ -33,6 +33,9 @@ pub trait Storage { /// `identifier` must be a zeroed-out allocation of enough bytes to have bits up to the highest /// bit index value stored in `component_map`. /// + /// `component_map` may only contain `usize` values up to the number of components in the + /// registry. + /// /// # Panics /// This method will panic if this entity contains a component that does not have an entry in /// the given `component_map`. @@ -96,6 +99,9 @@ where *identifier.get_unchecked_mut(index) |= 1 << bit; } - E::to_identifier(identifier, component_map); + // SAFETY: `identifier` is guaranteed by the safety contract of this method to be large + // enough to store bits up to the number of components. `component_map` will also still + // contain `usize` values not larger than the number of components in the full registry. + unsafe { E::to_identifier(identifier, component_map) }; } } From 4f4c30d31e4c32f3dda0c533a5874d6e1e11946f Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 17:19:08 -0700 Subject: [PATCH 12/71] Safety comments for entities storage. --- src/entities/seal/storage.rs | 64 +++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/src/entities/seal/storage.rs b/src/entities/seal/storage.rs index a6030ed5..b17d3a84 100644 --- a/src/entities/seal/storage.rs +++ b/src/entities/seal/storage.rs @@ -4,6 +4,19 @@ use core::{any::TypeId, mem::ManuallyDrop}; use hashbrown::HashMap; pub trait Storage { + /// Extend the component columns with the components contained in this heterogeneous list. + /// + /// This consumes the entities, moving the components into their appropriate columns. + /// + /// The components are stored within the `Vec`s defined by `components` and `length`, using + /// the given `component_map` to determine which column each components should be added to. + /// + /// # Safety + /// `component_map` must contain an entry for every type `C` that makes up these entities. Each + /// entry must contain a unique index corresponding with a valid `components` entry. + /// + /// `components`, together with `length`, must define a valid `Vec` for each component for + /// which `component_map` has an entry whose index references it. unsafe fn extend_components( self, component_map: &HashMap, @@ -11,6 +24,21 @@ pub trait Storage { length: usize, ); + /// Populate raw identifier bits corresponding to the entities' components. + /// + /// The bits are filled according to the provided `component_map`. This ideally should be a map + /// created using a `Registry` of components. + /// + /// # Safety + /// `identifier` must be a zeroed-out allocation of enough bytes to have bits up to the highest + /// bit index value stored in `component_map`. + /// + /// `component_map` may only contain `usize` values up to the number of components in the + /// registry. + /// + /// # Panics + /// This method will panic if the entities contain a component that does not have an entry in + /// the given `component_map`. unsafe fn to_identifier(identifier: &mut [u8], component_map: &HashMap); } @@ -38,20 +66,31 @@ where length: usize, ) { let component_column = - components.get_unchecked_mut(*component_map.get(&TypeId::of::()).unwrap_unchecked()); + // SAFETY: `component_map` is guaranteed to have an entry for `TypeId::of::`, and + // entry is guaranteed to be a valid index into `components`. + unsafe { + components.get_unchecked_mut(*component_map.get(&TypeId::of::()).unwrap_unchecked()) + }; if length == 0 { let mut v = ManuallyDrop::new(self.0); *component_column = (v.as_mut_ptr().cast::(), v.capacity()); } else { - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )); + let mut v = ManuallyDrop::new( + // SAFETY: The `component_column` extracted from `components` is guaranteed to, + // together with `length`, define a valid `Vec` for the current `C`, because the + // `component_column` extracted is guaranteed by the safety contract to correspond to + // the column for component `C`. + unsafe { + Vec::::from_raw_parts(component_column.0.cast::(), length, component_column.1) + }, + ); v.extend(self.0); *component_column = (v.as_mut_ptr().cast::(), v.capacity()); } - E::extend_components(self.1, component_map, components, length); + // SAFETY: Since `component_map`, `components`, and `length` all meet the safety + // requirements for the current method body, they will meet those same requirements for + // this method call. + unsafe { E::extend_components(self.1, component_map, components, length) }; } unsafe fn to_identifier(identifier: &mut [u8], component_map: &HashMap) { @@ -59,8 +98,15 @@ where let index = component_index / 8; let bit = component_index % 8; - *identifier.get_unchecked_mut(index) |= 1 << bit; + // SAFETY: `identifier` is guaranteed by the safety contract of this function to have + // enough bits to be indexed by any value in the given `component_map`. + unsafe { + *identifier.get_unchecked_mut(index) |= 1 << bit; + } - E::to_identifier(identifier, component_map); + // SAFETY: `identifier` is guaranteed by the safety contract of this method to be large + // enough to store bits up to the number of components. `component_map` will also still + // contain `usize` values not larger than the number of components in the full registry. + unsafe { E::to_identifier(identifier, component_map) }; } } From 71efbd2d8bdc70f2fd26d6fa0a3ecee32bf5cb87 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 17:46:22 -0700 Subject: [PATCH 13/71] Safety comments for archetype identifiers. --- src/archetype/identifier/mod.rs | 73 ++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 5 deletions(-) diff --git a/src/archetype/identifier/mod.rs b/src/archetype/identifier/mod.rs index 4a80e897..bafe4fa7 100644 --- a/src/archetype/identifier/mod.rs +++ b/src/archetype/identifier/mod.rs @@ -111,7 +111,9 @@ where /// /// [`Iter`]: crate::archetype::identifier::Iter pub(crate) unsafe fn iter(&self) -> Iter { - Iter::::new(self.pointer) + // SAFETY: `self.pointer` will be valid as long as the returned `Iter` exists, assuming the + // caller ensures the `Identifier` outlives it. + unsafe { Iter::::new(self.pointer) } } /// Returns the size of the components within the canonical entity represented by this @@ -174,12 +176,39 @@ where } } +/// A reference to an [`Identifier`]. +/// +/// A struct for this referencial relationship is defined to allow for a self-referential +/// relationship within a `World`. This works because `Identifier`s are owned and stored within the +/// [`Archetypes`] container and are never moved or dropped. +/// +/// This allows preserving space as registry size increases, as many `IdentifierRef`s will be +/// stored within the [`entity::Allocator`]. +/// +/// [`Archetypes`]: crate::archetypes::Archetypes +/// [`entity::Allocator`]: crate::entity::allocator::Allocator +/// [`Identifier`]: crate::archetype::identifier::Identifier pub(crate) struct IdentifierRef where R: Registry, { + /// The [`Registry`] defining the set of valid values of this identifier. + /// + /// Each identifier must exist within a set defined by a `Registry`. This defines a space over + /// which each identifier can uniquely define a set of components. Each bit within the + /// identifier corresponds with a component in the registry. + /// + /// The length of the allocated buffer is defined at compile-time as `(R::LEN + 7) / 8`. + /// + /// [`Registry`]: crate::registry::Registry registry: PhantomData, + /// Pointer to the allocated bytes. + /// + /// This allocation is not owned by this struct. It is up to the user of the sruct to ensure it + /// does not outlive the [`Identifier`] it references.s + /// + /// [`Identifier`]: crate::archetype::identifier::Identifier pointer: *const u8, } @@ -187,20 +216,48 @@ impl IdentifierRef where R: Registry, { + /// Returns a reference to the bytes defining this identifier. + /// + /// # Safety + /// The caller must ensure the referenced `Identifier` outlives the returned slice. pub(crate) unsafe fn as_slice(&self) -> &[u8] { - slice::from_raw_parts(self.pointer, (R::LEN + 7) / 8) + // SAFETY: `pointer` is invariantly guaranteed to point to an allocation of length + // `(R::LEN + 7) / 8`. + unsafe { slice::from_raw_parts(self.pointer, (R::LEN + 7) / 8) } } - pub(crate) unsafe fn iter(self) -> Iter { - Iter::::new(self.pointer) + /// Returns an iterator over the bits of this identifier. + /// + /// The returned iterator is guaranteed to return exactly `(R::LEN + 7) / 8` values, one for + /// each bit corresponding to the components of the registry. + /// + /// # Safety + /// The caller must ensure the referenced `Identifier` outlives the returned [`Iter`]. + /// + /// [`Iter`]: crate::archetype::identifier::Iter + pub(crate) unsafe fn iter(&self) -> Iter { + // SAFETY: `self.pointer` will be valid as long as the returned `Iter` exists, assuming the + // caller ensures the `Identifier` outlives it. + unsafe { Iter::::new(self.pointer) } } + /// Returns a copy of the bytes defining this identifier. pub(crate) fn as_vec(self) -> Vec { + // SAFETY: The reference created here will always live longer than the referenced + // `Identifier`, since it only lasts for the scope of this function. unsafe { self.as_slice() }.to_vec() } + /// Gets the bit at the specified index without performing bounds checks. + /// + /// # Safety + /// `index` must be less than `R::LEN`. pub(crate) unsafe fn get_unchecked(self, index: usize) -> bool { - (self.as_slice().get_unchecked(index / 8) >> (index % 8) & 1) != 0 + ( + // SAFETY: `index` is guaranteed to be less than R::LEN, so therefore index / 8 will be + // within the bounds of `self.as_slice()`. + unsafe {self.as_slice().get_unchecked(index / 8)} >> (index % 8) & 1 + ) != 0 } } @@ -227,6 +284,8 @@ where where H: Hasher, { + // SAFETY: The slice created here will be outlived by the referenced `Identifier`, as the + // slice will only live for the scope of this function. unsafe { self.as_slice() }.hash(state); } } @@ -236,6 +295,8 @@ where R: Registry, { fn eq(&self, other: &Self) -> bool { + // SAFETY: The slices created here will be outlived by the referenced `Identifier`, as the + // slices will only live for the scope of this function. unsafe { self.as_slice() == other.as_slice() } } } @@ -249,6 +310,8 @@ where fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut debug_list = f.debug_list(); + // SAFETY: The registry `R` upon which this method is called is the same as the registry + // `R` over which `self.iter()` is generic. unsafe { R::debug_identifier(&mut debug_list, self.iter()); } From b953afb5851ff5ac94551c15e74480861c221ee2 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 17:58:19 -0700 Subject: [PATCH 14/71] Add all missing unsafe blocks. --- src/archetype/identifier/iter.rs | 2 +- src/archetype/identifier/mod.rs | 4 +- src/archetype/mod.rs | 254 ++++++++++++++++++------------- src/archetypes/mod.rs | 16 +- src/entities/seal/storage.rs | 6 +- src/query/filter/seal.rs | 14 +- src/query/view/seal.rs | 64 ++++---- 7 files changed, 207 insertions(+), 153 deletions(-) diff --git a/src/archetype/identifier/iter.rs b/src/archetype/identifier/iter.rs index ddf58188..630ec3c5 100644 --- a/src/archetype/identifier/iter.rs +++ b/src/archetype/identifier/iter.rs @@ -23,7 +23,7 @@ where pointer, - current: if R::LEN > 0 { *pointer } else { 0 }, + current: if R::LEN > 0 { unsafe { *pointer } } else { 0 }, position: 0, } } diff --git a/src/archetype/identifier/mod.rs b/src/archetype/identifier/mod.rs index bafe4fa7..24ebea2a 100644 --- a/src/archetype/identifier/mod.rs +++ b/src/archetype/identifier/mod.rs @@ -256,7 +256,7 @@ where ( // SAFETY: `index` is guaranteed to be less than R::LEN, so therefore index / 8 will be // within the bounds of `self.as_slice()`. - unsafe {self.as_slice().get_unchecked(index / 8)} >> (index % 8) & 1 + unsafe { self.as_slice().get_unchecked(index / 8) } >> (index % 8) & 1 ) != 0 } } @@ -284,7 +284,7 @@ where where H: Hasher, { - // SAFETY: The slice created here will be outlived by the referenced `Identifier`, as the + // SAFETY: The slice created here will be outlived by the referenced `Identifier`, as the // slice will only live for the scope of this function. unsafe { self.as_slice() }.hash(state); } diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 578d9101..50182abe 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -59,7 +59,9 @@ where length: usize, ) -> Self { let mut component_map = HashMap::new(); - R::create_component_map_for_identifier(&mut component_map, 0, identifier_buffer.iter()); + unsafe { + R::create_component_map_for_identifier(&mut component_map, 0, identifier_buffer.iter()) + }; Self { identifier_buffer, @@ -75,22 +77,24 @@ where pub(crate) unsafe fn new(identifier_buffer: Identifier) -> Self { let mut entity_identifiers = ManuallyDrop::new(Vec::new()); - let entity_len = identifier_buffer.iter().filter(|b| *b).count(); + let entity_len = unsafe { identifier_buffer.iter() }.filter(|b| *b).count(); let mut components = Vec::with_capacity(entity_len); for _ in 0..entity_len { let mut v = ManuallyDrop::new(Vec::new()); components.push((v.as_mut_ptr(), v.capacity())); } - Self::from_raw_parts( - identifier_buffer, - ( - entity_identifiers.as_mut_ptr(), - entity_identifiers.capacity(), - ), - components, - 0, - ) + unsafe { + Self::from_raw_parts( + identifier_buffer, + ( + entity_identifiers.as_mut_ptr(), + entity_identifiers.capacity(), + ), + components, + 0, + ) + } } pub(crate) unsafe fn push( @@ -101,18 +105,20 @@ where where E: Entity, { - entity.push_components(&self.component_map, &mut self.components, self.length); + unsafe { entity.push_components(&self.component_map, &mut self.components, self.length) }; let entity_identifier = entity_allocator.allocate(Location { - identifier: self.identifier_buffer.as_ref(), + identifier: unsafe { self.identifier_buffer.as_ref() }, index: self.length, }); - let mut entity_identifiers = ManuallyDrop::new(Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - )); + let mut entity_identifiers = ManuallyDrop::new(unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }); entity_identifiers.push(entity_identifier); self.entity_identifiers = ( entity_identifiers.as_mut_ptr(), @@ -134,20 +140,26 @@ where { let component_len = entities.entities.component_len(); - entities - .entities - .extend_components(&self.component_map, &mut self.components, self.length); + unsafe { + entities.entities.extend_components( + &self.component_map, + &mut self.components, + self.length, + ) + }; let entity_identifiers = entity_allocator.allocate_batch(Locations::new( self.length..(self.length + component_len), - self.identifier_buffer.as_ref(), + unsafe { self.identifier_buffer.as_ref() }, )); - let mut entity_identifiers_v = ManuallyDrop::new(Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - )); + let mut entity_identifiers_v = ManuallyDrop::new(unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }); entity_identifiers_v.extend(entity_identifiers.iter()); self.entity_identifiers = ( entity_identifiers_v.as_mut_ptr(), @@ -193,19 +205,21 @@ where where C: Component, { - *slice::from_raw_parts_mut( - self.components - .get_unchecked( - *self - .component_map - .get(&TypeId::of::()) - .unwrap_unchecked(), - ) - .0 - .cast::(), - self.length, - ) - .get_unchecked_mut(index) = component; + unsafe { + *slice::from_raw_parts_mut( + self.components + .get_unchecked( + *self + .component_map + .get(&TypeId::of::()) + .unwrap_unchecked(), + ) + .0 + .cast::(), + self.length, + ) + .get_unchecked_mut(index) = component + }; } pub(crate) unsafe fn remove_row_unchecked( @@ -213,24 +227,30 @@ where index: usize, entity_allocator: &mut entity::Allocator, ) { - R::remove_component_row( - index, - &self.components, - self.length, - self.identifier_buffer.iter(), - ); + unsafe { + R::remove_component_row( + index, + &self.components, + self.length, + self.identifier_buffer.iter(), + ) + }; - let mut entity_identifiers = ManuallyDrop::new(Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - )); + let mut entity_identifiers = ManuallyDrop::new(unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }); // Update swapped index if this isn't the last row. if index < self.length - 1 { - entity_allocator.modify_location_index_unchecked( - *entity_identifiers.last().unwrap_unchecked(), - index, - ); + unsafe { + entity_allocator.modify_location_index_unchecked( + *entity_identifiers.last().unwrap_unchecked(), + index, + ) + }; } entity_identifiers.swap_remove(index); @@ -244,26 +264,32 @@ where ) -> (entity::Identifier, Vec) { let size_of_components = self.identifier_buffer.size_of_components(); let mut bytes = Vec::with_capacity(size_of_components); - R::pop_component_row( - index, - bytes.as_mut_ptr(), - &self.components, - self.length, - self.identifier_buffer.iter(), - ); - bytes.set_len(size_of_components); + unsafe { + R::pop_component_row( + index, + bytes.as_mut_ptr(), + &self.components, + self.length, + self.identifier_buffer.iter(), + ) + }; + unsafe { bytes.set_len(size_of_components) }; - let mut entity_identifiers = ManuallyDrop::new(Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - )); + let mut entity_identifiers = ManuallyDrop::new(unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }); // Update swapped index if this isn't the last row. if index < self.length - 1 { - entity_allocator.modify_location_index_unchecked( - *entity_identifiers.last().unwrap_unchecked(), - index, - ); + unsafe { + entity_allocator.modify_location_index_unchecked( + *entity_identifiers.last().unwrap_unchecked(), + index, + ) + }; } let entity_identifier = entity_identifiers.swap_remove(index); @@ -281,19 +307,23 @@ where where C: Component, { - R::push_components_from_buffer_and_component( - buffer, - MaybeUninit::new(component), - &mut self.components, - self.length, - self.identifier_buffer.iter(), - ); + unsafe { + R::push_components_from_buffer_and_component( + buffer, + MaybeUninit::new(component), + &mut self.components, + self.length, + self.identifier_buffer.iter(), + ) + }; - let mut entity_identifiers = ManuallyDrop::new(Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - )); + let mut entity_identifiers = ManuallyDrop::new(unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }); entity_identifiers.push(entity_identifier); self.entity_identifiers = ( entity_identifiers.as_mut_ptr(), @@ -313,19 +343,23 @@ where where C: Component, { - R::push_components_from_buffer_skipping_component( - buffer, - PhantomData::, - &mut self.components, - self.length, - self.identifier_buffer.iter(), - ); + unsafe { + R::push_components_from_buffer_skipping_component( + buffer, + PhantomData::, + &mut self.components, + self.length, + self.identifier_buffer.iter(), + ) + }; - let mut entity_identifiers = ManuallyDrop::new(Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - )); + let mut entity_identifiers = ManuallyDrop::new(unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }); entity_identifiers.push(entity_identifier); self.entity_identifiers = ( entity_identifiers.as_mut_ptr(), @@ -339,20 +373,24 @@ where pub(crate) unsafe fn clear(&mut self, entity_allocator: &mut entity::Allocator) { // Clear each column. - R::clear_components( - &mut self.components, - self.length, - self.identifier_buffer.iter(), - ); + unsafe { + R::clear_components( + &mut self.components, + self.length, + self.identifier_buffer.iter(), + ) + }; // Free each entity. - let mut entity_identifiers = ManuallyDrop::new(Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - )); + let mut entity_identifiers = ManuallyDrop::new(unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }); for entity_identifier in entity_identifiers.iter() { - entity_allocator.free_unchecked(*entity_identifier); + unsafe { entity_allocator.free_unchecked(*entity_identifier) }; } entity_identifiers.clear(); @@ -360,7 +398,7 @@ where } pub(crate) unsafe fn identifier(&self) -> IdentifierRef { - self.identifier_buffer.as_ref() + unsafe { self.identifier_buffer.as_ref() } } #[cfg(feature = "serde")] diff --git a/src/archetypes/mod.rs b/src/archetypes/mod.rs index ecb371af..e69ec819 100644 --- a/src/archetypes/mod.rs +++ b/src/archetypes/mod.rs @@ -94,12 +94,14 @@ where &mut self, identifier: archetype::IdentifierRef, ) -> &mut Archetype { - self.raw_archetypes - .get_mut( - Self::make_hash(identifier, &self.hash_builder), - Self::equivalent_identifier(identifier), - ) - .unwrap_unchecked() + unsafe { + self.raw_archetypes + .get_mut( + Self::make_hash(identifier, &self.hash_builder), + Self::equivalent_identifier(identifier), + ) + .unwrap_unchecked() + } } #[cfg(feature = "serde")] @@ -127,7 +129,7 @@ where pub(crate) unsafe fn clear(&mut self, entity_allocator: &mut entity::Allocator) { for archetype in self.iter_mut() { - archetype.clear(entity_allocator); + unsafe { archetype.clear(entity_allocator) }; } } } diff --git a/src/entities/seal/storage.rs b/src/entities/seal/storage.rs index b17d3a84..6f70c245 100644 --- a/src/entities/seal/storage.rs +++ b/src/entities/seal/storage.rs @@ -81,7 +81,11 @@ where // `component_column` extracted is guaranteed by the safety contract to correspond to // the column for component `C`. unsafe { - Vec::::from_raw_parts(component_column.0.cast::(), length, component_column.1) + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) }, ); v.extend(self.0); diff --git a/src/query/filter/seal.rs b/src/query/filter/seal.rs index 96203b96..09a8e09d 100644 --- a/src/query/filter/seal.rs +++ b/src/query/filter/seal.rs @@ -26,7 +26,7 @@ where { unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { match component_map.get(&TypeId::of::()) { - Some(index) => key.get_unchecked(index / 8) & (1 << (index % 8)) != 0, + Some(index) => unsafe { key.get_unchecked(index / 8) & (1 << (index % 8)) != 0 }, Option::None => false, } } @@ -37,7 +37,7 @@ where F: Filter, { unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - !F::filter(key, component_map) + unsafe { !F::filter(key, component_map) } } } @@ -47,7 +47,7 @@ where F2: Filter, { unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - F1::filter(key, component_map) && F2::filter(key, component_map) + unsafe { F1::filter(key, component_map) && F2::filter(key, component_map) } } } @@ -57,7 +57,7 @@ where F2: Filter, { unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - F1::filter(key, component_map) || F2::filter(key, component_map) + unsafe { F1::filter(key, component_map) || F2::filter(key, component_map) } } } @@ -66,7 +66,7 @@ where C: Component, { unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - Has::::filter(key, component_map) + unsafe { Has::::filter(key, component_map) } } } @@ -75,7 +75,7 @@ where C: Component, { unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - Has::::filter(key, component_map) + unsafe { Has::::filter(key, component_map) } } } @@ -115,6 +115,6 @@ where W: Views<'a>, { unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - And::::filter(key, component_map) + unsafe { And::::filter(key, component_map) } } } diff --git a/src/query/view/seal.rs b/src/query/view/seal.rs index d4bfdbcb..ae9a0904 100644 --- a/src/query/view/seal.rs +++ b/src/query/view/seal.rs @@ -36,13 +36,15 @@ where length: usize, component_map: &HashMap, ) -> Self::Result { - slice::from_raw_parts::<'a, C>( - columns - .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) - .0 - .cast::(), - length, - ) + unsafe { + slice::from_raw_parts::<'a, C>( + columns + .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) + .0 + .cast::(), + length, + ) + } .iter() } @@ -63,13 +65,15 @@ where length: usize, component_map: &HashMap, ) -> Self::Result { - slice::from_raw_parts_mut::<'a, C>( - columns - .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) - .0 - .cast::(), - length, - ) + unsafe { + slice::from_raw_parts_mut::<'a, C>( + columns + .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) + .0 + .cast::(), + length, + ) + } .iter_mut() } @@ -100,9 +104,11 @@ where ) -> Self::Result { match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( - slice::from_raw_parts(columns.get_unchecked(*index).0.cast::(), length) - .iter() - .map(wrap_some), + unsafe { + slice::from_raw_parts(columns.get_unchecked(*index).0.cast::(), length) + } + .iter() + .map(wrap_some), ), None => Either::Left(iter::repeat(None).take(length)), } @@ -134,9 +140,11 @@ where match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( - slice::from_raw_parts_mut(columns.get_unchecked(*index).0.cast::(), length) - .iter_mut() - .map(wrap_some), + unsafe { + slice::from_raw_parts_mut(columns.get_unchecked(*index).0.cast::(), length) + } + .iter_mut() + .map(wrap_some), ), None => Either::Left(iter::repeat_with(none as fn() -> Option<&'a mut C>).take(length)), } @@ -156,7 +164,7 @@ impl<'a> ViewSeal<'a> for entity::Identifier { length: usize, _component_map: &HashMap, ) -> Self::Result { - slice::from_raw_parts_mut::<'a, Self>(entity_identifiers.0, length) + unsafe { slice::from_raw_parts_mut::<'a, Self>(entity_identifiers.0, length) } .iter() .copied() } @@ -205,12 +213,14 @@ where length: usize, component_map: &HashMap, ) -> Self::Results { - V::view(columns, entity_identifiers, length, component_map).zip(W::view( - columns, - entity_identifiers, - length, - component_map, - )) + unsafe { + V::view(columns, entity_identifiers, length, component_map).zip(W::view( + columns, + entity_identifiers, + length, + component_map, + )) + } } fn assert_claims(buffer: &mut AssertionBuffer) { From 96624e1e20cb20f06e5c8c2695881aa205d0234a Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 22:26:07 -0700 Subject: [PATCH 15/71] Safety comments for archetype::iterator::Iter. --- src/archetype/identifier/iter.rs | 54 ++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-) diff --git a/src/archetype/identifier/iter.rs b/src/archetype/identifier/iter.rs index 630ec3c5..b8714b58 100644 --- a/src/archetype/identifier/iter.rs +++ b/src/archetype/identifier/iter.rs @@ -1,15 +1,42 @@ use crate::registry::Registry; use core::marker::PhantomData; +/// An iterator over the bits of an [`Identifier`]. +/// +/// This iterator is guaranteed to return exactly `(R::LEN + 7) / 8` boolean values indicating +/// the components within `R` that are identified. +/// +/// [`Identifier`]: crate::archetype::identifier::Identifier pub struct Iter where R: Registry, { + /// The [`Registry`] defining the set of valid values of this identifier. + /// + /// Each identifier must exist within a set defined by a `Registry`. This defines a space over + /// which each identifier can uniquely define a set of components. Each bit within the + /// identifier corresponds with a component in the registry. + /// + /// [`Registry`]: crate::registry::Registry registry: PhantomData, + /// A pointer to the allocated bits. + /// + /// Note that this allocation is not owned by this struct. It is owned by an [`Identifier`] and + /// is invariantly guaranteed to outlive this struct. + /// + /// As iteration progresses, this pointer will move along to point to the current byte. + /// + /// [`Identifier`]: crate::archetype::identifier::Identifier pointer: *const u8, + /// The current byte being iterated over. + /// + /// This byte just includes the remaining bits of the current value. current: u8, + /// The current bit position. + /// + /// If this value is greater than or equal to `(R::LEN + 7) / 8`, iteration has completed. position: usize, } @@ -17,13 +44,26 @@ impl Iter where R: Registry, { + /// Create a new iterator for an [`Identifier`]. + /// + /// # Safety + /// `pointer` must be a pointer to a valid `Identifier` allocation, and must be ensured to live + /// as long as the returned `Iter`. + /// + /// [`Identifier`]: crate::archetype::identifier::Identifier pub(super) unsafe fn new(pointer: *const u8) -> Self { Self { registry: PhantomData, pointer, - current: if R::LEN > 0 { unsafe { *pointer } } else { 0 }, + current: if R::LEN > 0 { + // SAFETY: `pointer` is a valid `Identifier` allocation that is nonempty, meaning + // it points to at least one `u8` which is dereferenced here. + unsafe { *pointer } + } else { + 0 + }, position: 0, } } @@ -42,8 +82,16 @@ where let result = self.current & 1 != 0; self.position += 1; if self.position < R::LEN && self.position % 8 == 0 { - self.pointer = unsafe { self.pointer.add(1) }; - self.current = unsafe { *self.pointer }; + self.pointer = + // SAFETY: The allocation pointed to is guaranteed to have at least + // `(R::LEN + 7) / 8` bytes. Therefore, since `self.position` is only + // incremented once on each iteration, we will only enter this block for every + // eighth byte and therefore not offset past the end of the allocation. + unsafe { self.pointer.add(1) }; + self.current = + // SAFETY: Due to the reasons above, `self.pointer` will always point to a + // valid `u8` that can be dereferenced here without fail. + unsafe { *self.pointer }; } else { self.current >>= 1; } From e5305cc709557f1739548a63c1c712b3a8c513e6 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 22:32:09 -0700 Subject: [PATCH 16/71] The rest of the unsafe blocks. --- src/query/view/par/seal/mod.rs | 66 +++++++++++++++++++--------------- src/registry/serde.rs | 52 ++++++++++++--------------- 2 files changed, 61 insertions(+), 57 deletions(-) diff --git a/src/query/view/par/seal/mod.rs b/src/query/view/par/seal/mod.rs index 152e84b3..df9fba07 100644 --- a/src/query/view/par/seal/mod.rs +++ b/src/query/view/par/seal/mod.rs @@ -43,13 +43,15 @@ where length: usize, component_map: &HashMap, ) -> Self::ParResult { - core::slice::from_raw_parts::<'a, C>( - columns - .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) - .0 - .cast::(), - length, - ) + unsafe { + core::slice::from_raw_parts::<'a, C>( + columns + .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) + .0 + .cast::(), + length, + ) + } .par_iter() } } @@ -66,13 +68,15 @@ where length: usize, component_map: &HashMap, ) -> Self::ParResult { - core::slice::from_raw_parts_mut::<'a, C>( - columns - .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) - .0 - .cast::(), - length, - ) + unsafe { + core::slice::from_raw_parts_mut::<'a, C>( + columns + .get_unchecked(*component_map.get(&TypeId::of::()).unwrap_unchecked()) + .0 + .cast::(), + length, + ) + } .par_iter_mut() } } @@ -99,9 +103,11 @@ where ) -> Self::ParResult { match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( - core::slice::from_raw_parts(columns.get_unchecked(*index).0.cast::(), length) - .par_iter() - .map(wrap_some), + unsafe { + core::slice::from_raw_parts(columns.get_unchecked(*index).0.cast::(), length) + } + .par_iter() + .map(wrap_some), ), None => Either::Left(iter::repeat(None).take(length)), } @@ -125,10 +131,12 @@ where ) -> Self::ParResult { match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( - core::slice::from_raw_parts_mut( - columns.get_unchecked(*index).0.cast::(), - length, - ) + unsafe { + core::slice::from_raw_parts_mut( + columns.get_unchecked(*index).0.cast::(), + length, + ) + } .par_iter_mut() .map(wrap_some), ), @@ -146,7 +154,7 @@ impl<'a> ParViewSeal<'a> for entity::Identifier { length: usize, _component_map: &HashMap, ) -> Self::ParResult { - core::slice::from_raw_parts_mut::<'a, Self>(entity_identifiers.0, length) + unsafe { core::slice::from_raw_parts_mut::<'a, Self>(entity_identifiers.0, length) } .par_iter() .cloned() } @@ -189,11 +197,13 @@ where length: usize, component_map: &HashMap, ) -> Self::ParResults { - V::par_view(columns, entity_identifiers, length, component_map).zip(W::par_view( - columns, - entity_identifiers, - length, - component_map, - )) + unsafe { + V::par_view(columns, entity_identifiers, length, component_map).zip(W::par_view( + columns, + entity_identifiers, + length, + component_map, + )) + } } } diff --git a/src/registry/serde.rs b/src/registry/serde.rs index 62444bce..ad957d4c 100644 --- a/src/registry/serde.rs +++ b/src/registry/serde.rs @@ -76,20 +76,16 @@ where R_: Registry, S: SerializeTuple, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked(0); - tuple.serialize_element(&SerializeColumn(&ManuallyDrop::new( - Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - ), - )))?; + if unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = unsafe { components.get_unchecked(0) }; + tuple.serialize_element(&SerializeColumn(&ManuallyDrop::new(unsafe { + Vec::::from_raw_parts(component_column.0.cast::(), length, component_column.1) + })))?; - components = components.get_unchecked(1..); + components = unsafe { components.get_unchecked(1..) }; } - R::serialize_components_by_column(components, length, tuple, identifier_iter) + unsafe { R::serialize_components_by_column(components, length, tuple, identifier_iter) } } unsafe fn serialize_components_by_row( @@ -103,21 +99,21 @@ where R_: Registry, S: SerializeTuple, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked(0); - tuple.serialize_element( + if unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = unsafe { components.get_unchecked(0) }; + tuple.serialize_element(unsafe { ManuallyDrop::new(Vec::::from_raw_parts( component_column.0.cast::(), length, component_column.1, )) - .get_unchecked(index), - )?; + .get_unchecked(index) + })?; - components = components.get_unchecked(1..); + components = unsafe { components.get_unchecked(1..) }; } - R::serialize_components_by_row(components, length, index, tuple, identifier_iter) + unsafe { R::serialize_components_by_row(components, length, index, tuple, identifier_iter) } } } @@ -187,7 +183,7 @@ where R_: Registry, V: SeqAccess<'de>, { - if identifier_iter.next().unwrap_unchecked() { + if unsafe { identifier_iter.next().unwrap_unchecked() } { // TODO: Better error messages? let component_column = seq .next_element_seed(DeserializeColumn::::new(length))? @@ -197,7 +193,7 @@ where components.push((component_column.0.cast::(), component_column.1)); } - R::deserialize_components_by_column(components, length, seq, identifier_iter) + unsafe { R::deserialize_components_by_column(components, length, seq, identifier_iter) } } unsafe fn deserialize_components_by_row( @@ -210,13 +206,11 @@ where R_: Registry, V: SeqAccess<'de>, { - if identifier_iter.next().unwrap_unchecked() { - let component_column = components.get_unchecked_mut(0); - let mut v = ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - 0, - component_column.1, - )); + if unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = unsafe { components.get_unchecked_mut(0) }; + let mut v = ManuallyDrop::new(unsafe { + Vec::::from_raw_parts(component_column.0.cast::(), 0, component_column.1) + }); v.push(seq.next_element()?.ok_or_else(|| { // TODO: the length returned here is incorrect. @@ -225,9 +219,9 @@ where component_column.0 = v.as_mut_ptr().cast::(); component_column.1 = v.capacity(); - components = components.get_unchecked_mut(1..); + components = unsafe { components.get_unchecked_mut(1..) }; } - R::deserialize_components_by_row(components, length, seq, identifier_iter) + unsafe { R::deserialize_components_by_row(components, length, seq, identifier_iter) } } } From 66d0010b4a00dd00ec78573b3b5fb0421ada1138 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 22 Mar 2022 22:37:34 -0700 Subject: [PATCH 17/71] Rename identifier_buffer to identifier. --- src/archetype/impl_debug.rs | 4 ++-- src/archetype/impl_drop.rs | 2 +- src/archetype/impl_eq.rs | 4 ++-- src/archetype/impl_serde.rs | 12 ++++++------ src/archetype/mod.rs | 32 ++++++++++++++++---------------- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/archetype/impl_debug.rs b/src/archetype/impl_debug.rs index bfbeeff7..40a318cc 100644 --- a/src/archetype/impl_debug.rs +++ b/src/archetype/impl_debug.rs @@ -68,7 +68,7 @@ where i, &self.components, &mut component_pointers, - self.identifier_buffer.iter(), + self.identifier.iter(), ); } debug_map.entry( @@ -77,7 +77,7 @@ where identifier: *unsafe { entity_identifiers.get_unchecked(i) }, components: Components { pointers: component_pointers, - identifier: unsafe { self.identifier_buffer.as_ref() }, + identifier: unsafe { self.identifier.as_ref() }, }, }, ); diff --git a/src/archetype/impl_drop.rs b/src/archetype/impl_drop.rs index 01449f5c..adb78db2 100644 --- a/src/archetype/impl_drop.rs +++ b/src/archetype/impl_drop.rs @@ -9,7 +9,7 @@ where #[inline] fn drop(&mut self) { unsafe { - R::free_components(&self.components, self.length, self.identifier_buffer.iter()); + R::free_components(&self.components, self.length, self.identifier.iter()); } drop(unsafe { Vec::from_raw_parts( diff --git a/src/archetype/impl_eq.rs b/src/archetype/impl_eq.rs index cb35e80c..86d15007 100644 --- a/src/archetype/impl_eq.rs +++ b/src/archetype/impl_eq.rs @@ -11,7 +11,7 @@ where { fn eq(&self, other: &Self) -> bool { self.length == other.length - && self.identifier_buffer == other.identifier_buffer + && self.identifier == other.identifier && ManuallyDrop::new(unsafe { Vec::from_raw_parts( self.entity_identifiers.0, @@ -30,7 +30,7 @@ where &self.components, &other.components, self.length, - self.identifier_buffer.iter(), + self.identifier.iter(), ) } } diff --git a/src/archetype/impl_serde.rs b/src/archetype/impl_serde.rs index 482c34d6..d83abc9a 100644 --- a/src/archetype/impl_serde.rs +++ b/src/archetype/impl_serde.rs @@ -55,7 +55,7 @@ where S: Serializer, { let mut tuple = serializer.serialize_tuple( - unsafe { self.0.identifier_buffer.iter() } + unsafe { self.0.identifier.iter() } .filter(|b| *b) .count() + 1, @@ -72,7 +72,7 @@ where &self.0.components, self.0.length, &mut tuple, - self.0.identifier_buffer.iter(), + self.0.identifier.iter(), )?; } tuple.end() @@ -92,7 +92,7 @@ where S: Serializer, { let mut tuple = serializer.serialize_tuple(3)?; - tuple.serialize_element(&self.0.identifier_buffer)?; + tuple.serialize_element(&self.0.identifier)?; tuple.serialize_element(&self.0.length)?; tuple.serialize_element(&SerializeColumns(self.0))?; tuple.end() @@ -116,7 +116,7 @@ where S: Serializer, { let mut tuple = serializer.serialize_tuple( - unsafe { self.archetype.identifier_buffer.iter() } + unsafe { self.archetype.identifier.iter() } .filter(|b| *b) .count() + 1, @@ -137,7 +137,7 @@ where self.archetype.length, self.index, &mut tuple, - self.archetype.identifier_buffer.iter(), + self.archetype.identifier.iter(), )?; } @@ -181,7 +181,7 @@ where S: Serializer, { let mut tuple = serializer.serialize_tuple(3)?; - tuple.serialize_element(&self.0.identifier_buffer)?; + tuple.serialize_element(&self.0.identifier)?; tuple.serialize_element(&self.0.length)?; tuple.serialize_element(&SerializeRows(self.0))?; tuple.end() diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 50182abe..338dbc2a 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -39,7 +39,7 @@ pub(crate) struct Archetype where R: Registry, { - identifier_buffer: Identifier, + identifier: Identifier, entity_identifiers: (*mut entity::Identifier, usize), components: Vec<(*mut u8, usize)>, @@ -53,18 +53,18 @@ where R: Registry, { pub(crate) unsafe fn from_raw_parts( - identifier_buffer: Identifier, + identifier: Identifier, entity_identifiers: (*mut entity::Identifier, usize), components: Vec<(*mut u8, usize)>, length: usize, ) -> Self { let mut component_map = HashMap::new(); unsafe { - R::create_component_map_for_identifier(&mut component_map, 0, identifier_buffer.iter()) + R::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) }; Self { - identifier_buffer, + identifier, entity_identifiers, components, @@ -74,10 +74,10 @@ where } } - pub(crate) unsafe fn new(identifier_buffer: Identifier) -> Self { + pub(crate) unsafe fn new(identifier: Identifier) -> Self { let mut entity_identifiers = ManuallyDrop::new(Vec::new()); - let entity_len = unsafe { identifier_buffer.iter() }.filter(|b| *b).count(); + let entity_len = unsafe { identifier.iter() }.filter(|b| *b).count(); let mut components = Vec::with_capacity(entity_len); for _ in 0..entity_len { let mut v = ManuallyDrop::new(Vec::new()); @@ -86,7 +86,7 @@ where unsafe { Self::from_raw_parts( - identifier_buffer, + identifier, ( entity_identifiers.as_mut_ptr(), entity_identifiers.capacity(), @@ -108,7 +108,7 @@ where unsafe { entity.push_components(&self.component_map, &mut self.components, self.length) }; let entity_identifier = entity_allocator.allocate(Location { - identifier: unsafe { self.identifier_buffer.as_ref() }, + identifier: unsafe { self.identifier.as_ref() }, index: self.length, }); @@ -150,7 +150,7 @@ where let entity_identifiers = entity_allocator.allocate_batch(Locations::new( self.length..(self.length + component_len), - unsafe { self.identifier_buffer.as_ref() }, + unsafe { self.identifier.as_ref() }, )); let mut entity_identifiers_v = ManuallyDrop::new(unsafe { @@ -232,7 +232,7 @@ where index, &self.components, self.length, - self.identifier_buffer.iter(), + self.identifier.iter(), ) }; @@ -262,7 +262,7 @@ where index: usize, entity_allocator: &mut entity::Allocator, ) -> (entity::Identifier, Vec) { - let size_of_components = self.identifier_buffer.size_of_components(); + let size_of_components = self.identifier.size_of_components(); let mut bytes = Vec::with_capacity(size_of_components); unsafe { R::pop_component_row( @@ -270,7 +270,7 @@ where bytes.as_mut_ptr(), &self.components, self.length, - self.identifier_buffer.iter(), + self.identifier.iter(), ) }; unsafe { bytes.set_len(size_of_components) }; @@ -313,7 +313,7 @@ where MaybeUninit::new(component), &mut self.components, self.length, - self.identifier_buffer.iter(), + self.identifier.iter(), ) }; @@ -349,7 +349,7 @@ where PhantomData::, &mut self.components, self.length, - self.identifier_buffer.iter(), + self.identifier.iter(), ) }; @@ -377,7 +377,7 @@ where R::clear_components( &mut self.components, self.length, - self.identifier_buffer.iter(), + self.identifier.iter(), ) }; @@ -398,7 +398,7 @@ where } pub(crate) unsafe fn identifier(&self) -> IdentifierRef { - unsafe { self.identifier_buffer.as_ref() } + unsafe { self.identifier.as_ref() } } #[cfg(feature = "serde")] From 45cf82fa791a46f1fb05ac624858ded4efaff2d0 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 2 May 2022 22:38:27 -0700 Subject: [PATCH 18/71] Safety comments for entity::Allocator serde impls. --- src/archetype/impl_serde.rs | 8 ++------ src/archetype/mod.rs | 19 +++---------------- src/entity/allocator/impl_serde.rs | 9 ++++++++- 3 files changed, 13 insertions(+), 23 deletions(-) diff --git a/src/archetype/impl_serde.rs b/src/archetype/impl_serde.rs index d83abc9a..23bde76a 100644 --- a/src/archetype/impl_serde.rs +++ b/src/archetype/impl_serde.rs @@ -54,12 +54,8 @@ where where S: Serializer, { - let mut tuple = serializer.serialize_tuple( - unsafe { self.0.identifier.iter() } - .filter(|b| *b) - .count() - + 1, - )?; + let mut tuple = serializer + .serialize_tuple(unsafe { self.0.identifier.iter() }.filter(|b| *b).count() + 1)?; tuple.serialize_element(&SerializeColumn(&ManuallyDrop::new(unsafe { Vec::from_raw_parts( self.0.entity_identifiers.0, diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 338dbc2a..d2be3524 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -59,9 +59,7 @@ where length: usize, ) -> Self { let mut component_map = HashMap::new(); - unsafe { - R::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) - }; + unsafe { R::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) }; Self { identifier, @@ -228,12 +226,7 @@ where entity_allocator: &mut entity::Allocator, ) { unsafe { - R::remove_component_row( - index, - &self.components, - self.length, - self.identifier.iter(), - ) + R::remove_component_row(index, &self.components, self.length, self.identifier.iter()) }; let mut entity_identifiers = ManuallyDrop::new(unsafe { @@ -373,13 +366,7 @@ where pub(crate) unsafe fn clear(&mut self, entity_allocator: &mut entity::Allocator) { // Clear each column. - unsafe { - R::clear_components( - &mut self.components, - self.length, - self.identifier.iter(), - ) - }; + unsafe { R::clear_components(&mut self.components, self.length, self.identifier.iter()) }; // Free each entity. let mut entity_identifiers = ManuallyDrop::new(unsafe { diff --git a/src/entity/allocator/impl_serde.rs b/src/entity/allocator/impl_serde.rs index 78adeb59..e41492b0 100644 --- a/src/entity/allocator/impl_serde.rs +++ b/src/entity/allocator/impl_serde.rs @@ -25,6 +25,7 @@ where for index in &self.0.free { seq.serialize_element(&entity::Identifier { index: *index, + // SAFETY: `index` is invariantly guaranteed to be a valid index into `slots`. generation: unsafe { self.0.slots.get_unchecked(*index) }.generation, })?; } @@ -227,6 +228,10 @@ where *slot = Some(Slot { generation: entity_identifier.generation, location: Some(Location { + // SAFETY: This `IdentifierRef` is guaranteed to be outlived by the + // `Identifier` it references, since the `Identifier` is contained + // in an `Archetype` that lives as long as its containing `World`, + // meaning it will at least live as long as this `Location`. identifier: unsafe { archetype.identifier() }, index: i, }), @@ -246,7 +251,9 @@ where Ok(Self { slots: slots .into_iter() - .map(|slot| unsafe { slot.unwrap_unchecked() }) + .map(|slot| + // SAFETY: We just checked above that each `slot` is not `None`. + unsafe { slot.unwrap_unchecked() }) .collect(), free: free .into_iter() From 6a16eb20c154734b21c4eb939e5822cef4f5585b Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 4 May 2022 00:08:29 -0700 Subject: [PATCH 19/71] Safety comments for Task::run(). --- src/system/schedule/task.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/system/schedule/task.rs b/src/system/schedule/task.rs index 1b09166e..db4429f5 100644 --- a/src/system/schedule/task.rs +++ b/src/system/schedule/task.rs @@ -21,12 +21,15 @@ where match self { Task::Seq(system) => { // Query world using system. + // SAFETY: The `Views` checks were already done when constructing the `Schedule`. let result = unsafe { (*world.0).query_unchecked::() }; // Run system using the query result. system.run(result); } Task::Par(system) => { // Query world using system. + // SAFETY: The `ParViews` checks were already done when constructing the + // `Schedule`. let result = unsafe { (*world.0).par_query_unchecked::() }; // Run system using the query result. system.run(result); From 3d9e9b1a90751b1092b4f877f0280ae8d2603a07 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 4 May 2022 00:21:53 -0700 Subject: [PATCH 20/71] Safety comments for identifier construction. --- src/world/mod.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/world/mod.rs b/src/world/mod.rs index 7fdab080..b1cc8fb7 100644 --- a/src/world/mod.rs +++ b/src/world/mod.rs @@ -155,9 +155,13 @@ where self.len += 1; let mut identifier = vec![0; (R::LEN + 7) / 8]; + // SAFETY: `identifier` is a zeroed-out allocation of `R::LEN` bits. `self.component_map` + // only contains `usize` values up to the number of components in the registry `R`. unsafe { E::to_identifier(&mut identifier, &self.component_map); } + // SAFETY: `identifier` is a properly-initialized buffer of `(R::LEN + 7) / 8` bytes whose + // bits correspond to each component in the registry `R`. let identifier_buffer = unsafe { archetype::Identifier::new(identifier) }; unsafe { @@ -194,9 +198,13 @@ where self.len += entities.len(); let mut identifier = vec![0; (R::LEN + 7) / 8]; + // SAFETY: `identifier` is a zeroed-out allocation of `R::LEN` bits. `self.component_map` + // only contains `usize` values up to the number of components in the registry `R`. unsafe { E::to_identifier(&mut identifier, &self.component_map); } + // SAFETY: `identifier` is a properly-initialized buffer of `(R::LEN + 7) / 8` bytes whose + // bits correspond to each component in the registry `R`. let identifier_buffer = unsafe { archetype::Identifier::new(identifier) }; unsafe { @@ -314,6 +322,10 @@ where } /// Performs a query, skipping checks on views. + /// + /// # Safety + /// The [`Views`] `V` must follow Rust's borrowing rules, meaning that a component that is + /// mutably borrowed is only borrowed once. #[cfg(feature = "parallel")] #[cfg_attr(doc_cfg, doc(cfg(feature = "parallel")))] pub(crate) unsafe fn query_unchecked<'a, V, F>(&'a mut self) -> result::Iter<'a, R, F, V> @@ -325,6 +337,10 @@ where } /// Performs a parallel query, skipping checks on views. + /// + /// # Safety + /// The [`ParViews`] `V` must follow Rust's borrowing rules, meaning that a component that is + /// mutably borrowed is only borrowed once. #[cfg(feature = "parallel")] #[cfg_attr(doc_cfg, doc(cfg(feature = "parallel")))] pub(crate) unsafe fn par_query_unchecked<'a, V, F>(&'a mut self) -> result::ParIter<'a, R, F, V> From ef3474f365b39e9b5a3a4318169ab7f827f0ca8d Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 4 May 2022 00:38:41 -0700 Subject: [PATCH 21/71] Safety comments in Archetype debug impl. --- src/archetype/impl_debug.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/archetype/impl_debug.rs b/src/archetype/impl_debug.rs index 40a318cc..b3c31531 100644 --- a/src/archetype/impl_debug.rs +++ b/src/archetype/impl_debug.rs @@ -19,6 +19,9 @@ where { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut debug_map = f.debug_map(); + // SAFETY: `pointers` is guaranteed to contain the same number of values as there are + // components, each pointing to a valid component of type `C`. Also, `self.identifier` will + // yield the same number of values as there are components in `R`. unsafe { R::debug_components(&self.pointers, &mut debug_map, self.identifier.iter()); } @@ -53,16 +56,24 @@ where fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut debug_map = f.debug_map(); - let entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let entity_identifiers = ManuallyDrop::new( + // SAFETY: `self.identifiers`, together with `self.length`, are guaranteed to be the + // raw parts for a valid `Vec`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); for i in 0..self.length { let mut component_pointers = Vec::new(); + // SAFETY: `self.components` contains the same number of values as + // `self.identifier.iter()` has bits. Each tuple in `components` corresponds to a valid + // `Vec` for each component `C` with a length of `self.length`. Therefore, `i` will + // be within the bounds of each of those `Vec`s as well. unsafe { R::extract_component_pointers( i, @@ -74,9 +85,12 @@ where debug_map.entry( &i, &Row:: { + // SAFETY: `entity_identifiers` is guaranteed to be of length `self.length`. identifier: *unsafe { entity_identifiers.get_unchecked(i) }, components: Components { pointers: component_pointers, + // SAFETY: This `IdentifierRef` will not outlive its referenced + // `Identifier`. identifier: unsafe { self.identifier.as_ref() }, }, }, From 241303674f1fbeaa2aa2c68504ad31fa30e34dd0 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 4 May 2022 00:46:52 -0700 Subject: [PATCH 22/71] Safety comments for archetype::Identifier's serde impls. --- src/archetype/identifier/impl_serde.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/archetype/identifier/impl_serde.rs b/src/archetype/identifier/impl_serde.rs index a53826eb..16f1002e 100644 --- a/src/archetype/identifier/impl_serde.rs +++ b/src/archetype/identifier/impl_serde.rs @@ -18,6 +18,7 @@ where { let mut tuple = serializer.serialize_tuple((R::LEN + 7) / 8)?; + // SAFETY: The slice returned here is guaranteed to be outlived by `self`. for byte in unsafe { self.as_slice() } { tuple.serialize_element(byte)?; } @@ -65,10 +66,12 @@ where } // Check that trailing bits are not set. + // SAFETY: `buffer` is guaranteed to have `(R::LEN + 7) / 8` elements, so this will + // always be within the bounds of `buffer.` let byte = unsafe { buffer.get_unchecked((R::LEN + 7) / 8 - 1) }; let bit = R::LEN % 8; if bit != 0 - && unsafe { buffer.get_unchecked((R::LEN + 7) / 8 - 1) } & (255 << bit) != 0 + && byte & (255 << bit) != 0 { return Err(de::Error::invalid_value( Unexpected::Unsigned(u64::from(*byte)), From 4ec83e97e0f048460643d5dcf4a15fd36dee2b01 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 4 May 2022 00:52:26 -0700 Subject: [PATCH 23/71] Safety comments for Entities. --- src/entities/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/entities/mod.rs b/src/entities/mod.rs index ddfe5e36..18f21d2d 100644 --- a/src/entities/mod.rs +++ b/src/entities/mod.rs @@ -142,6 +142,7 @@ where /// Panics if the columns are not all the same length. pub fn new(entities: E) -> Self { assert!(entities.check_len()); + // SAFETY: We just guaranteed the lengths of all columns are equal. unsafe { Self::new_unchecked(entities) } } @@ -232,6 +233,7 @@ where #[macro_export] macro_rules! entities { (($component:expr $(,$components:expr)* $(,)?); $n:expr) => { + // SAFETY: Each `Vec` created here will be of length `$n`. unsafe { $crate::entities::Batch::new_unchecked( ($crate::reexports::vec![$component; $n], $crate::entities!(@cloned ($($components),*); $n)) @@ -239,6 +241,8 @@ macro_rules! entities { } }; ($(($($components:expr),*)),+ $(,)?) => { + // SAFETY: During transposition, each column is guaranteed to have an equal number of + // components. unsafe { $crate::entities::Batch::new_unchecked( $crate::entities!(@transpose [] $(($($components),*)),+) @@ -246,11 +250,13 @@ macro_rules! entities { } }; ((); $n:expr) => { + // SAFETY: There are no columns to check. unsafe { $crate::entities::Batch::new_unchecked($crate::entities::Null) } }; () => { + // SAFETY: There are no columns to check. unsafe { $crate::entities::Batch::new_unchecked($crate::entities::Null) } From 73cba63dfa08356e4df9bdad5860e82387b63c61 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 22:07:30 -0700 Subject: [PATCH 24/71] Mark Archetype::new() as a safe function. --- src/archetype/mod.rs | 2 +- src/archetypes/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index d2be3524..8b0c6c1b 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -72,7 +72,7 @@ where } } - pub(crate) unsafe fn new(identifier: Identifier) -> Self { + pub(crate) fn new(identifier: Identifier) -> Self { let mut entity_identifiers = ManuallyDrop::new(Vec::new()); let entity_len = unsafe { identifier.iter() }.filter(|b| *b).count(); diff --git a/src/archetypes/mod.rs b/src/archetypes/mod.rs index e69ec819..7451b1da 100644 --- a/src/archetypes/mod.rs +++ b/src/archetypes/mod.rs @@ -84,7 +84,7 @@ where Some(archetype_bucket) => unsafe { archetype_bucket.as_mut() }, None => self.raw_archetypes.insert_entry( hash, - unsafe { Archetype::new(identifier_buffer) }, + Archetype::new(identifier_buffer), Self::make_hasher(&self.hash_builder), ), } From b2c96b94000d04d0f3996aa73f7ea751aa58cbc2 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 22:24:42 -0700 Subject: [PATCH 25/71] Safety comments for Archetypes. --- src/archetype/identifier/impl_serde.rs | 4 +- src/archetype/mod.rs | 4 ++ src/archetypes/mod.rs | 59 ++++++++++++++++++++++---- 3 files changed, 55 insertions(+), 12 deletions(-) diff --git a/src/archetype/identifier/impl_serde.rs b/src/archetype/identifier/impl_serde.rs index 16f1002e..8fc8ad9e 100644 --- a/src/archetype/identifier/impl_serde.rs +++ b/src/archetype/identifier/impl_serde.rs @@ -70,9 +70,7 @@ where // always be within the bounds of `buffer.` let byte = unsafe { buffer.get_unchecked((R::LEN + 7) / 8 - 1) }; let bit = R::LEN % 8; - if bit != 0 - && byte & (255 << bit) != 0 - { + if bit != 0 && byte & (255 << bit) != 0 { return Err(de::Error::invalid_value( Unexpected::Unsigned(u64::from(*byte)), &self, diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 8b0c6c1b..37d9b39c 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -384,7 +384,11 @@ where self.length = 0; } + /// # Safety + /// The `Archetype` must outlive the returned `IdentifierRef`. pub(crate) unsafe fn identifier(&self) -> IdentifierRef { + // SAFETY: The safety contract of this method guarantees the returned `IdentifierRef` will + // outlive `self.identifier`. unsafe { self.identifier.as_ref() } } diff --git a/src/archetypes/mod.rs b/src/archetypes/mod.rs index 7451b1da..6fc28516 100644 --- a/src/archetypes/mod.rs +++ b/src/archetypes/mod.rs @@ -55,13 +55,21 @@ where } fn make_hasher(hash_builder: &ahash::RandomState) -> impl Fn(&Archetype) -> u64 + '_ { - move |archetype| Self::make_hash(unsafe { archetype.identifier() }, hash_builder) + move |archetype| { + Self::make_hash( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the `archetype`. + unsafe { archetype.identifier() }, + hash_builder, + ) + } } fn equivalent_identifier( identifier: archetype::IdentifierRef, ) -> impl Fn(&Archetype) -> bool { - move |archetype: &Archetype| unsafe { archetype.identifier() } == identifier + move |archetype: &Archetype| + // SAFETY: The `IdentifierRef` obtained here does not live longer than the `archetype`. + unsafe { archetype.identifier() } == identifier } pub(crate) fn get(&self, identifier: archetype::IdentifierRef) -> Option<&Archetype> { @@ -75,13 +83,24 @@ where &mut self, identifier_buffer: archetype::Identifier, ) -> &mut Archetype { - let hash = Self::make_hash(unsafe { identifier_buffer.as_ref() }, &self.hash_builder); + let hash = Self::make_hash( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the + // `identifier_buffer`. + unsafe { identifier_buffer.as_ref() }, + &self.hash_builder, + ); match self.raw_archetypes.find( hash, - Self::equivalent_identifier(unsafe { identifier_buffer.as_ref() }), + Self::equivalent_identifier( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the + // `identifier_buffer`. + unsafe { identifier_buffer.as_ref() }, + ), ) { - Some(archetype_bucket) => unsafe { archetype_bucket.as_mut() }, + Some(archetype_bucket) => + // SAFETY: This reference to the archetype contained in this bucket is unique. + unsafe { archetype_bucket.as_mut() }, None => self.raw_archetypes.insert_entry( hash, Archetype::new(identifier_buffer), @@ -90,10 +109,14 @@ where } } + /// # Safety + /// An archetype must be stored with the given `identifier`. pub(crate) unsafe fn get_unchecked_mut( &mut self, identifier: archetype::IdentifierRef, ) -> &mut Archetype { + // SAFETY: The safety contract of this method guarantees that `get_mut()` will return a + // `Some` value. unsafe { self.raw_archetypes .get_mut( @@ -106,10 +129,18 @@ where #[cfg(feature = "serde")] pub(crate) fn insert(&mut self, archetype: Archetype) -> bool { - let hash = Self::make_hash(unsafe { archetype.identifier() }, &self.hash_builder); + let hash = Self::make_hash( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the `archetype`. + unsafe { archetype.identifier() }, + &self.hash_builder, + ); if let Some(_existing_archetype) = self.raw_archetypes.get( hash, - Self::equivalent_identifier(unsafe { archetype.identifier() }), + Self::equivalent_identifier( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the + // `archetype`. + unsafe { archetype.identifier() }, + ), ) { false } else { @@ -120,15 +151,25 @@ where } pub(crate) fn iter(&self) -> Iter { - Iter::new(unsafe { self.raw_archetypes.iter() }) + Iter::new( + // SAFETY: The `Iter` containing this `RawIter` is guaranteed to not outlive `self`. + unsafe { self.raw_archetypes.iter() }, + ) } pub(crate) fn iter_mut(&mut self) -> IterMut { - IterMut::new(unsafe { self.raw_archetypes.iter() }) + IterMut::new( + // SAFETY: The `IterMut` containing this `RawIter` is guaranteed to not outlive `self`. + unsafe { self.raw_archetypes.iter() }, + ) } + /// # Safety + /// `entity_allocator` must contain entries for each of the entities stored in the archetypes. pub(crate) unsafe fn clear(&mut self, entity_allocator: &mut entity::Allocator) { for archetype in self.iter_mut() { + // SAFETY: The `entity_allocator` is guaranteed to have an entry for each entity stored + // in `archetype`. unsafe { archetype.clear(entity_allocator) }; } } From eb35de30017452d988fa0c0fcc838a9bbd77e74e Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 22:27:14 -0700 Subject: [PATCH 26/71] Safety comments for Archetypes debug impl. --- src/archetypes/impl_debug.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/archetypes/impl_debug.rs b/src/archetypes/impl_debug.rs index 8e17148c..bb0f0bc0 100644 --- a/src/archetypes/impl_debug.rs +++ b/src/archetypes/impl_debug.rs @@ -7,10 +7,14 @@ where { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_map() - .entries( - self.iter() - .map(|archetype| (unsafe { archetype.identifier() }, archetype)), - ) + .entries(self.iter().map(|archetype| { + ( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the + // `archetype`. + unsafe { archetype.identifier() }, + archetype, + ) + })) .finish() } } From 0ddb936580e3dc3a0d331ebb37feebbd5bdc59b2 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 22:28:22 -0700 Subject: [PATCH 27/71] Safety comments for Archetypes Eq impls. --- src/archetypes/impl_eq.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/archetypes/impl_eq.rs b/src/archetypes/impl_eq.rs index 6c87614a..06e37b6a 100644 --- a/src/archetypes/impl_eq.rs +++ b/src/archetypes/impl_eq.rs @@ -14,7 +14,11 @@ where self.iter().all(|archetype| { other - .get(unsafe { archetype.identifier() }) + .get( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the + // `archetype`. + unsafe { archetype.identifier() }, + ) .map_or(false, |other_archetype| archetype == other_archetype) }) } From a5693dc4f906547529f912738aaf993786d6dd0d Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 22:32:28 -0700 Subject: [PATCH 28/71] Archetypes iterator safety comments. --- src/archetypes/iter.rs | 14 ++++++++------ src/archetypes/par_iter.rs | 11 +++++++++-- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/archetypes/iter.rs b/src/archetypes/iter.rs index 948bd177..81469ee8 100644 --- a/src/archetypes/iter.rs +++ b/src/archetypes/iter.rs @@ -31,9 +31,10 @@ where type Item = &'a Archetype; fn next(&mut self) -> Option { - self.raw_iter - .next() - .map(|archetype_bucket| unsafe { archetype_bucket.as_ref() }) + self.raw_iter.next().map(|archetype_bucket| + // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be + // unique. + unsafe { archetype_bucket.as_ref() }) } fn size_hint(&self) -> (usize, Option) { @@ -70,9 +71,10 @@ where type Item = &'a mut Archetype; fn next(&mut self) -> Option { - self.raw_iter - .next() - .map(|archetype_bucket| unsafe { archetype_bucket.as_mut() }) + self.raw_iter.next().map(|archetype_bucket| + // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be + // unique. + unsafe { archetype_bucket.as_mut() }) } fn size_hint(&self) -> (usize, Option) { diff --git a/src/archetypes/par_iter.rs b/src/archetypes/par_iter.rs index 75fb0b7f..63858fdd 100644 --- a/src/archetypes/par_iter.rs +++ b/src/archetypes/par_iter.rs @@ -37,7 +37,10 @@ where C: UnindexedConsumer, { self.raw_iter - .map(|archetype_bucket| unsafe { archetype_bucket.as_mut() }) + .map(|archetype_bucket| + // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be + // unique. + unsafe { archetype_bucket.as_mut() }) .drive_unindexed(consumer) } } @@ -48,6 +51,10 @@ where { #[cfg_attr(doc_cfg, doc(cfg(feature = "parallel")))] pub(crate) fn par_iter_mut(&mut self) -> ParIterMut { - ParIterMut::new(unsafe { self.raw_archetypes.par_iter() }) + ParIterMut::new( + // SAFETY: The `ParIterMut` containing this `RawIter` is guaranteed to not outlive + // `self`. + unsafe { self.raw_archetypes.par_iter() }, + ) } } From b747e1c35ffa1c323414373551647aa4f3201881 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 22:51:01 -0700 Subject: [PATCH 29/71] Fix a few lints in Archetypes regarding closure declarations. --- src/archetypes/iter.rs | 18 ++++++++++-------- src/archetypes/mod.rs | 10 +++++++--- src/archetypes/par_iter.rs | 5 +++-- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/archetypes/iter.rs b/src/archetypes/iter.rs index 81469ee8..63cd69c7 100644 --- a/src/archetypes/iter.rs +++ b/src/archetypes/iter.rs @@ -31,10 +31,11 @@ where type Item = &'a Archetype; fn next(&mut self) -> Option { - self.raw_iter.next().map(|archetype_bucket| - // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be - // unique. - unsafe { archetype_bucket.as_ref() }) + self.raw_iter.next().map(|archetype_bucket| { + // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be + // unique. + unsafe { archetype_bucket.as_ref() } + }) } fn size_hint(&self) -> (usize, Option) { @@ -71,10 +72,11 @@ where type Item = &'a mut Archetype; fn next(&mut self) -> Option { - self.raw_iter.next().map(|archetype_bucket| - // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be - // unique. - unsafe { archetype_bucket.as_mut() }) + self.raw_iter.next().map(|archetype_bucket| { + // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be + // unique. + unsafe { archetype_bucket.as_mut() } + }) } fn size_hint(&self) -> (usize, Option) { diff --git a/src/archetypes/mod.rs b/src/archetypes/mod.rs index 6fc28516..6ed1aff8 100644 --- a/src/archetypes/mod.rs +++ b/src/archetypes/mod.rs @@ -67,9 +67,13 @@ where fn equivalent_identifier( identifier: archetype::IdentifierRef, ) -> impl Fn(&Archetype) -> bool { - move |archetype: &Archetype| - // SAFETY: The `IdentifierRef` obtained here does not live longer than the `archetype`. - unsafe { archetype.identifier() } == identifier + move |archetype: &Archetype| { + ( + // SAFETY: The `IdentifierRef` obtained here does not live longer than the + // `archetype`. + unsafe { archetype.identifier() } + ) == identifier + } } pub(crate) fn get(&self, identifier: archetype::IdentifierRef) -> Option<&Archetype> { diff --git a/src/archetypes/par_iter.rs b/src/archetypes/par_iter.rs index 63858fdd..bb34d487 100644 --- a/src/archetypes/par_iter.rs +++ b/src/archetypes/par_iter.rs @@ -37,10 +37,11 @@ where C: UnindexedConsumer, { self.raw_iter - .map(|archetype_bucket| + .map(|archetype_bucket| { // SAFETY: The reference to the archetype stored in this bucket is guaranteed to be // unique. - unsafe { archetype_bucket.as_mut() }) + unsafe { archetype_bucket.as_mut() } + }) .drive_unindexed(consumer) } } From 7ee65b121fdc591f1390bef84090ec6248db81e9 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 22:55:13 -0700 Subject: [PATCH 30/71] Fix misplaced semicolons. --- src/archetype/mod.rs | 28 ++++++++++++++-------------- src/registry/seal/storage.rs | 8 ++++---- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 37d9b39c..e8e53163 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -143,8 +143,8 @@ where &self.component_map, &mut self.components, self.length, - ) - }; + ); + } let entity_identifiers = entity_allocator.allocate_batch(Locations::new( self.length..(self.length + component_len), @@ -216,8 +216,8 @@ where .cast::(), self.length, ) - .get_unchecked_mut(index) = component - }; + .get_unchecked_mut(index) = component; + } } pub(crate) unsafe fn remove_row_unchecked( @@ -242,8 +242,8 @@ where entity_allocator.modify_location_index_unchecked( *entity_identifiers.last().unwrap_unchecked(), index, - ) - }; + ); + } } entity_identifiers.swap_remove(index); @@ -264,8 +264,8 @@ where &self.components, self.length, self.identifier.iter(), - ) - }; + ); + } unsafe { bytes.set_len(size_of_components) }; let mut entity_identifiers = ManuallyDrop::new(unsafe { @@ -281,8 +281,8 @@ where entity_allocator.modify_location_index_unchecked( *entity_identifiers.last().unwrap_unchecked(), index, - ) - }; + ); + } } let entity_identifier = entity_identifiers.swap_remove(index); @@ -307,8 +307,8 @@ where &mut self.components, self.length, self.identifier.iter(), - ) - }; + ); + } let mut entity_identifiers = ManuallyDrop::new(unsafe { Vec::from_raw_parts( @@ -343,8 +343,8 @@ where &mut self.components, self.length, self.identifier.iter(), - ) - }; + ); + } let mut entity_identifiers = ManuallyDrop::new(unsafe { Vec::from_raw_parts( diff --git a/src/registry/seal/storage.rs b/src/registry/seal/storage.rs index 22cf9ede..cc4e3fd9 100644 --- a/src/registry/seal/storage.rs +++ b/src/registry/seal/storage.rs @@ -752,8 +752,8 @@ where components, length, identifier_iter, - ) - }; + ); + } } unsafe fn push_components_from_buffer_skipping_component( @@ -860,8 +860,8 @@ where components, length, identifier_iter, - ) - }; + ); + } } unsafe fn free_components( From 476ed74635fa3e2bc6f5566889a64951084f64fe Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 23:07:57 -0700 Subject: [PATCH 31/71] Safety comments for Archetype creation. --- src/archetype/mod.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index e8e53163..6e4c5d06 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -52,6 +52,12 @@ impl Archetype where R: Registry, { + /// # Safety + /// `entity_identifiers` must contain the raw parts for a valid `Vec` of + /// size `length`. + /// + /// `components` must contain the raw parts for valid `Vec`s of size `length` for each + /// component `C` in the registry `R`. pub(crate) unsafe fn from_raw_parts( identifier: Identifier, entity_identifiers: (*mut entity::Identifier, usize), @@ -59,6 +65,8 @@ where length: usize, ) -> Self { let mut component_map = HashMap::new(); + // SAFETY: `identifier.iter()` is generic over the same registry `R` that this associated + // function is being called on. unsafe { R::create_component_map_for_identifier(&mut component_map, 0, identifier.iter()) }; Self { @@ -75,13 +83,19 @@ where pub(crate) fn new(identifier: Identifier) -> Self { let mut entity_identifiers = ManuallyDrop::new(Vec::new()); - let entity_len = unsafe { identifier.iter() }.filter(|b| *b).count(); + let entity_len = + // SAFETY: The iterator returned here is outlived by `identifier`. + unsafe { identifier.iter() }.filter(|b| *b).count(); let mut components = Vec::with_capacity(entity_len); for _ in 0..entity_len { let mut v = ManuallyDrop::new(Vec::new()); components.push((v.as_mut_ptr(), v.capacity())); } + // SAFETY: `entity_identifiers` is an empty `Vec`, which matches the provided `length` of + // 0. There are also exactly the same number of elements in `components` as there are + // components in the registry `R`, and each of those elements are the valid raw parts for a + // `Vec` of length 0. unsafe { Self::from_raw_parts( identifier, From adacc20ac2aca8845556b6035820c03bde698e8b Mon Sep 17 00:00:00 2001 From: Anders429 Date: Thu, 5 May 2022 23:19:24 -0700 Subject: [PATCH 32/71] Safety comments for push and extend methods on Archetype. --- src/archetype/mod.rs | 69 +++++++++++++++++++++++++++++++++----------- 1 file changed, 52 insertions(+), 17 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 6e4c5d06..3f4f42bf 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -93,9 +93,9 @@ where } // SAFETY: `entity_identifiers` is an empty `Vec`, which matches the provided `length` of - // 0. There are also exactly the same number of elements in `components` as there are + // 0. There are also exactly the same number of elements in `components` as there are // components in the registry `R`, and each of those elements are the valid raw parts for a - // `Vec` of length 0. + // `Vec` of length 0. unsafe { Self::from_raw_parts( identifier, @@ -109,6 +109,11 @@ where } } + /// # Safety + /// `entity` must be made up of only components that are identified by this `Archetype`'s + /// `Identifier`. These can, however, be in any order. + /// + /// The `entity_allocator`, together with its contained `Location`s, must not outlive `self`. pub(crate) unsafe fn push( &mut self, entity: E, @@ -117,20 +122,33 @@ where where E: Entity, { + // SAFETY: `self.component_map` contains an entry for every component identified by the + // archetype's `Identifier`. Therefore, it also contains an entry for every component `C` + // contained in the entity `E`. + // + // Also, `self.components`, together with `self.length`, define valid `Vec`s for each + // component. unsafe { entity.push_components(&self.component_map, &mut self.components, self.length) }; let entity_identifier = entity_allocator.allocate(Location { - identifier: unsafe { self.identifier.as_ref() }, + identifier: + // SAFETY: `entity_allocator` is guaranteed to not outlive `self`. Therefore, the + // `Location` being stored in it will also not outlive `self`. + unsafe { self.identifier.as_ref() }, index: self.length, }); - let mut entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let mut entity_identifiers = ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts that, + // together with `self.length`, create a valid `Vec`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); entity_identifiers.push(entity_identifier); self.entity_identifiers = ( entity_identifiers.as_mut_ptr(), @@ -142,6 +160,11 @@ where entity_identifier } + /// # Safety + /// `entities` must be made up of only components that are identified by this `Archetype`'s + /// `Identifier`. These can, however, be in any order. + /// + /// The `entity_allocator`, together with its contained `Location`s, must not outlive `self`. pub(crate) unsafe fn extend( &mut self, entities: entities::Batch, @@ -152,6 +175,12 @@ where { let component_len = entities.entities.component_len(); + // SAFETY: `self.component_map` contains an entry for every component identified by the + // archetype's `Identifier`. Therefore, it also contains an entry for every component `C` + // contained in `entities`. + // + // Also, `self.components`, together with `self.length`, define valid `Vec`s for each + // component. unsafe { entities.entities.extend_components( &self.component_map, @@ -162,16 +191,22 @@ where let entity_identifiers = entity_allocator.allocate_batch(Locations::new( self.length..(self.length + component_len), + // SAFETY: `entity_allocator` is guaranteed to not outlive `self`. Therefore, the + // `Location`s being stored in it will also not outlive `self`. unsafe { self.identifier.as_ref() }, )); - let mut entity_identifiers_v = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let mut entity_identifiers_v = ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts that, + // together with `self.length`, create a valid `Vec`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); entity_identifiers_v.extend(entity_identifiers.iter()); self.entity_identifiers = ( entity_identifiers_v.as_mut_ptr(), From f7ec22e7a4833719237269bee9dfebfda7738fb8 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Fri, 6 May 2022 15:06:44 -0700 Subject: [PATCH 33/71] Mark Archetype::view() methods as unsafe and add safety comments. --- src/archetype/mod.rs | 26 ++++++++++++++++++++++++-- src/query/result/iter.rs | 4 ++-- src/query/result/par_iter.rs | 2 +- src/query/view/par/seal/mod.rs | 10 ++++++++++ src/query/view/seal.rs | 10 ++++++++++ 5 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 3f4f42bf..10b38879 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -218,10 +218,21 @@ where entity_identifiers } - pub(crate) fn view<'a, V>(&mut self) -> V::Results + /// # Safety + /// Each component viewed by `V` must also be identified by this archetype's `Identifier`. + pub(crate) unsafe fn view<'a, V>(&mut self) -> V::Results where V: Views<'a>, { + // SAFETY: `self.components` contains the raw parts for `Vec`s of size `self.length`, + // where each `C` is a component for which the entry in `component_map` corresponds to the + // correct index. + // + // `self.entity_identifiers` also contains the raw parts for a valid + // `Vec` of size `self.length`. + // + // Since each component viewed by `V` is also identified by this archetype's `Identifier`, + // `self.component` will contain an entry for every viewed component. unsafe { V::view( &self.components, @@ -232,12 +243,23 @@ where } } + /// # Safety + /// Each component viewed by `V` must also be identified by this archetype's `Identifier`. #[cfg(feature = "parallel")] #[cfg_attr(doc_cfg, doc(cfg(feature = "parallel")))] - pub(crate) fn par_view<'a, V>(&mut self) -> V::ParResults + pub(crate) unsafe fn par_view<'a, V>(&mut self) -> V::ParResults where V: ParViews<'a>, { + // SAFETY: `self.components` contains the raw parts for `Vec`s of size `self.length`, + // where each `C` is a component for which the entry in `component_map` corresponds to the + // correct index. + // + // `self.entity_identifiers` also contains the raw parts for a valid + // `Vec` of size `self.length`. + // + // Since each component viewed by `V` is also identified by this archetype's `Identifier`, + // `self.component` will contain an entry for every viewed component. unsafe { V::par_view( &self.components, diff --git a/src/query/result/iter.rs b/src/query/result/iter.rs index 28a0eef2..271889f3 100644 --- a/src/query/result/iter.rs +++ b/src/query/result/iter.rs @@ -103,7 +103,7 @@ where let archetype = self.archetypes_iter.find(|archetype| unsafe { And::::filter(archetype.identifier().as_slice(), self.component_map) })?; - self.current_results_iter = Some(archetype.view::()); + self.current_results_iter = Some(unsafe { archetype.view::() }); } } @@ -131,7 +131,7 @@ where self.archetypes_iter.fold(init, |acc, archetype| { if unsafe { And::::filter(archetype.identifier().as_slice(), self.component_map) } { - archetype.view::().fold(acc, &mut fold) + unsafe { archetype.view::() }.fold(acc, &mut fold) } else { acc } diff --git a/src/query/result/par_iter.rs b/src/query/result/par_iter.rs index ec599230..8262fd09 100644 --- a/src/query/result/par_iter.rs +++ b/src/query/result/par_iter.rs @@ -221,7 +221,7 @@ where fn consume(self, archetype: &'a mut Archetype) -> Self { if unsafe { And::::filter(archetype.identifier().as_slice(), self.component_map) } { let consumer = self.base.split_off_left(); - let result = archetype.par_view::().drive_unindexed(consumer); + let result = unsafe { archetype.par_view::() }.drive_unindexed(consumer); let previous = match self.previous { None => Some(result), diff --git a/src/query/view/par/seal/mod.rs b/src/query/view/par/seal/mod.rs index df9fba07..9905b688 100644 --- a/src/query/view/par/seal/mod.rs +++ b/src/query/view/par/seal/mod.rs @@ -23,6 +23,16 @@ use repeat::RepeatNone; pub trait ParViewSeal<'a>: View<'a> { type ParResult: IndexedParallelIterator; + /// # Safety + /// Each tuple in `columns` must contain the raw parts for a valid `Vec` of size `length` + /// for components `C`. Each of those components `C` must have an entry in `component_map`, + /// paired with the correct index corresponding to that component's entry in `columns`. + /// + /// `entity_identifiers` must contain the raw parts for a valid `Vec: Claim { type Result: Iterator; + /// # Safety + /// Each tuple in `columns` must contain the raw parts for a valid `Vec` of size `length` + /// for components `C`. Each of those components `C` must have an entry in `component_map`, + /// paired with the correct index corresponding to that component's entry in `columns`. + /// + /// `entity_identifiers` must contain the raw parts for a valid `Vec Date: Fri, 6 May 2022 15:12:58 -0700 Subject: [PATCH 34/71] Safety comments for Archetype::set_component_unchecked() --- src/archetype/mod.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 10b38879..e623d4f2 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -270,10 +270,23 @@ where } } + /// # Safety + /// `C` must be a component type that is contained within this archetype, meaning the + /// archetype's `Identifier` must have the `C` bit set. + /// + /// `index` must be a valid index within this archetype (meaning it must be less than + /// `self.length`). pub(crate) unsafe fn set_component_unchecked(&mut self, index: usize, component: C) where C: Component, { + // SAFETY: `self.component_map` is guaranteed to have an entry for `TypeId::of::()` by + // the safety contract of this method. Additionally, `self.components` is guaranteed to + // have an entry for the index returned from `self.component_map`, and furthermore that + // entry is guaranteed to be the valid raw parts for a `Vec` of length `self.length`. + // + // The slice view over the component column for `C` is also guaranteed by the safety + // contract of this method to have an entry for the given `index`. unsafe { *slice::from_raw_parts_mut( self.components From a5957976904b6dc29bff9a0687cdf010d26ae42f Mon Sep 17 00:00:00 2001 From: Anders429 Date: Fri, 6 May 2022 16:47:12 -0700 Subject: [PATCH 35/71] Safety comments for Archetype::remove_row_unchecked(). --- src/archetype/mod.rs | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index e623d4f2..4f70f246 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -304,24 +304,40 @@ where } } + /// # Safety + /// `entity_allocator` must contain entries for the entities stored in the archetype. This + /// archetype must also be nonempty. pub(crate) unsafe fn remove_row_unchecked( &mut self, index: usize, entity_allocator: &mut entity::Allocator, ) { + // SAFETY: `self.components` contains the same number of bits as are set in + // `self.identifier`. Also, each entry is `self.components` is guaranteed to contain the + // raw parts for a valid `Vec` for each `C` identified by `self.identifier`. Finally, + // `self.identifier` is generic over the same registry `R` as this method is being called + // on. unsafe { R::remove_component_row(index, &self.components, self.length, self.identifier.iter()) }; - let mut entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let mut entity_identifiers = ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `self.length`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); // Update swapped index if this isn't the last row. if index < self.length - 1 { + // SAFETY: `entity_allocator` contains an entry for the entity identifiers stored in + // `entity_identifiers`. + // + // Additionally, `entity_identifiers` is guaranteed to be nonempty. unsafe { entity_allocator.modify_location_index_unchecked( *entity_identifiers.last().unwrap_unchecked(), From 0804a5f5df67188ab6b293ecd44598f0d806f2e4 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Fri, 6 May 2022 23:10:48 -0700 Subject: [PATCH 36/71] Add a constraint on index to the safety contract of Archetype::remove_row_unchecked. --- src/archetype/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 4f70f246..9a7b847f 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -305,8 +305,8 @@ where } /// # Safety - /// `entity_allocator` must contain entries for the entities stored in the archetype. This - /// archetype must also be nonempty. + /// `entity_allocator` must contain entries for the entities stored in the archetype. The + /// `index` must be a valid index to a row in this archetype. pub(crate) unsafe fn remove_row_unchecked( &mut self, index: usize, @@ -337,7 +337,8 @@ where // SAFETY: `entity_allocator` contains an entry for the entity identifiers stored in // `entity_identifiers`. // - // Additionally, `entity_identifiers` is guaranteed to be nonempty. + // Additionally, `entity_identifiers` is guaranteed to be nonempty, because the index + // is not for the last row. unsafe { entity_allocator.modify_location_index_unchecked( *entity_identifiers.last().unwrap_unchecked(), From 6a027135a549b4e6f9dea1550982d63ec1a849ad Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 8 May 2022 17:30:51 -0700 Subject: [PATCH 37/71] Safety comments for Archetype::pop_row_unchecked(). --- src/archetype/mod.rs | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 9a7b847f..142b94ef 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -351,6 +351,9 @@ where self.length -= 1; } + /// # Safety + /// `entity_allocator` must contain entries for the entities stored in the archetype. The + /// `index` must be a valid index to a row in this archetype. pub(crate) unsafe fn pop_row_unchecked( &mut self, index: usize, @@ -358,6 +361,15 @@ where ) -> (entity::Identifier, Vec) { let size_of_components = self.identifier.size_of_components(); let mut bytes = Vec::with_capacity(size_of_components); + // SAFETY: `self.components` has the same number of values as there are set bits in + // `self.identifier`. Also, each element in `self.components` defines a `Vec` of size + // `self.length` for each `C` identified by `self.identifier`. + // + // `bytes` is valid for writes and points to an allocated buffer that is large enough to + // hold all components identified by `self.identiifer`. + // + // Finally, `self.identifier` is generic over the same `R` upon which this function is + // being called. unsafe { R::pop_component_row( index, @@ -367,17 +379,30 @@ where self.identifier.iter(), ); } + // SAFETY: After the previous call to `R::pop_component_row()`, `bytes` will have its + // entire allocation populated with the components, stored as raw bytes. Therefore, these + // bytes have been properly initialized. Additionally, the capacity was previously already + // set to `size_of_components`. unsafe { bytes.set_len(size_of_components) }; - let mut entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let mut entity_identifiers = ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `self.length`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); // Update swapped index if this isn't the last row. if index < self.length - 1 { + // SAFETY: `entity_allocator` contains an entry for the entity identifiers stored in + // `entity_identifiers`. + // + // Additionally, `entity_identifiers` is guaranteed to be nonempty, because the index + // is not for the last row. unsafe { entity_allocator.modify_location_index_unchecked( *entity_identifiers.last().unwrap_unchecked(), From 2dce2191b50ecdbe742c7ab59470b07a079a28ec Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 8 May 2022 17:42:14 -0700 Subject: [PATCH 38/71] Safety comments for Archetype::push_from_buffer_and_component(). --- src/archetype/mod.rs | 39 ++++++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 142b94ef..5df1cd19 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -417,6 +417,12 @@ where (entity_identifier, bytes) } + /// # Safety + /// `buffer` must be valid for reads and be an allocated buffer of packed, properly initialized + /// components corresponding to the components identified by this archetype's `identifier` + /// field. + /// + /// The registry `R` over which this archetype is generic must contain no duplicate components. pub(crate) unsafe fn push_from_buffer_and_component( &mut self, entity_identifier: entity::Identifier, @@ -426,6 +432,21 @@ where where C: Component, { + // SAFETY: `self.components` has the same number of values as there are set bits in + // `self.identifier`. Also, each element in `self.components` defines a `Vec` of size + // `self.length` for each `C` identified by `self.identifier`. + // + // `buffer` is valid for reads and is an allocated buffer of packed properly initialized + // components corresponding to the components identified by `self.identifier`, as is + // guaranteed by the safety contract of this method. + // + // The `MaybeUninit` provided here is properly initialized. + // + // `R` contains no duplicate components, as is guaranteed by the safety contract of this + // method. + // + // The `R` over which `self.identifier` is generic is the same `R` on which this function + // is being called. unsafe { R::push_components_from_buffer_and_component( buffer, @@ -436,13 +457,17 @@ where ); } - let mut entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let mut entity_identifiers = ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `self.length`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); entity_identifiers.push(entity_identifier); self.entity_identifiers = ( entity_identifiers.as_mut_ptr(), From 0b1586ba31e1668ba6f7f41d25195b33b6fd88f7 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 8 May 2022 17:47:50 -0700 Subject: [PATCH 39/71] Safety comments for Archetype::push_components_from_buffer_skipping_component(). --- src/archetype/mod.rs | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 5df1cd19..95a8acdf 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -421,7 +421,7 @@ where /// `buffer` must be valid for reads and be an allocated buffer of packed, properly initialized /// components corresponding to the components identified by this archetype's `identifier` /// field. - /// + /// /// The registry `R` over which this archetype is generic must contain no duplicate components. pub(crate) unsafe fn push_from_buffer_and_component( &mut self, @@ -446,7 +446,7 @@ where // method. // // The `R` over which `self.identifier` is generic is the same `R` on which this function - // is being called. + // is being called. unsafe { R::push_components_from_buffer_and_component( buffer, @@ -479,6 +479,12 @@ where self.length - 1 } + /// # Safety + /// `buffer` must be valid for reads and be an allocated buffer of packed, properly initialized + /// components corresponding to the components identified by this archetype's `identifier` + /// field, skipping the component `C`. + /// + /// The registry `R` over which this archetype is generic must contain no duplicate components. pub(crate) unsafe fn push_from_buffer_skipping_component( &mut self, entity_identifier: entity::Identifier, @@ -487,6 +493,19 @@ where where C: Component, { + // SAFETY: `self.components` has the same number of values as there are set bits in + // `self.identifier`. Also, each element in `self.components` defines a `Vec` of size + // `self.length` for each `C` identified by `self.identifier`. + // + // `buffer` is valid for reads and is an allocated buffer of packed properly initialized + // components corresponding to the components identified by `self.identifier`, skipping + // the component `C`, as is guaranteed by the safety contract of this method. + // + // `R` contains no duplicate components, as is guaranteed by the safety contract of this + // method. + // + // The `R` over which `self.identifier` is generic is the same `R` on which this function + // is being called. unsafe { R::push_components_from_buffer_skipping_component( buffer, @@ -497,13 +516,17 @@ where ); } - let mut entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let mut entity_identifiers = ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `self.length`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); entity_identifiers.push(entity_identifier); self.entity_identifiers = ( entity_identifiers.as_mut_ptr(), From 7995efdaa3943cd0ba83e23d9a71cc4cf4e237f2 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 8 May 2022 17:51:27 -0700 Subject: [PATCH 40/71] Safety comments for Archetype::clear(). --- src/archetype/mod.rs | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 95a8acdf..cbda99bc 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -538,19 +538,34 @@ where self.length - 1 } + /// # Safety + /// `entity_allocator` must contain entries for the entities stored in the archetype. The + /// `index` must be a valid index to a row in this archetype. pub(crate) unsafe fn clear(&mut self, entity_allocator: &mut entity::Allocator) { // Clear each column. + // SAFETY: `self.components` has the same number of values as there are set bits in + // `self.identifier`. Also, each element in `self.components` defines a `Vec` of size + // `self.length` for each `C` identified by `self.identifier`. + // + // The `R` over which `self.identifier` is generic is the same `R` on which this function + // is being called. unsafe { R::clear_components(&mut self.components, self.length, self.identifier.iter()) }; // Free each entity. - let mut entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + let mut entity_identifiers = ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `self.length`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); for entity_identifier in entity_identifiers.iter() { + // SAFETY: `entity_allocator` is guaranteed by the safety contract of this method to + // contain `entity_identifier`. unsafe { entity_allocator.free_unchecked(*entity_identifier) }; } entity_identifiers.clear(); From 4756525382014cf94cbe13a9f4112a52d0d0bd06 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 8 May 2022 17:52:04 -0700 Subject: [PATCH 41/71] Safety comments for Archetype::entity_identifiers(). --- src/archetype/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index cbda99bc..b68c6590 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -584,6 +584,8 @@ where #[cfg(feature = "serde")] #[cfg_attr(doc_cfg, doc(cfg(feature = "serde")))] pub(crate) fn entity_identifiers(&self) -> impl Iterator { + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `self.length`. unsafe { slice::from_raw_parts(self.entity_identifiers.0, self.length) }.iter() } From 3ab2c81087373cf86e9d56c98ab8ca9daa823140 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sun, 8 May 2022 17:53:07 -0700 Subject: [PATCH 42/71] Move misplaced semicolon. --- src/archetype/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index b68c6590..6d74583a 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -318,8 +318,8 @@ where // `self.identifier` is generic over the same registry `R` as this method is being called // on. unsafe { - R::remove_component_row(index, &self.components, self.length, self.identifier.iter()) - }; + R::remove_component_row(index, &self.components, self.length, self.identifier.iter()); + } let mut entity_identifiers = ManuallyDrop::new( // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid From 02726e34f7a82b36e01ba4a07ec15520e481ee40 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 10 May 2022 23:21:24 -0700 Subject: [PATCH 43/71] Don't borrow IdentifierRef in method. --- src/archetype/identifier/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/archetype/identifier/mod.rs b/src/archetype/identifier/mod.rs index 24ebea2a..6cf907c9 100644 --- a/src/archetype/identifier/mod.rs +++ b/src/archetype/identifier/mod.rs @@ -235,7 +235,7 @@ where /// The caller must ensure the referenced `Identifier` outlives the returned [`Iter`]. /// /// [`Iter`]: crate::archetype::identifier::Iter - pub(crate) unsafe fn iter(&self) -> Iter { + pub(crate) unsafe fn iter(self) -> Iter { // SAFETY: `self.pointer` will be valid as long as the returned `Iter` exists, assuming the // caller ensures the `Identifier` outlives it. unsafe { Iter::::new(self.pointer) } From 2a5790d8648bbf9106214b18098d14e53bbffb32 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 10 May 2022 23:29:07 -0700 Subject: [PATCH 44/71] Safety comments for Archetype Eq impls. --- src/archetype/impl_eq.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/archetype/impl_eq.rs b/src/archetype/impl_eq.rs index 86d15007..e8bcacf8 100644 --- a/src/archetype/impl_eq.rs +++ b/src/archetype/impl_eq.rs @@ -12,20 +12,36 @@ where fn eq(&self, other: &Self) -> bool { self.length == other.length && self.identifier == other.identifier - && ManuallyDrop::new(unsafe { + && ManuallyDrop::new( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a + // valid `Vec` of size `self.length`. + unsafe { Vec::from_raw_parts( self.entity_identifiers.0, self.length, self.entity_identifiers.1, ) - }) == ManuallyDrop::new(unsafe { + }) == ManuallyDrop::new( + // SAFETY: `other.entity_identifiers` is guaranteed to contain the raw parts for a + // valid `Vec` of size `other.length`. + unsafe { Vec::from_raw_parts( other.entity_identifiers.0, other.length, other.entity_identifiers.1, ) }) - && unsafe { + && + // SAFETY: Since `self.identifier` is equal to `other.identifier`, the components Vecs + // will contain the same number of values as there are bits in `self.identifier`. + // + // `self.components` and `other.components` both contain raw parts for valid `Vec`s + // for each identified component `C` of size `self.length` (since `self.length` and + // `other.length` are equal). + // + // `self.identifier` is generic over the same `R` upon which this function is being + // called. + unsafe { R::component_eq( &self.components, &other.components, From 32b75c68db4b97ef8dfe757473996b0c23487cd8 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 10 May 2022 23:42:29 -0700 Subject: [PATCH 45/71] Safety comments for Archetype Drop impl. --- src/archetype/impl_drop.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/archetype/impl_drop.rs b/src/archetype/impl_drop.rs index adb78db2..26d2fe0f 100644 --- a/src/archetype/impl_drop.rs +++ b/src/archetype/impl_drop.rs @@ -8,15 +8,22 @@ where { #[inline] fn drop(&mut self) { + // SAFETY: `self.components` contains the raw parts making valid `Vec`s of size + // `self.length` for each `C` identified by `self.identifier`. Also, `self.identifier` is + // generic over the same `R` upon which this function is called. unsafe { R::free_components(&self.components, self.length, self.identifier.iter()); } - drop(unsafe { - Vec::from_raw_parts( - self.entity_identifiers.0, - self.length, - self.entity_identifiers.1, - ) - }); + drop( + // SAFETY: `self.entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `self.length`. + unsafe { + Vec::from_raw_parts( + self.entity_identifiers.0, + self.length, + self.entity_identifiers.1, + ) + }, + ); } } From 835e5186328340fae6f32e50b9e2c802d967ecd8 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Sat, 14 May 2022 15:22:31 -0700 Subject: [PATCH 46/71] Safety comments for query filters. --- src/archetype/identifier/mod.rs | 2 +- src/query/filter/seal.rs | 138 +++++++++++++++++++++++++++----- src/query/result/iter.rs | 5 +- src/query/result/par_iter.rs | 2 +- 4 files changed, 122 insertions(+), 25 deletions(-) diff --git a/src/archetype/identifier/mod.rs b/src/archetype/identifier/mod.rs index 6cf907c9..5157b59a 100644 --- a/src/archetype/identifier/mod.rs +++ b/src/archetype/identifier/mod.rs @@ -188,7 +188,7 @@ where /// [`Archetypes`]: crate::archetypes::Archetypes /// [`entity::Allocator`]: crate::entity::allocator::Allocator /// [`Identifier`]: crate::archetype::identifier::Identifier -pub(crate) struct IdentifierRef +pub struct IdentifierRef where R: Registry, { diff --git a/src/query/filter/seal.rs b/src/query/filter/seal.rs index 09a8e09d..71eea58e 100644 --- a/src/query/filter/seal.rs +++ b/src/query/filter/seal.rs @@ -1,4 +1,5 @@ use crate::{ + archetype, component::Component, entity, query::{ @@ -6,16 +7,33 @@ use crate::{ view, view::{View, Views}, }, + registry::Registry, }; use core::any::TypeId; use hashbrown::HashMap; pub trait Seal { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool; + /// # Safety + /// `component_map` must contain an entry for the `TypeId` of each component `C` in the + /// registry `R` corresponding to the index of that component in the archetype identifier. + /// + /// Note that the component(s) being viewed do not necessarily need to be in the registry `R`. + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry; } impl Seal for None { - unsafe fn filter(_key: &[u8], _component_map: &HashMap) -> bool { + unsafe fn filter( + _identifier: archetype::IdentifierRef, + _component_map: &HashMap, + ) -> bool + where + R: Registry, + { true } } @@ -24,9 +42,17 @@ impl Seal for Has where C: Component, { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry, + { match component_map.get(&TypeId::of::()) { - Some(index) => unsafe { key.get_unchecked(index / 8) & (1 << (index % 8)) != 0 }, + Some(&index) => + // SAFETY: `index` is guaranteed to correspond to a valid component in `identifier`. + unsafe { identifier.get_unchecked(index) }, Option::None => false, } } @@ -36,8 +62,16 @@ impl Seal for Not where F: Filter, { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - unsafe { !F::filter(key, component_map) } + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry, + { + // SAFETY: The safety guarantees for this call are already upheld by the safety contract + // of this method. + unsafe { !F::filter(identifier, component_map) } } } @@ -46,8 +80,16 @@ where F1: Filter, F2: Filter, { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - unsafe { F1::filter(key, component_map) && F2::filter(key, component_map) } + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry, + { + // SAFETY: The safety guarantees for these calls are already upheld by the safety contract + // of this method. + unsafe { F1::filter(identifier, component_map) && F2::filter(identifier, component_map) } } } @@ -56,8 +98,16 @@ where F1: Filter, F2: Filter, { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - unsafe { F1::filter(key, component_map) || F2::filter(key, component_map) } + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry, + { + // SAFETY: The safety guarantees for these calls are already upheld by the safety contract + // of this method. + unsafe { F1::filter(identifier, component_map) || F2::filter(identifier, component_map) } } } @@ -65,8 +115,16 @@ impl Seal for &C where C: Component, { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - unsafe { Has::::filter(key, component_map) } + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry, + { + // SAFETY: The safety guarantees for this call are already upheld by the safety contract + // of this method. + unsafe { Has::::filter(identifier, component_map) } } } @@ -74,8 +132,16 @@ impl Seal for &mut C where C: Component, { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - unsafe { Has::::filter(key, component_map) } + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry, + { + // SAFETY: The safety guarantees for this call are already upheld by the safety contract + // of this method. + unsafe { Has::::filter(identifier, component_map) } } } @@ -83,7 +149,13 @@ impl Seal for Option<&C> where C: Component, { - unsafe fn filter(_key: &[u8], _component_map: &HashMap) -> bool { + unsafe fn filter( + _identifier: archetype::IdentifierRef, + _component_map: &HashMap, + ) -> bool + where + R: Registry, + { true } } @@ -92,19 +164,37 @@ impl Seal for Option<&mut C> where C: Component, { - unsafe fn filter(_key: &[u8], _component_map: &HashMap) -> bool { + unsafe fn filter( + _identifier: archetype::IdentifierRef, + _component_map: &HashMap, + ) -> bool + where + R: Registry, + { true } } impl Seal for entity::Identifier { - unsafe fn filter(_key: &[u8], _component_map: &HashMap) -> bool { + unsafe fn filter( + _identifier: archetype::IdentifierRef, + _component_map: &HashMap, + ) -> bool + where + R: Registry, + { true } } impl Seal for view::Null { - unsafe fn filter(_key: &[u8], _component_map: &HashMap) -> bool { + unsafe fn filter( + _identifier: archetype::IdentifierRef, + _component_map: &HashMap, + ) -> bool + where + R: Registry, + { true } } @@ -114,7 +204,15 @@ where V: View<'a>, W: Views<'a>, { - unsafe fn filter(key: &[u8], component_map: &HashMap) -> bool { - unsafe { And::::filter(key, component_map) } + unsafe fn filter( + identifier: archetype::IdentifierRef, + component_map: &HashMap, + ) -> bool + where + R: Registry, + { + // SAFETY: The safety guarantees for this call are already upheld by the safety contract + // of this method. + unsafe { And::::filter(identifier, component_map) } } } diff --git a/src/query/result/iter.rs b/src/query/result/iter.rs index 271889f3..ee17b96b 100644 --- a/src/query/result/iter.rs +++ b/src/query/result/iter.rs @@ -101,7 +101,7 @@ where } } let archetype = self.archetypes_iter.find(|archetype| unsafe { - And::::filter(archetype.identifier().as_slice(), self.component_map) + And::::filter(archetype.identifier(), self.component_map) })?; self.current_results_iter = Some(unsafe { archetype.view::() }); } @@ -129,8 +129,7 @@ where } self.archetypes_iter.fold(init, |acc, archetype| { - if unsafe { And::::filter(archetype.identifier().as_slice(), self.component_map) } - { + if unsafe { And::::filter(archetype.identifier(), self.component_map) } { unsafe { archetype.view::() }.fold(acc, &mut fold) } else { acc diff --git a/src/query/result/par_iter.rs b/src/query/result/par_iter.rs index 8262fd09..d7ddf0d9 100644 --- a/src/query/result/par_iter.rs +++ b/src/query/result/par_iter.rs @@ -219,7 +219,7 @@ where type Result = C::Result; fn consume(self, archetype: &'a mut Archetype) -> Self { - if unsafe { And::::filter(archetype.identifier().as_slice(), self.component_map) } { + if unsafe { And::::filter(archetype.identifier(), self.component_map) } { let consumer = self.base.split_off_left(); let result = unsafe { archetype.par_view::() }.drive_unindexed(consumer); From deb59cdd67ab6984ac435019078712af0ad66a64 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 15:06:12 -0700 Subject: [PATCH 47/71] Safety comments for Result iterator. --- src/query/result/iter.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/query/result/iter.rs b/src/query/result/iter.rs index ee17b96b..c59b6bd9 100644 --- a/src/query/result/iter.rs +++ b/src/query/result/iter.rs @@ -100,10 +100,17 @@ where return result; } } - let archetype = self.archetypes_iter.find(|archetype| unsafe { - And::::filter(archetype.identifier(), self.component_map) + let archetype = self.archetypes_iter.find(|archetype| { + // SAFETY: `self.component_map` contains an entry for each `TypeId` per + // component `C` in the registry `R`. + unsafe { And::::filter(archetype.identifier(), self.component_map) } })?; - self.current_results_iter = Some(unsafe { archetype.view::() }); + self.current_results_iter = Some( + // SAFETY: Each component viewed by `V` is guaranteed to be within the `archetype`, + // since the archetype was not removed by the `find()` method above which filters + // out archetypes that do not contain the viewed components. + unsafe { archetype.view::() }, + ); } } @@ -129,7 +136,12 @@ where } self.archetypes_iter.fold(init, |acc, archetype| { - if unsafe { And::::filter(archetype.identifier(), self.component_map) } { + if + // SAFETY: `self.component_map` contains an entry for each `TypeId` per component + // `C` in the registry `R`. + unsafe { And::::filter(archetype.identifier(), self.component_map) } { + // SAFETY: Each component viewed by `V` is guaranteed to be within the `archetype` + // since the `filter` function in the if-statement returned `true`. unsafe { archetype.view::() }.fold(acc, &mut fold) } else { acc From c7f5393658e43d8d0496407caea93bd2c0187f0d Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 15:16:21 -0700 Subject: [PATCH 48/71] Safety comments for View impls. --- src/query/view/seal.rs | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/query/view/seal.rs b/src/query/view/seal.rs index 8c194e8b..ca57bd69 100644 --- a/src/query/view/seal.rs +++ b/src/query/view/seal.rs @@ -23,7 +23,9 @@ pub trait ViewSeal<'a>: Claim { /// size `length`. /// /// `component_map` must contain an entry for every component `C` that is viewed by this - /// `View`. + /// `View`, and that entry must contain the index for the column of type `C` in `columns`. Note + /// that it is not required for optionally viewed components to be contained in the + /// `component_map`. unsafe fn view( columns: &[(*mut u8, usize)], entity_identifiers: (*mut entity::Identifier, usize), @@ -46,6 +48,10 @@ where length: usize, component_map: &HashMap, ) -> Self::Result { + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of size + // `length`. Since `component_map` contains an entry for the given component `C`'s entry in + // `columns`, then the column obtained here can be interpreted as a slice of type `C` of + // size `length`. unsafe { slice::from_raw_parts::<'a, C>( columns @@ -75,6 +81,10 @@ where length: usize, component_map: &HashMap, ) -> Self::Result { + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of size + // `length`. Since `component_map` contains an entry for the given component `C`'s entry in + // `columns`, then the column obtained here can be interpreted as a slice of type `C` of + // size `length`. unsafe { slice::from_raw_parts_mut::<'a, C>( columns @@ -114,6 +124,10 @@ where ) -> Self::Result { match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of + // size `length`. Since `component_map` contains an entry for the given component + // `C`'s entry in `columns`, then the column obtained here can be interpreted as a + // slice of type `C` of size `length`. unsafe { slice::from_raw_parts(columns.get_unchecked(*index).0.cast::(), length) } @@ -150,6 +164,10 @@ where match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of + // size `length`. Since `component_map` contains an entry for the given component + // `C`'s entry in `columns`, then the column obtained here can be interpreted as a + // slice of type `C` of size `length`. unsafe { slice::from_raw_parts_mut(columns.get_unchecked(*index).0.cast::(), length) } @@ -174,6 +192,8 @@ impl<'a> ViewSeal<'a> for entity::Identifier { length: usize, _component_map: &HashMap, ) -> Self::Result { + // SAFETY: `entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `length`. unsafe { slice::from_raw_parts_mut::<'a, Self>(entity_identifiers.0, length) } .iter() .copied() @@ -185,6 +205,18 @@ impl<'a> ViewSeal<'a> for entity::Identifier { pub trait ViewsSeal<'a>: Claim { type Results: Iterator; + /// # Safety + /// Each tuple in `columns` must contain the raw parts for a valid `Vec` of size `length` + /// for components `C`. Each of those components `C` must have an entry in `component_map`, + /// paired with the correct index corresponding to that component's entry in `columns`. + /// + /// `entity_identifiers` must contain the raw parts for a valid `Vec, ) -> Self::Results { + // SAFETY: The safety guarantees of this method are the exact what are required by the + // safety guarantees of both `V::view()` and `W::view()`. unsafe { V::view(columns, entity_identifiers, length, component_map).zip(W::view( columns, From 8f1916aa640bd505a887129ebed1708005454148 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 15:28:31 -0700 Subject: [PATCH 49/71] Safety comments for World. --- src/archetype/mod.rs | 3 +-- src/world/mod.rs | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index 6d74583a..e065bc5a 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -539,8 +539,7 @@ where } /// # Safety - /// `entity_allocator` must contain entries for the entities stored in the archetype. The - /// `index` must be a valid index to a row in this archetype. + /// `entity_allocator` must contain entries for the entities stored in the archetype. pub(crate) unsafe fn clear(&mut self, entity_allocator: &mut entity::Allocator) { // Clear each column. // SAFETY: `self.components` has the same number of values as there are set bits in diff --git a/src/world/mod.rs b/src/world/mod.rs index b1cc8fb7..62481dea 100644 --- a/src/world/mod.rs +++ b/src/world/mod.rs @@ -164,6 +164,11 @@ where // bits correspond to each component in the registry `R`. let identifier_buffer = unsafe { archetype::Identifier::new(identifier) }; + // SAFETY: Since the archetype was obtained using the `identifier_buffer` created from the + // entity `E`, then the entity is guaranteed to be made up of componpents identified by the + // archetype's identifier. + // + // `self.entity_allocator` is guaranteed to live as long as the archetype. unsafe { self.archetypes .get_mut_or_insert_new(identifier_buffer) @@ -207,6 +212,11 @@ where // bits correspond to each component in the registry `R`. let identifier_buffer = unsafe { archetype::Identifier::new(identifier) }; + // SAFETY: Since the archetype was obtained using the `identifier_buffer` created from the + // entities `E`, then the entities are guaranteed to be made up of componpents identified + // by the archetype's identifier. + // + // `self.entity_allocator` is guaranteed to live as long as the archetype. unsafe { self.archetypes .get_mut_or_insert_new(identifier_buffer) @@ -599,12 +609,17 @@ where // Get location of entity. if let Some(location) = self.entity_allocator.get(entity_identifier) { // Remove row from Archetype. + // SAFETY: `self.entity_allocator` contains entries for the entities stored in this + // world's archetypes. Also, `location.index` is invariantly guaranteed to be a valid + // index in the archetype. unsafe { self.archetypes .get_unchecked_mut(location.identifier) .remove_row_unchecked(location.index, &mut self.entity_allocator); } // Free slot in entity allocator. + // SAFETY: It was verified above that `self.entity_allocator` contains a valid slot for + // `entity_identifier`. unsafe { self.entity_allocator.free_unchecked(entity_identifier); } @@ -632,6 +647,8 @@ where /// world.clear(); /// ``` pub fn clear(&mut self) { + // SAFETY: `self.entity_allocator` contains entries for the entities stored in this world's + // archetypes. unsafe { self.archetypes.clear(&mut self.entity_allocator); } From cf2a0d15d0208201c5cfa2e44f1d2a040f8e8410 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 15:29:47 -0700 Subject: [PATCH 50/71] Remove unused lifetime identifier. --- src/query/claim.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/query/claim.rs b/src/query/claim.rs index f3f81a96..812ba070 100644 --- a/src/query/claim.rs +++ b/src/query/claim.rs @@ -50,7 +50,7 @@ impl Claim for view::Null { fn claim(_mutable_claims: &mut HashSet, _immutable_claims: &mut HashSet) {} } -impl<'a, V, W> Claim for (V, W) +impl Claim for (V, W) where V: Claim, W: Claim, From e979a6497a7fc38bbc956b91923e5b7365a5445b Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 15:56:27 -0700 Subject: [PATCH 51/71] Safety comments for Entry. --- src/archetype/mod.rs | 6 ++-- src/world/entry.rs | 68 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 65 insertions(+), 9 deletions(-) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index e065bc5a..cdc9e96f 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -482,7 +482,7 @@ where /// # Safety /// `buffer` must be valid for reads and be an allocated buffer of packed, properly initialized /// components corresponding to the components identified by this archetype's `identifier` - /// field, skipping the component `C`. + /// field, also including the component `C`. /// /// The registry `R` over which this archetype is generic must contain no duplicate components. pub(crate) unsafe fn push_from_buffer_skipping_component( @@ -498,8 +498,8 @@ where // `self.length` for each `C` identified by `self.identifier`. // // `buffer` is valid for reads and is an allocated buffer of packed properly initialized - // components corresponding to the components identified by `self.identifier`, skipping - // the component `C`, as is guaranteed by the safety contract of this method. + // components corresponding to the components identified by `self.identifier`, also + // including the component `C`, as is guaranteed by the safety contract of this method. // // `R` contains no duplicate components, as is guaranteed by the safety contract of this // method. diff --git a/src/world/entry.rs b/src/world/entry.rs index dc483338..b808bb84 100644 --- a/src/world/entry.rs +++ b/src/world/entry.rs @@ -71,8 +71,17 @@ where C: Component, { let component_index = *self.world.component_map.get(&TypeId::of::()).unwrap(); - if unsafe { self.location.identifier.get_unchecked(component_index) } { + if + // SAFETY: The `component_index` obtained from `self.world.component_map` is guaranteed to + // be a valid index into `self.location.identifier`. + unsafe { self.location.identifier.get_unchecked(component_index) } { // The component already exists within this entity. Replace it. + // SAFETY: An archetype with this identifier is guaranteed to exist, since there is an + // allocated location for it in the entity allocator. + // + // `C` is verified by the above if-statement to be contained within the identified + // archetype. Also, `self.location.index` is invariantly guaranteed to be a valid index + // within the archetype. unsafe { self.world .archetypes @@ -81,7 +90,15 @@ where } } else { // The component needs to be added to the entity. - let (entity_identifier, current_component_bytes) = unsafe { + let (entity_identifier, current_component_bytes) = + // SAFETY: An archetype with this identifier is guaranteed to exist, since there is an + // allocated location for it in the entity allocator. + // + // `self.world.entity_allocator` contains entries for entities stored in + // `self.world.archetypes`. As such, `self.location.index` is guaranteed to be a + // valid index to a row within this archetype, since they share the same archetype + // identifier. + unsafe { self.world .archetypes .get_unchecked_mut(self.location.identifier) @@ -90,9 +107,13 @@ where // Create new identifier buffer. let mut raw_identifier_buffer = self.location.identifier.as_vec(); // Set the component's bit. + // SAFETY: `component_index` is guaranteed to be a valid index to a bit in + // `raw_identifier_buffer`. *unsafe { raw_identifier_buffer.get_unchecked_mut(component_index / 8) } |= 1 << (component_index % 8); let identifier_buffer = + // SAFETY: Since `raw_identifier_buffer` was obtained from a valid identifier, it + // is of the proper length (which is `(R::LEN + 7) / 8`). unsafe { archetype::Identifier::::new(raw_identifier_buffer) }; // Insert to the corresponding archetype using the bytes and the new component. @@ -100,7 +121,15 @@ where .world .archetypes .get_mut_or_insert_new(identifier_buffer); - let index = unsafe { + let index = + // SAFETY: `current_component_bytes` is guaranteed to be an allcoated buffer of + // packed, properly initialized components that were contained in the old + // archetype's row, corresponding to the components identified by the archetype's + // identifier. + // + // Also, the registry `R` is invariantly guaranteed by the invariants in `World` to + // not contain any duplicates. + unsafe { archetype.push_from_buffer_and_component( entity_identifier, current_component_bytes.as_ptr(), @@ -109,6 +138,8 @@ where }; // Update the location. + // SAFETY: `entity_identifier` is guaranteed at creation of this `Entry` to be + // contained in `self.world.entity_allocator`. unsafe { self.world.entity_allocator.modify_location_unchecked( entity_identifier, @@ -145,9 +176,20 @@ where C: Component, { let component_index = *self.world.component_map.get(&TypeId::of::()).unwrap(); - if unsafe { self.location.identifier.get_unchecked(component_index) } { + if + // SAFETY: The `component_index` obtained from `self.world.component_map` is guaranteed to + // be a valid index into `self.location.identifier`. + unsafe { self.location.identifier.get_unchecked(component_index) } { // The component exists and needs to be removed. - let (entity_identifier, current_component_bytes) = unsafe { + let (entity_identifier, current_component_bytes) = + // SAFETY: An archetype with this identifier is guaranteed to exist, since there is an + // allocated location for it in the entity allocator. + // + // `self.world.entity_allocator` contains entries for entities stored in + // `self.world.archetypes`. As such, `self.location.index` is guaranteed to be a + // valid index to a row within this archetype, since they share the same archetype + // identifier. + unsafe { self.world .archetypes .get_unchecked_mut(self.location.identifier) @@ -156,9 +198,13 @@ where // Create new identifier buffer. let mut raw_identifier_buffer = self.location.identifier.as_vec(); // Unset the component's bit. + // SAFETY: `component_index` is guaranteed to be a valid index to a bit in + // `raw_identifier_buffer`. *unsafe { raw_identifier_buffer.get_unchecked_mut(component_index / 8) } ^= 1 << (component_index % 8); let identifier_buffer = + // SAFETY: Since `raw_identifier_buffer` was obtained from a valid identifier, it + // is of the proper length (which is `(R::LEN + 7) / 8`). unsafe { archetype::Identifier::::new(raw_identifier_buffer) }; // Insert to the corresponding archetype using the bytes, skipping the removed @@ -167,7 +213,15 @@ where .world .archetypes .get_mut_or_insert_new(identifier_buffer); - let index = unsafe { + let index = + // SAFETY: `current_component_bytes` is guaranteed to be an allcoated buffer of + // packed, properly initialized components that were contained in the old + // archetype's row, corresponding to the components identified by the archetype's + // identifier. This includes the component `C` which is being removed. + // + // Also, the registry `R` is invariantly guaranteed by the invariants in `World` to + // not contain any duplicates. + unsafe { archetype.push_from_buffer_skipping_component::( entity_identifier, current_component_bytes.as_ptr(), @@ -175,6 +229,8 @@ where }; // Update the location. + // SAFETY: `entity_identifier` is guaranteed at creation of this `Entry` to be + // contained in `self.world.entity_allocator`. unsafe { self.world.entity_allocator.modify_location_unchecked( entity_identifier, From e296087a001e94c81d4dc5fb58f41a6ec5d3fb66 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 16:04:38 -0700 Subject: [PATCH 52/71] Safety comments for ParResult iterator. --- src/query/result/par_iter.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/query/result/par_iter.rs b/src/query/result/par_iter.rs index d7ddf0d9..da2939c4 100644 --- a/src/query/result/par_iter.rs +++ b/src/query/result/par_iter.rs @@ -219,9 +219,15 @@ where type Result = C::Result; fn consume(self, archetype: &'a mut Archetype) -> Self { - if unsafe { And::::filter(archetype.identifier(), self.component_map) } { + if + // SAFETY: `self.component_map` contains an entry for each `TypeId` per component `C` in + // the registry `R`. + unsafe { And::::filter(archetype.identifier(), self.component_map) } { let consumer = self.base.split_off_left(); - let result = unsafe { archetype.par_view::() }.drive_unindexed(consumer); + let result = + // SAFETY: Each component viewed by `V` is guaranteed to be within the `archetype` + // since the `filter` function in the if-statement returned `true`. + unsafe { archetype.par_view::() }.drive_unindexed(consumer); let previous = match self.previous { None => Some(result), From 6050719ace532ab3716636eae35788661647d8d8 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 16:08:34 -0700 Subject: [PATCH 53/71] Safety comments for ParView impls. --- src/query/view/par/seal/mod.rs | 36 +++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/query/view/par/seal/mod.rs b/src/query/view/par/seal/mod.rs index 9905b688..86984bcd 100644 --- a/src/query/view/par/seal/mod.rs +++ b/src/query/view/par/seal/mod.rs @@ -32,7 +32,9 @@ pub trait ParViewSeal<'a>: View<'a> { /// size `length`. /// /// `component_map` must contain an entry for every component `C` that is viewed by this - /// `ParView`. + /// `ParView`, and that entry must contain the index for the column of type `C` in `columns`. + /// Note that it is not required for optionally viewed components to be contained in the + /// `component_map`. unsafe fn par_view( columns: &[(*mut u8, usize)], entity_identifiers: (*mut entity::Identifier, usize), @@ -53,6 +55,10 @@ where length: usize, component_map: &HashMap, ) -> Self::ParResult { + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of size + // `length`. Since `component_map` contains an entry for the given component `C`'s entry in + // `columns`, then the column obtained here can be interpreted as a slice of type `C` of + // size `length`. unsafe { core::slice::from_raw_parts::<'a, C>( columns @@ -78,6 +84,10 @@ where length: usize, component_map: &HashMap, ) -> Self::ParResult { + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of size + // `length`. Since `component_map` contains an entry for the given component `C`'s entry in + // `columns`, then the column obtained here can be interpreted as a slice of type `C` of + // size `length`. unsafe { core::slice::from_raw_parts_mut::<'a, C>( columns @@ -113,6 +123,10 @@ where ) -> Self::ParResult { match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of + // size `length`. Since `component_map` contains an entry for the given component + // `C`'s entry in `columns`, then the column obtained here can be interpreted as a + // slice of type `C` of size `length`. unsafe { core::slice::from_raw_parts(columns.get_unchecked(*index).0.cast::(), length) } @@ -141,6 +155,10 @@ where ) -> Self::ParResult { match component_map.get(&TypeId::of::()) { Some(index) => Either::Right( + // SAFETY: `columns` is guaranteed to contain raw parts for a valid `Vec` of + // size `length`. Since `component_map` contains an entry for the given component + // `C`'s entry in `columns`, then the column obtained here can be interpreted as a + // slice of type `C` of size `length`. unsafe { core::slice::from_raw_parts_mut( columns.get_unchecked(*index).0.cast::(), @@ -164,6 +182,8 @@ impl<'a> ParViewSeal<'a> for entity::Identifier { length: usize, _component_map: &HashMap, ) -> Self::ParResult { + // SAFETY: `entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of size `length`. unsafe { core::slice::from_raw_parts_mut::<'a, Self>(entity_identifiers.0, length) } .par_iter() .cloned() @@ -173,6 +193,18 @@ impl<'a> ParViewSeal<'a> for entity::Identifier { pub trait ParViewsSeal<'a>: Views<'a> { type ParResults: IndexedParallelIterator; + /// # Safety + /// Each tuple in `columns` must contain the raw parts for a valid `Vec` of size `length` + /// for components `C`. Each of those components `C` must have an entry in `component_map`, + /// paired with the correct index corresponding to that component's entry in `columns`. + /// + /// `entity_identifiers` must contain the raw parts for a valid `Vec, ) -> Self::ParResults { + // SAFETY: The safety guarantees of this method are the exact what are required by the + // safety guarantees of both `V::par_view()` and `W::par_view()`. unsafe { V::par_view(columns, entity_identifiers, length, component_map).zip(W::par_view( columns, From aee700d5a048d365bf0eac4e829a6724fae9e516 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 16:18:01 -0700 Subject: [PATCH 54/71] Soundness improvements in Schedule tasks. --- src/system/schedule/stage/seal.rs | 13 +++++++++++-- src/system/schedule/task.rs | 8 +++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/system/schedule/stage/seal.rs b/src/system/schedule/stage/seal.rs index f8b7ceeb..a27db962 100644 --- a/src/system/schedule/stage/seal.rs +++ b/src/system/schedule/stage/seal.rs @@ -7,6 +7,7 @@ use crate::{ }, ParSystem, System, }, + world::World, }; pub trait Seal<'a>: Send { @@ -143,11 +144,19 @@ where { match &mut self.0 { Stage::Start(task) => { - task.flush(world); + task.flush( + // SAFETY: This is guaranteed to be the only reference to this `World`, + // meaning this cast to a mutable reference is sound. + unsafe { &mut *(world.0 as *const World as *mut World) }, + ); } Stage::Continue(task) => { self.1.flush(world); - task.flush(world); + task.flush( + // SAFETY: This is guaranteed to be the only reference to this `World`, + // meaning this cast to a mutable reference is sound. + unsafe { &mut *(world.0 as *const World as *mut World) }, + ); } Stage::Flush => {} } diff --git a/src/system/schedule/task.rs b/src/system/schedule/task.rs index db4429f5..d2f9edd8 100644 --- a/src/system/schedule/task.rs +++ b/src/system/schedule/task.rs @@ -37,15 +37,13 @@ where } } - pub(crate) fn flush(&mut self, world: SendableWorld) + pub(crate) fn flush(&mut self, world: &mut World) where R: Registry, { - let mut_world = unsafe { &mut *(world.0 as *const World as *mut World) }; - match self { - Task::Seq(system) => system.world_post_processing(mut_world), - Task::Par(system) => system.world_post_processing(mut_world), + Task::Seq(system) => system.world_post_processing(world), + Task::Par(system) => system.world_post_processing(world), } } } From ca88d7e97dd5b10e4a1d673192dbb7c9582b5c06 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 16:57:49 -0700 Subject: [PATCH 55/71] Formatting on closure definition. --- src/entity/allocator/impl_serde.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/entity/allocator/impl_serde.rs b/src/entity/allocator/impl_serde.rs index e41492b0..4cadc94e 100644 --- a/src/entity/allocator/impl_serde.rs +++ b/src/entity/allocator/impl_serde.rs @@ -251,9 +251,10 @@ where Ok(Self { slots: slots .into_iter() - .map(|slot| + .map(|slot| { // SAFETY: We just checked above that each `slot` is not `None`. - unsafe { slot.unwrap_unchecked() }) + unsafe { slot.unwrap_unchecked() } + }) .collect(), free: free .into_iter() From 3629c07ff1c79c6d00d1bd51362fdaadc4ac58ec Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 17:06:25 -0700 Subject: [PATCH 56/71] Safety comments for Registry::serialize_components_by_column(). --- src/registry/serde.rs | 66 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/src/registry/serde.rs b/src/registry/serde.rs index ad957d4c..4a0d96c5 100644 --- a/src/registry/serde.rs +++ b/src/registry/serde.rs @@ -8,8 +8,21 @@ use ::serde::{de, de::SeqAccess, ser::SerializeTuple, Deserialize, Serialize}; use alloc::{format, vec::Vec}; use core::{any::type_name, mem::ManuallyDrop}; -#[cfg_attr(doc_cfg, doc(cfg(feature = "parallel")))] +#[cfg_attr(doc_cfg, doc(cfg(feature = "serde")))] pub trait RegistrySerialize: Registry { + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn serialize_components_by_column( components: &[(*mut u8, usize)], length: usize, @@ -76,15 +89,52 @@ where R_: Registry, S: SerializeTuple, { - if unsafe { identifier_iter.next().unwrap_unchecked() } { - let component_column = unsafe { components.get_unchecked(0) }; - tuple.serialize_element(&SerializeColumn(&ManuallyDrop::new(unsafe { - Vec::::from_raw_parts(component_column.0.cast::(), length, component_column.1) - })))?; + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } + { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(0) }; + tuple.serialize_element(&SerializeColumn(&ManuallyDrop::new( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract + // of this method to define a valid `Vec`. + unsafe { + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) + }, + )))?; - components = unsafe { components.get_unchecked(1..) }; + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(1..) }; } + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. unsafe { R::serialize_components_by_column(components, length, tuple, identifier_iter) } } @@ -117,7 +167,7 @@ where } } -#[cfg_attr(doc_cfg, doc(cfg(feature = "parallel")))] +#[cfg_attr(doc_cfg, doc(cfg(feature = "serde")))] pub trait RegistryDeserialize<'de>: Registry + 'de { unsafe fn deserialize_components_by_column( components: &mut Vec<(*mut u8, usize)>, From a84dbd85bad09cc3d5b0c426ad21875de0a6e9e4 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 17:09:43 -0700 Subject: [PATCH 57/71] Safety comments for Registry::serialize_components_by_row(). --- src/registry/serde.rs | 78 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 65 insertions(+), 13 deletions(-) diff --git a/src/registry/serde.rs b/src/registry/serde.rs index 4a0d96c5..340699dd 100644 --- a/src/registry/serde.rs +++ b/src/registry/serde.rs @@ -33,6 +33,21 @@ pub trait RegistrySerialize: Registry { R: Registry, S: SerializeTuple; + /// # Safety + /// `index` must be less than `length`. + /// + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `length`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn serialize_components_by_row( components: &[(*mut u8, usize)], length: usize, @@ -92,8 +107,7 @@ where if // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to // return a value for every component within the registry. - unsafe { identifier_iter.next().unwrap_unchecked() } - { + unsafe { identifier_iter.next().unwrap_unchecked() } { let component_column = // SAFETY: `components` is guaranteed to have the same number of values as there // set bits in `identifier_iter`. Since a bit must have been set to enter this @@ -149,20 +163,58 @@ where R_: Registry, S: SerializeTuple, { - if unsafe { identifier_iter.next().unwrap_unchecked() } { - let component_column = unsafe { components.get_unchecked(0) }; - tuple.serialize_element(unsafe { - ManuallyDrop::new(Vec::::from_raw_parts( - component_column.0.cast::(), - length, - component_column.1, - )) - .get_unchecked(index) - })?; + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(0) }; + tuple.serialize_element( + // SAFETY: The pointer, capacity, and length are guaranteed by the safety contract + // of this method to define a valid `Vec`. + // + // `index` is also guaranteed to be within the `Vec`, since it is less than + // `length`. + unsafe { + ManuallyDrop::new(Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + )) + .get_unchecked(index) + }, + )?; - components = unsafe { components.get_unchecked(1..) }; + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked(1..) }; } + // // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. + // + // Finally, `index` is guaranteed to be less than `length` by the safety contract of this + // method. unsafe { R::serialize_components_by_row(components, length, index, tuple, identifier_iter) } } } From 01e0157fcef33eff722ebbd747f1c5607ca18a49 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 17:14:53 -0700 Subject: [PATCH 58/71] Safety comments for Registry::deserialize_components_by_column(). --- src/registry/serde.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/registry/serde.rs b/src/registry/serde.rs index 340699dd..c1c49a90 100644 --- a/src/registry/serde.rs +++ b/src/registry/serde.rs @@ -221,6 +221,12 @@ where #[cfg_attr(doc_cfg, doc(cfg(feature = "serde")))] pub trait RegistryDeserialize<'de>: Registry + 'de { + /// # Safety + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn deserialize_components_by_column( components: &mut Vec<(*mut u8, usize)>, length: usize, @@ -285,7 +291,10 @@ where R_: Registry, V: SeqAccess<'de>, { - if unsafe { identifier_iter.next().unwrap_unchecked() } { + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { // TODO: Better error messages? let component_column = seq .next_element_seed(DeserializeColumn::::new(length))? @@ -295,6 +304,8 @@ where components.push((component_column.0.cast::(), component_column.1)); } + // SAFETY: Since one bit was consumed from `identifier_iter`, it still has the same number + // of bits remaining as there are components remaining. unsafe { R::deserialize_components_by_column(components, length, seq, identifier_iter) } } From 215e10c1b41cec943c84ed47ea2086ec1186f29c Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 17:18:24 -0700 Subject: [PATCH 59/71] Safety comments for Registry::deserialize_components_by_row(). --- src/registry/serde.rs | 59 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/src/registry/serde.rs b/src/registry/serde.rs index c1c49a90..62418188 100644 --- a/src/registry/serde.rs +++ b/src/registry/serde.rs @@ -195,7 +195,7 @@ where unsafe { components.get_unchecked(1..) }; } - // // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two // possibilities here: either the bit was set or it was not. // // If the bit was set, then the `components` slice will no longer include the first value, @@ -237,6 +237,19 @@ pub trait RegistryDeserialize<'de>: Registry + 'de { R: Registry, V: SeqAccess<'de>; + /// # Safety + /// `components` must contain the same number of values as there are set bits in the + /// `identifier_iter`. + /// + /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a + /// `Vec` of length `0`, where `C` is the component corresponding to the set bit in + /// `identifier_iter`. + /// + /// When called externally, the `Registry` `R` provided to the method must by the same as the + /// `Registry` on which this method is being called. + /// + /// When called internally, the `identifier_iter` must have the same amount of bits left as + /// there are components remaining. unsafe fn deserialize_components_by_row( components: &mut [(*mut u8, usize)], length: usize, @@ -319,11 +332,22 @@ where R_: Registry, V: SeqAccess<'de>, { - if unsafe { identifier_iter.next().unwrap_unchecked() } { - let component_column = unsafe { components.get_unchecked_mut(0) }; - let mut v = ManuallyDrop::new(unsafe { - Vec::::from_raw_parts(component_column.0.cast::(), 0, component_column.1) - }); + if + // SAFETY: `identifier_iter` is guaranteed by the safety contract of this method to + // return a value for every component within the registry. + unsafe { identifier_iter.next().unwrap_unchecked() } { + let component_column = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked_mut(0) }; + let mut v = ManuallyDrop::new( + // SAFETY: The pointer and capacity are guaranteed by the safety contract of this + // method to define a valid `Vec` of length `0`. + unsafe { + Vec::::from_raw_parts(component_column.0.cast::(), 0, component_column.1) + }, + ); v.push(seq.next_element()?.ok_or_else(|| { // TODO: the length returned here is incorrect. @@ -332,9 +356,30 @@ where component_column.0 = v.as_mut_ptr().cast::(); component_column.1 = v.capacity(); - components = unsafe { components.get_unchecked_mut(1..) }; + components = + // SAFETY: `components` is guaranteed to have the same number of values as there + // set bits in `identifier_iter`. Since a bit must have been set to enter this + // block, there must be at least one component column. + unsafe { components.get_unchecked_mut(1..) }; } + // SAFETY: At this point, one bit of `identifier_iter` has been consumed. There are two + // possibilities here: either the bit was set or it was not. + // + // If the bit was set, then the `components` slice will no longer include the first value, + // which means the slice will still contain the same number of pointer and capacity tuples + // as there are set bits in `identifier_iter`. Additionally, since the first value was + // removed from the slice, which corresponded to the component identified by the consumed + // bit, all remaining component values will still correspond to valid `Vec`s identified + // by the remaining set bits in `identifier_iter`. + // + // If the bit was not set, then `components` is unaltered, and there are still the same + // number of elements as there are set bits in `identifier_iter`, which still make valid + // `Vec`s for each `C` identified by the remaining set bits in `identifier_iter`. + // + // Furthermore, regardless of whether the bit was set or not, `R` is one component smaller + // than `(C, R)`, and since `identifier_iter` has had one bit consumed, it still has the + // same number of bits remaining as `R` has components remaining. unsafe { R::deserialize_components_by_row(components, length, seq, identifier_iter) } } } From a42ae17c254c1786fa00931d8304e7896116296b Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 22:32:14 -0700 Subject: [PATCH 60/71] Safety comments for Archetype Serialize impl. --- src/archetype/impl_serde.rs | 53 +++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/archetype/impl_serde.rs b/src/archetype/impl_serde.rs index 23bde76a..55016de5 100644 --- a/src/archetype/impl_serde.rs +++ b/src/archetype/impl_serde.rs @@ -54,15 +54,24 @@ where where S: Serializer, { - let mut tuple = serializer - .serialize_tuple(unsafe { self.0.identifier.iter() }.filter(|b| *b).count() + 1)?; - tuple.serialize_element(&SerializeColumn(&ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.0.entity_identifiers.0, - self.0.length, - self.0.entity_identifiers.1, - ) - })))?; + let mut tuple = serializer.serialize_tuple( + // SAFETY: The identifier here will outlive the derived `Iter`. + unsafe { self.0.identifier.iter() }.filter(|b| *b).count() + 1, + )?; + tuple.serialize_element(&SerializeColumn(&ManuallyDrop::new( + // SAFETY: `entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of length `length`. + unsafe { + Vec::from_raw_parts( + self.0.entity_identifiers.0, + self.0.length, + self.0.entity_identifiers.1, + ) + }, + )))?; + // SAFETY: `self.0.components` contains the raw parts for `Vec`s of size `length` for + // each component `C` identified by the `identifier`. Also, the `R` upon which the + // identifier is generic is the same `R` upon which this function is called. unsafe { R::serialize_components_by_column( &self.0.components, @@ -112,21 +121,31 @@ where S: Serializer, { let mut tuple = serializer.serialize_tuple( + // SAFETY: The identifier here will outlive the derived `Iter`. unsafe { self.archetype.identifier.iter() } .filter(|b| *b) .count() + 1, )?; - tuple.serialize_element(unsafe { - ManuallyDrop::new(Vec::from_raw_parts( - self.archetype.entity_identifiers.0, - self.archetype.length, - self.archetype.entity_identifiers.1, - )) - .get_unchecked(self.index) - })?; + tuple.serialize_element( + // SAFETY: `entity_identifiers` is guaranteed to contain the raw parts for a valid + // `Vec` of length `length`. + unsafe { + ManuallyDrop::new(Vec::from_raw_parts( + self.archetype.entity_identifiers.0, + self.archetype.length, + self.archetype.entity_identifiers.1, + )) + .get_unchecked(self.index) + }, + )?; + // SAFETY: `self.0.components` contains the raw parts for `Vec`s of size `length` for + // each component `C` identified by the `identifier`. Also, the `R` upon which the + // identifier is generic is the same `R` upon which this function is called. Finally, + // `self.index` is invariantly guaranteed to be a valid index into the archetype (meaning + // it is less than its length). unsafe { R::serialize_components_by_row( &self.archetype.components, From af58873536ee5a613eca8614c981e06727a7ee27 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 22:58:10 -0700 Subject: [PATCH 61/71] Safety comments for Archetype Deserialize impl. --- src/archetype/impl_serde.rs | 179 +++++++++++++++++++++++++----------- src/archetype/mod.rs | 2 +- src/registry/serde.rs | 10 +- 3 files changed, 131 insertions(+), 60 deletions(-) diff --git a/src/archetype/impl_serde.rs b/src/archetype/impl_serde.rs index 55016de5..2d8ede40 100644 --- a/src/archetype/impl_serde.rs +++ b/src/archetype/impl_serde.rs @@ -236,6 +236,10 @@ impl<'a, 'de, R> DeserializeRow<'a, 'de, R> where R: RegistryDeserialize<'de>, { + /// # Safety + /// `entity_identifiers` must be the valid raw parts for a `Vec` of size + /// `length`. Each element in `components` must be the valid raw parts for a `Vec` of size + /// `length` for each component `C` identified by `identifier`. unsafe fn new( identifier: archetype::IdentifierRef, entity_identifiers: &'a mut (*mut entity::Identifier, usize), @@ -283,13 +287,17 @@ where where A: SeqAccess<'de>, { - let mut entity_identifiers = ManuallyDrop::new(unsafe { - Vec::from_raw_parts( - self.0.entity_identifiers.0, - self.0.length, - self.0.entity_identifiers.1, - ) - }); + let mut entity_identifiers = ManuallyDrop::new( + // SAFETY: `entity_identifiers` contains the valid raw parts for a + // `Vec of size `length`. + unsafe { + Vec::from_raw_parts( + self.0.entity_identifiers.0, + self.0.length, + self.0.entity_identifiers.1, + ) + }, + ); entity_identifiers .push(seq.next_element()?.ok_or_else(|| { de::Error::invalid_length(0, &"number of components + 1") @@ -299,6 +307,10 @@ where entity_identifiers.capacity(), ); + // SAFETY: Each element of `self.0.components` contains the raw parts for a valid + // `Vec` of size `self.0.length` for each component `C` identified by the + // identifier. The registry `R` over which `self.0.identifier` is generic is the + // same `R` on which this function is called. unsafe { R::deserialize_components_by_row( self.0.components, @@ -313,6 +325,7 @@ where } deserializer.deserialize_tuple( + // SAFETY: The identifier here will outlive the derived `Iter`. unsafe { self.identifier.iter() }.filter(|b| *b).count() + 1, DeserializeRowVisitor(self), ) @@ -368,8 +381,12 @@ where entity_identifiers_vec.capacity(), ); - let components_len = unsafe { self.0.identifier.iter() }.filter(|b| *b).count(); + let components_len = + // SAFETY: The identifier here will outlive the derived `Iter`. + unsafe { self.0.identifier.iter() }.filter(|b| *b).count(); let mut components = Vec::with_capacity(components_len); + // SAFETY: The registry `R` over which `self.0.identifier` is generic is the same + // `R` on which this function is called. unsafe { R::new_components_with_capacity( &mut components, @@ -380,38 +397,62 @@ where let mut vec_length = 0; for i in 0..self.0.length { - let result = seq.next_element_seed(unsafe { - DeserializeRow::new( - self.0.identifier.as_ref(), - &mut entity_identifiers, - &mut components, - vec_length, - ) - }); - if let Err(error) = result { - drop(unsafe { - Vec::from_raw_parts( - entity_identifiers.0, + let result = seq.next_element_seed( + // SAFETY: `entity_identifiers` and `components` both contain the raw parts + // for valid `Vec`s of length `vec_length`. + unsafe { + DeserializeRow::new( + self.0.identifier.as_ref(), + &mut entity_identifiers, + &mut components, vec_length, - entity_identifiers.1, ) - }); + }, + ); + if let Err(error) = result { + drop( + // SAFETY: `entity_identifiers` contains the raw parts for a valid + // `Vec` of size `vec_length`. + unsafe { + Vec::from_raw_parts( + entity_identifiers.0, + vec_length, + entity_identifiers.1, + ) + }, + ); + // SAFETY: `components` contains the raw parts for valid `Vec`s of + // length `vec_length` for each component identified by the identifier. The + // registry `R` over which `self.0.identifier` is generic is the same `R` + // on which this function is called. unsafe { R::free_components(&components, vec_length, self.0.identifier.iter()); } return Err(error); } - if let Some(()) = unsafe { result.unwrap_unchecked() } { + if let Some(()) = + // SAFETY: If the `result` was an `Err` variant, the function would have + // returned in the previous `if` block. + unsafe { result.unwrap_unchecked() } + { vec_length += 1; } else { - drop(unsafe { - Vec::from_raw_parts( - entity_identifiers.0, - vec_length, - entity_identifiers.1, - ) - }); + drop( + // SAFETY: `entity_identifiers` contains the raw parts for a valid + // `Vec` of size `vec_length`. + unsafe { + Vec::from_raw_parts( + entity_identifiers.0, + vec_length, + entity_identifiers.1, + ) + }, + ); + // SAFETY: `components` contains the raw parts for valid `Vec`s of + // length `vec_length` for each component identified by the identifier. The + // registry `R` over which `self.0.identifier` is generic is the same `R` + // on which this function is called. unsafe { R::free_components(&components, vec_length, self.0.identifier.iter()); } @@ -420,14 +461,19 @@ where } } - Ok(unsafe { - Archetype::from_raw_parts( - self.0.identifier, - entity_identifiers, - components, - self.0.length, - ) - }) + Ok( + // SAFETY: `entity_identifiers` and `components` both contain the raw parts for + // valid `Vec`s of length `self.0.length` for the entity identifiers and + // components identified by `self.0.identifier`. + unsafe { + Archetype::from_raw_parts( + self.0.identifier, + entity_identifiers, + components, + self.0.length, + ) + }, + ) } } @@ -554,9 +600,14 @@ where .next_element_seed(DeserializeColumn::new(self.0.length))? .ok_or_else(|| de::Error::invalid_length(2, &self))?; - let mut components = - Vec::with_capacity(unsafe { self.0.identifier.iter() }.filter(|b| *b).count()); - let result = unsafe { + let mut components = Vec::with_capacity( + // SAFETY: The identifier here will outlive the derived `Iter`. + unsafe { self.0.identifier.iter() }.filter(|b| *b).count(), + ); + let result = + // SAFETY: The `R` over which `self.0.identifier` is generic is the same `R` on + // which this function is being called. + unsafe { R::deserialize_components_by_column( &mut components, self.0.length, @@ -566,13 +617,23 @@ where }; if let Err(error) = result { // Free columns, since they are invalid and must be dropped. - drop(unsafe { - Vec::from_raw_parts( - entity_identifiers.0, - self.0.length, - entity_identifiers.1, - ) - }); + drop( + // SAFETY: `entity_identifiers` are the raw parts for a valid + // `Vec` of length `self.0.length`. + unsafe { + Vec::from_raw_parts( + entity_identifiers.0, + self.0.length, + entity_identifiers.1, + ) + }, + ); + // SAFETY: All elements in `components` are raw parts for valid `Vec`s for + // each component `C` identified by `self.0.identifier` (although there may not + // necessarily be the same number of elements as there are components, which is + // allowed in the safety contract). The registry `R` over which + // `self.0.identifier` is generic is the same `R` on which this function is + // called. unsafe { R::try_free_components( &components, @@ -584,18 +645,24 @@ where return Err(error); } - Ok(unsafe { - Archetype::from_raw_parts( - self.0.identifier, - entity_identifiers, - components, - self.0.length, - ) - }) + Ok( + // SAFETY: `entity_identifiers` and `components` both contain the raw parts for + // valid `Vec`s of length `self.0.length` for the entity identifiers and + // components identified by `self.0.identifier`. + unsafe { + Archetype::from_raw_parts( + self.0.identifier, + entity_identifiers, + components, + self.0.length, + ) + }, + ) } } deserializer.deserialize_tuple( + // SAFETY: The identifier here will outlive the derived `Iter`. unsafe { self.identifier.iter() }.filter(|b| *b).count() + 1, DeserializeColumnsVisitor(self), ) diff --git a/src/archetype/mod.rs b/src/archetype/mod.rs index cdc9e96f..158afd9c 100644 --- a/src/archetype/mod.rs +++ b/src/archetype/mod.rs @@ -57,7 +57,7 @@ where /// size `length`. /// /// `components` must contain the raw parts for valid `Vec`s of size `length` for each - /// component `C` in the registry `R`. + /// component `C` identified by `identifier`. pub(crate) unsafe fn from_raw_parts( identifier: Identifier, entity_identifiers: (*mut entity::Identifier, usize), diff --git a/src/registry/serde.rs b/src/registry/serde.rs index 62418188..8416b55f 100644 --- a/src/registry/serde.rs +++ b/src/registry/serde.rs @@ -242,7 +242,7 @@ pub trait RegistryDeserialize<'de>: Registry + 'de { /// `identifier_iter`. /// /// Each `(*mut u8, usize)` in `components` must be the pointer and capacity respectively of a - /// `Vec` of length `0`, where `C` is the component corresponding to the set bit in + /// `Vec` of size `length`, where `C` is the component corresponding to the set bit in /// `identifier_iter`. /// /// When called externally, the `Registry` `R` provided to the method must by the same as the @@ -343,9 +343,13 @@ where unsafe { components.get_unchecked_mut(0) }; let mut v = ManuallyDrop::new( // SAFETY: The pointer and capacity are guaranteed by the safety contract of this - // method to define a valid `Vec` of length `0`. + // method to define a valid `Vec` of length `length`. unsafe { - Vec::::from_raw_parts(component_column.0.cast::(), 0, component_column.1) + Vec::::from_raw_parts( + component_column.0.cast::(), + length, + component_column.1, + ) }, ); From b60e255e109609868b35b855691162c9d9891108 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 23:01:01 -0700 Subject: [PATCH 62/71] Add missing backtick in documentation. --- src/entities/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/entities/mod.rs b/src/entities/mod.rs index 18f21d2d..708529aa 100644 --- a/src/entities/mod.rs +++ b/src/entities/mod.rs @@ -136,7 +136,7 @@ where /// ``` /// /// [`Entities`]: crate::entities::Entities - /// [`entities!]: crate::entities! + /// [`entities!`]: crate::entities! /// /// # Panics /// Panics if the columns are not all the same length. @@ -167,7 +167,7 @@ where /// let batch = unsafe { Batch::new_unchecked((vec![42; 10], (vec![true; 10], entities::Null))) }; /// ``` /// [`Entities`]: crate::entities::Entities - /// [`entities!]: crate::entities! + /// [`entities!`]: crate::entities! pub unsafe fn new_unchecked(entities: E) -> Self { Self { len: entities.component_len(), From 5923109a97227fa8d678d7556d571318b7e34097 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 23:02:32 -0700 Subject: [PATCH 63/71] Disable fail-fast on CI tests. --- .github/workflows/test.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index dc3db0fa..55e5a3ad 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -17,6 +17,7 @@ jobs: - stable - beta - nightly + fail-fast: false steps: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 From 410349ab93cbe41cb4ce66f8dc91fd490bb90bf2 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Mon, 16 May 2022 23:12:00 -0700 Subject: [PATCH 64/71] Fix trybuild test. --- tests/trybuild.rs | 2 +- .../entities/length_not_integer.stderr | 32 ++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/tests/trybuild.rs b/tests/trybuild.rs index 968edf8f..736c9888 100644 --- a/tests/trybuild.rs +++ b/tests/trybuild.rs @@ -2,8 +2,8 @@ macro_rules! trybuild_test { ($test_name:ident) => { - #[test] #[rustversion::attr(not(nightly), ignore)] + #[test] fn $test_name() { trybuild::TestCases::new().compile_fail(concat!( "tests/trybuild/", diff --git a/tests/trybuild/entities/length_not_integer.stderr b/tests/trybuild/entities/length_not_integer.stderr index 72499e1a..0ba4f50f 100644 --- a/tests/trybuild/entities/length_not_integer.stderr +++ b/tests/trybuild/entities/length_not_integer.stderr @@ -1,5 +1,29 @@ error[E0308]: mismatched types - --> tests/trybuild/entities/length_not_integer.rs:10:38 - | -10 | let entities = entities!((A, B); 1.5); - | ^^^ expected `usize`, found floating-point number + --> tests/trybuild/entities/length_not_integer.rs:10:38 + | +10 | let entities = entities!((A, B); 1.5); + | ------------------^^^- + | | | + | | expected `usize`, found floating-point number + | arguments to this function are incorrect + | +note: function defined here + --> $RUST/alloc/src/vec/mod.rs + | + | pub fn from_elem(elem: T, n: usize) -> Vec { + | ^^^^^^^^^ + +error[E0308]: mismatched types + --> tests/trybuild/entities/length_not_integer.rs:10:38 + | +10 | let entities = entities!((A, B); 1.5); + | ------------------^^^- + | | | + | | expected `usize`, found floating-point number + | arguments to this function are incorrect + | +note: function defined here + --> $RUST/alloc/src/vec/mod.rs + | + | pub fn from_elem(elem: T, n: usize) -> Vec { + | ^^^^^^^^^ From 6af7a02084a31f6802492bea14035f556a328c5b Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 17 May 2022 16:06:05 -0700 Subject: [PATCH 65/71] Update trybuild to 1.0.61. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 1884d85a..8f05e8cd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ serde = {version = "1.0.133", default-features = false, features = ["alloc"], op claim = "0.5.0" rustversion = "1.0.6" serde_test = "1.0.133" -trybuild = "1.0.56" +trybuild = "1.0.61" [features] # TODO: Rename this to "rayon" when namespaced dependencies are stabilized in 1.60.0. From 19c38d1e285990debdabfbb98f4001ee6f5dc88e Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 17 May 2022 16:43:35 -0700 Subject: [PATCH 66/71] Install rust-src component in CI tests. --- .github/workflows/test.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 55e5a3ad..3e1ae176 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -24,6 +24,7 @@ jobs: with: toolchain: ${{ matrix.rust }} override: true + components: rust-src - uses: actions-rs/install@v0.1 with: crate: cargo-hack From 9f1a3f058da88337d20b9591c76c1f439060eaa5 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 17 May 2022 16:46:37 -0700 Subject: [PATCH 67/71] Install rust-src component in CI valgrind and code-cov jobs. --- .github/workflows/test.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 3e1ae176..77639284 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -180,6 +180,7 @@ jobs: with: toolchain: nightly override: true + components: rust-src - uses: actions-rs/install@v0.1 with: crate: cargo-valgrind @@ -201,7 +202,7 @@ jobs: with: toolchain: nightly override: true - components: llvm-tools-preview + components: llvm-tools-preview, rust-src - uses: actions-rs/install@v0.1 with: crate: cargo-llvm-cov From 321bbe2b9523629b591b74827957dac12e4e6636 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Tue, 17 May 2022 17:25:51 -0700 Subject: [PATCH 68/71] Remove unnecessary component installation from valgrind CI job. --- .github/workflows/test.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 77639284..51e60487 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -180,7 +180,6 @@ jobs: with: toolchain: nightly override: true - components: rust-src - uses: actions-rs/install@v0.1 with: crate: cargo-valgrind From cd19907f9ee845eb283c13747b54e6472504ebca Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 18 May 2022 06:55:29 -0700 Subject: [PATCH 69/71] Install newest version of valgrind from source. --- .github/workflows/test.yaml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 51e60487..dab500d3 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -174,8 +174,16 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 + - run: wget https://sourceware.org/pub/valgrind/valgrind-3.19.0.tar.bz2 + - run: tar xvf valgrind-3.19.0.tar.bz2 + - run: ./configure + working-directory: ./valgrind-3.19.0 + - run: make + working-directory: ./valgrind-3.19.0 + - run: sudo make install + working-directory: ./valgrind-3.19.0 - run: sudo apt-get update - - run: sudo apt-get install valgrind + - run: sudo apt-get install libc6-dbg - uses: actions-rs/toolchain@v1 with: toolchain: nightly From 9d58c53f5a11c12aa52670237c409ea481aeceee Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 18 May 2022 07:12:14 -0700 Subject: [PATCH 70/71] Prevent memory leaks in Registry tests. --- src/registry/seal/storage.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/registry/seal/storage.rs b/src/registry/seal/storage.rs index cc4e3fd9..99ea1acf 100644 --- a/src/registry/seal/storage.rs +++ b/src/registry/seal/storage.rs @@ -1244,6 +1244,11 @@ mod tests { assert_eq!(components.get(0).unwrap().1, CAPACITY); assert_eq!(components.get(1).unwrap().1, CAPACITY); assert_eq!(components.get(2).unwrap().1, CAPACITY); + + // Free components to avoid leaking memory. + unsafe { + Registry::free_components(&mut components, 0, identifier.iter()) + } } #[test] @@ -1262,6 +1267,11 @@ mod tests { assert_eq!(components.get(0).unwrap().1, CAPACITY); assert_eq!(components.get(1).unwrap().1, CAPACITY); + + // Free components to avoid leaking memory. + unsafe { + Registry::free_components(&mut components, 0, identifier.iter()) + } } #[test] From bd40ee73f80255b111709b4daa0d316731ac6285 Mon Sep 17 00:00:00 2001 From: Anders429 Date: Wed, 18 May 2022 07:13:31 -0700 Subject: [PATCH 71/71] Formatting. --- src/registry/seal/storage.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/registry/seal/storage.rs b/src/registry/seal/storage.rs index 99ea1acf..4ee1a036 100644 --- a/src/registry/seal/storage.rs +++ b/src/registry/seal/storage.rs @@ -1246,9 +1246,7 @@ mod tests { assert_eq!(components.get(2).unwrap().1, CAPACITY); // Free components to avoid leaking memory. - unsafe { - Registry::free_components(&mut components, 0, identifier.iter()) - } + unsafe { Registry::free_components(&mut components, 0, identifier.iter()) } } #[test] @@ -1269,9 +1267,7 @@ mod tests { assert_eq!(components.get(1).unwrap().1, CAPACITY); // Free components to avoid leaking memory. - unsafe { - Registry::free_components(&mut components, 0, identifier.iter()) - } + unsafe { Registry::free_components(&mut components, 0, identifier.iter()) } } #[test]