Handle Sub Overflow Edge Case

[matthewoestreich]

Hello! Thank you for this amazing library!!

The Issue

When trying to do something like Value::SignedInt(0) - Value::SignedInt(i64::MIN) I would receive a panic : attempt to subtract with overflow.

As far as I could tell, sub-underflow was accounted for, but not sub-overflow. However, it is entirely possible I overlooked something and this PR is incorrect.

The Fix

I created another shim for f64, CheckedSub:

// shim for the missing method on f64
trait CheckedSub: Sized + ops::Sub<Output = Self> {
    fn checked_sub(self, rhs: Self) -> Option<Self>;
}

impl CheckedSub for f64 {
    fn checked_sub(self, rhs: Self) -> Option<Self> {
        Some(self - rhs)
    }
}

and mirrored the AddAssign implementation:

impl<Rhs> ops::SubAssign<Rhs> for Value
where
    Rhs: Into<Value>,
{
    fn sub_assign(&mut self, rhs: Rhs) {
        let mut rhs = rhs.into();
        if rhs > *self {
            self.promote_to_signed();
        }
        *self = dispatch_operation!(self, rhs, n, |rhs| (*n).checked_sub(rhs).map(Value::from))
            .unwrap_or_else(|| {
                self.promote();
                dispatch_operation!(self, rhs, n, |rhs| Value::from(*n - rhs))
            })
    }
}

I also added edge case tests:

Click to show test

#[test]
fn sub_overflow_promotes_to_signed_or_float() {
    // ------------------------------------------------------------------------
    // -- sub overflow i64 bounds, result should promote to SignedBigInt
    // ------------------------------------------------------------------------
    let left: Value = 0_i64.into();
    let right: Value = i64::MIN.into();
    let result = left - right;
    let expected_order = Order::SignedBigInt;
    assert_eq!(
        result.order(),
        expected_order,
        "sub overflow should promote this to {expected_order:?} : got = {result:?}"
    );
    let expected_value = (9223372036854775808_i128).into();
    assert_eq!(
        result, expected_value,
        "sub overflow value should be {expected_value:?} : got = {result:?}"
    );

    // ------------------------------------------------------------------------
    // -- sub overflow i128 bounds, result should promote to Float
    // ------------------------------------------------------------------------
    let left: Value = 0_i128.into();
    let right: Value = i128::MIN.into();
    let result = left - right;
    let expected_order = Order::Float;
    assert_eq!(
        result.order(),
        expected_order,
        "sub overflow should promote this to {expected_order:?} : got = {result:?}"
    );
    let expected_value = (1.7014118346046923e38).into();
    assert_eq!(
        result, expected_value,
        "sub overflow value should be {expected_value:?} : got = {result:?}"
    );
}

Now sub-overflow and promotion of the result seem to be working as expected.

Please let me know if you have any questions or concerns!

[coriolinus]

Thanks for doing this work! I'm going make a small change, to parametrize the tests you wrote. But good catch, and I appreciate the fix!

[coriolinus]

Rather I'm going to ask you to apply this patch to the test cases, because I prefer that independent cases run independently:

diff --git a/src/value/arithmetic.rs b/src/value/arithmetic.rs
index 17c1860..83f4401 100644
--- a/src/value/arithmetic.rs
+++ b/src/value/arithmetic.rs
@@ -397,39 +397,26 @@ mod tests {
         assert_eq!(result, (-1_i64).into());
     }
 
-    #[test]
-    fn sub_overflow_promotes_to_signed_or_float() {
-        // ------------------------------------------------------------------------
-        // -- sub overflow i64 bounds, result should promote to SignedBigInt
-        // ------------------------------------------------------------------------
-        let left: Value = 0_i64.into();
-        let right: Value = i64::MIN.into();
+    #[rstest]
+    #[case::i64_overflows_to_i128(0_i64, i64::MIN, Order::SignedBigInt, 9223372036854775808_i128)]
+    #[case::i128_overflows_to_float(0_i128, i128::MIN, Order::Float, 1.7014118346046923e38)]
+    fn sub_overflow_promotes_to_signed_or_float(
+        #[case] left: impl Into<Value>,
+        #[case] right: impl Into<Value>,
+        #[case] expected_order: Order,
+        #[case] expected_value: impl Into<Value>,
+    ) {
+        let left = left.into();
+        let right = right.into();
         let result = left - right;
-        let expected_order = Order::SignedBigInt;
-        assert_eq!(
-            result.order(),
-            expected_order,
-            "sub overflow should promote this to {expected_order:?} : got = {result:?}"
-        );
-        let expected_value = (9223372036854775808_i128).into();
-        assert_eq!(
-            result, expected_value,
-            "sub overflow value should be {expected_value:?} : got = {result:?}"
-        );
 
-        // ------------------------------------------------------------------------
-        // -- sub overflow i128 bounds, result should promote to Float
-        // ------------------------------------------------------------------------
-        let left: Value = 0_i128.into();
-        let right: Value = i128::MIN.into();
-        let result = left - right;
-        let expected_order = Order::Float;
         assert_eq!(
             result.order(),
             expected_order,
             "sub overflow should promote this to {expected_order:?} : got = {result:?}"
         );
-        let expected_value = (1.7014118346046923e38).into();
+
+        let expected_value = expected_value.into();
         assert_eq!(
             result, expected_value,
             "sub overflow value should be {expected_value:?} : got = {result:?}"

Once that's in, I'll merge.

[matthewoestreich]

Makes sense! I have applied those changes.

Also, on another note, I noticed this same 'promotion overflow' issue in MulAssign, DivAssign, and RemAssign. Should I send over PR's for each individually, or are you OK with me sending one PR that covers all?

[coriolinus]

A batch PR makes sense. The main thing is that test cases should cover all changes.