Documentation for ManuallyDrop seems to contradict itself

[Diggsey]

In particular, initializing a ManuallyDrop<&mut T> with mem::zeroed is undefined behavior. If you need to handle uninitialized data, use MaybeUninit instead.

pub unsafe fn drop(slot: &mut ManuallyDrop<T>)

This function runs the destructor of the contained value and thus the wrapped value now represents uninitialized data.

So which is it? Is ManuallyDrop allowed to contain uninitialised data, or is ManuallyDrop::drop always insta-UB to call? What counts as "using" a ManuallyDrop type after dropping it?

edit:
On a related note, the documentation for ManuallyDrop::drop should probably guarantee that the value is dropped in-place, without moving (and is thus OK to use with pinned data).

[Lokathor]

Notice the types are different. It makes all the difference.

ManuallyDrop<&mut T> is a manually dropped unique reference to a T.

&mut ManuallyDrop<T> is a unique reference to a manually dropped T.

A reference in rust has many rules, one of which is that it can never be null.

[Diggsey]

@Lokathor &mut T is just an example of a type that has validity rules, it could be any type there.

Having thought about it a bit more, I think the problem with the docs is with this line:

This function runs the destructor of the contained value and thus the wrapped value now represents uninitialized data.

This is at best misleading: after running the destructor, the wrapped value is not the same thing as uninitialised data: it is a sequence of bytes which are guaranteed to be valid under the validity rules for T, but do not represent an instance of T.

[comex]

it is a sequence of bytes which are guaranteed to be valid under the validity rules for T, but do not represent an instance of T.

Are they even that? Box<T> in particular may or may not have "points to allocated memory" as part of its validity invariant. (We used to emit LLVM dereferenceable, then stopped, but only because LLVM assumed dereferenceable lasted for the entire function regardless of deallocation calls.) If it does have it, then after calling the destructor of Box<T>, the bytes no longer satisfy its validity invariant.

[Lokathor]

Oh, well yes. It's logically uninitialized, not physically uninitialized, i suppose the docs should say. if that distinction makes sense.

[Lokathor]

(oops, that was to Diggsey, comex sniped me by a few seconds)

[Diggsey]

If it does have it, then after calling the destructor of Box, the bytes no longer satisfy its validity invariant.

If so, then the rules for what can be in a ManuallyDrop<T> are even more unusual, and to describe it means we'd need a step between "invalid" and "valid". I am imagining a hierarchy like:

1. Uninitialized (can't even say anything about what values are in memory)
2. Invalid (we may know something about the value in memory)
3. Representable (will not break any layout-optimisations)
4. Valid (will not cause UB if accessed from code via the type we are discussing)
5. Safe (can be exposed to safe code)

edit:
This also ties in to #95

[RalfJung]

I agree that this wording is bad:

This function runs the destructor of the contained value and thus the wrapped value now represents uninitialized data.

The value is not uninitialized. It is, in fact, the same value as before. In particular it satisfies anything that enum optimizations rely on.

If validity invariants make assumptions about memory contents (which I am increasingly inclined to think they should not), then we need something weaker, which people have called "bit-level validity", and that is the part that layout optimizations rely on. ManuallyDrop::drop would guarantee that the value remains bit-level valid.

But I think we should just avoid that extra layer. Instead, the fact that references and Box are dereferencable follows from their alias requirements. That allows us to use "validity invariant" as terminology consistently both for dropped and non-dropped data.

On a related note, the documentation for ManuallyDrop::drop should probably guarantee that the value is dropped in-place, without moving (and is thus OK to use with pinned data).

Agreed (modulo packed structs). Do you want to submit a PR?

[Diggsey]

Agreed (modulo packed structs). Do you want to submit a PR?

What is the caveat regarding packed structs?

[RalfJung]

Packed structs have to move their fields even for drop_in_place as otherwise they would create an unaligned &mut T. That's why the pin documentation already points out that you may not use structural pinning of fields for packed structs.

[Diggsey]

@RalfJung ah, but we can still say that ManuallyDrop::drop is exactly equivalent to calling ptr::drop_in_place - ie. the special-casing is entirely within the compiler.

[RalfJung]

@Diggsey true. I feel we should call out the packed-struct exception somewhere though. Maybe in the drop_in_place docs.

[RalfJung]

This has been closed by rust-lang/rust#71625. Thanks @Diggsey!