From 3c5032d2babf97fdda91b13eb4730f14d91b93ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Th=C3=A4ter?= Date: Tue, 31 Jan 2023 10:06:57 +0100 Subject: [PATCH 1/7] Add 'with()' and 'with_mut()' as associated functions to ErasedPtr This allows one to use ErasedPtr where rust won't allow a typed reference. For example in recursive types. --- crates/erasable/src/lib.rs | 48 ++++++++++++++++++++++++++++++++++ crates/erasable/tests/smoke.rs | 27 +++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/crates/erasable/src/lib.rs b/crates/erasable/src/lib.rs index 8b15078..3c4ca3e 100644 --- a/crates/erasable/src/lib.rs +++ b/crates/erasable/src/lib.rs @@ -185,6 +185,54 @@ pub unsafe trait ErasablePtr { /// /// The erased pointer must have been created by `erase`. unsafe fn unerase(this: ErasedPtr) -> Self; + + /// Run a closure on a borrow of the real pointer. Unlike the `Thin` wrapper this does + /// not carry the original type around. Thus it is required to specify the original impl + /// type when calling this function. + /// + /// ``` + /// # use {erasable::*, std::rc::Rc}; + /// let rc: Rc = Rc::new(123); + /// + /// let erased: ErasedPtr = ErasablePtr::erase(rc); + /// + /// let cloned = unsafe { + /// as ErasablePtr>::with(&erased, |rc| rc.clone()) + /// }; + /// + /// assert_eq!(*cloned, 123); + /// ``` + /// + /// The main purpose of this function is to be able implement recursive types that would + /// be otherwise not representable in rust. + /// + /// # Safety + /// + /// * The erased pointer must have been created by `erase`. + /// * The specified impl type must be the original type. + unsafe fn with(this: &ErasedPtr, f: F) -> T + where + Self: Sized, + F: FnOnce(&Self) -> T, + { + f(&ManuallyDrop::new(&Self::unerase(*this))) + } + + /// Run a closure on a mutable borrow of the real pointer. Unlike the `Thin` wrapper + /// this does not carry the original type around. Thus it is required to specify the + /// original impl type when calling this function. + /// + /// # Safety + /// + /// * The erased pointer must have been created by `erase`. + /// * The specified impl type must be the original type. + unsafe fn with_mut(this: &mut ErasedPtr, f: F) -> T + where + Self: Sized, + F: FnOnce(&mut Self) -> T, + { + f(&mut ManuallyDrop::new(&mut Self::unerase(*this))) + } } /// A pointee type that supports type-erased pointers (thin pointers). diff --git a/crates/erasable/tests/smoke.rs b/crates/erasable/tests/smoke.rs index 6452a08..8868792 100644 --- a/crates/erasable/tests/smoke.rs +++ b/crates/erasable/tests/smoke.rs @@ -32,3 +32,30 @@ fn thinning() { Thin::with_mut(&mut thin, |thin| *thin = Default::default()); let boxed = Thin::into_inner(thin); } + +#[test] +fn withfn() { + let boxed: Box = Default::default(); + + let erased: ErasedPtr = ErasablePtr::erase(boxed); + + unsafe { + as ErasablePtr>::with(&erased, |bigbox| { + assert_eq!(*bigbox, Default::default()); + }) + } +} + +#[test] +fn with_mut_fn() { + let boxed: Box = Default::default(); + + let mut erased: ErasedPtr = ErasablePtr::erase(boxed); + + unsafe { + as ErasablePtr>::with_mut(&mut erased, |bigbox| { + bigbox.0[0] = 123456; + assert_ne!(*bigbox, Default::default()); + }) + } +} From 1492d5f5773d1f66a26b89963f2e0356a80554ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Th=C3=A4ter?= Date: Wed, 1 Feb 2023 18:23:26 +0100 Subject: [PATCH 2/7] Fix: ErasablePtr::with() * remove references of the unerased value * fix memory leaks in tests by unerase/drop the erased values --- crates/erasable/src/lib.rs | 3 ++- crates/erasable/tests/smoke.rs | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/erasable/src/lib.rs b/crates/erasable/src/lib.rs index 3c4ca3e..fa8f98a 100644 --- a/crates/erasable/src/lib.rs +++ b/crates/erasable/src/lib.rs @@ -201,6 +201,7 @@ pub unsafe trait ErasablePtr { /// }; /// /// assert_eq!(*cloned, 123); + /// # unsafe { as ErasablePtr>::unerase(erased)}; // drop it /// ``` /// /// The main purpose of this function is to be able implement recursive types that would @@ -215,7 +216,7 @@ pub unsafe trait ErasablePtr { Self: Sized, F: FnOnce(&Self) -> T, { - f(&ManuallyDrop::new(&Self::unerase(*this))) + f(&ManuallyDrop::new(Self::unerase(*this))) } /// Run a closure on a mutable borrow of the real pointer. Unlike the `Thin` wrapper diff --git a/crates/erasable/tests/smoke.rs b/crates/erasable/tests/smoke.rs index 8868792..62f07e1 100644 --- a/crates/erasable/tests/smoke.rs +++ b/crates/erasable/tests/smoke.rs @@ -34,7 +34,7 @@ fn thinning() { } #[test] -fn withfn() { +fn with_fn() { let boxed: Box = Default::default(); let erased: ErasedPtr = ErasablePtr::erase(boxed); @@ -44,6 +44,9 @@ fn withfn() { assert_eq!(*bigbox, Default::default()); }) } + + // drop it, otherwise we would leak memory here + unsafe { as ErasablePtr>::unerase(erased) }; } #[test] From 0288c33b4ce8bfafe97f6a3dac4938e4b12b2ab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Th=C3=A4ter?= Date: Wed, 1 Feb 2023 21:15:08 +0100 Subject: [PATCH 3/7] Fix: ErasedPtr::with_mut() - using scopeguard to write the pointer back into its original place - correct the ManuallyDrop() to wrap the actual value and take it out in the scopeguard --- crates/erasable/src/lib.rs | 7 ++++++- crates/erasable/tests/smoke.rs | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/crates/erasable/src/lib.rs b/crates/erasable/src/lib.rs index fa8f98a..6fc5693 100644 --- a/crates/erasable/src/lib.rs +++ b/crates/erasable/src/lib.rs @@ -232,7 +232,12 @@ pub unsafe trait ErasablePtr { Self: Sized, F: FnOnce(&mut Self) -> T, { - f(&mut ManuallyDrop::new(&mut Self::unerase(*this))) + // SAFETY: guard is required to write potentially changed pointer value, even on unwind + let mut that = scopeguard::guard(ManuallyDrop::new(Self::unerase(*this)), |unerased| { + ptr::write(this, ErasablePtr::erase(ManuallyDrop::into_inner(unerased))); + }); + + f(&mut that) } } diff --git a/crates/erasable/tests/smoke.rs b/crates/erasable/tests/smoke.rs index 62f07e1..4e7859e 100644 --- a/crates/erasable/tests/smoke.rs +++ b/crates/erasable/tests/smoke.rs @@ -61,4 +61,29 @@ fn with_mut_fn() { assert_ne!(*bigbox, Default::default()); }) } + + // drop it, otherwise we would leak memory here + unsafe { as ErasablePtr>::unerase(erased) }; +} + +#[test] +fn with_mut_fn_replacethis() { + let boxed: Box = Default::default(); + + let mut erased: ErasedPtr = ErasablePtr::erase(boxed); + let e1 = erased.as_ptr() as usize; + unsafe { + as ErasablePtr>::with_mut(&mut erased, |bigbox| { + let mut newboxed: Box = Default::default(); + newboxed.0[0] = 123456; + *bigbox = newboxed; + assert_ne!(*bigbox, Default::default()); + }) + } + + let e2 = erased.as_ptr() as usize; + assert_ne!(e1, e2); + + // drop it, otherwise we would leak memory here + unsafe { as ErasablePtr>::unerase(erased) }; } From 25524cbf9ec6fab31ece02de831d0dee0c7c2d74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Th=C3=A4ter?= Date: Wed, 1 Feb 2023 21:28:25 +0100 Subject: [PATCH 4/7] cleanup, no need for usize conversion (debug artifact) --- crates/erasable/tests/smoke.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/erasable/tests/smoke.rs b/crates/erasable/tests/smoke.rs index 4e7859e..b0f3c59 100644 --- a/crates/erasable/tests/smoke.rs +++ b/crates/erasable/tests/smoke.rs @@ -71,7 +71,7 @@ fn with_mut_fn_replacethis() { let boxed: Box = Default::default(); let mut erased: ErasedPtr = ErasablePtr::erase(boxed); - let e1 = erased.as_ptr() as usize; + let e1 = erased.as_ptr(); unsafe { as ErasablePtr>::with_mut(&mut erased, |bigbox| { let mut newboxed: Box = Default::default(); @@ -81,7 +81,7 @@ fn with_mut_fn_replacethis() { }) } - let e2 = erased.as_ptr() as usize; + let e2 = erased.as_ptr(); assert_ne!(e1, e2); // drop it, otherwise we would leak memory here From 69f7b4819b81db65174a34db54a98980dc67c93e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Th=C3=A4ter?= Date: Thu, 2 Feb 2023 10:03:01 +0100 Subject: [PATCH 5/7] implement ErasedPtr as newtype This changes ErasedPtr from type alias to a newtype implementation. Thus ErasedPtr implements few methods now: * `as_unit_ptr(&self) -> *const ()` getting the pointer value for diagnostics. * `with(FnOnce(&E)-> T)` and `with_mut(FnOnce(&E)-> T)` are moved from the Eraseable trait to ErasedPtr. --- crates/erasable/src/lib.rs | 126 ++++++++++++++++++--------------- crates/erasable/tests/smoke.rs | 23 ++++-- 2 files changed, 86 insertions(+), 63 deletions(-) diff --git a/crates/erasable/src/lib.rs b/crates/erasable/src/lib.rs index 6fc5693..10e2cb7 100644 --- a/crates/erasable/src/lib.rs +++ b/crates/erasable/src/lib.rs @@ -47,7 +47,73 @@ use core::{ /// The current implementation uses a `struct Erased` with size 0 and align 1. /// If you want to offset the pointer, make sure to cast to a `u8` or other known type pointer first. /// When `Erased` becomes an extern type, it will properly have unknown size and align. -pub type ErasedPtr = ptr::NonNull; +#[derive(Debug, Copy, Clone, PartialEq)] +#[repr(transparent)] +pub struct ErasedPtr(ptr::NonNull); + +impl ErasedPtr { + /// Gets the raw pointer as '*const ()' unit type. This keeps the internal representation + /// hidden and is not very useful but for diagnostics like logging memory addresses and + /// comparing pointers for partial equality. + pub fn as_unit_ptr(&self) -> *const () { + self.0.as_ptr() as *const _ + } + + /// Run a closure on a borrow of the real pointer. Unlike the `Thin` wrapper this does + /// not carry the original type around. Thus it is required to specify the original impl + /// type as closure parameter. + /// + /// ``` + /// # use {erasable::*, std::rc::Rc}; + /// let rc: Rc = Rc::new(123); + /// + /// let erased: ErasedPtr = ErasablePtr::erase(rc); + /// + /// let cloned = unsafe { + /// // must specify a reference to the original type here + /// erased.with(|rc: &Rc| rc.clone()) + /// }; + /// + /// assert_eq!(*cloned, 123); + /// # unsafe { as ErasablePtr>::unerase(erased)}; // drop it + /// ``` + /// + /// # Safety + /// + /// * The erased pointer must have been created by `erase`. + /// * The specified impl type must be the original type. + pub unsafe fn with(&self, f: F) -> T + where + E: ErasablePtr, + F: FnOnce(&E) -> T, + { + f(&ManuallyDrop::new(::unerase(*self))) + } + + /// Run a closure on a mutable borrow of the real pointer. Unlike the `Thin` wrapper + /// this does not carry the original type around. Thus it is required to specify the + /// original impl type as closure parameter. + /// + /// # Safety + /// + /// * The erased pointer must have been created by `erase`. + /// * The specified impl type must be the original type. + pub unsafe fn with_mut(&mut self, f: F) -> T + where + E: ErasablePtr, + F: FnOnce(&mut E) -> T, + { + // SAFETY: guard is required to write potentially changed pointer value, even on unwind + let mut this = scopeguard::guard( + ManuallyDrop::new(::unerase(*self)), + |unerased| { + ptr::write(self, ErasablePtr::erase(ManuallyDrop::into_inner(unerased))); + }, + ); + + f(&mut this) + } +} #[cfg(not(has_extern_type))] pub(crate) use priv_in_pub::Erased; @@ -185,60 +251,6 @@ pub unsafe trait ErasablePtr { /// /// The erased pointer must have been created by `erase`. unsafe fn unerase(this: ErasedPtr) -> Self; - - /// Run a closure on a borrow of the real pointer. Unlike the `Thin` wrapper this does - /// not carry the original type around. Thus it is required to specify the original impl - /// type when calling this function. - /// - /// ``` - /// # use {erasable::*, std::rc::Rc}; - /// let rc: Rc = Rc::new(123); - /// - /// let erased: ErasedPtr = ErasablePtr::erase(rc); - /// - /// let cloned = unsafe { - /// as ErasablePtr>::with(&erased, |rc| rc.clone()) - /// }; - /// - /// assert_eq!(*cloned, 123); - /// # unsafe { as ErasablePtr>::unerase(erased)}; // drop it - /// ``` - /// - /// The main purpose of this function is to be able implement recursive types that would - /// be otherwise not representable in rust. - /// - /// # Safety - /// - /// * The erased pointer must have been created by `erase`. - /// * The specified impl type must be the original type. - unsafe fn with(this: &ErasedPtr, f: F) -> T - where - Self: Sized, - F: FnOnce(&Self) -> T, - { - f(&ManuallyDrop::new(Self::unerase(*this))) - } - - /// Run a closure on a mutable borrow of the real pointer. Unlike the `Thin` wrapper - /// this does not carry the original type around. Thus it is required to specify the - /// original impl type when calling this function. - /// - /// # Safety - /// - /// * The erased pointer must have been created by `erase`. - /// * The specified impl type must be the original type. - unsafe fn with_mut(this: &mut ErasedPtr, f: F) -> T - where - Self: Sized, - F: FnOnce(&mut Self) -> T, - { - // SAFETY: guard is required to write potentially changed pointer value, even on unwind - let mut that = scopeguard::guard(ManuallyDrop::new(Self::unerase(*this)), |unerased| { - ptr::write(this, ErasablePtr::erase(ManuallyDrop::into_inner(unerased))); - }); - - f(&mut that) - } } /// A pointee type that supports type-erased pointers (thin pointers). @@ -327,7 +339,7 @@ pub unsafe trait Erasable { /// Erase a pointer. #[inline(always)] pub fn erase(ptr: ptr::NonNull) -> ErasedPtr { - unsafe { ptr::NonNull::new_unchecked(ptr.as_ptr() as *mut Erased) } + unsafe { ErasedPtr(ptr::NonNull::new_unchecked(ptr.as_ptr() as *mut Erased)) } } /// Wrapper struct to create thin pointer types. @@ -670,7 +682,7 @@ where unsafe impl Erasable for T { unsafe fn unerase(this: ErasedPtr) -> ptr::NonNull { // SAFETY: must not read the pointer for the safety of the impl directly below. - this.cast() + this.0.cast() } const ACK_1_1_0: bool = true; diff --git a/crates/erasable/tests/smoke.rs b/crates/erasable/tests/smoke.rs index b0f3c59..4c6c728 100644 --- a/crates/erasable/tests/smoke.rs +++ b/crates/erasable/tests/smoke.rs @@ -13,7 +13,7 @@ fn erasing() { let boxed: Box = Box::new(Big::default()); let ptr = &*boxed as *const _ as usize; let erased: ErasedPtr = ErasablePtr::erase(boxed); - assert_eq!(erased.as_ptr() as usize, ptr); + assert_eq!(erased.as_unit_ptr() as usize, ptr); let boxed: Box = unsafe { ErasablePtr::unerase(erased) }; assert_eq!(&*boxed as *const _ as usize, ptr); } @@ -40,7 +40,17 @@ fn with_fn() { let erased: ErasedPtr = ErasablePtr::erase(boxed); unsafe { - as ErasablePtr>::with(&erased, |bigbox| { + // clippy errs here: + // warning: you seem to be trying to use `&Box`. Consider using just `&T` + // --> crates/erasable/tests/smoke.rs:45:30 + // | + // 45 | erased.with(|bigbox: &Box| { + // | ^^^^^^^^^ help: try: `&Big` + // + // We really need to borrow a &Box in this case because that what we constructed + // the ErasedPtr from. + #[allow(clippy::borrowed_box)] + erased.with(|bigbox: &Box| { assert_eq!(*bigbox, Default::default()); }) } @@ -56,7 +66,7 @@ fn with_mut_fn() { let mut erased: ErasedPtr = ErasablePtr::erase(boxed); unsafe { - as ErasablePtr>::with_mut(&mut erased, |bigbox| { + erased.with_mut(|bigbox: &mut Box| { bigbox.0[0] = 123456; assert_ne!(*bigbox, Default::default()); }) @@ -71,9 +81,10 @@ fn with_mut_fn_replacethis() { let boxed: Box = Default::default(); let mut erased: ErasedPtr = ErasablePtr::erase(boxed); - let e1 = erased.as_ptr(); + let e1 = erased.as_unit_ptr(); + unsafe { - as ErasablePtr>::with_mut(&mut erased, |bigbox| { + erased.with_mut(|bigbox: &mut Box| { let mut newboxed: Box = Default::default(); newboxed.0[0] = 123456; *bigbox = newboxed; @@ -81,7 +92,7 @@ fn with_mut_fn_replacethis() { }) } - let e2 = erased.as_ptr(); + let e2 = erased.as_unit_ptr(); assert_ne!(e1, e2); // drop it, otherwise we would leak memory here From 7846a3531f4d649c35a44d759c5f95778a387617 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Th=C3=A4ter?= Date: Thu, 2 Feb 2023 10:37:54 +0100 Subject: [PATCH 6/7] make Thin::with()/with_mut() use ErasedPtr::with()/with_mut() --- crates/erasable/src/lib.rs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/crates/erasable/src/lib.rs b/crates/erasable/src/lib.rs index 10e2cb7..187034c 100644 --- a/crates/erasable/src/lib.rs +++ b/crates/erasable/src/lib.rs @@ -380,10 +380,6 @@ impl From

for Thin

{ } impl Thin

{ - fn inner(this: &Self) -> ManuallyDrop

{ - unsafe { ManuallyDrop::new(P::unerase(this.ptr)) } - } - // noinspection RsSelfConvention // `From` can't be impl'd because it's an impl on an uncovered type // `Into` can't be impl'd because it conflicts with the reflexive impl @@ -397,7 +393,8 @@ impl Thin

{ where F: FnOnce(&P) -> T, { - f(&Thin::inner(this)) + // SAFETY: The type &P is always correct here + unsafe { this.ptr.with(f) } } /// Run a closure with a mutable borrow of the real pointer. @@ -405,13 +402,8 @@ impl Thin

{ where F: FnOnce(&mut P) -> T, { - // SAFETY: guard is required to write potentially changed pointer value, even on unwind - let mut this = unsafe { - scopeguard::guard(P::unerase(this.ptr), |unerased| { - ptr::write(this, Thin::from(unerased)) - }) - }; - f(&mut this) + // SAFETY: The type &P is always correct here + unsafe { this.ptr.with_mut(f) } } /// Check two thin pointers for pointer equivalence. From 3c3c5f1c94d2b14654eb105b830a22f614290b92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Th=C3=A4ter?= Date: Sat, 4 Feb 2023 07:03:16 +0100 Subject: [PATCH 7/7] inline `as_unit_ptr()` and `with()` --- crates/erasable/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/erasable/src/lib.rs b/crates/erasable/src/lib.rs index 187034c..dea1b86 100644 --- a/crates/erasable/src/lib.rs +++ b/crates/erasable/src/lib.rs @@ -55,6 +55,7 @@ impl ErasedPtr { /// Gets the raw pointer as '*const ()' unit type. This keeps the internal representation /// hidden and is not very useful but for diagnostics like logging memory addresses and /// comparing pointers for partial equality. + #[inline(always)] pub fn as_unit_ptr(&self) -> *const () { self.0.as_ptr() as *const _ } @@ -82,6 +83,7 @@ impl ErasedPtr { /// /// * The erased pointer must have been created by `erase`. /// * The specified impl type must be the original type. + #[inline(always)] pub unsafe fn with(&self, f: F) -> T where E: ErasablePtr,