Partially stabilize box_vec_non_null

[theemathas]

View all comments

Closes#130364

r? libs-api

What's being stabilized

The following is being stabilized in this PR:

impl<T: ?Sized>Box<T>{pubunsafefnfrom_non_null(ptr:NonNull<T>) -> Self{ .... }pubfninto_non_null(b:Self) -> NonNull<T>{ .... }}impl<T>Vec<T>{pubconstunsafefnfrom_parts(ptr:NonNull<T>,length:usize,capacity:usize) -> Self{ .... }pubconstfninto_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>{pubunsafefnfrom_non_null_in(raw:NonNull<T>,alloc:A) -> Self{ .... }pubfninto_non_null_with_allocator(b:Self) -> (NonNull<T>,A){ .... }}impl<T,A:Allocator>Vec<T,A>{pubconstunsafefnfrom_parts_in(ptr:NonNull<T>,length:usize,capacity:usize,alloc:A) -> Self{ .... }pubconstfninto_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>{pubconstfnas_non_null(&mutself) -> 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 resolved by Partially stabilize box_vec_non_null #157226 (comment)
• from_parts and into_parts naming resolved by 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]

@riking As currently implemented, both Box::into_non_null and Vec::into_parts currently require the default Global allocator. The PR description and the tracking issue were previously incorrect, and I've now edited them. Sorry for the discrepancy.

[joshtriplett]

@BurntSushi wrote:

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

I've seen various FFI code that uses from_raw_partsextensively at the FFI boundary. I think there's value in steering people towards the use of NonNull, and in particular not steering them away from it by having a name longer than from_raw_parts (e.g. from_non_null_parts, into_non_null_parts).

I think NonNull is a sensible canonical representation, because it's trivially bidirectionally mappable to a raw pointer: you can declare your FFI interface to take Option<NonNull<T>>, and every value is represented, with NULL mapping to None.

You can't declare your FFI interface to take Something<NonNullAligned<T>>, for instance; you could only get one of those by doing further manual validation, which is often something people don't need to do on the FFI boundary (because it's not necessarily the job of an FFI boundary to defensively assert things the library does not do, like create an unaligned structure). Checking for NULL is more important because it's easy to do by accident; unaligned typically requires going out of your way to do something wrong.

On that specific basis, I think from_parts/into_parts have the advantage of steering people to use them and make safer unsafe code, and the advantage of declaring a usable canonical representation.

[BurntSushi]

I'm convinced by @joshtriplett's argument here. And in particular, that FFI code is not commonly concerning itself with whether a pointer is aligned or not.

@rfcbot resolve from_parts and into_parts naming

[rust-rfcbot]

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

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

[nia-e]

r? me

@bors r+

[rust-bors]

📌 Commit a18a972 has been approved by nia-e

It is now in the queue for this repository.

[bjoernager]

Confusing that nonzero was accepted as the snake case form of NonZero by libs-api (see #154237 (comment)) if non_null is going to be the snake case form of NonNull?

[clarfonthey]

Yes: #157226 (comment)