Rust: Tracking issue for `ManuallyDrop::take`

[CAD97]

This is a tracking issue for the function ManuallyDrop::take, gated on the feature manually_drop_take.

Steps:

• [X] Implementation (#55421)
• [ ] Adjust documentation (#68066)
• [ ] Stabilization PR (#68066)

Unresolved questions:

• [X] Naming (https://github.com/rust-lang/rust/issues/55422#issuecomment-572653609)
• [X] Should this function take &Self or &mut Self? (https://github.com/rust-lang/rust/issues/55422#issuecomment-565860100)

[RalfJung]

I am a bit worried about the name of this method being too innocent-looking. We have take as a safe method in other places, like Option and Iterator. This leaves the fact that the method is unsafe as the only safeguard -- which is a problem e.g. when this gets (accidentally) used inside an unsafe fn or in a large unsafe block.

I'd prefer a name that already indicates this to be a dangerous operation. Strawman: take_unchecked because it doesn't check you are taking twice?

[CAD97]

Note that it has to be written as ManuallyDrop::take, which eliminates some of the risk of using a familiar name, as it's attached to ManuallyDrop, which is known to have interesting semantics, and thus is hard to use accidentally.

(Procedural note: I'll update OP with useful information soon.)

[SimonSapin]

@RalfJung I’m sympathetic to this argument, but we also have ManuallyDrop::drop which is (almost) as innocent-looking, and also doesn’t check if we are dropping twice.

(I also notice that ManuallyDrop::into_inner is safe, even though it would be unsound to call after drop or take.)

So I’m inclined to FCP to stabilize as-is. Or do you feel strongly we should rename?

[RalfJung]

No, not very strongly. And not being able to say foo.take() helps a lot.

[stjepang]

Anything blocking stabilization? I've wanted this method from time to time.

[stjepang]

The naming issue might be related to MaybeUninit::into_initialized() vs MaybeUninit::read_initialized(). Would it make sense to name the method here ManuallyDrop::read()?

[scottmcm]

Anything that needs to be resolved for this to stabilize, libs?

I just ended up here because someone on discord was just using mem::replace(x, mem::uninitialized()) (😭) where hopefully they would have just used this if it were stable.

[RalfJung]

The corresponding (unstable) method on MaybeUninit is (currently) called "read", because the safety concerns are a lot like the ones of ptr::read. Would the same make sense here as well?

[SimonSapin]

This sounds reasonable. In fact both methods are implemented based on ptr::read. We can repeat it again in the docs:

Like std::ptr::read, this function semantically moves […]

@rust-lang/libs, what do you think?

[CAD97]

Documentation is updated and rename is done in #62198. I've also finally updated the OP with the proper details. I can roll stabilization into #62198 if desired, or it can be done separately of course.

[HyeonuPark]

I think this method is only reasonable only if we don't have MaybeUninit in stable. Practically we use ManuallyDrop when we need untagged Option, because we don't have any other way to prevent accidentally dropping invalid data. But now we have MaybeUninit in stable which exactly is for this purpose.

Is there some use case that can't be reasonably expressed using MaybeUninit?

[CAD97]

@HyeonuPark ManuallyDrop is for a place that is valid (semantically) 99.99% of the time, or just needs its drop glue to be skipped. To this end, it derefs to its target so you can use &ManuallyDrop<T> as &T. Of _important_ note is that while ManuallyDrop is allowed to be in a logically invalid state (i.e. a dropped value that needs to not be reused), it _is not allowed_ to be in a physically invalid bit pattern, therefore preserving the niche of the underlying type.

Note also that MaybeUninit::read is also still unstable, so the functionality offered by this function is (implementable in userland but) not available in std.

[HyeonuPark]

@CAD97 I agree ManuallyDrop is useful, but not sure about ManuallyDrop::read(). Isn't this thread for stabilizing this method? Is there some use case that can't be reasonably expressed using MaybeUninit?

[CAD97]

@HyeonuPark The main use case is for in Drop::drop(&mut self) and you want to consume one of your members by value, usually because of a strict API that you're wrapping.

[RalfJung]

ManuallyDrop is for a place that is valid 99.99% of the time

No, ManuallyDrop must be valid (in the sense of the validity invariant) 100% of the time. A ManuallyDrop<bool> of value 8 is insta-UB. The thing is, dropping leaves the validity invariant intact (it has to, drop glue would go terribly wrong otherwise).

[alexcrichton]

I'm going to de-nominate this because stabilization happens outside of triage meetings and happens when a libs team member feels confident enough to propose stabilization.

[RalfJung]

Re: the parallel with MaybeUninit; my thinking is actually that read should probably be renamed to read_init or read_assume_init or so, something that indicates that this may only be done after initialization is complete.

[petertodd]

Should this function take &Self or &mut Self?

I think &mut Self is the better API, as it's consistent with the general case that take() transfers ownership and thus should only be called once; in the rare case where you really need to take() more than once you can always use ptr::read() instead.

Having said that, there is a difference in variance between the two, though I think it's a 100% pedantic difference. The &Self version is covariant over T, while the &mut Self version is invariant over T. This means that the following won't compile:

unsafe fn foo<'a>(src: &mut ManuallyDrop<&'static ()>) -> &'a () {
    ManuallyDrop::<&'a ()>::take(src)
}

A &mut ManuallyDrop<T> used this way is kind of like owning a T; the analogous Box<T> version does compile:

fn foo<'a>(src: Box<&'static ()>) -> &'a () {
    *src
}

But as I said above, I'm pretty sure this is a very pedantic objection, as you have to go out of your way to see this difference in action. The following compiles just fine, with Rust coercing the lifetime after the take() call:

unsafe fn foo<'a>(src: &mut ManuallyDrop<&'static ()>) -> &'a () {
    ManuallyDrop::take(src)
}

But I'm relatively new to thinking hard about variance, so if anyone thinks I missed something in the above I'd love to hear. :)

[petertodd]

Naming

take_unchecked() would be a logical analogy to the safe Option::take() - the latter checks if you call it twice, while the former doesn't. However we already have the unsafe, unchecked, ManuallyDrop::drop(), so I'd vote for sticking to the existing take() name. After all, it has to be called with ManuallyDrop::take(src), so it should be clear to the reader that you aren't doing a standard, safe, take().

[RalfJung]

Or alternatively we could deprecate ManuallyDrop::drop and rename it to ManuallyDrop::drop_unchecked.

[petertodd]

@RalfJung So counter-argument to my "it should be clear" argument: How many people who are unfamiliar with ManuallyDrop read it and think "ah, drop flags!"?

[jyn514]

IMO, anyone who sees ManuallyDrop and unsafe already knows there's something fishy going on, I don't think it needs to be reinforced by _unchecked.

[Amanieu]

I would like to stabilize this function as it is. For reference, here is the signature:

impl<T> ManuallyDrop<T> {
    pub unsafe fn take(slot: &mut ManuallyDrop<T>) -> T;
}

@rfcbot fcp merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• [x] @Amanieu
• [x] @KodrAus
• [x] @SimonSapin
• [x] @alexcrichton
• [x] @dtolnay
• [x] @sfackler
• [ ] @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[CAD97]

I went ahead and filed the stabilization PR at #68066 along with reclaimed doc improvements from the closed rename PR.

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.