Overflow promotion fix

[matthewoestreich]

PR that was mentioned earlier today..

I went ahead and combined the checked_* methods into a single trait, CheckedMaths, which seemed "cleaner" to me. If you do not like this, I have no problem changing it back to one trait per checked_* method.

Click to view CheckedMaths trait

// shim for the missing method on f64
trait CheckedMaths:
    Sized
    + ops::Add<Output = Self>
    + ops::Sub<Output = Self>
    + ops::Div<Output = Self>
    + ops::Mul<Output = Self>
    + ops::Rem<Output = Self>
{
    fn checked_add(self, rhs: Self) -> Option<Self>;
    fn checked_sub(self, rhs: Self) -> Option<Self>;
    fn checked_div(self, rhs: Self) -> Option<Self>;
    fn checked_mul(self, rhs: Self) -> Option<Self>;
    fn checked_rem(self, rhs: Self) -> Option<Self>;
}

impl CheckedMaths for f64 {
    fn checked_add(self, rhs: Self) -> Option<Self> {
        Some(self + rhs)
    }

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

    fn checked_div(self, rhs: Self) -> Option<Self> {
        Some(self / rhs)
    }

    fn checked_mul(self, rhs: Self) -> Option<Self> {
        Some(self * rhs)
    }

    fn checked_rem(self, rhs: Self) -> Option<Self> {
        Some(self % rhs)
    }
}

---

Separate Concern

TLDR

Click to view TLDR demo

println!("\t=============== RAW VALUES ==============\n");
println!("i128::MAX = {}", i128::MAX);
let a = i128::MAX as f64;
println!("i128::MAX as f64 = {a:e} [no fmt] = {a}");
println!("\n\t============== CONVERSION TO f64 PRIOR TO DIVISION ==============\n");
println!("i128::MAX / 2 = {}", i128::MAX / 2);
let b = (i128::MAX as f64) / 2.0;
println!("(i128::MAX as f64) / 2.0 = {b:e} [no fmt] = {b} <-- precision is lost!");
println!("\n\t============== CONVERSION FROM f64 BACK INTO i128 ==============\n");
let c = b as i128;
println!("((i128::MAX as f64) / 2.0) as i128 = {c:e} [no fmt] = {c} <-- loss of precision reflected");

//        =============== RAW VALUES ==============
//
// i128::MAX = 170141183460469231731687303715884105727
// i128::MAX as f64 = 1.7014118346046923e38 [no fmt] = 170141183460469230000000000000000000000
//
//        ============== CONVERSION TO f64 PRIOR TO DIVISION ==============
//
// i128::MAX / 2 = 85070591730234615865843651857942052863
// (i128::MAX as f64) / 2.0 = 8.507059173023462e37 [no fmt] = 85070591730234620000000000000000000000 <-- precision is lost!
//
//        ============== CONVERSION FROM f64 BACK INTO i128 ==============
//
// ((i128::MAX as f64) / 2.0) as i128 = 8.5070591730234615865843651857942052864e37 [no fmt] = 85070591730234615865843651857942052864 <-- loss of precision reflected

Extended Explanation

Also, I noticed the existing implementation of DivAssign causes issues with trunc_div.

For example, notice the last digit (3 vs 4):

println!("{}", i128::MAX / 2); 
//-> 85070591730234615865843651857942052863
println!("{:?}", Value::SignedBigInt(i128::MAX).trunc_div(2)); 
//-> UnsignedBigInt(85070591730234615865843651857942052864)

I could be wrong, but I believe this is because the DivAssign implementation promotes to float prior to dividing, which causes the i128::MAX to lose some precision, thus producing an incorrect answer.

Implementation of DivAssign for reference:

FYI - this is the new implementation, but the same issue exists in the old implementation as well - this change is not what caused the issue.

Click to view DivAssign

impl<Rhs> ops::DivAssign<Rhs> for Value
where
    Rhs: Into<Value>,
{
    fn div_assign(&mut self, rhs: Rhs) {
        self.promote_to_float();
        let mut rhs = rhs.into();
        rhs.promote_to_float();
        *self = dispatch_operation!(self, rhs, n, |rhs| (*n).checked_div(rhs).map(Value::from))
            .unwrap_or_else(|| {
                self.promote();
                dispatch_operation!(self, rhs, n, |rhs| Value::from(*n / rhs))
            });
    }
}

Changing the implementation to the code below solves the issue described above.

• I didn't want to make this change because I saw comments mentioning that divison always produces a float
• This change would mean all division would follow the same promotion rules as the AddAssign/SubAssign/MulAssign/RemAssign implementations
• This change also causes a bunch of tests to fail for the same promotional reason (not all divison would produce a float)

Click to view DivAssign fix

impl<Rhs> ops::DivAssign<Rhs> for Value
where
    Rhs: Into<Value>,
{
    fn div_assign(&mut self, rhs: Rhs) {
        let mut rhs = rhs.into();
        *self = dispatch_operation!(self, rhs, n, |rhs| (*n).checked_div(rhs).map(Value::from))
            .unwrap_or_else(|| {
                self.promote();
                dispatch_operation!(self, rhs, n, |rhs| Value::from(*n / rhs))
            });
    }
}

Thoughts?