Make Ord and Hash iterative to avoid stack overflow

src/cmp.rs

@@ -40,29 +40,56 @@ impl<T: Clone + Integer> Ord for Ratio<T> {
         // division below, or even always avoid it for BigInt and BigUint.
         // FIXME- future breaking change to add Checked* to Integer?
 
-        // Compare as floored integers and remainders
-        let (self_int, self_rem) = self.numer.div_mod_floor(&self.denom);
-        let (other_int, other_rem) = other.numer.div_mod_floor(&other.denom);
-        match self_int.cmp(&other_int) {
-            Ordering::Greater => Ordering::Greater,
-            Ordering::Less => Ordering::Less,
-            Ordering::Equal => {
-                match (self_rem.is_zero(), other_rem.is_zero()) {
-                    (true, true) => Ordering::Equal,
-                    (true, false) => Ordering::Less,
-                    (false, true) => Ordering::Greater,
-                    (false, false) => {
-                        // Compare the reciprocals of the remaining fractions in reverse
-                        let self_recip = Ratio::new_raw(self.denom.clone(), self_rem);
-                        let other_recip = Ratio::new_raw(other.denom.clone(), other_rem);
-                        self_recip.cmp(&other_recip).reverse()
-                    }
+        let (mut lhs_nbuf, mut lhs_dbuf, mut rhs_nbuf, mut rhs_dbuf);
+        let (mut lhs_numer, mut lhs_denom) = (&self.numer, &self.denom);
+        let (mut rhs_numer, mut rhs_denom) = (&other.numer, &other.denom);
+        loop {
+            match cmp_int(lhs_numer, lhs_denom, rhs_numer, rhs_denom) {
+                Ok(ord) => return ord,
+                Err((lhs_rem, rhs_rem)) => {
+                    lhs_nbuf = lhs_rem;
+                    lhs_numer = &lhs_nbuf;
+                    rhs_nbuf = rhs_rem;
+                    rhs_numer = &rhs_nbuf;
+                }
+            }
+
+            match cmp_int(lhs_denom, lhs_numer, rhs_denom, rhs_numer) {
+                Ok(ord) => return ord.reverse(),
+                Err((lhs_rem, rhs_rem)) => {
+                    lhs_dbuf = lhs_rem;
+                    lhs_denom = &lhs_dbuf;
+                    rhs_dbuf = rhs_rem;
+                    rhs_denom = &rhs_dbuf;
                 }
             }
         }
     }
 }
 
+/// Compare as floored integers; if equal then return remainders
+#[inline(always)]
+fn cmp_int<T: Integer>(
+    lhs_numer: &T,
+    lhs_denom: &T,
+    rhs_numer: &T,
+    rhs_denom: &T,
+) -> Result<Ordering, (T, T)> {
+    // Compare as floored integers and remainders
+    let (lhs_int, lhs_rem) = lhs_numer.div_mod_floor(lhs_denom);
+    let (rhs_int, rhs_rem) = rhs_numer.div_mod_floor(rhs_denom);
+    match lhs_int.cmp(&rhs_int) {
+        Ordering::Greater => Ok(Ordering::Greater),
+        Ordering::Less => Ok(Ordering::Less),
+        Ordering::Equal => match (lhs_rem.is_zero(), rhs_rem.is_zero()) {
+            (true, true) => Ok(Ordering::Equal),
+            (true, false) => Ok(Ordering::Less),
+            (false, true) => Ok(Ordering::Greater),
+            (false, false) => Err((lhs_rem, rhs_rem)),
+        },
+    }
+}
+
 impl<T: Clone + Integer> PartialOrd for Ratio<T> {
     #[inline]
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
@@ -83,15 +110,31 @@ impl<T: Clone + Integer> Eq for Ratio<T> {}
 // with `Eq` even for non-reduced ratios.
 impl<T: Clone + Integer + Hash> Hash for Ratio<T> {
     fn hash<H: Hasher>(&self, state: &mut H) {
-        recurse(&self.numer, &self.denom, state);
+        // This looping structure is similar to `Ord::cmp`
+        let (mut nbuf, mut dbuf);
+        let (mut numer, mut denom) = (&self.numer, &self.denom);
 
-        fn recurse<T: Integer + Hash, H: Hasher>(numer: &T, denom: &T, state: &mut H) {
-            if !denom.is_zero() {
+        loop {
+            {
                 let (int, rem) = numer.div_mod_floor(denom);
                 int.hash(state);
-                recurse(denom, &rem, state);
-            } else {
-                denom.hash(state);
+                if rem.is_zero() {
+                    rem.hash(state);
+                    return;
+                }
+                nbuf = rem;
+                numer = &nbuf;
+            }
+
+            {
+                let (int, rem) = denom.div_mod_floor(numer);
+                int.hash(state);
+                if rem.is_zero() {
+                    rem.hash(state);
+                    return;
+                }
+                dbuf = rem;
+                denom = &dbuf;
             }
         }
     }

src/tests.rs

@@ -223,6 +223,54 @@ fn test_cmp_overflow() {
     }
 }
 
+#[test]
+#[ignore] // 2^30 combinations are slow in debug, but manageable with `--release`
+fn test_cmp_i8_full() {
+    // All (non-reduced) `Ratio<i8>` with normal denom > 0
+    let ratios = (i8::MIN..=i8::MAX)
+        .flat_map(|numer| (1..=i8::MAX).map(move |denom| Ratio::new_raw(numer, denom)));
+    for lhs in ratios.clone() {
+        let lhs_float = f64::from(lhs.numer) / f64::from(lhs.denom);
+        for rhs in ratios.clone() {
+            let rhs_float = f64::from(rhs.numer) / f64::from(rhs.denom);
+            let ord_ratio = lhs.cmp(&rhs);
+            let ord_float = lhs_float.partial_cmp(&rhs_float).unwrap();
+            assert_eq!(
+                ord_ratio, ord_float,
+                "{lhs} ({lhs_float}) <=> {rhs} ({rhs_float})"
+            );
+
+            #[cfg(feature = "std")]
+            if ord_ratio.is_eq() {
+                assert_eq!(hash_one(&lhs), hash_one(&rhs));
+            }
+        }
+    }
+}
+
+#[cfg(feature = "num-bigint")]
+fn big_ratios() -> (BigRational, BigRational) {
+    let mut numer = BigInt::from(29u32);
+    let mut denom = BigInt::from(28u32);
+    for _ in 0..13 {
+        numer = &numer * &numer;
+        denom = &denom * &denom;
+    }
+    let one = BigInt::one();
+    let a = Ratio::new_raw(numer, denom);
+    let b = Ratio::new_raw(a.numer() + &one, a.denom() + &one);
+    (a, b)
+}
+
+#[test]
+#[cfg(feature = "num-bigint")]
+fn test_cmp_stack_overflow() {
+    // When `cmp` was recursive, this test caused a stack overflow. (num-rational#140)
+    let (a, b) = big_ratios();
+    assert!(a > b);
+    assert!(a != b);
+}
+
 #[test]
 fn test_to_integer() {
     assert_eq!(_0.to_integer(), 0);
@@ -649,20 +697,20 @@ fn test_signed() {
     assert!(!_0.is_negative());
 }
 
-#[test]
+// TODO(MSRV 1.71): use `BuildHasher::hash_one`
 #[cfg(feature = "std")]
-fn test_hash() {
-    // TODO(MSRV 1.71): use `BuildHasher::hash_one`
-    #[cfg(feature = "std")]
-    fn hash_one<T: std::hash::Hash>(x: &T) -> u64 {
-        use std::collections::hash_map::RandomState;
-        use std::hash::{BuildHasher, Hasher};
+fn hash_one<T: std::hash::Hash>(x: &T) -> u64 {
+    use std::collections::hash_map::RandomState;
+    use std::hash::{BuildHasher, Hasher};
 
-        let mut hasher = <RandomState as BuildHasher>::Hasher::new();
-        x.hash(&mut hasher);
-        hasher.finish()
-    }
+    let mut hasher = <RandomState as BuildHasher>::Hasher::new();
+    x.hash(&mut hasher);
+    hasher.finish()
+}
 
+#[test]
+#[cfg(feature = "std")]
+fn test_hash() {
     assert!(hash_one(&_0) != hash_one(&_1));
     assert!(hash_one(&_0) != hash_one(&_3_2));
 
@@ -678,6 +726,16 @@ fn test_hash() {
     assert_eq!(hash_one(&a), hash_one(&b));
 }
 
+#[test]
+#[cfg(all(feature = "std", feature = "num-bigint"))]
+fn test_hash_stack_overflow() {
+    // When `hash` was recursive, this test caused a stack overflow.
+    // (related to num-rational#140)
+    let (a, b) = big_ratios();
+    let _ = hash_one(&a);
+    let _ = hash_one(&b);
+}
+
 #[test]
 fn test_into_pair() {
     assert_eq!((0, 1), _0.into());