audit regressions, tests, fixes

timschmidt · timschmidt · commit f181675f0226 · 2026-05-26T00:32:40.000-04:00
diff --git a/README.md b/README.md
@@ -167,7 +167,7 @@ control. `hyperreal` uses several small performance strategies together:
 
 ## Current Status
 
-Version `0.12.0` is active and benchmark-driven. Current implementation work includes:
+Version `0.13.0` is active and benchmark-driven. Current implementation work includes:
 
 - exact rational and dyadic fast paths;
 - dedicated constructors and cached constants for common zeros, ones, `pi`, `tau`, `e`,
diff --git a/benches/dispatch_trace.rs b/benches/dispatch_trace.rs
@@ -542,8 +542,8 @@ fn collect_rows(filters: &[String]) -> BTreeMap<String, hyperreal::dispatch_trac
         || {
             let minus_pi = Computable::pi().negate();
             let pi = Computable::pi();
-            black_box(minus_pi.compare_to(&pi));
-            black_box(pi.compare_to(&minus_pi));
+            black_box(minus_pi.try_compare_to(&pi));
+            black_box(pi.try_compare_to(&minus_pi));
         },
     );
     trace_row(
@@ -557,9 +557,9 @@ fn collect_rows(filters: &[String]) -> BTreeMap<String, hyperreal::dispatch_trac
                 .multiply(Computable::rational(Rational::from_bigint(
                     BigInt::from(1_u8) << 200,
                 )));
-            black_box(huge.compare_to(&base));
-            black_box(base.compare_to(&huge));
-            black_box(huge.negate().compare_to(&base.negate()));
+            black_box(huge.try_compare_to(&base));
+            black_box(base.try_compare_to(&huge));
+            black_box(huge.negate().try_compare_to(&base.negate()));
         },
     );
     trace_row(
@@ -569,9 +569,9 @@ fn collect_rows(filters: &[String]) -> BTreeMap<String, hyperreal::dispatch_trac
         || {
             let lhs = Computable::rational(rational(3, 7));
             let rhs = Computable::rational(rational(2, 7));
-            black_box(rhs.compare_to(&lhs));
-            black_box(lhs.compare_to(&rhs));
-            black_box(lhs.compare_to(&lhs));
+            black_box(rhs.try_compare_to(&lhs));
+            black_box(lhs.try_compare_to(&rhs));
+            black_box(lhs.try_compare_to(&lhs));
         },
     );
     trace_row(
diff --git a/benches/numerical_micro.rs b/benches/numerical_micro.rs
@@ -828,7 +828,7 @@ fn bench_computable_compare(c: &mut Criterion) {
     let minus_pi = Computable::pi().negate();
     let pi = Computable::pi();
     group.bench_function("compare_to_opposite_sign", |b| {
-        b.iter(|| black_box(minus_pi.compare_to(&pi)))
+        b.iter(|| black_box(minus_pi.try_compare_to(&pi)))
     });
 
     let base = Computable::pi();
@@ -839,7 +839,7 @@ fn bench_computable_compare(c: &mut Criterion) {
             BigInt::from(1_u8) << 200,
         )));
     group.bench_function("compare_to_exact_msd_gap", |b| {
-        b.iter(|| black_box(huge.compare_to(&base)))
+        b.iter(|| black_box(huge.try_compare_to(&base)))
     });
 
     let left = Computable::rational(Rational::fraction(-7, 8).unwrap());
diff --git a/src/computable/node.rs b/src/computable/node.rs
@@ -4437,34 +4437,43 @@ impl Computable {
         }
     }
 
-    /// Do not call this function if `self` and `other` may be the same.
-    pub fn compare_to(&self, other: &Self) -> Ordering {
+    /// Try to compare two computable values exactly.
+    ///
+    /// Returns `None` when bounded refinement cannot prove an ordering. This is
+    /// the public comparison API for callers that may be comparing equal or
+    /// semantically equivalent values.
+    pub fn try_compare_to(&self, other: &Self) -> Option<Ordering> {
+        if Self::internal_structural_eq(self, other) {
+            return Some(Ordering::Equal);
+        }
+
         // Keep exact leaf comparisons allocation-free for the hot path where both
         // operands are already exact. This avoids creating temporary rationals
         // on every comparator call.
         if let Some(order) = self.exact_rational_leaf_cmp(other) {
             crate::trace_dispatch!("computable", "compare_to", "exact-rational");
-            return order;
+            return Some(order);
         }
 
         if let (Some(left), Some(right)) = (self.exact_rational(), other.exact_rational()) {
             // Exact rationals compare directly; escalating to approximate comparison here is
             // both slower and can burn cache precision unnecessarily.
             crate::trace_dispatch!("computable", "compare_to", "exact-rational");
-            return left
-                .partial_cmp(&right)
-                .expect("exact rationals should be comparable");
+            return Some(
+                left.partial_cmp(&right)
+                    .expect("exact rationals should be comparable"),
+            );
         }
 
         if let (Some(left), Some(right)) = (self.exact_sign(), other.exact_sign()) {
             match (left, right) {
                 (Sign::Minus, Sign::Plus | Sign::NoSign) | (Sign::NoSign, Sign::Plus) => {
                     crate::trace_dispatch!("computable", "compare_to", "exact-sign-opposite");
-                    return Ordering::Less;
+                    return Some(Ordering::Less);
                 }
                 (Sign::Plus, Sign::Minus | Sign::NoSign) | (Sign::NoSign, Sign::Minus) => {
                     crate::trace_dispatch!("computable", "compare_to", "exact-sign-opposite");
-                    return Ordering::Greater;
+                    return Some(Ordering::Greater);
                 }
                 _ => {}
             }
@@ -4480,11 +4489,11 @@ impl Computable {
                 // Same-sign values with different most-significant digits have a known
                 // order without evaluating either value to a requested precision.
                 crate::trace_dispatch!("computable", "compare_to", "exact-sign-msd-gap");
-                return match left {
+                return Some(match left {
                     Sign::Plus => left_msd.cmp(&right_msd),
                     Sign::Minus => right_msd.cmp(&left_msd),
                     Sign::NoSign => unreachable!(),
-                };
+                });
             }
         }
 
@@ -4496,13 +4505,13 @@ impl Computable {
             match (left, right) {
                 (Sign::Minus, Sign::Plus) | (Sign::NoSign, Sign::Plus) => {
                     crate::trace_dispatch!("computable", "compare_to", "cheap-bound-opposite-sign");
-                    return Ordering::Less;
+                    return Some(Ordering::Less);
                 }
                 (Sign::Plus, Sign::Minus) | (Sign::Plus, Sign::NoSign) => {
                     crate::trace_dispatch!("computable", "compare_to", "cheap-bound-opposite-sign");
-                    return Ordering::Greater;
+                    return Some(Ordering::Greater);
                 }
-                (Sign::NoSign, Sign::NoSign) => return Ordering::Equal,
+                (Sign::NoSign, Sign::NoSign) => return Some(Ordering::Equal),
                 _ => {}
             }
             if left == right
@@ -4513,23 +4522,23 @@ impl Computable {
                 // Same-sign structural bounds can decide exact ordering
                 // before entering tolerance refinement.
                 crate::trace_dispatch!("computable", "compare_to", "cheap-bound-msd-gap");
-                return match left {
+                return Some(match left {
                     Sign::Plus => left_msd.cmp(&right_msd),
                     Sign::Minus => right_msd.cmp(&left_msd),
                     Sign::NoSign => Ordering::Equal,
-                };
+                });
             }
         }
         crate::trace_dispatch!("computable", "compare_to", "approx-refinement");
         let mut tolerance = -20;
         while tolerance > Precision::MIN {
             let order = self.compare_absolute(other, tolerance);
             if order != Ordering::Equal {
-                return order;
+                return Some(order);
             }
             tolerance *= 2;
         }
-        panic!("Apparently called Computable::compare_to on equal values");
+        None
     }
 
     /// Compare two values to a specified tolerance (more negative numbers are more precise).
@@ -4813,9 +4822,9 @@ mod tests {
         let five = Computable::integer(five.clone());
         let four = Computable::integer(four.clone());
 
-        assert_eq!(six.compare_to(&five), Ordering::Greater);
-        assert_eq!(five.compare_to(&six), Ordering::Less);
-        assert_eq!(four.compare_to(&six), Ordering::Less);
+        assert_eq!(six.try_compare_to(&five), Some(Ordering::Greater));
+        assert_eq!(five.try_compare_to(&six), Some(Ordering::Less));
+        assert_eq!(four.try_compare_to(&six), Some(Ordering::Less));
     }
 
     #[test]
@@ -5138,11 +5147,21 @@ mod tests {
     fn compare_to_uses_exact_sign_and_rational_shortcuts() {
         let minus_pi = Computable::pi().negate();
         let pi = Computable::pi();
-        assert_eq!(minus_pi.compare_to(&pi), Ordering::Less);
+        assert_eq!(minus_pi.try_compare_to(&pi), Some(Ordering::Less));
 
         let left = Computable::rational(Rational::fraction(7, 8).unwrap());
         let right = Computable::rational(Rational::fraction(9, 10).unwrap());
-        assert_eq!(left.compare_to(&right), Ordering::Less);
+        assert_eq!(left.try_compare_to(&right), Some(Ordering::Less));
+    }
+
+    #[test]
+    fn try_compare_to_handles_identical_symbolic_values() {
+        let pi = Computable::pi();
+        assert_eq!(pi.try_compare_to(&pi), Some(Ordering::Equal));
+
+        let left = Computable::rational(Rational::fraction(3, 7).unwrap());
+        let right = Computable::rational(Rational::fraction(3, 7).unwrap());
+        assert_eq!(left.try_compare_to(&right), Some(Ordering::Equal));
     }
 
     #[test]
@@ -5154,13 +5173,16 @@ mod tests {
             .multiply(Computable::rational(Rational::from_bigint(
                 BigInt::from(1_u8) << 200,
             )));
-        assert_eq!(huge.compare_to(&base), Ordering::Greater);
-        assert_eq!(base.compare_to(&huge), Ordering::Less);
+        assert_eq!(huge.try_compare_to(&base), Some(Ordering::Greater));
+        assert_eq!(base.try_compare_to(&huge), Some(Ordering::Less));
 
         let minus_base = base.negate();
         let minus_huge = huge.negate();
-        assert_eq!(minus_huge.compare_to(&minus_base), Ordering::Less);
-        assert_eq!(minus_base.compare_to(&minus_huge), Ordering::Greater);
+        assert_eq!(minus_huge.try_compare_to(&minus_base), Some(Ordering::Less));
+        assert_eq!(
+            minus_base.try_compare_to(&minus_huge),
+            Some(Ordering::Greater)
+        );
     }
 
     #[test]
diff --git a/src/rational/arithmetic.rs b/src/rational/arithmetic.rs
@@ -3,7 +3,7 @@ use crate::structural::{RationalFacts, RationalStorageClass};
 use num::bigint::Sign::{self, *};
 use num::{BigInt, BigUint, ToPrimitive};
 use num::{One, Zero};
-use serde::{Deserialize, Serialize};
+use serde::{Deserialize, Deserializer, Serialize};
 use std::cmp::Ordering;
 use std::sync::LazyLock;
 
@@ -49,13 +49,35 @@ use std::sync::LazyLock;
 /// assert_eq!(four, Rational::new(4));
 /// ```
 
-#[derive(Clone, Debug, Serialize, Deserialize)]
+#[derive(Clone, Debug, Serialize)]
 pub struct Rational {
     sign: Sign,
     numerator: BigUint,
     denominator: BigUint,
 }
 
+impl<'de> Deserialize<'de> for Rational {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        struct RationalWire {
+            sign: Sign,
+            numerator: BigUint,
+            denominator: BigUint,
+        }
+
+        let wire = RationalWire::deserialize(deserializer)?;
+        if wire.denominator.is_zero() {
+            return Err(serde::de::Error::custom(
+                "Rational denominator must be nonzero",
+            ));
+        }
+        Ok(Self::from_fraction_parts(wire.sign, wire.numerator, wire.denominator).reduce())
+    }
+}
+
 static ONE: LazyLock<BigUint> = LazyLock::new(BigUint::one);
 // Small positive constants use their narrow primitive source type; this keeps
 // construction direct and avoids the older `ToBigUint` conversion shim.
@@ -1613,11 +1635,11 @@ impl std::str::FromStr for Rational {
             if numerator.is_zero() {
                 sign = NoSign;
             }
-            Ok(Self {
-                sign,
-                numerator,
-                denominator: BigUint::parse_bytes(d.as_bytes(), 10).ok_or(Problem::BadFraction)?,
-            })
+            let denominator = BigUint::parse_bytes(d.as_bytes(), 10).ok_or(Problem::BadFraction)?;
+            if denominator.is_zero() {
+                return Err(Problem::DivideByZero);
+            }
+            Ok(Self::from_fraction_parts(sign, numerator, denominator).reduce())
         } else if let Some((i, d)) = s.split_once('.') {
             let numerator = BigUint::parse_bytes(i.as_bytes(), 10).ok_or(Problem::BadDecimal)?;
             let whole = if numerator.is_zero() {
@@ -1963,6 +1985,27 @@ mod tests {
         assert_eq!(answer, expected);
     }
 
+    #[test]
+    fn parse_fraction_rejects_zero_denominator_and_reduces() {
+        assert_eq!("1/0".parse::<Rational>(), Err(Problem::DivideByZero));
+        assert_eq!("0/0".parse::<Rational>(), Err(Problem::DivideByZero));
+
+        let reduced: Rational = "9/18".parse().unwrap();
+        assert_eq!(reduced, Rational::fraction(1, 2).unwrap());
+        assert_eq!(format!("{reduced}"), "1/2");
+    }
+
+    #[test]
+    fn serde_rejects_invalid_or_uncanonical_rational_state() {
+        let bad = r#"{"sign":1,"numerator":[1],"denominator":[]}"#;
+        assert!(serde_json::from_str::<Rational>(bad).is_err());
+
+        let unreduced = r#"{"sign":1,"numerator":[9],"denominator":[18]}"#;
+        let decoded: Rational = serde_json::from_str(unreduced).unwrap();
+        assert_eq!(decoded, Rational::fraction(1, 2).unwrap());
+        assert_eq!(format!("{decoded}"), "1/2");
+    }
+
     #[test]
     fn square_reduced() {
         let thirty_two = Rational::new(32);
diff --git a/src/real/tests.rs b/src/real/tests.rs
@@ -1978,8 +1978,8 @@ mod tests {
     fn computable_atan2_axes() {
         use crate::Computable;
         use num::Zero;
-        // compare_to(&equal) on Computable can loop forever (kernel docs warn
-        // about this), so axis cases are validated through approx values.
+        // Axis cases are validated through approximations because these values
+        // exercise the symbolic zero branches in the computable kernel.
         let zero_plus = Computable::zero().atan2(Computable::one());
         assert!(zero_plus.approx(-30).is_zero());
         let zero_minus = Computable::zero().atan2(Computable::one().negate());
diff --git a/src/serde.rs b/src/serde.rs
@@ -3,6 +3,7 @@ use std::str::FromStr;
 use crate::{Problem, Rational, real::Real};
 use ciborium::{Value, de, ser};
 use num::BigInt;
+use num::bigint::Sign as BigIntSign;
 
 impl Real {
     /// Serializes the Real to a JSON string.
@@ -68,11 +69,19 @@ impl TryFrom<&Value> for Real {
                     }
                 }
                 // If we receive an array of length 2, we attempt to interpret it as the numerator and denominator of a Rational.
-                let numerator = as_big_int(&x[0])?;
+                let mut numerator = as_big_int(&x[0])?;
                 let denominator = as_big_int(&x[1])?;
-                Ok(Real::new(
-                    Rational::from_bigint(numerator) / Rational::from_bigint(denominator),
-                ))
+                let (denominator_sign, denominator_magnitude) = denominator.into_parts();
+                if denominator_sign == BigIntSign::NoSign {
+                    return Err(Problem::DivideByZero);
+                }
+                if denominator_sign == BigIntSign::Minus {
+                    numerator = -numerator;
+                }
+                Ok(Real::new(Rational::from_bigint_fraction(
+                    numerator,
+                    denominator_magnitude,
+                )?))
             }
             _ => {
                 // In this branch, we should be whatever type ciborium decided to use for `Real`.
@@ -262,4 +271,19 @@ mod tests {
         let y: Real = (&value).try_into().unwrap();
         assert_eq!(x, y);
     }
+
+    #[test]
+    fn cbor_rational_pair_rejects_zero_denominator() {
+        let value = Value::Array(vec![Value::Integer(1.into()), Value::Integer(0.into())]);
+        assert_eq!(Real::try_from(&value), Err(Problem::DivideByZero));
+    }
+
+    #[test]
+    fn cbor_rational_pair_normalizes_negative_denominator() {
+        let value = Value::Array(vec![Value::Integer(1.into()), Value::Integer((-2).into())]);
+        assert_eq!(
+            Real::try_from(&value).unwrap(),
+            Real::new(Rational::fraction(-1, 2).unwrap())
+        );
+    }
 }
diff --git a/src/simple.rs b/src/simple.rs
diff --git a/tests/readme_metadata.rs b/tests/readme_metadata.rs
