From b06cbf94aa28c974e8e50a3b008df10231a6a9ae Mon Sep 17 00:00:00 2001 From: Ellen Date: Mon, 18 Apr 2022 23:46:29 +0100 Subject: [PATCH 1/5] docs Co-authored-by: Alice Cecile --- crates/bevy_ecs/src/ptr.rs | 40 +++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/ptr.rs b/crates/bevy_ecs/src/ptr.rs index 8c65044b9e288..d48b13f743285 100644 --- a/crates/bevy_ecs/src/ptr.rs +++ b/crates/bevy_ecs/src/ptr.rs @@ -1,20 +1,50 @@ use std::{cell::UnsafeCell, marker::PhantomData, mem::MaybeUninit, ptr::NonNull}; -/// Type-erased pointer into memory. Guaranteed to be correctly aligned, non-null and safe to read for a particular type. +/// Type-erased borrow of some unknown type chosen when constructing this type. +/// +/// This type tries to act "borrow-like" which means that: +/// - It should be considered immutable: its target must not be changed while this pointer is alive. +/// - It must always points to a valid value of whatever the pointee type is. +/// - The lifetime `'a` accurately represents how long the pointer is valid for. +/// +/// It may be helpful to think of this type as similar to `&'a dyn Any` but without +/// the metadata and able to point to data that does not correspond to a Rust type. #[derive(Copy, Clone)] pub struct Ptr<'a>(NonNull, PhantomData<&'a u8>); -/// Type-erased pointer into memory. Guaranteed to be correctly aligned, non-null and safe to modify for a particular type. +/// Type-erased mutable borrow of some unknown type chosen when constructing this type. +/// +/// This type tries to act "borrow-like" which means that: +/// - Pointer is considered exclusive and mutable. It cannot be cloned as this would lead to +/// aliased mutability. +/// - It must always points to a valid value of whatever the pointee type is. +/// - The lifetime `'a` accurately represents how long the pointer is valid for. +/// +/// It may be helpful to think of this type as similar to `&'a mut dyn Any` but without +/// the metadata and able to point to data that does not correspond to a Rust type. pub struct PtrMut<'a>(NonNull, PhantomData<&'a mut u8>); -/// Type-erased pointer into memory. Guaranteed to be correctly aligned, non-null and safe to move out of for a particular type. +/// Type-erased Box-like pointer to some unknown type chosen when constructing this type. +/// Conceptually represents ownership of whatever data is being pointed to and so is +/// responsible for calling its `Drop` impl. This pointer is _not_ responsible for freeing +/// the memory pointed to by this pointer as it may be pointing to an element in a `Vec` or +/// to a local in a function etc. +/// +/// This type tries to act "borrow-like" like which means that: +/// - Pointer should be considered exclusive and mutable. It cannot be cloned as this would lead +/// to aliased mutability and potentially use after free bugs. +/// - It must always points to a valid value of whatever the pointee type is. +/// - The lifetime `'a` accurately represents how long the pointer is valid for. +/// +/// It may be helpful to think of this type as similar to `&'a mut ManuallyDrop` but +/// without the metadata and able to point to data that does not correspond to a Rust type. pub struct OwningPtr<'a>(NonNull, PhantomData<&'a mut u8>); macro_rules! impl_ptr { ($ptr:ident) => { impl $ptr<'_> { /// # Safety - /// the offset cannot make the existing ptr null, or take it out of bounds for it's allocation. + /// the offset cannot make the existing ptr null, or take it out of bounds for its allocation. pub unsafe fn offset(self, count: isize) -> Self { Self( NonNull::new_unchecked(self.0.as_ptr().offset(count)), @@ -23,7 +53,7 @@ macro_rules! impl_ptr { } /// # Safety - /// the offset cannot make the existing ptr null, or take it out of bounds for it's allocation. + /// the offset cannot make the existing ptr null, or take it out of bounds for its allocation. pub unsafe fn add(self, count: usize) -> Self { Self( NonNull::new_unchecked(self.0.as_ptr().add(count)), From 41c98d8e99dea84c24532333583175577841b116 Mon Sep 17 00:00:00 2001 From: Ellen Date: Tue, 19 Apr 2022 00:05:14 +0100 Subject: [PATCH 2/5] revert some needless changes to `world/mod.rs` --- crates/bevy_ecs/src/world/mod.rs | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 8358d926710a5..f5a60ecb34088 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -197,8 +197,7 @@ impl World { /// .insert(Position { x: 0.0, y: 0.0 }) /// .id(); /// - /// let entity = world.entity(entity); - /// let position = entity.get::().unwrap(); + /// let position = world.entity(entity).get::().unwrap(); /// assert_eq!(position.x, 0.0); /// ``` #[inline] @@ -338,7 +337,7 @@ impl World { /// .insert_bundle((Num(1), Label("hello"))) // add a bundle of components /// .id(); /// - /// let position = world.get::(entity).unwrap(); + /// let position = world.entity(entity).get::().unwrap(); /// assert_eq!(position.x, 0.0); /// ``` pub fn spawn(&mut self) -> EntityMut { @@ -415,7 +414,7 @@ impl World { /// ``` #[inline] pub fn get(&self, entity: Entity) -> Option<&T> { - unsafe { get(self, entity, self.get_entity(entity)?.location()) } + self.get_entity(entity)?.get() } /// Retrieves a mutable reference to the given `entity`'s [Component] of the given type. @@ -684,7 +683,7 @@ impl World { // SAFE: if a resource column exists, row 0 exists as well. caller takes ownership of the // ptr value / drop is called when R is dropped let (ptr, _) = unsafe { column.swap_remove_and_forget_unchecked(0) }; - // SAFE: column is of type T + // SAFE: column is of type R Some(unsafe { ptr.read::() }) } @@ -1063,16 +1062,16 @@ impl World { }; // SAFE: pointer is of type R // Read the value onto the stack to avoid potential mut aliasing. - let mut val = unsafe { ptr.read::() }; - let value = Mut { - value: &mut val, + let mut value = unsafe { ptr.read::() }; + let value_mut = Mut { + value: &mut value, ticks: Ticks { component_ticks: &mut ticks, last_change_tick, change_tick, }, }; - let result = f(self, value); + let result = f(self, value_mut); assert!(!self.contains_resource::()); let resource_archetype = self.archetypes.resource_mut(); @@ -1081,9 +1080,9 @@ impl World { .get_mut(component_id) .unwrap_or_else(|| panic!("resource does not exist: {}", std::any::type_name::())); - OwningPtr::make(val, |ptr| { + OwningPtr::make(value, |ptr| { unsafe { - // SAFE: pointer is of type T + // SAFE: pointer is of type R column.push(ptr, ticks); } }); @@ -1146,17 +1145,17 @@ impl World { /// # Safety /// `component_id` must be valid and correspond to a resource component of type `R` #[inline] - unsafe fn insert_resource_with_id(&mut self, component_id: ComponentId, value: T) { + unsafe fn insert_resource_with_id(&mut self, component_id: ComponentId, value: R) { let change_tick = self.change_tick(); let column = self.initialize_resource_internal(component_id); if column.is_empty() { - // SAFE: column is of type T and has been allocated above + // SAFE: column is of type R and has been allocated above OwningPtr::make(value, |ptr| { column.push(ptr, ComponentTicks::new(change_tick)); }); } else { - // SAFE: column is of type T and has already been allocated - *column.get_data_unchecked_mut(0).deref_mut::() = value; + // SAFE: column is of type R and has already been allocated + *column.get_data_unchecked_mut(0).deref_mut::() = value; column.get_ticks_unchecked_mut(0).set_changed(change_tick); } } From c04dd6421e92cbda37d3706bdbc9fbb4716f979e Mon Sep 17 00:00:00 2001 From: Ellen Date: Tue, 19 Apr 2022 00:31:07 +0100 Subject: [PATCH 3/5] review `entity_ref.rs` --- crates/bevy_ecs/src/reflect.rs | 6 ++++-- crates/bevy_ecs/src/world/entity_ref.rs | 17 ++++------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index b2da3e32c779c..4881dd83f0edd 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -103,8 +103,10 @@ impl FromType for ReflectComponent { .entity_mut(destination_entity) .insert(destination_component); }, - reflect_component: |world, entity| unsafe { - crate::world::get::(world, entity, world.get_entity(entity)?.location()) + reflect_component: |world, entity| { + world + .get_entity(entity)? + .get::() .map(|c| c as &dyn Reflect) }, reflect_component_mut: |world, entity| unsafe { diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index a9f49aab0ae94..a4964debff951 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -65,7 +65,10 @@ impl<'w> EntityRef<'w> { #[inline] pub fn get(&self) -> Option<&'w T> { // SAFE: entity location is valid and returned component is of type T - unsafe { get(self.world, self.entity, self.location) } + unsafe { + get_component_with_type(self.world, TypeId::of::(), self.entity, self.location) + .map(|value| value.deref::()) + } } /// Gets a mutable reference to the component of type `T` associated with @@ -721,18 +724,6 @@ fn sorted_remove(source: &mut Vec, remove: &[T]) { }); } -// SAFETY: EntityLocation must be valid -#[inline] -pub(crate) unsafe fn get( - world: &World, - entity: Entity, - location: EntityLocation, -) -> Option<&T> { - // SAFE: entity location is valid and returned component is of type T - get_component_with_type(world, TypeId::of::(), entity, location) - .map(|value| value.deref::()) -} - // SAFETY: EntityLocation must be valid #[inline] pub(crate) unsafe fn get_mut( From 3efa7ae4cbbad9d0e86eafa8da433cd653dcbc65 Mon Sep 17 00:00:00 2001 From: Ellen Date: Tue, 19 Apr 2022 00:39:44 +0100 Subject: [PATCH 4/5] Fix odd bound on `UnsafeCellDeref::read` --- crates/bevy_ecs/src/component.rs | 2 +- crates/bevy_ecs/src/ptr.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 5f3abf784d715..67a73b2a363aa 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -350,7 +350,7 @@ impl Components { } } -#[derive(Clone, Debug)] +#[derive(Copy, Clone, Debug)] pub struct ComponentTicks { pub(crate) added: u32, pub(crate) changed: u32, diff --git a/crates/bevy_ecs/src/ptr.rs b/crates/bevy_ecs/src/ptr.rs index d48b13f743285..619d9712ba139 100644 --- a/crates/bevy_ecs/src/ptr.rs +++ b/crates/bevy_ecs/src/ptr.rs @@ -122,7 +122,7 @@ pub(crate) trait UnsafeCellDeref<'a, T> { unsafe fn deref(self) -> &'a T; unsafe fn read(self) -> T where - Self: Copy; + T: Copy; } impl<'a, T> UnsafeCellDeref<'a, T> for &'a UnsafeCell { unsafe fn deref_mut(self) -> &'a mut T { @@ -134,7 +134,7 @@ impl<'a, T> UnsafeCellDeref<'a, T> for &'a UnsafeCell { unsafe fn read(self) -> T where - Self: Copy, + T: Copy, { self.get().read() } From 8089f6d4631b7d4ecfdaa52230b54dccc8520a15 Mon Sep 17 00:00:00 2001 From: Ellen Date: Tue, 19 Apr 2022 00:44:43 +0100 Subject: [PATCH 5/5] Undo changes --- crates/bevy_ecs/src/system/query.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 05b61e73137cf..45bb7d8cec2cf 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -452,16 +452,16 @@ where /// # bevy_ecs::system::assert_is_system(report_names_system); /// ``` #[inline] - pub fn for_each<'this, FN: FnMut(ROQueryItem<'this, Q>)>(&'this self, f: FN) { + pub fn for_each<'this>(&'this self, f: impl FnMut(ROQueryItem<'this, Q>)) { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { - self.state.for_each_unchecked_manual::, FN>( + self.state.for_each_unchecked_manual::, _>( self.world, f, self.last_change_tick, self.change_tick, - ) + ); }; } @@ -521,17 +521,17 @@ where ///* `batch_size` - The number of batches to spawn ///* `f` - The function to run on each item in the query #[inline] - pub fn par_for_each<'this, FN: Fn(ROQueryItem<'this, Q>) + Send + Sync + Clone>( + pub fn par_for_each<'this>( &'this self, task_pool: &TaskPool, batch_size: usize, - f: FN, + f: impl Fn(ROQueryItem<'this, Q>) + Send + Sync + Clone, ) { // SAFE: system runs without conflicts with other systems. same-system queries have runtime // borrow checks when they conflict unsafe { self.state - .par_for_each_unchecked_manual::, FN>( + .par_for_each_unchecked_manual::, _>( self.world, task_pool, batch_size,