Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cehteh
Copy link

@cehteh cehteh commented Jan 31, 2023

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

@CAD97
Copy link
Owner

CAD97 commented Jan 31, 2023

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.

@cehteh
Copy link
Author

cehteh commented Jan 31, 2023

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 cehteh force-pushed the master branch 3 times, most recently from 494e8e3 to cd77ea9 Compare February 1, 2023 17:37
@cehteh
Copy link
Author

cehteh commented Feb 1, 2023

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.

This allows one to use ErasedPtr where rust won't allow a typed reference. For example in
recursive types.
* remove references of the unerased value
* fix memory leaks in tests by unerase/drop the erased values
 - 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants