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

Tracking Issue for nonzero_ops #84186

Open
7 of 8 tasks
iago-lito opened this issue Apr 14, 2021 · 19 comments
Open
7 of 8 tasks

Tracking Issue for nonzero_ops #84186

iago-lito opened this issue Apr 14, 2021 · 19 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@iago-lito
Copy link
Contributor

iago-lito commented Apr 14, 2021

Feature gate: #![feature(nonzero_ops)]

This is the tracking issue for #82703. The common arithmetic operators that keep the invariants of NonZero* types enforced are implemented for these types.

Public API

Example with U8/I8, but same for U16/I16 etc.

// core::num

impl NonZeroU8 {
    pub const fn checked_add(self, other: u8) -> Option<Self>;
    pub const fn saturating_add(self, other: u8) -> Self;
    pub const unsafe fn unchecked_add(self, other: u8) -> Self;
    pub const fn checked_next_power_of_two(self) -> Option<Self>;
}

impl NonZeroI8 {
    pub const fn abs(self) -> Self;
    pub const fn checked_abs(self) -> Option<Self>;
    pub const fn overflowing_abs(self) -> (Self, bool);
    pub const fn saturating_abs(self) -> Self;
    pub const fn wrapping_abs(self) -> Self;
    pub const fn unsigned_abs(self) -> NonZeroU8;
}

impl NonZeroU8 /* and NonZeroI8 */ {
    pub const fn checked_mul(self, other: Self) -> Option<Self>;
    pub const fn saturating_mul(self, other: Self) -> Self;
    pub const unsafe fn unchecked_mul(self, other: Self) -> Self;
    pub const fn checked_pow(self, other: u32) -> Option<Self>;
    pub const fn saturating_pow(self, other: u32) -> Self;
}

/* Not yet on nightly, as it is insta-stable:
impl Neg for NonZeroI8 {
    type Output = Self;
    fn neg(self) -> Self;
}
*/

Steps / History

Unresolved Questions

  • Should unchecked_add and unchecked_mul be marked const and how? Would it imply that their const-ness for the underlying types be upgraded in std?
    -> Leave as-is (non-const) until add const-ub RFC rfcs#3016 lands. (Implement nonzero arithmetics for NonZero types. #82703 (comment))
    -> It has landed now, mark them const in Mark unsafe methods NonZero*::unchecked_(add|mul) as const. #87910.
  • Naming: how to avoid confusion regarding the term (un)checked, referring to checking for nonzeroness in std with the NonZeroInt::new_unchecked method, but here referring to checking overflow? Discussion here.
    -> check consistently means "check everything that needs be checked in the current context, and refer to the doc to verify what needs to be checked in this context". The doc has been made explicit about what needs to be checked for every method above.
@iago-lito iago-lito added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 14, 2021
@iago-lito iago-lito changed the title Tracking Issue for #82703 #![feature(nonzero_ops)] Tracking Issue for nonzero_ops Apr 14, 2021
@iago-lito
Copy link
Contributor Author

-> Leave as-is (non-const) until rust-lang/rfcs#3016 lands. (#82703 (comment))

How to not forget re-checking the constness of unchecked methods when this happens? Is there a way to leave some kind of mark in the repo that'll automatically trigger this discussion again when it's time?

@programmerjake
Copy link
Member

-> Leave as-is (non-const) until rust-lang/rfcs#3016 lands. (#82703 (comment))

How to not forget re-checking the constness of unchecked methods when this happens? Is there a way to leave some kind of mark in the repo that'll automatically trigger this discussion again when it's time?

rust-lang/rfcs#3016 has landed...hoping these can be stabilized soon!

Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…metics_as_const, r=joshtriplett

Mark unsafe methods NonZero*::unchecked_(add|mul) as const.

Now that rust-lang/rfcs#3016 has landed, these two unstable `std` function can be marked `const`, according to this detail of rust-lang#84186.
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 4, 2021
…metics_as_const, r=joshtriplett

Mark unsafe methods NonZero*::unchecked_(add|mul) as const.

Now that rust-lang/rfcs#3016 has landed, these two unstable `std` function can be marked `const`, according to this detail of rust-lang#84186.
@Noratrieb
Copy link
Member

I just had a use for this, what's the state of this feature?

@iago-lito
Copy link
Contributor Author

Hi @Nilstrieb @jschwe. The state is that I don't quite know what's next. IIUC someone needs to write a stabilisation report for the feature to move forward, but this requires that a few use cases be collected first, right? I don't have many on my side, since I think I've only used it once then, but I'm happy to help :)

@jschwe
Copy link
Contributor

jschwe commented Apr 19, 2022

@iago-lito Is nonzero_ops not just a "library feature"? Looking at the guide for Stabilizing a library feature a stabilisation report does not seem to be necessary.

@iago-lito
Copy link
Contributor Author

Hi @m-ou-se, is this the right time to ask for a FCP on this feature?

@m-ou-se
Copy link
Member

m-ou-se commented Apr 19, 2022

I've updated the top comment with an up to date summary of this feature.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 19, 2022

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 19, 2022
@dtolnay
Copy link
Member

dtolnay commented Apr 20, 2022

It's not clear to me from the stabilization proposal, but presumably we're not stabilizing unchecked_add and unchecked_mul as part of this? That is not stable on the primitive integers yet and has a different tracking issue: #85122.

@iago-lito
Copy link
Contributor Author

@dtolnay I don't remember that two methods here were not supposed to be stabilized along with the others. How come I didn't have to use the e.g. unchecked_math feature gate to implement these if e.g. u8::unchecked_add were not stabilized?

@andylizi
Copy link
Contributor

How come I didn't have to use the e.g. unchecked_math feature gate to implement these

Presumably because unchecked_math is also defined in libcore, and

// Only the cross-crate scenario matters when checking unstable APIs
let cross_crate = !def_id.is_local();
if !cross_crate {
return EvalResult::Allow;
}

@iago-lito
Copy link
Contributor Author

Hm, I think I see. So, to answer your question @dtolnay : no, this was not at all anticipated, at least on my side. What are the options then?

@jschwe
Copy link
Contributor

jschwe commented Apr 22, 2022

As a user, I would prefer to at least have the checked methods available on stable, since this is currently all that is preventing me from using the types.

@Noratrieb
Copy link
Member

I agree, stabilizing the checked methods would be useful right now, we can always stabilize unchecked later.

@TennyZhuang
Copy link
Contributor

How about splitting them to nonzero_checked_ops and nonzero_unchecked_ops?

@iago-lito
Copy link
Contributor Author

That's a good idea @TennyZhuang :) I'm just back from vacations, I'll do this as soon as I emerge from the global catchup.

@scottmcm
Copy link
Member

Note that splitting the feature causes some churn for any users on nightly.

You might consider a PR to stabilize the checked ops as nonzero_checked_op -- since there's no churn to add that to features since they're stable -- while leaving the unchecked ones under the previous feature name. Then either that could be the thing that gets FCP'd, or used to answer the question about specifically what's being stabilized.

See the upcoming text in rust-lang/std-dev-guide#32 (comment) for more details.

@yaahc
Copy link
Member

yaahc commented Jun 8, 2022

moving the FCP to the new partial stabilization

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Jun 8, 2022

@yaahc proposal cancelled.

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests