From e08d799b16779e3d46ee600edd3206022588bdd6 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 10 Nov 2022 11:54:52 -0500 Subject: [PATCH 1/7] add safe conversions to untyped pointers --- crates/bevy_ptr/src/lib.rs | 14 ++++++++++++++ crates/bevy_reflect/src/type_registry.rs | 11 +++-------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 8f1f476a906e2..50d89e8a2ffda 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -92,6 +92,12 @@ macro_rules! impl_ptr { Self(inner, PhantomData) } } + impl<'a, T> From<&'a T> for $ptr<'a> { + #[inline] + fn from(val: &'a T) -> Self { + unsafe { Self::new(NonNull::from(val).cast()) } + } + } }; } @@ -125,6 +131,7 @@ impl<'a> Ptr<'a> { self.0.as_ptr() } } + impl_ptr!(PtrMut); impl<'a> PtrMut<'a> { /// Transforms this [`PtrMut`] into an [`OwningPtr`] @@ -155,6 +162,13 @@ impl<'a> PtrMut<'a> { self.0.as_ptr() } } +impl<'a, T> From<&'a mut T> for PtrMut<'a> { + #[inline] + fn from(val: &'a mut T) -> Self { + unsafe { Self::new(NonNull::from(val).cast()) } + } +} + impl_ptr!(OwningPtr); impl<'a> OwningPtr<'a> { /// Consumes a value and creates an [`OwningPtr`] to it while ensuring a double drop does not happen. diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs index 00f3bdb2f7177..2a6fc6f8168be 100644 --- a/crates/bevy_reflect/src/type_registry.rs +++ b/crates/bevy_reflect/src/type_registry.rs @@ -475,7 +475,7 @@ impl Deserialize<'a> + Reflect> FromType for ReflectDeserialize { /// type_registry.register::(); /// /// let mut value = Reflected("Hello world!".to_string()); -/// let value = unsafe { Ptr::new(NonNull::from(&mut value).cast()) }; +/// let value = Ptr::from(&value); /// /// let reflect_data = type_registry.get(std::any::TypeId::of::()).unwrap(); /// let reflect_from_ptr = reflect_data.data::().unwrap(); @@ -534,8 +534,6 @@ impl FromType for ReflectFromPtr { #[cfg(test)] mod test { - use std::ptr::NonNull; - use crate::{GetTypeRegistration, ReflectFromPtr, TypeRegistration}; use bevy_ptr::{Ptr, PtrMut}; use bevy_utils::HashMap; @@ -560,8 +558,7 @@ mod test { let mut value = Foo { a: 1.0 }; { - // SAFETY: lifetime doesn't outlive original value, access is unique - let value = unsafe { PtrMut::new(NonNull::from(&mut value).cast()) }; + let value = PtrMut::from(&mut value); // SAFETY: reflect_from_ptr was constructed for the correct type let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr_mut(value) }; match dyn_reflect.reflect_mut() { @@ -573,10 +570,8 @@ mod test { } { - // SAFETY: lifetime doesn't outlive original value - let value = unsafe { Ptr::new(NonNull::from(&mut value).cast()) }; // SAFETY: reflect_from_ptr was constructed for the correct type - let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr(value) }; + let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr(Ptr::from(&value)) }; match dyn_reflect.reflect_ref() { bevy_reflect::ReflectRef::Struct(strukt) => { let a = strukt.field("a").unwrap().downcast_ref::().unwrap(); From 5e35ab69f499777023d6b4408b84bbc4c8f216c0 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 10 Nov 2022 12:18:39 -0500 Subject: [PATCH 2/7] Add safety comments --- crates/bevy_ptr/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 50d89e8a2ffda..853b47825af65 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -95,6 +95,7 @@ macro_rules! impl_ptr { impl<'a, T> From<&'a T> for $ptr<'a> { #[inline] fn from(val: &'a T) -> Self { + // SAFETY: The returned pointer has the same lifetime as the passed reference. unsafe { Self::new(NonNull::from(val).cast()) } } } @@ -165,6 +166,7 @@ impl<'a> PtrMut<'a> { impl<'a, T> From<&'a mut T> for PtrMut<'a> { #[inline] fn from(val: &'a mut T) -> Self { + // SAFETY: The returned pointer has the same lifetime as the passed reference. unsafe { Self::new(NonNull::from(val).cast()) } } } From 4bc754d35d0c50b1403c7dd8966f75878d8a2d2f Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 10 Nov 2022 12:28:39 -0500 Subject: [PATCH 3/7] remove a definition from `impl_ptr!` --- crates/bevy_ptr/src/lib.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 853b47825af65..c728a8deb1356 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -92,13 +92,6 @@ macro_rules! impl_ptr { Self(inner, PhantomData) } } - impl<'a, T> From<&'a T> for $ptr<'a> { - #[inline] - fn from(val: &'a T) -> Self { - // SAFETY: The returned pointer has the same lifetime as the passed reference. - unsafe { Self::new(NonNull::from(val).cast()) } - } - } }; } @@ -132,6 +125,13 @@ impl<'a> Ptr<'a> { self.0.as_ptr() } } +impl<'a, T> From<&'a T> for Ptr<'a> { + #[inline] + fn from(val: &'a T) -> Self { + // SAFETY: The returned pointer has the same lifetime as the passed reference. + unsafe { Self::new(NonNull::from(val).cast()) } + } +} impl_ptr!(PtrMut); impl<'a> PtrMut<'a> { From 73d3810c6ad520ee0692f7828767a0dec2cb2c56 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 10 Nov 2022 14:17:30 -0500 Subject: [PATCH 4/7] simplify `OwningPtr::make` --- crates/bevy_ptr/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index c728a8deb1356..d2f63ffe0f50d 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -2,7 +2,7 @@ #![no_std] #![warn(missing_docs)] -use core::{cell::UnsafeCell, marker::PhantomData, mem::MaybeUninit, ptr::NonNull}; +use core::{cell::UnsafeCell, marker::PhantomData, mem::ManuallyDrop, ptr::NonNull}; /// Type-erased borrow of some unknown type chosen when constructing this type. /// @@ -176,10 +176,10 @@ impl<'a> OwningPtr<'a> { /// Consumes a value and creates an [`OwningPtr`] to it while ensuring a double drop does not happen. #[inline] pub fn make) -> R, R>(val: T, f: F) -> R { - let mut temp = MaybeUninit::new(val); - // SAFETY: `temp.as_mut_ptr()` is a reference to a local value on the stack, so it cannot be null - let ptr = unsafe { NonNull::new_unchecked(temp.as_mut_ptr().cast::()) }; - f(Self(ptr, PhantomData)) + let mut temp = ManuallyDrop::new(val); + // SAFETY: The value behind the pointer will not get dropped or observed later, + // so it's safe to promote it to an owning pointer. + f(unsafe { PtrMut::from(&mut temp).promote() }) } /// Consumes the [`OwningPtr`] to obtain ownership of the underlying data of type `T`. From cbacd8e59ec5863683cb35cddd4fed1e81b9d129 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 10 Nov 2022 18:03:57 -0500 Subject: [PATCH 5/7] safety --- crates/bevy_ptr/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index d2f63ffe0f50d..ded82e5126aff 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -129,6 +129,7 @@ impl<'a, T> From<&'a T> for Ptr<'a> { #[inline] fn from(val: &'a T) -> Self { // SAFETY: The returned pointer has the same lifetime as the passed reference. + // Access is immutable. unsafe { Self::new(NonNull::from(val).cast()) } } } @@ -167,6 +168,7 @@ impl<'a, T> From<&'a mut T> for PtrMut<'a> { #[inline] fn from(val: &'a mut T) -> Self { // SAFETY: The returned pointer has the same lifetime as the passed reference. + // The reference is mutable, and thus will not alias. unsafe { Self::new(NonNull::from(val).cast()) } } } From 61aa47cb49e1dd0c7df2515f3a487eff8d1e782c Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 10 Nov 2022 18:05:08 -0500 Subject: [PATCH 6/7] spacing --- crates/bevy_ptr/src/lib.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index ded82e5126aff..2f373c9a819c7 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -96,6 +96,9 @@ macro_rules! impl_ptr { } impl_ptr!(Ptr); +impl_ptr!(PtrMut); +impl_ptr!(OwningPtr); + impl<'a> Ptr<'a> { /// Transforms this [`Ptr`] into an [`PtrMut`] /// @@ -125,6 +128,7 @@ impl<'a> Ptr<'a> { self.0.as_ptr() } } + impl<'a, T> From<&'a T> for Ptr<'a> { #[inline] fn from(val: &'a T) -> Self { @@ -134,7 +138,6 @@ impl<'a, T> From<&'a T> for Ptr<'a> { } } -impl_ptr!(PtrMut); impl<'a> PtrMut<'a> { /// Transforms this [`PtrMut`] into an [`OwningPtr`] /// @@ -164,6 +167,7 @@ impl<'a> PtrMut<'a> { self.0.as_ptr() } } + impl<'a, T> From<&'a mut T> for PtrMut<'a> { #[inline] fn from(val: &'a mut T) -> Self { @@ -173,7 +177,6 @@ impl<'a, T> From<&'a mut T> for PtrMut<'a> { } } -impl_ptr!(OwningPtr); impl<'a> OwningPtr<'a> { /// Consumes a value and creates an [`OwningPtr`] to it while ensuring a double drop does not happen. #[inline] From 124a9d6e973b15b5198cd05f781cbd9f5d166d19 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sat, 12 Nov 2022 22:39:59 -0500 Subject: [PATCH 7/7] use an explicit deref --- crates/bevy_ptr/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ptr/src/lib.rs b/crates/bevy_ptr/src/lib.rs index 2f373c9a819c7..c71dd2d23238c 100644 --- a/crates/bevy_ptr/src/lib.rs +++ b/crates/bevy_ptr/src/lib.rs @@ -184,7 +184,7 @@ impl<'a> OwningPtr<'a> { let mut temp = ManuallyDrop::new(val); // SAFETY: The value behind the pointer will not get dropped or observed later, // so it's safe to promote it to an owning pointer. - f(unsafe { PtrMut::from(&mut temp).promote() }) + f(unsafe { PtrMut::from(&mut *temp).promote() }) } /// Consumes the [`OwningPtr`] to obtain ownership of the underlying data of type `T`.