Restore rational boundary validation

Author: timschmidt

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);

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())
+        );
+    }
 }

src/simple.rs

@@ -681,7 +681,16 @@ impl std::str::FromStr for Simple {
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let mut chars = s.chars().peekable();
-        Simple::parse(&mut chars)
+        let parsed = Simple::parse(&mut chars)?;
+        while let Some(c) = chars.peek() {
+            match c {
+                ' ' | '\t' => {
+                    chars.next();
+                }
+                _ => return Err("Trailing input after expression"),
+            }
+        }
+        Ok(parsed)
     }
 }
 
@@ -696,6 +705,15 @@ mod tests {
         assert_eq!(xpr, Err("Incomplete expression"))
     }
 
+    #[test]
+    fn parse_rejects_trailing_input() {
+        let xpr: Result<Simple, &str> = "(+ 1 2) junk".parse();
+        assert_eq!(xpr, Err("Trailing input after expression"));
+
+        let xpr: Result<Simple, &str> = "(+ 1 2) \t".parse();
+        assert!(xpr.is_ok());
+    }
+
     #[test]
     fn parse_named_operators() {
         let cases = [