stabilize Box::take

[edwloef]

View all comments

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.

Closes: #147212

[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_*.

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

[edwloef]

Given that the stabilization of Box::map has been merged, I'll assume this is ready as well?

[Darksonn]

It looks like @Amanieu libs-nominated this for discussion, but then the label was lost. Did any further discussion occur / is it needed?

[programmerjake]

My own opinion on this is that the property that should be dropped is 3

I think we should drop 3 (thereby allowing you to scribble over memory when/after dropping the T living there); I'm not sure about 2 (Box<T, NoOp<'_>> being interconvertible with &own T), though I don't think they should be the exact same type.

[nia-e]

Apologies, we thought this got through to the relevant folks. Our conclusion was that we're fine with stabilising this as-is

[Darksonn]

@bors r+

[rust-bors]

📌 Commit c918352 has been approved by Darksonn

It is now in the queue for this repository.