Pointerfication followup: Type safety and cleanup (#4621)

# Objective
The `Ptr` types gives free access to the underlying `NonNull<u8>`, which adds more publicly visible pointer wrangling than there needs to be. There are also a few edge cases where Ptr types could be more readily utilized for properly validating the soundness of ECS operations.

## Solution
 - Replace `*Ptr(Mut)::inner` with `cast` which requires a concrete type to give the pointer. This function could also have a `debug_assert` with an alignment check to ensure that the pointer is aligned properly, but is currently not included.
 - Use `OwningPtr::read` in ECS macros over casting the inner pointer around.

Author: james7132

crates/bevy_ecs/macros/src/lib.rs

@@ -134,7 +134,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
                 #ecs_path::ptr::OwningPtr::make(self.#field, &mut func);
             });
             field_from_components.push(quote! {
-                #field: func(ctx).inner().as_ptr().cast::<#field_type>().read(),
+                #field: func(ctx).read::<#field_type>(),
             });
         }
     }

crates/bevy_ecs/src/bundle.rs

@@ -112,10 +112,7 @@ macro_rules! tuple_impl {
                 F: FnMut(&mut T) -> OwningPtr<'_>
             {
                 #[allow(non_snake_case)]
-                let ($(mut $name,)*) = (
-                    $(func(ctx).inner().cast::<$name>(),)*
-                );
-                ($($name.as_ptr().read(),)*)
+                ($(func(ctx).read::<$name>(),)*)
             }
 
             #[allow(unused_variables, unused_mut)]

crates/bevy_ecs/src/component.rs

@@ -186,7 +186,7 @@ impl std::fmt::Debug for ComponentDescriptor {
 impl ComponentDescriptor {
     // SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value.
     unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {
-        x.inner().cast::<T>().as_ptr().drop_in_place()
+        x.drop_as::<T>()
     }
 
     pub fn new<T: Component>() -> Self {

crates/bevy_ecs/src/ptr.rs

@@ -43,57 +43,86 @@ pub struct OwningPtr<'a>(NonNull<u8>, PhantomData<&'a mut u8>);
 macro_rules! impl_ptr {
     ($ptr:ident) => {
         impl $ptr<'_> {
+            /// Calculates the offset from a pointer.
+            /// As the pointer is type-erased, there is no size information available. The provided
+            /// `count` parameter is in raw bytes.
+            ///
+            /// *See also: [`ptr::offset`][ptr_offset]*
+            ///
             /// # Safety
             /// the offset cannot make the existing ptr null, or take it out of bounds for its allocation.
+            ///
+            /// [ptr_offset]: https://doc.rust-lang.org/std/primitive.pointer.html#method.offset
             #[inline]
             pub unsafe fn offset(self, count: isize) -> Self {
                 Self(
-                    NonNull::new_unchecked(self.0.as_ptr().offset(count)),
+                    NonNull::new_unchecked(self.as_ptr().offset(count)),
                     PhantomData,
                 )
             }
 
+            /// Calculates the offset from a pointer (convenience for `.offset(count as isize)`).
+            /// As the pointer is type-erased, there is no size information available. The provided
+            /// `count` parameter is in raw bytes.
+            ///
+            /// *See also: [`ptr::add`][ptr_add]*
+            ///
             /// # Safety
             /// the offset cannot make the existing ptr null, or take it out of bounds for its allocation.
+            ///
+            /// [ptr_add]: https://doc.rust-lang.org/std/primitive.pointer.html#method.add
             #[inline]
             pub unsafe fn add(self, count: usize) -> Self {
                 Self(
-                    NonNull::new_unchecked(self.0.as_ptr().add(count)),
+                    NonNull::new_unchecked(self.as_ptr().add(count)),
                     PhantomData,
                 )
             }
 
-            /// # Safety
+            /// Creates a new instance from a raw pointer.
             ///
+            /// # Safety
             /// The lifetime for the returned item must not exceed the lifetime `inner` is valid for
             #[inline]
             pub unsafe fn new(inner: NonNull<u8>) -> Self {
                 Self(inner, PhantomData)
             }
-
-            #[inline]
-            pub fn inner(&self) -> NonNull<u8> {
-                self.0
-            }
         }
     };
 }
 
 impl_ptr!(Ptr);
 impl<'a> Ptr<'a> {
-    /// # Safety
+    /// Transforms this [`Ptr`] into an [`PtrMut`]
     ///
+    /// # Safety
     /// Another [`PtrMut`] for the same [`Ptr`] must not be created until the first is dropped.
     #[inline]
     pub unsafe fn assert_unique(self) -> PtrMut<'a> {
         PtrMut(self.0, PhantomData)
     }
 
+    /// Transforms this [`Ptr<T>`] into a `&T` with the same lifetime
+    ///
     /// # Safety
     /// Must point to a valid `T`
     #[inline]
     pub unsafe fn deref<T>(self) -> &'a T {
-        &*self.0.as_ptr().cast()
+        &*self.as_ptr().cast()
+    }
+
+    /// Gets the underlying pointer, erasing the associated lifetime.
+    ///
+    /// If possible, it is strongly encouraged to use [`deref`](Self::deref) over this function,
+    /// as it retains the lifetime.
+    ///
+    /// # Safety
+    /// All subsequent operations to the returned pointer must be valid inside the
+    /// associated lifetime.
+    #[inline]
+    #[allow(clippy::wrong_self_convention)]
+    pub unsafe fn as_ptr(self) -> *mut u8 {
+        self.0.as_ptr()
     }
 }
 impl_ptr!(PtrMut);
@@ -113,7 +142,21 @@ impl<'a> PtrMut<'a> {
     /// Must point to a valid `T`
     #[inline]
     pub unsafe fn deref_mut<T>(self) -> &'a mut T {
-        &mut *self.inner().as_ptr().cast()
+        &mut *self.as_ptr().cast()
+    }
+
+    /// Gets the underlying pointer, erasing the associated lifetime.
+    ///
+    /// If possible, it is strongly encouraged to use [`deref_mut`](Self::deref_mut) over
+    /// this function, as it retains the lifetime.
+    ///
+    /// # Safety
+    /// All subsequent operations to the returned pointer must be valid inside the
+    /// associated lifetime.
+    #[inline]
+    #[allow(clippy::wrong_self_convention)]
+    pub unsafe fn as_ptr(self) -> *mut u8 {
+        self.0.as_ptr()
     }
 }
 impl_ptr!(OwningPtr);
@@ -132,7 +175,30 @@ impl<'a> OwningPtr<'a> {
     /// Must point to a valid `T`.
     #[inline]
     pub unsafe fn read<T>(self) -> T {
-        self.inner().as_ptr().cast::<T>().read()
+        self.as_ptr().cast::<T>().read()
+    }
+
+    //// Consumes the [`OwningPtr`] to drop the underlying data of type `T`.
+    ///
+    /// # Safety
+    /// Must point to a valid `T`.
+    #[inline]
+    pub unsafe fn drop_as<T>(self) {
+        self.as_ptr().cast::<T>().drop_in_place()
+    }
+
+    /// Gets the underlying pointer, erasing the associated lifetime.
+    ///
+    /// If possible, it is strongly encouraged to use the other more type-safe functions
+    /// over this function.
+    ///
+    /// # Safety
+    /// All subsequent operations to the returned pointer must be valid inside the
+    /// associated lifetime.
+    #[inline]
+    #[allow(clippy::wrong_self_convention)]
+    pub unsafe fn as_ptr(self) -> *mut u8 {
+        self.0.as_ptr()
     }
 }
 

crates/bevy_ecs/src/storage/blob_vec.rs

@@ -101,7 +101,7 @@ impl BlobVec {
                 std::alloc::alloc(new_layout)
             } else {
                 std::alloc::realloc(
-                    self.get_ptr_mut().inner().as_ptr(),
+                    self.get_ptr_mut().as_ptr(),
                     array_layout(&self.item_layout, self.capacity)
                         .expect("array layout should be valid"),
                     new_layout.size(),
@@ -121,11 +121,7 @@ impl BlobVec {
     pub unsafe fn initialize_unchecked(&mut self, index: usize, value: OwningPtr<'_>) {
         debug_assert!(index < self.len());
         let ptr = self.get_unchecked_mut(index);
-        std::ptr::copy_nonoverlapping::<u8>(
-            value.inner().as_ptr(),
-            ptr.inner().as_ptr(),
-            self.item_layout.size(),
-        );
+        std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr.as_ptr(), self.item_layout.size());
     }
 
     /// # Safety
@@ -142,15 +138,11 @@ impl BlobVec {
         // in the collection), so we get a double drop. To prevent that, we set len to 0 until we're
         // done.
         let old_len = self.len;
-        let ptr = self.get_unchecked_mut(index).promote().inner();
+        let ptr = self.get_unchecked_mut(index).promote().as_ptr();
         self.len = 0;
         // Drop the old value, then write back, justifying the promotion
-        (self.drop)(OwningPtr::new(ptr));
-        std::ptr::copy_nonoverlapping::<u8>(
-            value.inner().as_ptr(),
-            ptr.as_ptr(),
-            self.item_layout.size(),
-        );
+        (self.drop)(OwningPtr::new(NonNull::new_unchecked(ptr)));
+        std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr, self.item_layout.size());
         self.len = old_len;
     }
 
@@ -192,13 +184,13 @@ impl BlobVec {
         let last = self.len - 1;
         let swap_scratch = self.swap_scratch.as_ptr();
         std::ptr::copy_nonoverlapping::<u8>(
-            self.get_unchecked_mut(index).inner().as_ptr(),
+            self.get_unchecked_mut(index).as_ptr(),
             swap_scratch,
             self.item_layout.size(),
         );
         std::ptr::copy::<u8>(
-            self.get_unchecked_mut(last).inner().as_ptr(),
-            self.get_unchecked_mut(index).inner().as_ptr(),
+            self.get_unchecked_mut(last).as_ptr(),
+            self.get_unchecked_mut(index).as_ptr(),
             self.item_layout.size(),
         );
         self.len -= 1;
@@ -280,7 +272,7 @@ impl Drop for BlobVec {
             array_layout(&self.item_layout, self.capacity).expect("array layout should be valid");
         if array_layout.size() > 0 {
             unsafe {
-                std::alloc::dealloc(self.get_ptr_mut().inner().as_ptr(), array_layout);
+                std::alloc::dealloc(self.get_ptr_mut().as_ptr(), array_layout);
                 std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout);
             }
         }
@@ -350,7 +342,7 @@ mod tests {
 
     // SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value.
     unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {
-        x.inner().cast::<T>().as_ptr().drop_in_place()
+        x.drop_as::<T>()
     }
 
     /// # Safety