Repository: ron-rs/ron
Pull request #210 (2020-04-18 18:11:42)

Integer support
Roughly following the discussion from https://github.com/ron-rs/ron/issues/47 and related issues and PRs, this PR extends the Number type (and so indirectly the Value type) to support integers. I hope I didn't grossly misunderstand the discussion there, but I am keen to make adjustments here to make the project better.

~~I tried to limit the impact to the API to a single breaking change by extending the already opaque `Number` type rather than the `Value` enum which exposes its structure. The breaking change is that the `get` method in `Number` is replaced by `as_f64` and `as_i64` for getting the appropriate type.~~
Since `Integer` and `Float` types are distinct, they will not be ordered by value (as explained below). This is a breaking change in itself.

To maintain the traits on `Number`, `Integer`s are simply ordered before `Float`s. This makes float and integer not comparable in value (e.g. `Number::Integer(2)` < `Number::Float(Float(1.0))`) because I didn't see any necessity to do so, but I'm open to suggestions.

The tests are adjusted to verify that both integers and floats are working.

kvark: @torkleyy would you want to review? I figured you know the value semantics better :)
elrnv: The rebase is done. Is there anything else needed here from me?
kvark: @elrnv no, thank you!
Let's ping @torkleyy again and give them a few days, then proceed.
kvark: Sorry, this needs to be rebased again.
Otherwise, we are all good to go.
elrnv: @kvark No problemo. Should good now.
FYI I added a couple of `From` impls for the `Number` type that do proper (lossless) conversion from `u64` and `i32`. Previously I just had u64 truncating to i64.
kvark: @CAD97 looks like GHA isn't configured correctly. It doesn't even run on this PR.
kvark: @elrnv thank you! Unfortunately we can't proceed until we get CI looking at this.
kvark: Thank you! I think the CI fix need to go separately first.
elrnv: I thought there was an option to run GHA on forks, but I can't find it anymore. I was just trying to see if this would work... :/
CAD97: @kvark I set it up the same way that Travis was, to only run on pushes to master/staging/trying. We can set it up to run on PRs as well, that's just another entry to the `on` key.
kvark: @CAD97 travis was definitely checking other forks PRs
CAD97: #227 bumps up the CI to always run on any PR or any push. Bors would have (of course) still tested on CI.


kvark: Can we rebase and squash please?

Patch:
diff --git a/src/de/value.rs b/src/de/value.rs
index a9ba017e..92acca20 100644
--- a/src/de/value.rs
+++ b/src/de/value.rs
@@ -53,7 +53,7 @@ impl<'de> Visitor<'de> for ValueVisitor {
     where
         E: Error,
     {
-        self.visit_i128(v as i128)
+        Ok(Value::Number(Number::new(v)))
     }
 
     fn visit_i128<E>(self, v: i128) -> Result<Self::Value, E>
@@ -67,7 +67,7 @@ impl<'de> Visitor<'de> for ValueVisitor {
     where
         E: Error,
     {
-        self.visit_u128(v as u128)
+        Ok(Value::Number(Number::new(v)))
     }
 
     fn visit_u128<E>(self, v: u128) -> Result<Self::Value, E>
@@ -205,9 +205,9 @@ mod tests {
     #[test]
     fn test_tuples_basic() {
         assert_eq!(
-            eval("(3, 4, 5)"),
+            eval("(3, 4.0, 5.0)"),
             Value::Seq(vec![
-                Value::Number(Number::new(3.0)),
+                Value::Number(Number::new(3)),
                 Value::Number(Number::new(4.0)),
                 Value::Number(Number::new(5.0)),
             ],),
@@ -217,11 +217,11 @@ mod tests {
     #[test]
     fn test_tuples_ident() {
         assert_eq!(
-            eval("(true, 3, 4, 5)"),
+            eval("(true, 3, 4, 5.0)"),
             Value::Seq(vec![
                 Value::Bool(true),
-                Value::Number(Number::new(3.0)),
-                Value::Number(Number::new(4.0)),
+                Value::Number(Number::new(3)),
+                Value::Number(Number::new(4)),
                 Value::Number(Number::new(5.0)),
             ]),
         );
@@ -260,8 +260,8 @@ mod tests {
     Room ( width: 20, height: 5, name: \"The Room\" ),
 
     (
-        width: 10,
-        height: 10,
+        width: 10.0,
+        height: 10.0,
         name: \"Another room\",
         enemy_levels: {
             \"Enemy1\": 3,
@@ -276,11 +276,11 @@ mod tests {
                     vec![
                         (
                             Value::String("width".to_owned()),
-                            Value::Number(Number::new(20.0)),
+                            Value::Number(Number::new(20)),
                         ),
                         (
                             Value::String("height".to_owned()),
-                            Value::Number(Number::new(5.0)),
+                            Value::Number(Number::new(5)),
                         ),
                         (
                             Value::String("name".to_owned()),
@@ -310,15 +310,15 @@ mod tests {
                                 vec![
                                     (
                                         Value::String("Enemy1".to_owned()),
-                                        Value::Number(Number::new(3.0)),
+                                        Value::Number(Number::new(3)),
                                     ),
                                     (
                                         Value::String("Enemy2".to_owned()),
-                                        Value::Number(Number::new(5.0)),
+                                        Value::Number(Number::new(5)),
                                     ),
                                     (
                                         Value::String("Enemy3".to_owned()),
-                                        Value::Number(Number::new(7.0)),
+                                        Value::Number(Number::new(7)),
                                     ),
                                 ]
                                 .into_iter()
diff --git a/src/ser/value.rs b/src/ser/value.rs
index d3109ec3..1a477589 100644
--- a/src/ser/value.rs
+++ b/src/ser/value.rs
@@ -1,6 +1,6 @@
 use serde::ser::{Serialize, Serializer};
 
-use crate::value::Value;
+use crate::value::{Number, Value};
 
 impl Serialize for Value {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
@@ -11,7 +11,8 @@ impl Serialize for Value {
             Value::Bool(b) => serializer.serialize_bool(b),
             Value::Char(c) => serializer.serialize_char(c),
             Value::Map(ref m) => Serialize::serialize(m, serializer),
-            Value::Number(ref n) => serializer.serialize_f64(n.get()),
+            Value::Number(Number::Float(ref f)) => serializer.serialize_f64(f.get()),
+            Value::Number(Number::Integer(i)) => serializer.serialize_i64(i),
             Value::Option(Some(ref o)) => serializer.serialize_some(o.as_ref()),
             Value::Option(None) => serializer.serialize_none(),
             Value::String(ref s) => serializer.serialize_str(s),
diff --git a/src/value.rs b/src/value.rs
index 1998c39d..e5387df0 100644
--- a/src/value.rs
+++ b/src/value.rs
@@ -131,14 +131,22 @@ type MapInner = std::collections::BTreeMap<Value, Value>;
 #[cfg(feature = "indexmap")]
 type MapInner = indexmap::IndexMap<Value, Value>;
 
-/// A wrapper for `f64` which guarantees that the inner value
+/// A wrapper for a number, which can be either `f64` or `i64`.
+#[derive(Copy, Clone, Debug, PartialEq, PartialOrd, Eq, Hash, Ord)]
+pub enum Number {
+    Integer(i64),
+    Float(Float),
+}
+
+/// A wrapper for `f64`, which guarantees that the inner value
 /// is finite and thus implements `Eq`, `Hash` and `Ord`.
 #[derive(Copy, Clone, Debug)]
-pub struct Number(f64);
+pub struct Float(f64);
 
-impl Number {
+impl Float {
+    /// Construct a new `Float`.
     pub fn new(v: f64) -> Self {
-        Number(v)
+        Float(v)
     }
 
     /// Returns the wrapped float.
@@ -147,23 +155,129 @@ impl Number {
     }
 }
 
+impl Number {
+    /// Construct a new number.
+    pub fn new(v: impl Into<Number>) -> Self {
+        v.into()
+    }
+
+    /// Returns the `f64` representation of the number regardless of whether the number is stored
+    /// as a float or integer.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use ron::value::Number;
+    /// let i = Number::new(5);
+    /// let f = Number::new(2.0);
+    /// assert_eq!(i.into_f64(), 5.0);
+    /// assert_eq!(f.into_f64(), 2.0);
+    /// ```
+    pub fn into_f64(self) -> f64 {
+        self.map_to(|i| i as f64, |f| f)
+    }
+
+    /// If the `Number` is a float, return it. Otherwise return `None`.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use ron::value::Number;
+    /// let i = Number::new(5);
+    /// let f = Number::new(2.0);
+    /// assert_eq!(i.as_f64(), None);
+    /// assert_eq!(f.as_f64(), Some(2.0));
+    /// ```
+    pub fn as_f64(self) -> Option<f64> {
+        self.map_to(|_| None, Some)
+    }
+
+    /// If the `Number` is an integer, return it. Otherwise return `None`.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use ron::value::Number;
+    /// let i = Number::new(5);
+    /// let f = Number::new(2.0);
+    /// assert_eq!(i.as_i64(), Some(5));
+    /// assert_eq!(f.as_i64(), None);
+    /// ```
+    pub fn as_i64(self) -> Option<i64> {
+        self.map_to(Some, |_| None)
+    }
+
+    /// Map this number to a single type using the appropriate closure.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use ron::value::Number;
+    /// let i = Number::new(5);
+    /// let f = Number::new(2.0);
+    /// assert!(i.map_to(|i| i > 3, |f| f > 3.0));
+    /// assert!(!f.map_to(|i| i > 3, |f| f > 3.0));
+    /// ```
+    pub fn map_to<T>(
+        self,
+        integer_fn: impl FnOnce(i64) -> T,
+        float_fn: impl FnOnce(f64) -> T,
+    ) -> T {
+        match self {
+            Number::Integer(i) => integer_fn(i),
+            Number::Float(Float(f)) => float_fn(f),
+        }
+    }
+}
+
+impl From<f64> for Number {
+    fn from(f: f64) -> Number {
+        Number::Float(Float(f))
+    }
+}
+
+impl From<i64> for Number {
+    fn from(i: i64) -> Number {
+        Number::Integer(i)
+    }
+}
+
+impl From<i32> for Number {
+    fn from(i: i32) -> Number {
+        Number::Integer(i as i64)
+    }
+}
+
+// The following number conversion checks if the integer fits losslessly into an i64, before
+// constructing a Number::Integer variant. If not, the conversion defaults to float.
+
+impl From<u64> for Number {
+    fn from(i: u64) -> Number {
+        if i as i64 as u64 == i {
+            Number::Integer(i as i64)
+        } else {
+            Number::new(i as f64)
+        }
+    }
+}
+
 /// Partial equality comparison
 /// In order to be able to use `Number` as a mapping key, NaN floating values
-/// wrapped in `Number` are equals to each other. It is not the case for
+/// wrapped in `Float` are equals to each other. It is not the case for
 /// underlying `f64` values itself.
-impl PartialEq for Number {
+impl PartialEq for Float {
     fn eq(&self, other: &Self) -> bool {
         self.0.is_nan() && other.0.is_nan() || self.0 == other.0
     }
 }
 
 /// Equality comparison
-/// In order to be able to use `Number` as a mapping key, NaN floating values
-/// wrapped in `Number` are equals to each other. It is not the case for
+/// In order to be able to use `Float` as a mapping key, NaN floating values
+/// wrapped in `Float` are equals to each other. It is not the case for
 /// underlying `f64` values itself.
-impl Eq for Number {}
+impl Eq for Float {}
 
-impl Hash for Number {
+impl Hash for Float {
     fn hash<H: Hasher>(&self, state: &mut H) {
         state.write_u64(self.0 as u64);
     }
@@ -172,13 +286,13 @@ impl Hash for Number {
 /// Partial ordering comparison
 /// In order to be able to use `Number` as a mapping key, NaN floating values
 /// wrapped in `Number` are equals to each other and are less then any other
-/// floating value. It is not the case for underlying `f64` values itself.
+/// floating value. It is not the case for the underlying `f64` values themselves.
 /// ```
 /// use ron::value::Number;
 /// assert!(Number::new(std::f64::NAN) < Number::new(std::f64::NEG_INFINITY));
 /// assert_eq!(Number::new(std::f64::NAN), Number::new(std::f64::NAN));
 /// ```
-impl PartialOrd for Number {
+impl PartialOrd for Float {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         match (self.0.is_nan(), other.0.is_nan()) {
             (true, true) => Some(Ordering::Equal),
@@ -190,11 +304,11 @@ impl PartialOrd for Number {
 }
 
 /// Ordering comparison
-/// In order to be able to use `Number` as a mapping key, NaN floating values
-/// wrapped in `Number` are equals to each other and are less then any other
+/// In order to be able to use `Float` as a mapping key, NaN floating values
+/// wrapped in `Float` are equals to each other and are less then any other
 /// floating value. It is not the case for underlying `f64` values itself. See
 /// the `PartialEq` implementation.
-impl Ord for Number {
+impl Ord for Float {
     fn cmp(&self, other: &Self) -> Ordering {
         self.partial_cmp(other).expect("Bug: Contract violation")
     }
@@ -244,7 +358,8 @@ impl<'de> Deserializer<'de> for Value {
                 keys: m.keys().cloned().rev().collect(),
                 values: m.values().cloned().rev().collect(),
             }),
-            Value::Number(n) => visitor.visit_f64(n.get()),
+            Value::Number(Number::Float(ref f)) => visitor.visit_f64(f.get()),
+            Value::Number(Number::Integer(i)) => visitor.visit_i64(i),
             Value::Option(Some(o)) => visitor.visit_some(*o),
             Value::Option(None) => visitor.visit_none(),
             Value::String(s) => visitor.visit_string(s),
@@ -282,7 +397,7 @@ impl<'de> Deserializer<'de> for Value {
         V: Visitor<'de>,
     {
         match self {
-            Value::Number(n) => visitor.visit_i64(n.get() as i64),
+            Value::Number(Number::Integer(i)) => visitor.visit_i64(i),
             v => Err(RonError::custom(format!("Expected a number, got {:?}", v))),
         }
     }
@@ -313,7 +428,7 @@ impl<'de> Deserializer<'de> for Value {
         V: Visitor<'de>,
     {
         match self {
-            Value::Number(n) => visitor.visit_u64(n.get() as u64),
+            Value::Number(Number::Integer(i)) => visitor.visit_u64(i as u64),
             v => Err(RonError::custom(format!("Expected a number, got {:?}", v))),
         }
     }


Test patch:
diff --git a/tests/value.rs b/tests/value.rs
index 3e7ea8f0..1e1ff5b9 100644
--- a/tests/value.rs
+++ b/tests/value.rs
@@ -15,14 +15,14 @@ fn char() {
 #[test]
 fn map() {
     let mut map = Map::new();
-    map.insert(Value::Char('a'), Value::Number(Number::new(1f64)));
+    map.insert(Value::Char('a'), Value::Number(Number::new(1)));
     map.insert(Value::Char('b'), Value::Number(Number::new(2f64)));
-    assert_eq!("{ 'a': 1, 'b': 2 }".parse(), Ok(Value::Map(map)));
+    assert_eq!("{ 'a': 1, 'b': 2.0 }".parse(), Ok(Value::Map(map)));
 }
 
 #[test]
 fn number() {
-    assert_eq!("42".parse(), Ok(Value::Number(Number::new(42f64))));
+    assert_eq!("42".parse(), Ok(Value::Number(Number::new(42))));
     assert_eq!("3.1415".parse(), Ok(Value::Number(Number::new(3.1415f64))));
 }
 
@@ -59,10 +59,10 @@ fn string() {
 #[test]
 fn seq() {
     let seq = vec![
-        Value::Number(Number::new(1f64)),
+        Value::Number(Number::new(1)),
         Value::Number(Number::new(2f64)),
     ];
-    assert_eq!("[1, 2]".parse(), Ok(Value::Seq(seq)));
+    assert_eq!("[1, 2.0]".parse(), Ok(Value::Seq(seq)));
 }
 
 #[test]