pointer-utils icon indicating copy to clipboard operation
pointer-utils copied to clipboard

Add 'with()' and 'with_mut()' as associated functions to ErasedPtr

Open cehteh opened this issue 2 years ago • 3 comments

This allows one to use ErasedPtr where rust won't allow a typed reference. For example in recursive types.

cehteh avatar Jan 31 '23 09:01 cehteh

I believe the API is sound, but the implementation isn't. The ManuallyDrop needs to be created directly around the unerased pointer to avoid dropping it, and the &mut version needs the drop guard used in Thin::with_mut to be sound if the callback unwinds.

Apparently, I configured miri to not run for PRs, but if you run cargo +nightly miri test locally, you should see the failure.

Additionally,

  • Since there's no potential other implementation for these functions, they're probably better off not on the Erasable trait.
  • If we're adding the functionality to Erasable/ErasedPtr, it's probably better for Thin to use that directly rather than a separate implementation.
  • I don't see the need for the functionality for recursive types; types can name and contain e.g. Rc<Self> just fine. I do agree that these functions are useful for working directly with ErasedPtr, but the more time you're holding a typed pointer that implements Drop the better.

CAD97 avatar Jan 31 '23 09:01 CAD97

Note: I should've made this WIP/RFC

I believe the API is sound, but the implementation isn't. The ManuallyDrop needs to be created directly around the unerased pointer to avoid dropping it, and the &mut version needs the drop guard used in Thin::with_mut to be sound if the callback unwinds.

OOps I was a bit in a hurry earlier, I'll fix this later.

Apparently, I configured miri to not run for PRs, but if you run cargo +nightly miri test locally, you should see the failure.

Additionally,

* Since there's no potential other implementation for these functions, they're probably better off not on the `Erasable` trait.

I agree there, just didn't want to make a too intrusive change. Is there any reason that ErasedPtr is a type alias rather than a newtype? I would prefer

#[repr(transparent)] 
pub struct ErasedPtr(ptr::NonNull<Erased>);

Which offers 2 advantages:

  1. erasable can itself impl this, like the 'with()/with_mut()' I am proposing.
  2. Users can define implement their own traits on ErasedPtr which (arguably) may add some ergonomics.
* If we're adding the functionality to `Erasable`/`ErasedPtr`, it's probably better for `Thin` to use that directly rather than a separate implementation.

Makes sense

* I don't see the need for the functionality for recursive types; types can name and contain e.g. `Rc<Self>` just fine.

I have a somewhat more convoluted problem here where i need to name/declare a recursive type which obliviously wont work.

I do agree that these functions are useful for working directly with ErasedPtr, but the more time you're holding a typed pointer that implements Drop the better.

Make me wonder if it would have some benefit to add some tricks to make sure that ErasedPtr (or likely some extra ErasedRequiresDropPtrThatNeedsABetterName) that panics when it wont be unerased (and potentially dropped)

Would be even nicer if this can be turned into a compile error, but i think that's not possible :/

cehteh avatar Jan 31 '23 18:01 cehteh

Fixed the ManuallyDrop/Guard issues. Still left the functions in the trait, I'd like to what you prefer, either making ErasedPtr a newtype as I suggested above or make with()/with_mut() toplevel functions under erasable::. When that is settled I'll provide the modifications for Thin.

cehteh avatar Feb 01 '23 20:02 cehteh