Implement a stop-gap “MaybeUninit” using ManuallyDrop

[bluss]

This is a cautious version of MaybeUninit, since we don't have one in
libstd, based on ManuallyDrop.

This moves ArrayVec to a nice, no size overhead implementation by
default. It is understood this is not a perfect solution! We do the best
we can with stable Rust, and will adopt std's MaybeUninit or equivalent
as soon as it becomes available.

ArrayString can already in this PR use what's been designated as the
sound solution, since its backing array is Copy. See MaybeUninitCopy in the PR.

I think we can enable the future stable MaybeUninit equivalent
conditionally, automatically, and don't need user visible API changes for that.

This removes nodrop as a dependency, finally.

Look here for previous discussion: #76 (comment)

Closes #86
Fixes #62

[Thomasdezeeuw]

I really like the design of RFC #1892, which defines a new type MaybeUninit:

union MaybeUninit<T> {
    uninit: (),
    value: T,
}

I think that would be a great fit for ArrayVec use case, something like this:

pub struct ArrayVec<A: Array, T> {
    data: A<Item = MaybeUninit<T>>,
    len: A::Index,
}

[RalfJung]

Hm, notice that for ManuallyDrop<T> we will likely want to decide that it always has to contain at least a "bit-valid" T. That's required for unsized ManuallyDrop in combination with thin dyn types. So, the ManuallyDrop here is likely not correct.

Is it possible to have a union of a ManuallyDrop<T> and a ()? That would be safe, I think.

[bluss]

Thanks for the pointers!

[bluss]

@RalfJung I'll do the best I can with stable Rust here and insert MaybeUninit as soon as it is stable (with a version check). A union is not possible since we want to have non-Copy T:

error[E0658]: unions with non-`Copy` fields are unstable (see issue #32836)
  --> src/maybe_uninit.rs:15:1
   |
15 | / union Test<T> {
16 | |     empty: (),
17 | |     value: ManuallyDrop<T>,
18 | | }
   | |_^
   |
   = help: add #![feature(untagged_unions)] to the crate attributes to enable

[bluss]

@RalfJung It's long ago now that I discussed this with @arielb1, and we came into discussion about the difference between "moved from" (like ptr::read or ManuallyDrop::drop would leave the interior. [Put in nuance if those are different]) and wholly uninitialized.

Current ManuallyDrop::drop basically says it leaves the interior uninitialized. So in that sense this type can already contain this.

[bluss]

another problem from ArrayString: it is Copy, and we'd like to be able to Copy it while the array is partially or wholly uninitialized.

[RalfJung]

I'm afraid this is not correct -- it is as incorrect as just plain mem::uninitialized.

Using terminology from my blog post, the issue with mem::uninitialized is that it returns data that violates the validity invariant. While ManuallyDrop can handle data that is already dropped, it does not have any effect on the validity invariant: ManuallyDrop<bool> may only be 0x0 or 0x1, and hence

let x: MaybeUninit<bool> = MaybeUninit::uninitialized();

with your type is UB.

For non-Copy types, there is currently no way to do this. We should really work towards solving the remaining open questions in rust-lang/rust#53491!

[bluss]

FWIW; I know this is not correct, this is the current best effort with stable Rust like the PR says.

The current state of arrayvec as it is released, can't be said to be any better than this, I'm sure?

[bluss]

@RalfJung With MaybeUninit, what's the importance of using MaybeUninit<[T; 10]> vs [MaybeUninit<T>; 10] ? The latter I don't even have clear picture of how to initialize, when we start with T values that are not Copy.

If I'd follow the now-removed array_vec.rs in librustc_data_structures, it uses mem::uninitialized() to initialize a whole value of [MaybeUninit<T>; 10]. Which takes us close to be back at the starting point? Or is that fine?

[RalfJung]

The current state of arrayvec as it is released, can't be said to be any better than this, I'm sure?

I do not know that state. This is pretty much equivalent to using mem::uninitialized directly.

it uses mem::uninitialized() to initialize a whole value of [MaybeUninit; 10].

oO I had no idea.^^ Where can I find its sources?

Time that we get mem::uninitialized deprecated so that these things are rejected by -D warnings.

But given that MaybeUninit is a union, it is actually okay to not initialize it. The problem with mem::uninitialized is that it almost always violates the validity invariant of the type it returns; but for MaybeUninit, the validity invariant accepts literally any data (including uninitialized data), and hence there is no problem with that particular usage of mem::uninitialized.

[RalfJung]

Looking at ArrayVec when it got removed in rust-lang/rust@687cc29, it uses mem::uninitialized on ManuallyDrop, which is not okay.

[RalfJung]

With MaybeUninit, what's the importance of using MaybeUninit<[T; 10]> vs [MaybeUninit; 10] ?

The two types are the same, in the sense that you may transmute between them. However, their different type has effects on e.g. into_inner, which for MaybeUninit<[T; 10]> asserts that all elements are initialized.

[bluss]

Ok! By the way I'm following the stacked borrows discussion and unsafe code guidelines now. It will have a big impact on ndarray, I believe. Just like the rest of the ecosystem we are gradually getting a clearer picture of what we can do in unsafe code and can dot our i's and cross the t's here.

I would go with using MaybeUninit<[T; 10]> and offsetting into it, because the other way around leads to code with more transmutes when getting initialized [T; 10] wrapped into the ArrayVec. We of course never use anything like into_inner except when we know it is fully initialized.

Given this, it sounds like it is ok to use mem::uninitialized with [MaybeUninit<T>; 10], even if the array is the outer type. That's the only answer to how to set a value to such a field that I can see (or equivalent union trick).

[eddyb]

A union is not possible since we want to have non-Copy T:

I thought we were going to allow ManuallyDrop<T>, alongside any Copy types, as the property we're interested in isn't really Copy, but "needs no drop".

Did we not stabilize that? If so, why?

[RalfJung]

Did we not stabilize that? If so, why?

We accepted the RFC. It hasn't been implemented yet, though.

[bluss]

This is confirmed broken, it's something that has changed in practice since this was first prototyped using Rust 1.20.

Now that ManuallyDrop<T> switched to struct, its interior is no longer exempt from being used enum layout optimization, and this makes an arrayvec based on ManuallyDrop noticeably unsound, and smallvec has the same problem.

Testing the current PR:

#[test]
fn test_still_works_with_option_arrayvec() {
    type RefArray = ArrayVec<[&'static i32; 2]>;
    let array = Some(RefArray::new());
    assert!(array.is_some());
    println!("{:?}", array);
}

The test fails in debug mode, it passes in release mode but the output is "None", which is incorrect.

See also servo/rust-smallvec/issues/126 for smallvec

[RalfJung]

The fact that ManuallyDrop was exempt from layout optimizations was an implementation limitation, it was never a guarantee.

[bluss]

@RalfJung I get this. It's a regression in a popular crate (smallvec) so it needs to be taken seriously as a practical problem at some level.

[RalfJung]

Agreed, it is a problem. However, the relevant change happened several months ago and got released on stable with 1.29, 10 weeks ago.

I think the main problem here is people relying on things that just "happen to be the case". I am not sure how to prevent this, other than repeating all the time "don't do that", and working on stabilizing APIs that let people better express their intent.

[bluss]

@RalfJung It's great that things are becoming clearer on the unsafe code guidelines front, and Rust as a whole is really maturing. This crate is clearly a quite old and big hack, but it has kept afloat pragmatically since it fills a need, just like smallvec.

I think it's good to approach it with pragmatism, what's the best solution right now and how do we bring these up on “dry land”. It's looking like we are close now.

I don't quite agree with your "happen to be the case", this PR was prepared after long discussions for example with arielb and we made some good guesses about where ManuallyDrop would go at the time. And we were wrong!

And the original crate was written long before anyone could answer what was and wasn't allowed, it was just unclear.

[RalfJung]

IOW, we should work hard towards stabilizing MaybeUninit :D

And the original crate was written long before anyone could answer what was and wasn't allowed, it was just unclear.

Relying on unclear things is exactly what I meant. It's dangerous.

But I also appreciate that so many things are unclear still that it can be hard to avoid.

[bluss]

@RalfJung 😄 rust-lang/rust#56466 The old sins are disappearing

[bluss]

See PR #114 for another pragmatic mitigation—fighting the fire where we can—conditional use of unions and a good solution (only on nightly!)

[bluss]

This is replaced by #114 , #115, #116 (which only affects nightly users so far). We'll do the best we can with stable, as soon as we are able.