stabilize Box::take

[edwloef]

Tracking issue: #147212

There hasn't been any activity on the tracking issue for a while, and this is a pretty small feature, so opening this to hopefully get a FCP started.

[rustbot]

r? @Darksonn

rustbot has assigned @Darksonn.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[Darksonn]

@rustbot label +I-libs-api-nominated

[Amanieu]

@rfcbot merge libs-api

[rust-rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @the8472

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.

[rust-rfcbot]

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

[coolreader18]

I wonder if there might be a better name - currently in the standard library, take means either "given a reference, replace with Default::default() and return value" (Option::take, mem::take, RefCell::take) or "limit the number of items in this stream" (Iterator::take, io::Read::take). This is definitely a variant of the former, but it takes by value and returns a different type. I think it would be reached for in different situations (as an optimization, rather than for mutating data), and it's valuable to not overload the names of operations to reduce confusion. Perhaps a name that makes it clear this is reclaiming the allocation?

[edwloef]

I like to think of the use of Box::take as a type-level Box<Option<T>>, where it would be the equivalent of taking the Some value out of that Option (and Box::write would be the equivalent of setting the option to a Some value). @camsteffen also noted in the tracking issue that "It fits the common semantic of removing a value from something", or written differently, taking a value out of something.

Box::read would be a nice parallel to both Box::write and ptr::read, but of course that's already taken by impl<R: Read> for Box<R>. Though that might be okay, since the signatures differ? At least Box::write is also present in triplicate thanks to impl<W: Write> Write for Box<W> and impl<T: Hasher> Hasher for Box<T>.

The other suggestion in the tracking issue was into_inner_and_uninit, but I dislike that because there's no precedent in the standard library for into_inner_and_*.

[rust-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.

[ais523]

This was discovered slightly too late for the FCP, but it turns out that stabilizing this will significantly close down options for future features. Although I'm personally in favor of sending future Rust development in a direction that would not conflict with this stabilization, not everyone agrees, so I'm going to flag up the conflict in case it makes people reconsider.

There's a three-way conflict (you can have any two of these, but not all three):

1. The ability to take a Box<T,A> (with any allocator A) and convert it into a Box<MaybeUninit<T>,A> (this commit);
2. The ability to take an existing place containing a value of type T, and convert it in-place into a Box<T, NoOpDeallocator> (subsequently treating the place as having been moved out of); there is a lot of discussion at Owning references (&own T) rfcs#4000 which discusses &own references (references that own the targeted value, borrow the memory that contained it), together with an observation that they appear to be identical to a Box with a no-op deallocator, and a suggestion that the two types should maybe be equivalent.
3. The assumption that if a place of type T is moved out from via a reference, it will still contain a bit pattern that's valid for T, even if no reference that assumes that the place contains a T is ever used again. This is discussed in Writing an invalid value through a reference the caller can never use again unsafe-code-guidelines#618. Although there is (as I understand it) general agreement that the assumption is not made in most cases, there is disagreement about the situation in which the place is an enum field whose niche provides the discriminant of the enum.

The conflict occurs because if you have all three, you can start with an enum that uses a niche (say, Option<&mut u64>, convert the field from the enum to a box (e.g. &own &mut u64), take the field from the box (leaving behind a &own MaybeUninit<&mut u64>, then use the MaybeUninit to overwrite the memory that previously contained the &mut u64 with all zeroes, changing the discriminant of the enum.

My own opinion on this is that the property that should be dropped is 3, and that doing this should be allowed (and that converting enum fields to boxes should be interpreted as destructively destructuring the enum, so that it would not be expected to have a meaningful discriminant afterwards). But some people (e.g. @danielhenrymantilla, @ia0. @programmerjake ) have suggested that doing this sort of thing should produce a "typed MaybeUninit" that can only store bit patterns that are valid for the original type, rather than being able to store arbitrary data as well. In that situation, we would end up losing either ability 1 or ability 2 from my list above (and ability 1 is this commit).

[ia0]

In that situation, we would end up losing either ability 1 or ability 2 from my list above (and ability 1 is this commit).

Indeed, I believe we should lose ability 2 from the list, because Box is different from &own. Box is not a reference, it always points to a full allocation.

That's a cross post from rust-lang/rfcs#4000 (comment).

[edwloef]

My own opinion on this is that the property that should be dropped is 3, and that doing this should be allowed (and that converting enum fields to boxes should be interpreted as destructively destructuring the enum, so that it would not be expected to have a meaningful discriminant afterwards).

I agree. Sorry, misread, I would agree with dropping 2. It would make more sense to my mental model if a Box<T> owned its entire backing allocation, and not only the portion that contains valid bitpatterns of T. Even if not:

But some people have suggested that doing this sort of thing should produce a "typed MaybeUninit" that can only store bit patterns that are valid for the original type, rather than being able to store arbitrary data as well.

While this would be fine regarding this API, it would put Box::{map, try_map} (#144419) in jeopardy of being UB in the case where they don't reallocate (indeed, they are implemented internally via Box::take). Box::map is currently being stabilized as well: #160534.

[RalfJung]

Indeed, I believe we should lose ability 2 from the list, because Box is different from &own. Box is not a reference, it always points to a full allocation.

I agree. A Box<T, A> pointing to a field of an enum (or struct) is IMO a misuse of the Box API, we shouldn't use hacks like that. A full allocation is logically distinct from just any old block of memory.

[ais523]

A Box pointing to a struct field is entirely reasonable if you have a custom allocator. I've written such allocators myself (they store a struct on the stack as a source of memory, allocate from that, and move to the heap if they run out of space on the stack). (And a Box that uses a custom allocator is usually not returning full allocations.)

From my point of view, Box doesn't really have anything to do with the heap (the important properties of a Box from my point of view are that it owns a value and is able to return the contained memory so that it can be used for other purposes). As an example, the allocations that contain a function's local variables are as far as I can tell indistinguishable from a set of Boxes with a short-lived lifetime (they are allocated when the function starts running, deallocated when the function ends, and you can move out of them), and I would expect them to be modeled as boxes in formalizations like minirust.

[RalfJung]

A Box pointing to a struct field is entirely reasonable if you have a custom allocator. I've written such allocators myself (they store a struct on the stack as a source of memory, allocate from that, and move to the heap if they run out of space on the stack). (And a Box that uses a custom allocator is usually not returning full allocations.)

If the allocator does the work of ensuring that the field behaves like an allocation, then yes, it is.
But an allocator has to support arbitrary data being written into the memory it returns, and obviously that's not the case if you return a pointer into the field of an Option<bool>.

---

After a quick read of the mentioned RFC, I think that under my preferred interpretation of the safety contract for &own, there isn't even a problem here. We can have both Box::take and "&own-as-Box". Box and &own should have in common that it's fine to put arbitrary garbage data into them before doing a shallow drop.

[Darksonn]

I agree that 2 is to be dropped. It's misuse of box, and if we want &own, it can be a new type.

[joshtriplett]

But an allocator has to support arbitrary data being written into the memory it returns, and obviously that's not the case if you return a pointer into the field of an Option<bool>.

You should not be able to get an &own T (or a Box<T, NoDeallocation>) from a T without consuming the T, so that you can't get the T back later in an invalid state. (Exceptions would be things like a MaybeUninit or similar type that is allowed to contain an invalid bit pattern.)

This is the same reason drop_in_place is unsafe. Being able to get an &own T from a T and later get the T back later after dropping the &own T would be equivalent to drop_in_place.