Partially stabilize ptr_alignment_type as alignment_type

[GrigorenkoPV]

View all comments

Tracking issue: #102070

Stabilized API

All used to be gated under ptr_alignment_type, now stabilized under alignment_type:

// in `core::mem`#[derive(Copy,Clone,PartialEq,Eq)]pubstructAlignment{ ... }implAlignment{pubconstMIN:Self;pubconstfnof<T>() -> Self;pubconstfnof_val<T:MetaSized>(val:&T) -> Self;pubconstfnnew(align:usize) -> Option<Self>;pubconstunsafefnnew_unchecked(align:usize) -> Self;pubconstfnas_usize(self) -> usize;pubconstfnas_nonzero_usize(self) -> NonZero<usize>;}implDebugforAlignment;implTryFrom<NonZero<usize>>forAlignment;implTryFrom<usize>forAlignment;implFrom<Alignment>forNonZero<usize>;implFrom<Alignment>forusize;implOrdforAlignment;implPartialOrdforAlignment;implHashforAlignment;implDefaultforAlignment;

Things not getting stabilized

Remains under ptr_alignment_type:

// in core::ptr#[deprecated(note = "moved from `ptr` to `mem`")]pubtypeAlignment = mem::Alignment;// in `core::mem`implAlignment{pubconstfnlog2(self) -> u32;#[deprecated(note = "renamed to `as_nonzero_usize`", suggestion = "as_nonzero_usize")]pubconstfnas_nonzero(self) -> NonZero<usize>{self.as_nonzero_usize()}}// in `core::layout`implLayout{// `pub const fn from_size_align(size: usize, align: usize) -> Result<Self, LayoutError>`'s siblingpubconstfnfrom_size_alignment(size:usize,alignment:Alignment) -> Result<Self,LayoutError>;// `pub const unsafe fn from_size_align_unchecked(size: usize, align: usize) -> Self`'s siblingpubconstunsafefnfrom_size_alignment_unchecked(size:usize,alignment:Alignment) -> Self;// `pub const fn align(&self) -> usize`'s siblingpubconstfnalignment(&self) -> Alignment;// `pub const fn align_to(&self, align: usize) -> Result<Self, LayoutError>`'s siblingpubconstfnadjust_alignment_to(&self,alignment:Alignment) -> Result<Self,LayoutError>;// no `align: usize` siblingpubconstfnpadding_needed_for(&self,alignment:Alignment) -> usize;}

Moved from ptr_alignment_type to layout_for_ptr (#69835):

implAlignment{pubconstunsafefnof_val_raw<T:MetaSized>(val:*constT) -> Self;}

Moved from ptr_alignment_type to ptr_mask (#98290):

implAlignmentpubconstfnmask(self) -> usize;}

const trait impls

These are stabilized as non-const (see "Stabilized APIs"), but const counterparts remain unstable under the same feature gates:

under const_convert (#143773)

implconstTryFrom<NonZero<usize>>forAlignment;implconstTryFrom<usize>forAlignment;implconstFrom<Alignment>forNonZero<usize>;implconstFrom<Alignment>forusize;

under const_default (#143894).

implconstDefaultforAlignment;

under const_cmp (#143800).

implconstPartialOrdforAlignment;implconstOrdforAlignment;implconstPartialEqforAlignment;implconstEqforAlignment;

under const_clone (#142757).

implconstCloneforAlignment;

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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: @scottmcm, libs
• @scottmcm, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, joboet, scottmcm

[GrigorenkoPV]

For reasons behind what exactly is being stabilized, why moved from core::ptr to core::mem, and why renamed from as_nonzero to as_nonzero_usize see the discussion on the tracking issue starting with this comment:
#102070 (comment)

@rustbot label -T-libs +T-libs-api +I-libs-api-nominated

[GrigorenkoPV]

This currently fails due to bumpalo having an internal padding_needed_for method in its UnstableLayoutMethods trait, but their method accepts an usize instead of Alignment.

So we either have to change our name, patch bumpalo locally, or convince bumpalo to change their and wait until this repo updates to a newer version of bumpalo.

[clarfonthey]

So, part of the reason why I suggested just stabilising the type instead of, for example, the Layout methods is so that it could be used in new stable APIs without worrying about extra potential conflicts like that. I would argue that the bumpalo conflict is a compelling argument to hold off on stabilising those for now.

[GrigorenkoPV]

Good argument, I've updated the PR and the description accordingly.

[clarfonthey]

Side note: it's kind of bad form to stabilise stuff under one flag while keeping other methods unstable. You can just name the stuff left out as ptr_alignment_type_extras or something, or if you want to make things a bit clearer:

• log2: alignment_type_extras
• Layout methods: layout_alignment_methods

(ptr_alignment doesn't make sense any more if this is moved to mem::Alignment)

---

Rereading the PR, you did already rename the feature flags, so, my bad. You can do this suggestion to further separate the names if you want, but it's not actually as necessary as I thought it was, since the stable/unstable features are different.

[nia-e]

This was discussed in today's @rust-lang/libs-api meeting and we concluded that we'd like to see the methods on Layout stabilised also. There seemed to be some discussion in the tracking issue regarding which module to place everything in, but there was consensus on everything living in core::mem.

[tgross35]

Could the renames and moves be split to a separate PR from stabilization? There is a lot going on here at once.

We also don't need to add deprecated methods/aliases IMO. Users of unstable API need to be aware that there will occasionally be breaking changes, we shouldn't take on the extra burden of trying to smooth this except in rare cases.

[scottmcm]

(Came here from the meeting notes) I'd proposed the partial here, @nia-e, because I figured some pieces of this were "obvious" (assuming we want the type at all) like new+new_unchecked+as_nonzero, at least under some spelling*, but things like Alignment::mask or Layout::padding_needed_for are currently under the same feature gate and not as obviously necessary as part of the original stabilization.

I'm perfectly happy to see more stabilized if folks are happy with it :)

[scottmcm]

Oh, and +💯 to separating out the move from any stabilizations. That can be landed easily without FCP, but with the mir-opt implications and such it's noisy so valuable to have separate from the feature gating changes.

[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

Concerns:

• non_zero_name_consistency (Partially stabilize ptr_alignment_type as alignment_type #153261 (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.

[scottmcm]

suggestion: maybe just delete the old name as part of this PR?

[GrigorenkoPV]

Sure, given that when this PR lands, there will have been quite some time since the deprecation of the unstable names, I think it's fine to remove them as part of this PR.

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

[BurntSushi]

In #157226, we seem poised to stabilize on non_null naming. But here, we have nonzero. I don't think we have any stable methods containing non_null, nonnull, non_zero or nonzero. So we have a chance here to pick a precedent and stick to it. I think the underscore is most consistent. i.e., go with what's in #157226 and rename nonzero here to non_zero.

@rfcbot concern non_zero_name_consistency

[theemathas]

@BurntSushi See #154237 (comment)

[BurntSushi]

@theemathas Thank you! I hadn't seen that. I'm not really convinced by that though on its own. As of now, we're headed in a direction where we have nonzero and non_null, which feels oddly inconsistent given the type names of NonZero and NonNull, respectively.

(To me it's not about the as_<type name> consistency. I'm on board with that not being a hard rule or whatever. It's more about being consistent with how as_<whatever> is spelled. And in more contexts than just as_.)

[rust-bors]

☔ The latest upstream changes (presumably #157882) made this pull request unmergeable. Please resolve the merge conflicts.

[scottmcm]

Hi libs-api! This looks like it's waiting on a new "please do ______" from you.

Notably, there's a concern here contradicting the previous agreement in #154237 (comment) -- what do you as a team want? Should someone be making a PR to rename to non_zero? (cc @bjoernager#102070 (comment))

[clarfonthey]

FWIW, we don't have this decided in any stable APIs yet, even though non_null has been decided in several stable APIs. The only existing other example is #84908 for ExitStatusError::code_nonzero, which is still unstable.

as_non_zero_usize would be fine to me, honestly. But we can paint the shed Tuesday.

[nia-e]

cc @BurntSushi - my opinion on this has been that non_null is two words while nonzero is one since only the latter is a single word in the dictionary. we don't have any stable APIs with nonzero currently, so it's not decided by any means, but i'm in favour of having it be one word based on that

[tgross35]

I don't like the inconsistency between NonZero and non_zero if that's what we wind up with. But perhaps there's a case that NonZero should be renamed Nonzero with an alias? (I already make that typo all the time...)

Interestingly it seems like usage is pretty equally split this century with a slight edge to "nonzero" https://books.google.com/ngrams/graph?content=nonzero%2Cnon+zero%2Cnon-zero&year_start=1800&year_end=2022&corpus=en&smoothing=3

[clarfonthey]

To be fair, NonZero reads to me as non-zero which makes non_zero seem reasonable. but I get that nonzero is literally a word by itself.