Signed type impls of Into<UInt> are now fallible (#570)

Fixes #569.

* Signed integer primitives now implement `TryInto<UInt>` and fail if they are negative.
* `Coefficient::new` now takes `I: Into<Magnitude>` where `Magnitude` is a wrapper that signed integers infallibly convert to via absolute value.

src/binary/int.rs

@@ -197,7 +197,7 @@ impl From<DecodedInt> for Coefficient {
         } = int;
         use types::Sign::{Negative, Positive};
         let sign = if is_negative { Negative } else { Positive };
-        Coefficient::new(sign, value)
+        Coefficient::new(sign, value.unsigned_abs())
     }
 }
 

src/binary/non_blocking/binary_buffer.rs

@@ -690,7 +690,7 @@ mod tests {
         let mut buffer = BinaryBuffer::new(&[0b1000_0000]);
         let var_int = buffer.read_uint(buffer.remaining())?;
         assert_eq!(var_int.size_in_bytes(), 1);
-        assert_eq!(var_int.value(), &UInt::from(128));
+        assert_eq!(var_int.value(), &UInt::from(128u64));
         Ok(())
     }
 
@@ -699,7 +699,7 @@ mod tests {
         let mut buffer = BinaryBuffer::new(&[0b0111_1111, 0b1111_1111]);
         let var_int = buffer.read_uint(buffer.remaining())?;
         assert_eq!(var_int.size_in_bytes(), 2);
-        assert_eq!(var_int.value(), &UInt::from(32_767));
+        assert_eq!(var_int.value(), &UInt::from(32_767u64));
         Ok(())
     }
 
@@ -708,7 +708,7 @@ mod tests {
         let mut buffer = BinaryBuffer::new(&[0b0011_1100, 0b1000_0111, 0b1000_0001]);
         let var_int = buffer.read_uint(buffer.remaining())?;
         assert_eq!(var_int.size_in_bytes(), 3);
-        assert_eq!(var_int.value(), &UInt::from(3_966_849));
+        assert_eq!(var_int.value(), &UInt::from(3_966_849u64));
         Ok(())
     }
 

src/binary/uint.rs

@@ -221,23 +221,23 @@ mod tests {
         let data = &[0b1000_0000];
         let uint = DecodedUInt::read(&mut Cursor::new(data), data.len()).expect(READ_ERROR_MESSAGE);
         assert_eq!(uint.size_in_bytes(), 1);
-        assert_eq!(uint.value(), &UInt::from(128));
+        assert_eq!(uint.value(), &UInt::from(128u64));
     }
 
     #[test]
     fn test_read_two_byte_uint() {
         let data = &[0b0111_1111, 0b1111_1111];
         let uint = DecodedUInt::read(&mut Cursor::new(data), data.len()).expect(READ_ERROR_MESSAGE);
         assert_eq!(uint.size_in_bytes(), 2);
-        assert_eq!(uint.value(), &UInt::from(32_767));
+        assert_eq!(uint.value(), &UInt::from(32_767u64));
     }
 
     #[test]
     fn test_read_three_byte_uint() {
         let data = &[0b0011_1100, 0b1000_0111, 0b1000_0001];
         let uint = DecodedUInt::read(&mut Cursor::new(data), data.len()).expect(READ_ERROR_MESSAGE);
         assert_eq!(uint.size_in_bytes(), 3);
-        assert_eq!(uint.value(), &UInt::from(3_966_849));
+        assert_eq!(uint.value(), &UInt::from(3_966_849u64));
     }
 
     #[test]

src/lazy/binary/immutable_buffer.rs

@@ -823,7 +823,7 @@ mod tests {
         let buffer = ImmutableBuffer::new(&[0b1000_0000]);
         let var_int = buffer.read_uint(buffer.len())?.0;
         assert_eq!(var_int.size_in_bytes(), 1);
-        assert_eq!(var_int.value(), &UInt::from(128));
+        assert_eq!(var_int.value(), &UInt::from(128u64));
         Ok(())
     }
 
@@ -832,7 +832,7 @@ mod tests {
         let buffer = ImmutableBuffer::new(&[0b0111_1111, 0b1111_1111]);
         let var_int = buffer.read_uint(buffer.len())?.0;
         assert_eq!(var_int.size_in_bytes(), 2);
-        assert_eq!(var_int.value(), &UInt::from(32_767));
+        assert_eq!(var_int.value(), &UInt::from(32_767u64));
         Ok(())
     }
 
@@ -841,7 +841,7 @@ mod tests {
         let buffer = ImmutableBuffer::new(&[0b0011_1100, 0b1000_0111, 0b1000_0001]);
         let var_int = buffer.read_uint(buffer.len())?.0;
         assert_eq!(var_int.size_in_bytes(), 3);
-        assert_eq!(var_int.value(), &UInt::from(3_966_849));
+        assert_eq!(var_int.value(), &UInt::from(3_966_849u64));
         Ok(())
     }
 

src/types/coefficient.rs

@@ -16,6 +16,67 @@ pub enum Sign {
     Positive,
 }
 
+/// A simple wrapper type around a [`UInt`]. Users are not expected to construct
+/// instances of `Magnitude` directly; it is provided for conversion ergonomics.
+///
+/// Signed integer types cannot implement `Into<UInt>`. Instead, they implement
+/// `TryInto<UInt>` and report an error if the input integer is negative.
+///
+/// Signed integers can infallibly implement `Into<Magnitude>` by using their
+/// absolute value.
+pub struct Magnitude(UInt);
+
+impl Magnitude {
+    fn new<I: Into<UInt>>(value: I) -> Self {
+        Magnitude(value.into())
+    }
+}
+
+impl From<UInt> for Magnitude {
+    fn from(value: UInt) -> Self {
+        Magnitude(value)
+    }
+}
+
+impl From<Magnitude> for UInt {
+    fn from(value: Magnitude) -> Self {
+        value.0
+    }
+}
+
+impl From<BigUint> for Magnitude {
+    fn from(value: BigUint) -> Self {
+        let uint: UInt = value.into();
+        Magnitude(uint)
+    }
+}
+
+macro_rules! impl_magnitude_from_small_unsigned_int_types {
+    ($($t:ty),*) => ($(
+        impl From<$t> for Magnitude {
+            fn from(value: $t) -> Magnitude {
+                let uint: UInt = value.into();
+                Magnitude(uint)
+            }
+        }
+    )*)
+}
+
+impl_magnitude_from_small_unsigned_int_types!(u8, u16, u32, u64, u128, usize);
+
+macro_rules! impl_magnitude_from_small_signed_int_types {
+    ($($t:ty),*) => ($(
+        impl From<$t> for Magnitude {
+            fn from(value: $t) -> Magnitude {
+                let uint: UInt = (value.unsigned_abs() as u64).into();
+                Magnitude(uint)
+            }
+        }
+    )*)
+}
+
+impl_magnitude_from_small_signed_int_types!(i8, i16, i32, i64, isize);
+
 /// A signed integer that can be used as the coefficient of a Decimal value. This type does not
 /// consider `0` and `-0` to be equal and supports magnitudes of arbitrary size.
 // These trait derivations rely closely on the manual implementations of PartialEq and Ord on
@@ -27,8 +88,9 @@ pub struct Coefficient {
 }
 
 impl Coefficient {
-    pub(crate) fn new<I: Into<UInt>>(sign: Sign, magnitude: I) -> Self {
-        let magnitude = magnitude.into();
+    pub(crate) fn new<I: Into<Magnitude>>(sign: Sign, magnitude: I) -> Self {
+        let magnitude: Magnitude = magnitude.into();
+        let magnitude: UInt = magnitude.into();
         Coefficient { sign, magnitude }
     }
 
@@ -114,7 +176,7 @@ macro_rules! impl_coefficient_from_signed_int_types {
         impl From<$t> for Coefficient {
             fn from(value: $t) -> Coefficient {
                 let sign = if value < <$t>::zero() { Sign::Negative } else { Sign::Positive };
-                Coefficient::new(sign, value)
+                Coefficient::new(sign, value.unsigned_abs())
             }
         }
     )*)

src/types/integer.rs

@@ -1,7 +1,7 @@
 use crate::element::Element;
 use crate::ion_data::{IonEq, IonOrd};
 use num_bigint::{BigInt, BigUint, ToBigUint};
-use num_traits::{ToPrimitive, Zero};
+use num_traits::{Signed, ToPrimitive, Zero};
 use std::cmp::Ordering;
 use std::fmt::{Display, Formatter};
 use std::ops::{Add, Neg};
@@ -215,34 +215,39 @@ macro_rules! impl_uint_from_small_unsigned_int_types {
 
 impl_uint_from_small_unsigned_int_types!(u8, u16, u32, u64, usize);
 
-macro_rules! impl_uint_from_small_signed_int_types {
+macro_rules! impl_uint_try_from_small_signed_int_types {
     ($($t:ty),*) => ($(
-        impl From<$t> for UInt {
-            fn from(value: $t) -> UInt {
-                let abs_value = value.unsigned_abs();
-                UIntData::U64(abs_value.try_into().unwrap()).into()
+        impl TryFrom<$t> for UInt {
+            // If conversion fails, we discard the error. `TryFromIntError` provides no
+            // additional context and cannot be constructed outside of `std`. We'll return
+            // the unit type, `()`.
+            type Error = ();
+            fn try_from(value: $t) -> Result<Self, Self::Error> {
+                if value < 0 {
+                    return Err(());
+                }
+                Ok((value.unsigned_abs() as u64).into())
             }
         }
     )*)
 }
 
-impl_uint_from_small_signed_int_types!(i8, i16, i32, i64, isize);
+impl_uint_try_from_small_signed_int_types!(i8, i16, i32, i64, isize);
 
 impl From<u128> for UInt {
     fn from(value: u128) -> UInt {
         UIntData::BigUInt(BigUint::from(value)).into()
     }
 }
 
-macro_rules! impl_int_types_from_uint {
+macro_rules! impl_int_types_try_from_uint {
     ($($t:ty),*) => ($(
         impl TryFrom<&UInt> for $t {
-            // This allows the caller to reclaim the UInt in the event that conversion fails.
-            type Error = ();
-
             // If conversion fails, we discard the error. `TryFromIntError` provides no
             // additional context and cannot be constructed outside of `std`. We'll return
             // the unit type, `()`.
+            type Error = ();
+
             fn try_from(value: &UInt) -> Result<Self, Self::Error> {
                 match &value.data {
                     UIntData::U64(value) => <$t>::try_from(*value).map_err(|_|()),
@@ -253,22 +258,39 @@ macro_rules! impl_int_types_from_uint {
     )*)
 }
 
-impl_int_types_from_uint!(i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize);
+impl_int_types_try_from_uint!(i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize);
 
-impl From<i128> for UInt {
-    fn from(value: i128) -> UInt {
-        UIntData::BigUInt(value.abs().to_biguint().unwrap()).into()
+impl TryFrom<i128> for UInt {
+    // If the i128 is negative, this will return the unit type.
+    type Error = ();
+
+    fn try_from(value: i128) -> Result<Self, Self::Error> {
+        if value < 0 {
+            Err(())
+        } else {
+            Ok(value.unsigned_abs().to_biguint().unwrap().into())
+        }
     }
 }
 
-impl From<Int> for UInt {
-    fn from(value: Int) -> Self {
+impl TryFrom<Int> for UInt {
+    // Returns the unit type if the input `Int` is negative.
+    type Error = ();
+
+    fn try_from(value: Int) -> Result<Self, Self::Error> {
         match value {
-            Int::I64(i) => i.into(),
+            Int::I64(i) if i >= 0 => Ok(i.unsigned_abs().into()),
+            Int::I64(_i) => Err(()),
             // num_bigint::BigInt's `into_parts` consumes the BigInt and returns a
-            // (sign: Sign, magnitude: BigUint) tuple. We only care about the magnitude, so we
-            // extract it here with `.1` -----------v and then convert the BigUint to a UInteger
-            Int::BigInt(i) => i.into_parts().1.into(), // <-- using `.into()`
+            // (sign: Sign, magnitude: BigUint) tuple.
+            Int::BigInt(i) => {
+                let (sign, magnitude) = i.into_parts();
+                if sign == num_bigint::Sign::Minus {
+                    Err(())
+                } else {
+                    Ok(magnitude.into())
+                }
+            }
         }
     }
 }
@@ -294,6 +316,14 @@ pub enum Int {
 }
 
 impl Int {
+    /// Returns a [`UInt`] representing the unsigned magnitude of this `Int`.
+    pub(crate) fn unsigned_abs(&self) -> UInt {
+        match self {
+            Int::I64(i) => i.unsigned_abs().into(),
+            Int::BigInt(i) => i.abs().into_parts().1.into(),
+        }
+    }
+
     /// Compares a [i64] integer with a [BigInt] to see if they are equal. This method never
     /// allocates. It will always prefer to downgrade a BigUint and compare the two integers as
     /// u64 values. If this is not possible, then the two numbers cannot be equal anyway.
@@ -561,11 +591,11 @@ mod integer_tests {
     }
 
     #[rstest]
-    #[case::u64(UInt::from(5), UInt::from(4), Ordering::Greater)]
-    #[case::u64_equal(UInt::from(5), UInt::from(5), Ordering::Equal)]
-    #[case::u64_gt_big_uint(UInt::from(4), UInt::from(BigUint::from(3u64)), Ordering::Greater)]
-    #[case::u64_lt_big_uint(UInt::from(3), UInt::from(BigUint::from(5u64)), Ordering::Less)]
-    #[case::u64_eq_big_uint(UInt::from(3), UInt::from(BigUint::from(3u64)), Ordering::Equal)]
+    #[case::u64(UInt::from(5u64), UInt::from(4u64), Ordering::Greater)]
+    #[case::u64_equal(UInt::from(5u64), UInt::from(5u64), Ordering::Equal)]
+    #[case::u64_gt_big_uint(UInt::from(4u64), UInt::from(BigUint::from(3u64)), Ordering::Greater)]
+    #[case::u64_lt_big_uint(UInt::from(3u64), UInt::from(BigUint::from(5u64)), Ordering::Less)]
+    #[case::u64_eq_big_uint(UInt::from(3u64), UInt::from(BigUint::from(3u64)), Ordering::Equal)]
     #[case::big_uint(
         UInt::from(BigUint::from(1100u64)),
         UInt::from(BigUint::from(1005u64)),
@@ -585,7 +615,7 @@ mod integer_tests {
     }
 
     #[rstest]
-    #[case(UInt::from(1), 1)] // only one test case for U64 as that's delegated to another impl
+    #[case(UInt::from(1u64), 1)] // only one test case for U64 as that's delegated to another impl
     #[case(UInt::from(BigUint::from(0u64)), 1)]
     #[case(UInt::from(BigUint::from(1u64)), 1)]
     #[case(UInt::from(BigUint::from(10u64)), 2)]
@@ -607,8 +637,8 @@ mod integer_tests {
     }
 
     #[rstest]
-    #[case(UInt::from(5), "5")]
-    #[case(UInt::from(0), "0")]
+    #[case(UInt::from(5u64), "5")]
+    #[case(UInt::from(0u64), "0")]
     #[case(UInt::from(BigUint::from(0u64)), "0")]
     #[case(UInt::from(BigUint::from(1100u64)), "1100")]
     fn uint_display_test(#[case] value: UInt, #[case] expect: String) {