Auto merge of rust-lang#121571 - clarfonthey:unchecked-math-preconditions, r=<try>

Add debug_assert_nounwind to unchecked_{add,sub,neg,mul,shl,shr} methods

(Old PR is haunted, opening a new one. See rust-lang#117494 for previous discussion.)

This ensures that these preconditions are actually checked in debug mode, and hopefully should let people know if they messed up. I've also replaced the calls (I could find) in the code that use these intrinsics directly with those that use these methods, so that the asserts actually apply.

More discussions on people misusing these methods in the tracking issue: rust-lang#85122.

Author: bors

library/core/src/num/int_macros.rs

@@ -510,9 +510,14 @@ macro_rules! int_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_add(self, rhs: Self) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_add`.
-            unsafe { intrinsics::unchecked_add(self, rhs) }
+            debug_assert_nounwind!(
+                !self.overflowing_add(rhs).1,
+                concat!(stringify!($SelfT), "::unchecked_add cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
+            unsafe {
+                intrinsics::unchecked_add(self, rhs)
+            }
         }
 
         /// Checked addition with an unsigned integer. Computes `self + rhs`,
@@ -648,9 +653,14 @@ macro_rules! int_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_sub(self, rhs: Self) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_sub`.
-            unsafe { intrinsics::unchecked_sub(self, rhs) }
+            debug_assert_nounwind!(
+                !self.overflowing_sub(rhs).1,
+                concat!(stringify!($SelfT), "::unchecked_sub cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
+            unsafe {
+                intrinsics::unchecked_sub(self, rhs)
+            }
         }
 
         /// Checked subtraction with an unsigned integer. Computes `self - rhs`,
@@ -786,9 +796,14 @@ macro_rules! int_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_mul(self, rhs: Self) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_mul`.
-            unsafe { intrinsics::unchecked_mul(self, rhs) }
+            debug_assert_nounwind!(
+                !self.overflowing_mul(rhs).1,
+                concat!(stringify!($SelfT), "::unchecked_mul cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
+            unsafe {
+                intrinsics::unchecked_mul(self, rhs)
+            }
         }
 
         /// Checked integer division. Computes `self / rhs`, returning `None` if `rhs == 0`
@@ -1125,9 +1140,15 @@ macro_rules! int_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_neg(self) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_neg`.
-            unsafe { intrinsics::unchecked_sub(0, self) }
+            debug_assert_nounwind!(
+                !self.overflowing_neg().1,
+                concat!(stringify!($SelfT), "::unchecked_neg cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
+            unsafe {
+                let n = self;
+                intrinsics::unchecked_sub(0, n)
+            }
         }
 
         /// Strict negation. Computes `-self`, panicking if `self == MIN`.
@@ -1179,7 +1200,7 @@ macro_rules! int_impl {
         #[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
         #[must_use = "this returns the result of the operation, \
                       without modifying the original"]
-        #[inline]
+        #[inline(always)]
         pub const fn checked_shl(self, rhs: u32) -> Option<Self> {
             let (a, b) = self.overflowing_shl(rhs);
             if unlikely!(b) { None } else { Some(a) }
@@ -1241,10 +1262,15 @@ macro_rules! int_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_shl(self, rhs: u32) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_shl`.
+            debug_assert_nounwind!(
+                rhs < Self::BITS,
+                concat!(stringify!($SelfT), "::unchecked_shl cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
             // Any legal shift amount is losslessly representable in the self type.
-            unsafe { intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
+            unsafe {
+                intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs))
+            }
         }
 
         /// Checked shift right. Computes `self >> rhs`, returning `None` if `rhs` is
@@ -1262,7 +1288,7 @@ macro_rules! int_impl {
         #[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
         #[must_use = "this returns the result of the operation, \
                       without modifying the original"]
-        #[inline]
+        #[inline(always)]
         pub const fn checked_shr(self, rhs: u32) -> Option<Self> {
             let (a, b) = self.overflowing_shr(rhs);
             if unlikely!(b) { None } else { Some(a) }
@@ -1324,10 +1350,15 @@ macro_rules! int_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_shr(self, rhs: u32) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_shr`.
+            debug_assert_nounwind!(
+                rhs < Self::BITS,
+                concat!(stringify!($SelfT), "::unchecked_shr cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
             // Any legal shift amount is losslessly representable in the self type.
-            unsafe { intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
+            unsafe {
+                intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs))
+            }
         }
 
         /// Checked absolute value. Computes `self.abs()`, returning `None` if
@@ -1991,7 +2022,9 @@ macro_rules! int_impl {
             // SAFETY: the masking by the bitsize of the type ensures that we do not shift
             // out of bounds
             unsafe {
-                self.unchecked_shl(rhs & (Self::BITS - 1))
+                // FIXME: we can't optimize out the extra check here,
+                // so, we can't just call the method for now
+                intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1)))
             }
         }
 
@@ -2021,7 +2054,9 @@ macro_rules! int_impl {
             // SAFETY: the masking by the bitsize of the type ensures that we do not shift
             // out of bounds
             unsafe {
-                self.unchecked_shr(rhs & (Self::BITS - 1))
+                // FIXME: we can't optimize out the extra check here,
+                // so, we can't just call the method for now
+                intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1)))
             }
         }
 

library/core/src/num/mod.rs

@@ -7,6 +7,7 @@ use crate::hint;
 use crate::intrinsics;
 use crate::mem;
 use crate::ops::{Add, Mul, Sub};
+use crate::panic::debug_assert_nounwind;
 use crate::str::FromStr;
 
 // Used because the `?` operator is not allowed in a const context.

library/core/src/num/uint_macros.rs

@@ -518,9 +518,15 @@ macro_rules! uint_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_add(self, rhs: Self) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_add`.
-            unsafe { intrinsics::unchecked_add(self, rhs) }
+            debug_assert_nounwind!(
+                !self.overflowing_add(rhs).1,
+                concat!(stringify!($SelfT), "::unchecked_add cannot overflow"),
+            );
+
+            // SAFETY: this is guaranteed to be safe by the caller.
+            unsafe {
+                intrinsics::unchecked_add(self, rhs)
+            }
         }
 
         /// Checked addition with a signed integer. Computes `self + rhs`,
@@ -662,9 +668,14 @@ macro_rules! uint_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_sub(self, rhs: Self) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_sub`.
-            unsafe { intrinsics::unchecked_sub(self, rhs) }
+            debug_assert_nounwind!(
+                !self.overflowing_sub(rhs).1,
+                concat!(stringify!($SelfT), "::unchecked_sub cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
+            unsafe {
+                intrinsics::unchecked_sub(self, rhs)
+            }
         }
 
         /// Checked integer multiplication. Computes `self * rhs`, returning
@@ -744,9 +755,14 @@ macro_rules! uint_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_mul(self, rhs: Self) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_mul`.
-            unsafe { intrinsics::unchecked_mul(self, rhs) }
+            debug_assert_nounwind!(
+                !self.overflowing_mul(rhs).1,
+                concat!(stringify!($SelfT), "::unchecked_mul cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
+            unsafe {
+                intrinsics::unchecked_mul(self, rhs)
+            }
         }
 
         /// Checked integer division. Computes `self / rhs`, returning `None`
@@ -1239,7 +1255,7 @@ macro_rules! uint_impl {
         #[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
         #[must_use = "this returns the result of the operation, \
                       without modifying the original"]
-        #[inline]
+        #[inline(always)]
         pub const fn checked_shl(self, rhs: u32) -> Option<Self> {
             let (a, b) = self.overflowing_shl(rhs);
             if unlikely!(b) { None } else { Some(a) }
@@ -1301,10 +1317,15 @@ macro_rules! uint_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_shl(self, rhs: u32) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_shl`.
+            debug_assert_nounwind!(
+                rhs < Self::BITS,
+                concat!(stringify!($SelfT), "::unchecked_shl cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
             // Any legal shift amount is losslessly representable in the self type.
-            unsafe { intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
+            unsafe {
+                intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs))
+            }
         }
 
         /// Checked shift right. Computes `self >> rhs`, returning `None`
@@ -1322,7 +1343,7 @@ macro_rules! uint_impl {
         #[rustc_const_stable(feature = "const_checked_int_methods", since = "1.47.0")]
         #[must_use = "this returns the result of the operation, \
                       without modifying the original"]
-        #[inline]
+        #[inline(always)]
         pub const fn checked_shr(self, rhs: u32) -> Option<Self> {
             let (a, b) = self.overflowing_shr(rhs);
             if unlikely!(b) { None } else { Some(a) }
@@ -1384,10 +1405,15 @@ macro_rules! uint_impl {
         #[inline(always)]
         #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
         pub const unsafe fn unchecked_shr(self, rhs: u32) -> Self {
-            // SAFETY: the caller must uphold the safety contract for
-            // `unchecked_shr`.
+            debug_assert_nounwind!(
+                rhs < Self::BITS,
+                concat!(stringify!($SelfT), "::unchecked_shr cannot overflow"),
+            );
+            // SAFETY: this is guaranteed to be safe by the caller.
             // Any legal shift amount is losslessly representable in the self type.
-            unsafe { intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs)) }
+            unsafe {
+                intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs))
+            }
         }
 
         /// Checked exponentiation. Computes `self.pow(exp)`, returning `None` if
@@ -1878,7 +1904,9 @@ macro_rules! uint_impl {
             // SAFETY: the masking by the bitsize of the type ensures that we do not shift
             // out of bounds
             unsafe {
-                self.unchecked_shl(rhs & (Self::BITS - 1))
+                // FIXME: we can't optimize out the extra check here,
+                // so, we can't just call the method for now
+                intrinsics::unchecked_shl(self, conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1)))
             }
         }
 
@@ -1911,7 +1939,9 @@ macro_rules! uint_impl {
             // SAFETY: the masking by the bitsize of the type ensures that we do not shift
             // out of bounds
             unsafe {
-                self.unchecked_shr(rhs & (Self::BITS - 1))
+                // FIXME: we can't optimize out the extra check here,
+                // so, we can't just call the method for now
+                intrinsics::unchecked_shr(self, conv_rhs_for_unchecked_shift!($SelfT, rhs & (Self::BITS - 1)))
             }
         }
 

library/core/src/ops/index_range.rs

@@ -1,4 +1,3 @@
-use crate::intrinsics::{unchecked_add, unchecked_sub};
 use crate::iter::{FusedIterator, TrustedLen};
 use crate::num::NonZero;
 
@@ -44,7 +43,7 @@ impl IndexRange {
     #[inline]
     pub const fn len(&self) -> usize {
         // SAFETY: By invariant, this cannot wrap
-        unsafe { unchecked_sub(self.end, self.start) }
+        unsafe { self.end.unchecked_sub(self.start) }
     }
 
     /// # Safety
@@ -55,7 +54,7 @@ impl IndexRange {
 
         let value = self.start;
         // SAFETY: The range isn't empty, so this cannot overflow
-        self.start = unsafe { unchecked_add(value, 1) };
+        self.start = unsafe { value.unchecked_add(1) };
         value
     }
 
@@ -66,7 +65,7 @@ impl IndexRange {
         debug_assert!(self.start < self.end);
 
         // SAFETY: The range isn't empty, so this cannot overflow
-        let value = unsafe { unchecked_sub(self.end, 1) };
+        let value = unsafe { self.end.unchecked_sub(1) };
         self.end = value;
         value
     }
@@ -81,7 +80,7 @@ impl IndexRange {
         let mid = if n <= self.len() {
             // SAFETY: We just checked that this will be between start and end,
             // and thus the addition cannot overflow.
-            unsafe { unchecked_add(self.start, n) }
+            unsafe { self.start.unchecked_add(n) }
         } else {
             self.end
         };
@@ -100,7 +99,7 @@ impl IndexRange {
         let mid = if n <= self.len() {
             // SAFETY: We just checked that this will be between start and end,
             // and thus the addition cannot overflow.
-            unsafe { unchecked_sub(self.end, n) }
+            unsafe { self.end.unchecked_sub(n) }
         } else {
             self.start
         };

library/core/src/panic.rs

@@ -167,7 +167,7 @@ pub macro debug_assert_nounwind {
             }
         }
     },
-    ($cond:expr, $message:expr) => {
+    ($cond:expr, $message:expr $(,)?) => {
         if $crate::intrinsics::debug_assertions() {
             if !$cond {
                 $crate::panicking::panic_nounwind($message);

library/core/src/ptr/const_ptr.rs

@@ -1021,8 +1021,8 @@ impl<T: ?Sized> *const T {
     #[must_use = "returns a new pointer rather than modifying its argument"]
     #[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
     // We could always go back to wrapping if unchecked becomes unacceptable
-    #[rustc_allow_const_fn_unstable(const_int_unchecked_arith)]
     #[inline(always)]
+    #[rustc_allow_const_fn_unstable(unchecked_math)]
     #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
     pub const unsafe fn sub(self, count: usize) -> Self
     where
@@ -1035,7 +1035,10 @@ impl<T: ?Sized> *const T {
             // SAFETY: the caller must uphold the safety contract for `offset`.
             // Because the pointee is *not* a ZST, that means that `count` is
             // at most `isize::MAX`, and thus the negation cannot overflow.
-            unsafe { self.offset(intrinsics::unchecked_sub(0, count as isize)) }
+            // FIXME: replacing unchecked_sub with unchecked_neg and replacing the
+            // unchecked_math flag with unchecked_neg will anger the UnstableInStable lint
+            // and I cannot for the life of me understand why
+            unsafe { self.offset(0isize.unchecked_sub(count as isize)) }
         }
     }
 

library/core/src/ptr/mut_ptr.rs

@@ -1121,7 +1121,7 @@ impl<T: ?Sized> *mut T {
     #[must_use = "returns a new pointer rather than modifying its argument"]
     #[rustc_const_stable(feature = "const_ptr_offset", since = "1.61.0")]
     // We could always go back to wrapping if unchecked becomes unacceptable
-    #[rustc_allow_const_fn_unstable(const_int_unchecked_arith)]
+    #[rustc_allow_const_fn_unstable(unchecked_math)]
     #[inline(always)]
     #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
     pub const unsafe fn sub(self, count: usize) -> Self
@@ -1135,7 +1135,10 @@ impl<T: ?Sized> *mut T {
             // SAFETY: the caller must uphold the safety contract for `offset`.
             // Because the pointee is *not* a ZST, that means that `count` is
             // at most `isize::MAX`, and thus the negation cannot overflow.
-            unsafe { self.offset(intrinsics::unchecked_sub(0, count as isize)) }
+            // FIXME: replacing unchecked_sub with unchecked_neg and replacing the
+            // unchecked_math flag with unchecked_neg will anger the UnstableInStable lint
+            // and I cannot for the life of me understand why
+            unsafe { self.offset(0isize.unchecked_sub(count as isize)) }
         }
     }