Impl {Add,Sub,Mul,Div,Rem,BitXor,BitOr,BitAnd}Assign<$t> for Wrapping<$t> for rust 1.60.0

[kellerkindt]

Tracking issue #93204

This is about adding basic integer operations to the Wrapping type:

let mut value = Wrapping(2u8);
value += 3u8;
value -= 1u8;
value *= 2u8;
value /= 2u8;
value %= 2u8;
value ^= 255u8;
value |= 123u8;
value &= 2u8;

Because this adds stable impls on a stable type, it runs into the following issue if an #[unstable(...)] attribute is used:

an `#[unstable]` annotation here has no effect
note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information

This means - if I understood this correctly - the new impls have to be stabilized instantly.
Which in turn means, this PR has to kick of an FCP on the tracking issue as well?

This impl is analog to 1c0dc18 #92356 for the Saturating type @dtolnay @Mark-Simulacrum

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[m-ou-se]

I feel a bit conflicted about these. I agree Wrapping can be annoying to work with, and this is a big improvement to that. But this also adds some inconcistencies that we might not be able to fix:

• This adds Add<i32> for Wrapping<i32> (etc.), but do we also want Add<Wrapping<i32>> for i32? Having w + 1 work but not 1 + w seems bad. But adding even more implementations on i32 doesn't seem great either.
• Should we also add Add<&i32> for Wrapping<i32> (etc.), just like we have Add<&Wrapping> for Wrapping? It would be consistent, but it would technically be a breaking change since Wrapping(1) + &Default::default() would no longer compile.
• How about PartialEq/PartialOrd? We can't add those as it'd be a breaking change, but having w + 1 work, while still requiring an explicit w == Wrapping(1) also doesn't feel great.
• What if someone wants a non-wrapping result type? E.g. they write let b: i32 = w + 1; and forget the .0 in w.0 to get the non-wrapping type. Today, they get an error that they can't add i32 to Wrapping<i32> and they need to pick either the wrapping behaviour or the non-wrapping behaviour explicitly, by wrapping the 1 in Wrapping(1) or by unwrapping the w with .0. But with this change, the addition would happen in a wrapping way, and then give the an error about assigning a Wrapping<i32> to a i32. Or not give any error if : i32 wasn't explicit.

This makes me think it might be better if we only do this for the *Assign traits. wrapping_i32 += 1; clearly follows the behaviour of the wrapping_i32 variable, and some_i32 += Wrapping(1); won't compile.

These are just some initial thoughts after thinking about this for a few minutes. I'd love to hear some input from the rest of @rust-lang/libs-api.

This means - if I understood this correctly - the new impls have to be stabilized instantly. Which in turn means, this PR has to kick of an FCP on the tracking issue as well?

In such cases we usually do the FCP on the PR itself.

[jackh726]

This makes me think it might be better if we only do this for the *Assign traits. wrapping_i32 += 1; clearly follows the behaviour of the wrapping_i32 variable, and some_i32 += Wrapping(1); won't compile.

Just a fly on the wall here, but I do want to say that I really like this idea. And a reminder to make sure to change the Saturating impls if this changes!

[kellerkindt]

This makes me think it might be better if we only do this for the *Assign traits. wrapping_i32 += 1; clearly follows the behaviour of the wrapping_i32 variable, and some_i32 += Wrapping(1); won't compile.

This was my initial intention before I thought the other impls would be nice to have as well. Thanks for pointing out the issues. I have updated the commit and I will post a PR to remove the impls from the Saturating type once this PR is merged.

[yaahc]

These are just some initial thoughts after thinking about this for a few minutes. I'd love to hear some input from the rest of @rust-lang/libs-api.

I very much like and support your suggestion of starting with just the Assign traits since they're much easier to reason about here.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting, and given the change to only impl the *Assign traits, we'd like to merge this:

@rfcbot merge

Also, we'd like to be explicit that this should not be taken as precedent for adding methods to do normal_num += wrapping, only wrapping += normal_num. Similarly, while we should enable saturating += normal_num, we should not enable saturating += wrapping or wrapping += saturating.

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

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]

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

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

This will be merged soon.

[m-ou-se]

@bors r+

[bors]

📌 Commit 14ff58c has been approved by m-ou-se