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

ACP: impl {Div,Rem}Assign<NonZeroX> for X #346

Closed
JarvisCraft opened this issue Mar 2, 2024 · 1 comment
Closed

ACP: impl {Div,Rem}Assign<NonZeroX> for X #346

JarvisCraft opened this issue Mar 2, 2024 · 1 comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@JarvisCraft
Copy link

JarvisCraft commented Mar 2, 2024

Proposal

Problem statement

At the moment, there are impl {Div,Rem}<NonZeroX> for X which use the fact that the division by a non-zero value are always infallible.

Yet there seem to be no corresponding {Div,Rem}Assign implementations. While the lhs %= rhs and lhs/= rhs can be described in user code with lhs = lhs / rhs and lhs = lhs % rhs respectively, there seems to be no reason to not implement the corresponding traits on numeric types themselves.

Motivating examples or use cases

Simply,

let mut b = 123u8;
let a = NonZeroU8::new(1).unwrap();
b = b / a;

compiles; but

let mut b = 123u8;
let a = NonZeroU8::new(1).unwrap();
b /= a;

does not.

Solution sketch

The implementation (tested against master) adds the following into core::num::nonzero::nonzero_integer_signedness_dependent_impls:

        #[stable(feature = "nonzero_div2", since = "CURRENT_RUSTC_VERSION")]
        impl DivAssign<$Ty> for $Int {
            /// This operation rounds towards zero,
            /// truncating any fractional part of the exact result, and cannot panic.
            #[inline]
            fn div_assign(&mut self, other: $Ty) {
                *self = *self / other;
            }
        }

        #[stable(feature = "nonzero_div2", since = "CURRENT_RUSTC_VERSION")]
        impl RemAssign<$Ty> for $Int {
            /// This operation satisfies `n % d == n - (n / d) * d`, and cannot panic.
            #[inline]
            fn rem_assign(&mut self, other: $Ty) {
                *self = *self % other;
            }
        }
Diff
diff --git a/library/core/src/num/nonzero.rs b/library/core/src/num/nonzero.rs
index 9e34c0d240d..61257cd71e3 100644
--- a/library/core/src/num/nonzero.rs
+++ b/library/core/src/num/nonzero.rs
@@ -5,7 +5,7 @@
 use crate::hash::{Hash, Hasher};
 use crate::intrinsics;
 use crate::marker::StructuralPartialEq;
-use crate::ops::{BitOr, BitOrAssign, Div, Neg, Rem};
+use crate::ops::{BitOr, BitOrAssign, Div, DivAssign, Neg, Rem, RemAssign};
 use crate::str::FromStr;
 
 use super::from_str_radix;
@@ -800,6 +800,16 @@ fn div(self, other: $Ty) -> $Int {
             }
         }
 
+        #[stable(feature = "nonzero_div2", since = "CURRENT_RUSTC_VERSION")]
+        impl DivAssign<$Ty> for $Int {
+            /// This operation rounds towards zero,
+            /// truncating any fractional part of the exact result, and cannot panic.
+            #[inline]
+            fn div_assign(&mut self, other: $Ty) {
+                *self = *self / other;
+            }
+        }
+
         #[stable(feature = "nonzero_div", since = "1.51.0")]
         impl Rem<$Ty> for $Int {
             type Output = $Int;
@@ -812,6 +822,15 @@ fn rem(self, other: $Ty) -> $Int {
                 unsafe { intrinsics::unchecked_rem(self, other.get()) }
             }
         }
+
+        #[stable(feature = "nonzero_div2", since = "CURRENT_RUSTC_VERSION")]
+        impl RemAssign<$Ty> for $Int {
+            /// This operation satisfies `n % d == n - (n / d) * d`, and cannot panic.
+            #[inline]
+            fn rem_assign(&mut self, other: $Ty) {
+                *self = *self % other;
+            }
+        }
     };
 
     // Impls for signed nonzero types only.

I've used the synthetic nonzero_div2 feature for these and, IMO, they may be an insta-stable API.

Alternatives

  • Status quo: these trait implementations are not obligatory and thus we can just not ignore them, but I don't see any reason for it, while implementing them seems to be a consistency enhancement.
@JarvisCraft JarvisCraft added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Mar 2, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Mar 5, 2024

We briefly discussed this in the libs-api meeting. This looks fine to us.

Since this is insta-stable, we'll propose an FCP on the PR.

@m-ou-se m-ou-se closed this as completed Mar 5, 2024
@m-ou-se m-ou-se added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Mar 5, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 17, 2024
feat: implement `{Div,Rem}Assign<NonZero<X>>` on `X`

# Description

This PR implements `DivAssign<X>` and `RemAssign<X>` on `X` as suggested in rust-lang/libs-team#346.

Since this is just a trait implementation on an already stable type, for which non-assign operator traits are already stable, I suggest that it is an insta-stable feature.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 18, 2024
feat: implement `{Div,Rem}Assign<NonZero<X>>` on `X`

# Description

This PR implements `DivAssign<X>` and `RemAssign<X>` on `X` as suggested in rust-lang/libs-team#346.

Since this is just a trait implementation on an already stable type, for which non-assign operator traits are already stable, I suggest that it is an insta-stable feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

2 participants