Partially stabilize box_vec_non_null

[theemathas]

Closes #130364

r? libs-api

What's being stabilized

The following is being stabilized in this PR:

impl<T: ?Sized> Box<T> {
    pub unsafe fn from_non_null(ptr: NonNull<T>) -> Self { .... }
}
impl<T: ?Sized, A: Allocator> Box<T, A> {
    pub fn into_non_null(b: Self) -> NonNull<T> { .... }
}
impl<T> Vec<T> {
    pub const unsafe fn from_parts(ptr: NonNull<T>, length: usize, capacity: usize) -> Self { .... }
}
impl<T, A: Allocator> Vec<T, A> {
    pub const fn into_parts(self) -> (NonNull<T>, usize, usize) { .... }
}

Note that Vec::from_parts and Vec::into_parts remain const-unstable behind the const_heap feature (like Vec::from_raw_parts and Vec::into_raw_parts).

The following APIs remain gated behind allocator_api

impl<T: ?Sized, A: Allocator> Box<T, A> {
    pub unsafe fn from_non_null_in(raw: NonNull<T>, alloc: A) -> Self { .... }
    pub fn into_non_null_with_allocator(b: Self) -> (NonNull<T>, A) { .... }
}
impl<T, A: Allocator> Vec<T, A> {
    pub const unsafe fn from_parts_in(ptr: NonNull<T>, length: usize, capacity: usize, alloc: A) -> Self { .... }
    pub const fn into_parts_with_alloc(self) -> (NonNull<T>, usize, usize, A) { .... }
}

The following API is split into a new unstable feature in #157843, and remains unstable:

impl<T, A: Allocator> Vec<T, A> {
    pub const fn as_non_null(&mut self) -> NonNull<T> { .... }
}

Implementation History

Most of this feature was ACP'ed in rust-lang/libs-team#418, and implemented in #130061. Vec::as_non_null was ACP'ed separately in rust-lang/libs-team#440, and implemented in #130624.

Potential issues

• Should Vec::from_parts and Vec::into_parts be the name used for this? Or should those names be used for functions that take/return Box<[MaybeUninit<T>]>? (Raised at Tracking Issue for box_vec_non_null #130364 (comment))
• Should these function names be named non_null or nonnull? (Raised at Tracking Issue for box_vec_non_null #130364 (comment). See mem::Alignment: Naming the -> NonZero<usize> method. #154237 (comment).)
• Will the methods on Vec collide with anything via autoderef?

[ds84182]

An additional issue: Should Vec::into_parts return a struct with pub fields rather than a tuple? Having two usize tuple elements seems a bit error prone. Alternatively it could return (NonNull<[T]>, usize).

EDIT: It could also be (NonNull<[MaybeUninit<T>]>, usize), the pointer being the storage (ptr + cap) and the usize being the length.

[clarfonthey]

Just cataloguing the similar methods from rustdoc:

from_raw_parts/slice_from_raw_parts

• (*const T, usize) <-> &[T]
• (*const u8, usize) <-> str (unstable)
• (NonNull<T>, usize) <-> NonNull<[T]>

into_raw_parts/from_raw_parts

• String <-> (*mut u8, usize, usize)
• Vec<T> <-> (*mut T, usize, usize)'

from_parts/into_parts: (proposed)

• Vec<T> <-> (NonNull<T>, usize, usize)

to_raw_parts/from_raw_parts (unstable)

• *const T <-> (*const (), T::Metadata)
• NonNull<T> <-> `(NonNull<()>, T::Metadata)

I think that (NonNull<T>, usize, usize) would be a good choice due to the analogy to from_raw_parts/into_raw_parts, but that maybe it should be named from_non_null_parts instead.

The only other methods in libstd that use the term "parts" are BufWriter::into_parts, returning (W, Result<Vec<u8>, WriterPanicked) and IntoInnerError::into_parts (returned by BufWriter::into_inner) which returns (Error, W). (yes, annoyingly, those are reversed.)

Would say that maybe we should not label any "parts" as canonical for now at least, since it's not clear what version would be best to use.

[nia-e]

cc @rust-lang/opsem - is there a reason on your end to make as_non_null take a &mut instead of &? If not, we'd like to change it to & (same for rust-lang/libs-team#799). Otherwise, ready to open FCP on the other changes

[joshtriplett]

Shall we stabilize these?

@rfcbot merge libs-api

@rfcbot concern as_non_null-shared-reference

[rust-rfcbot]

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

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

Concerns:

• as_non_null-shared-reference (Partially stabilize box_vec_non_null #157226 (comment))
• from_parts and into_parts naming (Partially stabilize box_vec_non_null #157226 (comment))

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.

[RalfJung]

Which exact function is this about?

EDIT: I assume Vec::as_non_null. The outer reference doesn't affect the inner pointer in any of our current models, and I don't think it should. But we also have not conclusively ruled that out.

Though given that the returned pointer is meant to be mutable, my personal preference would be &mut self. We have repeatedly seen violations of shared reference immutability correlate with poor mutability discipline for raw pointers (const-to-mut-raw-ptr-cast is allowed but should be considered highly suspicious); I'd rather not see std proliferate patterns of that sort.

[BurntSushi]

I like these. I'm concerned about the from_parts and into_parts naming though. It suggests, to me, that these are the canonical "parts" of a Vec, and it's not clear to me that's true. I think I'd rather see from_non_null_parts and into_non_null_parts. It also matches the naming convention used for the rest of the methods.

@rfcbot concern from_parts and into_parts naming

[theemathas]

Just to confirm: T-libs-api has agreed to stabilize with the name non_null, and not nonnull, right? The meeting notes didn't mention this.

@BurntSushi The reason the Vec functions were named with parts was due to a libs-api request. Has the consensus on the best name changed since then?

[BurntSushi]

@theemathas Not sure if anything has changed or if it has been discussed. That comment is from almost 2 years ago though. @joshtriplett Thoughts on the naming here?

[nia-e]

Just to confirm: T-libs-api has agreed to stabilize with the name non_null, and not nonnull, right? The meeting notes didn't mention this.

I think we have generally agreed in the past that non-null is non_null. we kept the contracted form only for nonzero.

[nia-e]

To resolve the & vs &mut concern, we decided in this week's libs-api meeting to defer stabilising as_non_null for now. The rest of the methods can get stabilised here as-is. cc @joshtriplett as you raised the concern

[theemathas]

@nia-e I assume this also means:

• from_parts and into_parts are to be stabilized as-is.
• It's spelled non_null and not nonnull

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[theemathas]

I've updated the PR and the PR description, so Vec::as_non_null() will not be stabilized in this PR.

[theemathas]

@BurntSushi @joshtriplett May the concerns be resolved?

[BurntSushi]

I don't think my concern has been addressed. Like, from_parts/into_parts implies a level of canonical-ness that isn't obviously warranted to me. We already have from_raw_parts/into_raw_parts, so I think we should just go with from_non_null_parts/into_non_null_parts. This leaves the door open for other routines in the future, e.g., using Box<[MaybeUninit]> as noted in the OP.

I also don't think these routines are used enough to warrant the shorter name.