Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Box::take() method #93653

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions compiler/rustc_data_structures/src/functor.rs
Original file line number Diff line number Diff line change
@@ -17,16 +17,8 @@ impl<T> IdFunctor for Box<T> {
where
F: FnMut(Self::Inner) -> Result<Self::Inner, E>,
{
let raw = Box::into_raw(self);
Ok(unsafe {
// SAFETY: The raw pointer points to a valid value of type `T`.
let value = raw.read();
// SAFETY: Converts `Box<T>` to `Box<MaybeUninit<T>>` which is the
// inverse of `Box::assume_init()` and should be safe.
let raw: Box<mem::MaybeUninit<T>> = Box::from_raw(raw.cast());
// SAFETY: Write the mapped value back into the `Box`.
Box::write(raw, f(value)?)
})
let (value, allocation) = Box::take(self);
Ok(Box::write(allocation, f(value)?))
}
}

56 changes: 56 additions & 0 deletions library/alloc/src/boxed.rs
Original file line number Diff line number Diff line change
@@ -578,6 +578,62 @@ impl<T, A: Allocator> Box<T, A> {
{
*boxed
}

/// Safely reads the value on heap without deallocating.
///
/// This method is mostly useful for avoiding reallocations.
/// The method consumes the box to prevent double-free.
/// The value on heap should be considered truly uninitialized and MUST NOT be read.
/// Specifically, this code is definitely UB:
///
/// ```no_run
/// #![feature(new_uninit)]
///
/// let value = Box::new("hello".to_owned());
/// // Double free here!
/// unsafe { Box::take(value).1.assume_init(); }
/// ```
///
/// # Examples
///
/// ```
/// #![feature(new_uninit)]
///
/// let boxed_value = Box::new("hello".to_owned());
/// let (mut value, allocation) = Box::take(boxed_value);
/// assert_eq!(value, "hello");
/// // More realistic code would consume the value and produce other value.
/// // For simple demonstration we just modify it here.
/// value.push_str(" world");
/// let boxed_value = Box::write(allocation, value);
/// assert_eq!(*boxed_value, "hello world");
/// ```
#[unstable(feature = "new_uninit", issue = "63291")]
#[rustc_const_unstable(feature = "const_box", issue = "92521")]
#[inline]
pub const fn take(this: Self) -> (T, Box<mem::MaybeUninit<T>, A>) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take to me has a very strong implication of mem::take (or Option::take or ...) to me, with &mut self and such, so I would encourage a different name here.

👍 to the functionality, though! Makes good sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually picked the name because it is similar to mem::take in the sense of "taking the value out and replacing it with something", in this case replacing it with nothing and tracking it in the state. However if people feel it's strongly associated with &mut self I'm open to a better name, I'd just like to hear some suggestions on alternatives or a feedback on my own ides for alternatives (see above).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see the parallel, though to me this API doesn't do the "and replacing it with something" part that distinguishes take from into_inner kinds of things.

But more importantly to me, mem::take(&mut my_box) works today, so I think Box::take doing something pretty different would be surprising. If anything, I might expect it to be mem::take(&mut *my_box) -- note the deref -- like how RefCell::take works. (And note that mem::take(&mut my_option) does exactly the same thing as Option::take(&mut my_option), as another example.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, what do you think bout into_parts()? That one feels like the next best to me now.

let (raw, alloc) = Box::into_raw_with_allocator(this);
unsafe {
// SAFETY:
// * The pointer given to from_raw_in was obtained from box above using the same
// allocator
// * Casting `*mut T` to `*mut MaybeUninit<T>` is sound because they have same layout
// * Safely obtaining `&mut T` pointing into the allocation will become impossible after
// this call because we consume `this` (no variance issues)
// * `MaybeUninit` disables destructor of `T` so it can't double free
// * Leak shouldn't happen because `assume_init_read` below doesn't panic and the value
// can be dropped afterwards
let new_box = Box::from_raw_in(raw.cast::<mem::MaybeUninit<T>>(), alloc);
// SAFETY:
// * The value being read was proven to be initialized when this function was called
// * We didn't touch the value inside this function, just cast the pointer, so it's
// still valid.
// * We don't read the value on heap again and prevent safe code from reading it by
// marking the box with `MaybeUninit`.
let value = new_box.assume_init_read();
(value, new_box)
}
}
}

impl<T> Box<[T]> {
1 change: 1 addition & 0 deletions library/alloc/src/lib.rs
Original file line number Diff line number Diff line change
@@ -101,6 +101,7 @@
#![feature(const_size_of_val)]
#![feature(const_align_of_val)]
#![feature(const_ptr_read)]
#![feature(const_maybe_uninit_assume_init_read)]
#![feature(const_maybe_uninit_write)]
#![feature(const_maybe_uninit_as_mut_ptr)]
#![feature(const_refs_to_cell)]