Hide numeric types #64

drusellers · 2017-04-22T16:30:57Z

Copies the Number type from serde_json and converts this project to use it. Known issue compiling a hash function.

Closes: #60

drusellers · 2017-04-23T12:10:50Z

I'm down to two failing doc tests on number.rs around is_f64

killercup

Thanks! Let me know when you fixed the remaining tests.

I left a few inline comments. Aside from that, please add u64 to the partialeq_numeric! call, and make sure to s/JSON/YAML in a few places in the new number.rs :)

killercup · 2017-04-23T12:20:53Z

src/value/ser.rs

+                } else if let Some(f) = n.as_f64() {
+                    serializer.serialize_f64(f)
+                } else {
+                    panic!("unexpected number")


I don't think this should panic. It's either unreachable!() or an error we should propagate.

killercup · 2017-04-23T12:23:50Z

src/value/de.rs

+                } else if let Some(f) = n.as_f64() {
+                    Unexpected::Float(f)
+                } else {
+                    panic!("unexpected number")


I don't think this should panic. It's either unreachable!() or a new Unexpected case.

killercup · 2017-04-23T12:24:11Z

src/value/de.rs

+                } else if let Some(f) = n.as_f64() {
+                    visitor.visit_f64(f)
+                } else {
+                    panic!("unexpected number")


unreachable!()?

killercup · 2017-04-23T12:25:35Z

src/value/from.rs

@@ -9,7 +10,7 @@ macro_rules! from_integer {
        $(
            impl From<$ty> for Value {
                fn from(n: $ty) -> Self {
-                    Value::I64(n as i64)
+                    Value::Number(Number::from(n as i64))


Also make sure to add u64 in the macro call below

I haven't done much with macro's yet. Would I build a

from_integer! { i8 i16 i32 i64 isize }

and a

from_uinteger! { u8 u16 u32 usize }

IIUC, you can omit the as i64 on this line as Number already implements From for a bunch of integer sizes and just change the call below to

from_integer! { i8 i16 i32 i64 isize u8 u16 u32 u64 usize }

drusellers · 2017-04-23T12:43:00Z

src/value/number.rs

+    ///
+    /// // Integers.
+    /// assert!(!v["b"].is_f64());
+    /// assert!(!v["c"].is_f64());


These two asserts are failing and I can't reason out how !v["b"].is_f64() could be returning true.

Ah, I think I can see what the problem is. v["b"] returns a Value, so v["b"].is_f64() calls Value::is_f64 and not Number::is_f64. The implementation of Value::is_f64 is pretty simple: It returns true if the value is a number that can be represented as a f64.

I think the best course of action is to change the Value::is_f64 impl to use Number::is_f64 internally. This should work:

match *self { Value::Number(ref n) => n.is_f64(), _ => false, }

drusellers · 2017-04-23T14:05:04Z

Would love any comments on how the group would like to see https://travis-ci.org/dtolnay/serde-yaml/jobs/224911514#L203 addressed

killercup · 2017-04-23T14:19:49Z

error: you are implementing Hash explicitly but have derived PartialEq

The clippy docs describe this lint as a warning, that you have to be careful and manually make sure that k1 == k2 ⇒ hash(k1) == hash(k2) still holds for two Numbers k1 and k2. What does serde-json do? If you are certain this is fine, add

#[cfg(feature = "cargo-clippy", allow(derive_hash_xor_eq))] // This is fine because …

error: used unwrap() on an Option value.

I see you made this an expect now, but: Your from_f64 also fails when the input is not a finite number. I'd say we try to propagate this error somehow. Do you think we need change some error types to use a custom one and not just serde::de::Error?

drusellers · 2017-04-23T21:51:03Z

Your from_f64 also fails when the input is not a finite number

This is taken straight from serder-json - so I'm not sure on the best path to take forward. That said, do we wish to support NaN or Positive / Negative Infinity? My guess is no, so if None doesn't make since (and it does to the JSON crew https://github.com/serde-rs/json/blob/master/src/number.rs#L219) then I think an error is good.

serde::de::Error sounds like a good fit as this is a deserialization error and as a consumer I'd prefer one error type to catch rather than N error types to catch. But my rust-fu isn't the strongest and I'm happy to do what the group deems best. I just serde 1.0 EVERYWHERE. :)

drusellers · 2017-04-23T22:22:10Z

Ok, everything is green now. @killercup let me know what you think and now that the ⛰ has been moved we can sweep up the pebbles.

killercup · 2017-04-24T07:58:32Z

@drusellers very nice! I've added one more commit to make the new allow attrs depend on the clippy feature. I'll try to do another review later.

drusellers · 2017-04-24T12:32:59Z

src/value/number.rs

@@ -59,7 +59,7 @@ impl Number {
    /// # }
    /// ```
    #[inline]
-    #[allow(cast_sign_loss)]
+    #[cfg_attr(feature = "cargo-clippy", allow(cast_sign_loss))]


drusellers · 2017-04-24T12:33:11Z

src/value/number.rs

@@ -369,7 +369,7 @@ macro_rules! from_signed {
        $(
            impl From<$signed_ty> for Number {
                #[inline]
-                #[allow(cast_sign_loss)] 
+                #[cfg_attr(feature = "cargo-clippy", allow(cast_sign_loss))]


drusellers · 2017-04-24T12:33:37Z

Awesome, let me know if there is anything else I can do to help.

killercup

Okay, looking at http://yaml.org/type/float.html, we need to support NaN, inf and -inf.

Otherwise, I'd really like to replace the all expect, unwrap, panic!, and unreachable! calls with return Err(_).

killercup · 2017-04-24T19:22:34Z

src/value/de.rs

            }

            fn visit_f64<E>(self, f: f64) -> Result<Value, E>
                where E: SError,
            {
-                Ok(Value::F64(f))
+                Ok(Value::Number(Number::from_f64(f).expect("wrapping an existing float")))


This does not work for infinite values! cf. http://yaml.org/type/float.html for valid syntax. I locally added this example to the as_f64 docs of value:

/// ```rust /// # use std::f64; /// # fn yaml(i: &str) -> serde_yaml::Value { serde_yaml::from_str(i).unwrap() } /// assert_eq!(yaml("inf").as_f64(), Some(f64::INFINITY)); /// assert_eq!(yaml("-inf").as_f64(), Some(f64::NEG_INFINITY)); /// assert!(yaml("NaN").as_f64().unwrap().is_nan()); /// ```

Ok, cool. So I'm thinking two directions. One

enum N { PosInt(u64), /// Always less than zero. NegInt(i64), /// Always finite. Float(f64), NaN, InfinityPos, InfinityNeg }

but that feels like its mixes up float concerns and non float concerns.

Instead add an enum to wrap the float values like:

enum F { PosInfinity, NegInfinity, Finite(f64), NaN, }

so that N becomes

enum N { PosInt(u64), NegInt(i64), Float(F), }

Does that jive with you?

What's the reason for requiring the Float variant to be finite? We can always add is_finite and is_nan methods to Number (that return true/false (respectively) for integers).

No requirement really. I'll go your suggested route. I'm mostly just following what the JSON team did.

Given the recent change, I'm not sure it makes sense to have from_f64 return a Result any more. 🤔

killercup · 2017-04-24T19:24:45Z

src/value/number.rs

+    }
+
+    /// Converts a finite `f64` to a `Number`. Infinite or NaN values are not JSON
+    /// numbers.


Infinite or NaN values are not JSON numbers.

Well, they are YAML numbers, so this needs to change.

ha, roger that!

killercup

Thanks for keeping working on this! A few more nits but I think we are really close!

killercup · 2017-04-25T13:41:36Z

src/value/de.rs

@@ -41,22 +41,19 @@ impl<'de> Deserialize<'de> for Value {
            fn visit_i64<E>(self, i: i64) -> Result<Value, E>
                where E: SError,
            {
-                Ok(Value::I64(i))
+                Ok(Value::Number(Number::from(i)))


FYI, reading this again I can't help but think I'd write Value::Number(i.into())

easy change!

killercup · 2017-04-25T13:43:04Z

src/value/de.rs

            }

            fn visit_f64<E>(self, f: f64) -> Result<Value, E>
                where E: SError,
            {
-                Ok(Value::F64(f))
+                Ok(Value::Number(Number::from_f64(f).expect("wrapping an existing float")))


killercup · 2017-04-25T13:44:21Z

src/value/number.rs

+    /// assert_eq!(v["c"].as_f64(), Some(-64.0));
+    /// # }
+    /// ```
+    /// ```rust


Please add an empty line above this one

killercup · 2017-04-25T13:45:22Z

src/value/number.rs

+    /// #
+    /// assert!(Number::from_f64(256.0).is_some());
+    ///
+    /// //NAN is a number in YAML


Sorry, I don't think these comments add anything, the code basically says just that.

no worries - removing

killercup

Very nice. Looks good to me!

@dtolnay, do you, the original author of the JSON Number abstraction, want to review this as well?

dtolnay · 2017-04-25T14:43:11Z

Thank you both! Yes I'll get this in today.

dtolnay · 2017-04-25T18:26:16Z

src/value/number.rs

+    /// assert!(!Number::from_f64(f64::NEG_INFINITY).is_finite());
+    /// ```
+    #[inline]
+    pub fn from_f64(f: f64) -> Number {


This should just be a normal From<f64> conversion. JSON is different because not every f64 value can be represented in JSON.

dtolnay · 2017-04-25T18:27:17Z

src/value/de.rs

@@ -41,22 +41,19 @@ impl<'de> Deserialize<'de> for Value {
            fn visit_i64<E>(self, i: i64) -> Result<Value, E>
                where E: SError,
            {
-                Ok(Value::I64(i))
+                Ok(Value::Number(i.into()))


I would prefer this to be consistent across all three number methods. Here one uses Into::into, one uses Number::from, and one uses Number::from_f64.

dtolnay · 2017-04-25T18:30:49Z

src/value/de.rs

@@ -128,8 +125,17 @@ impl<'de> Deserializer<'de> for Value {
        match self {
            Value::Null => visitor.visit_unit(),
            Value::Bool(v) => visitor.visit_bool(v),
-            Value::I64(i) => visitor.visit_i64(i),
-            Value::F64(f) => visitor.visit_f64(f),
+            Value::Number(ref n) => {


This does not need to use ref. We own the number.

dtolnay · 2017-04-25T18:30:54Z

src/value/de.rs

-            Value::I64(i) => visitor.visit_i64(i),
-            Value::F64(f) => visitor.visit_f64(f),
+            Value::Number(ref n) => {
+                if let Some(u) = n.as_u64() {


This should use Number as Deserializer<'de> like serde_json does. That way you avoid the if / else if / else if / else unreachable mess.

Value::Number(n) => n.deserialize_any(visitor)

dtolnay · 2017-04-25T18:33:14Z

src/value/number.rs

@@ -0,0 +1,473 @@
+// Copied from serde_json


I would prefer to put this file at src/number.rs like how serde_json has it. The Number type is useful independently outside of the context of Value.

dtolnay · 2017-04-25T19:20:02Z

src/value/number.rs

+    PosInt(u64),
+    /// Always less than zero.
+    NegInt(i64),
+    /// Always finite.


This comment does not match the implementation.

dtolnay · 2017-04-25T19:27:18Z

src/value/number.rs

+    pub fn as_u64(&self) -> Option<u64> {
+        match self.n {
+            N::PosInt(n) => Some(n),
+            N::NegInt(n) => NumCast::from(n),


This case is impossible - a negative i64 won't convert to u64.

dtolnay · 2017-04-25T19:30:24Z

src/value/number.rs

+        }
+    }
+
+    /// Checks to see if a number is a number


dtolnay · 2017-04-25T19:33:23Z

src/value/number.rs

+        match self.n {
+            N::PosInt(i) => Display::fmt(&i, formatter),
+            N::NegInt(i) => Display::fmt(&i, formatter),
+            N::Float(f) => Display::fmt(&f, formatter),


This should have a special case for inf and nan. YAML has a distinct formatting for them.

dtolnay · 2017-04-25T19:44:18Z

src/value/ser.rs

@@ -9,8 +9,17 @@ impl Serialize for Value {
        match *self {
            Value::Null => serializer.serialize_unit(),
            Value::Bool(b) => serializer.serialize_bool(b),
-            Value::I64(i) => serializer.serialize_i64(i),
-            Value::F64(f) => serializer.serialize_f64(f),
+            Value::Number(ref n) => {


This should be implemented as n.serialize(serializer) to avoid the unreachable case.

dtolnay · 2017-04-25T19:55:32Z

Thanks, I merged this with some fixes in 486240a and e5293cc.

drusellers · 2017-04-25T20:34:27Z

@dtolnay and @killercup thank you so much for the mentoring. and 🎉 Serde 1.0 yay

First pass at hiding the numberic types

817eb67

drusellers changed the title ~~Hide numeric types~~ WIP: Hide numeric types Apr 22, 2017

Fixing tests

960096e

killercup reviewed Apr 23, 2017

View reviewed changes

drusellers added 4 commits April 23, 2017 07:38

Switch to unreachable calls

0b81758

s/JSON/YAML/g

382d90f

Adding u64 to the macro rules

1e319d1

Macro correction

0454a70

drusellers commented Apr 23, 2017

View reviewed changes

Delegate Value.is_f64 to Number.is_f64

ef72b31

drusellers changed the title ~~WIP: Hide numeric types~~ Hide numeric types Apr 23, 2017

drusellers added 2 commits April 23, 2017 08:59

Correcting clippy lints

d1bf380

fixing clippy lints

995dac9

turn clippy off

466a35e

Adjust lint allowance using clippy feature

bc121fc

drusellers commented Apr 24, 2017

View reviewed changes

killercup suggested changes Apr 24, 2017

View reviewed changes

drusellers added 2 commits April 25, 2017 08:19

Adding NaN and Infinite support

f9526af

Add integer tests

c8952e5

killercup reviewed Apr 25, 2017

View reviewed changes

Tidying up the mess

a2f17a9

killercup approved these changes Apr 25, 2017

View reviewed changes

dtolnay reviewed Apr 25, 2017

View reviewed changes

dtolnay merged commit 630b077 into dtolnay:master Apr 25, 2017

CAD97 mentioned this pull request Oct 3, 2019

Value == u64 #139

Merged

dtolnay mentioned this pull request Jul 25, 2022

Replace handwritten PartialEq with derive for Value #273

Merged

Hide numeric types #64

Hide numeric types #64

Conversation

drusellers commented Apr 22, 2017

drusellers commented Apr 23, 2017

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Apr 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drusellers commented Apr 23, 2017

killercup commented Apr 23, 2017

drusellers commented Apr 23, 2017

drusellers commented Apr 23, 2017

killercup commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drusellers commented Apr 24, 2017

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Apr 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Apr 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup left a comment

Choose a reason for hiding this comment

dtolnay commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtolnay commented Apr 25, 2017

drusellers commented Apr 25, 2017

killercup Apr 23, 2017 •

edited

killercup Apr 25, 2017 •

edited

killercup Apr 24, 2017 •

edited