ptrfication reviews round... 4(?) (#44)

* docs


Co-authored-by: Alice Cecile <[email protected]>

* revert some needless changes to `world/mod.rs`

* review `entity_ref.rs`

* Fix odd bound on `UnsafeCellDeref::read`

* Undo changes

Co-authored-by: Alice Cecile <[email protected]>

crates/bevy_ecs/src/component.rs

@@ -351,7 +351,7 @@ impl Components {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug)]
 pub struct ComponentTicks {
     pub(crate) added: u32,
     pub(crate) changed: u32,

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<u8>, 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<u8>, 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<dyn Any>` but
+/// without the metadata and able to point to data that does not correspond to a Rust type.
 pub struct OwningPtr<'a>(NonNull<u8>, 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)),
@@ -100,7 +130,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<T> {
     unsafe fn deref_mut(self) -> &'a mut T {
@@ -112,7 +142,7 @@ impl<'a, T> UnsafeCellDeref<'a, T> for &'a UnsafeCell<T> {
 
     unsafe fn read(self) -> T
     where
-        Self: Copy,
+        T: Copy,
     {
         self.get().read()
     }

crates/bevy_ecs/src/reflect.rs

@@ -103,8 +103,10 @@ impl<C: Component + Reflect + FromWorld> FromType<C> for ReflectComponent {
                     .entity_mut(destination_entity)
                     .insert(destination_component);
             },
-            reflect_component: |world, entity| unsafe {
-                crate::world::get::<C>(world, entity, world.get_entity(entity)?.location())
+            reflect_component: |world, entity| {
+                world
+                    .get_entity(entity)?
+                    .get::<C>()
                     .map(|c| c as &dyn Reflect)
             },
             reflect_component_mut: |world, entity| unsafe {

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::<ROQueryFetch<Q>, FN>(
+            self.state.for_each_unchecked_manual::<ROQueryFetch<Q>, _>(
                 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::<ROQueryFetch<Q>, FN>(
+                .par_for_each_unchecked_manual::<ROQueryFetch<Q>, _>(
                     self.world,
                     task_pool,
                     batch_size,

crates/bevy_ecs/src/world/entity_ref.rs

@@ -65,7 +65,10 @@ impl<'w> EntityRef<'w> {
     #[inline]
     pub fn get<T: Component>(&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::<T>(), self.entity, self.location)
+                .map(|value| value.deref::<T>())
+        }
     }
 
     /// Gets a mutable reference to the component of type `T` associated with
@@ -721,18 +724,6 @@ fn sorted_remove<T: Eq + Ord + Copy>(source: &mut Vec<T>, remove: &[T]) {
     });
 }
 
-// SAFETY: EntityLocation must be valid
-#[inline]
-pub(crate) unsafe fn get<T: Component>(
-    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::<T>(), entity, location)
-        .map(|value| value.deref::<T>())
-}
-
 // SAFETY: EntityLocation must be valid
 #[inline]
 pub(crate) unsafe fn get_mut<T: Component>(

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::<Position>().unwrap();
+    /// let position = world.entity(entity).get::<Position>().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::<Position>(entity).unwrap();
+    /// let position = world.entity(entity).get::<Position>().unwrap();
     /// assert_eq!(position.x, 0.0);
     /// ```
     pub fn spawn(&mut self) -> EntityMut {
@@ -415,7 +414,7 @@ impl World {
     /// ```
     #[inline]
     pub fn get<T: Component>(&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::<R>() })
     }
 
@@ -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::<R>() };
-        let value = Mut {
-            value: &mut val,
+        let mut value = unsafe { ptr.read::<R>() };
+        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::<R>());
 
         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::<R>()));
 
-        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<T>(&mut self, component_id: ComponentId, value: T) {
+    unsafe fn insert_resource_with_id<R>(&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::<T>() = value;
+            // SAFE: column is of type R and has already been allocated
+            *column.get_data_unchecked_mut(0).deref_mut::<R>() = value;
             column.get_ticks_unchecked_mut(0).set_changed(change_tick);
         }
     }