From 40ace5add9df2031819f9dbf52ee258f347d02c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 13:19:37 +0200 Subject: [PATCH 01/11] Impl`Eq+Hash` for `Number` using `ordered_float`. --- Cargo.toml | 1 + src/number.rs | 31 ++++++++++++++++++------------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0b09caeae..057a2297c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ serde = { version = "1.0.100", default-features = false } indexmap = { version = "1.5", optional = true } itoa = { version = "0.4.3", default-features = false } ryu = "1.0" +ordered-float = "2.8" [dev-dependencies] automod = "1.0" diff --git a/src/number.rs b/src/number.rs index b6b617f64..80dc528bc 100644 --- a/src/number.rs +++ b/src/number.rs @@ -1,6 +1,7 @@ use crate::de::ParserNumber; use crate::error::Error; use crate::lib::*; +use ordered_float::NotNan; use serde::de::{self, Unexpected, Visitor}; use serde::{ forward_to_deserialize_any, serde_if_integer128, Deserialize, Deserializer, Serialize, @@ -16,25 +17,21 @@ use serde::de::{IntoDeserializer, MapAccess}; pub(crate) const TOKEN: &str = "$serde_json::private::Number"; /// Represents a JSON number, whether integer or floating point. -#[derive(Clone, Eq, PartialEq)] +#[derive(Clone, PartialEq, Eq, Hash)] pub struct Number { n: N, } #[cfg(not(feature = "arbitrary_precision"))] -#[derive(Copy, Clone, PartialEq)] +#[derive(Copy, Clone, PartialEq, Eq, Hash)] enum N { PosInt(u64), /// Always less than zero. NegInt(i64), /// Always finite. - Float(f64), + Float(NotNan), } -// Implementing Eq is fine since any float values are always finite. -#[cfg(not(feature = "arbitrary_precision"))] -impl Eq for N {} - #[cfg(feature = "arbitrary_precision")] type N = String; @@ -208,7 +205,7 @@ impl Number { match self.n { N::PosInt(n) => Some(n as f64), N::NegInt(n) => Some(n as f64), - N::Float(n) => Some(n), + N::Float(n) => Some(n.into_inner()), } #[cfg(feature = "arbitrary_precision")] self.n.parse::().ok().filter(|float| float.is_finite()) @@ -232,7 +229,11 @@ impl Number { let n = { #[cfg(not(feature = "arbitrary_precision"))] { - N::Float(f) + N::Float(unsafe { + // This is safe because `f.is_finite()` ensures + // that `f` is not `NaN`. + NotNan::new_unchecked(f) + }) } #[cfg(feature = "arbitrary_precision")] { @@ -307,7 +308,7 @@ impl Serialize for Number { match self.n { N::PosInt(u) => serializer.serialize_u64(u), N::NegInt(i) => serializer.serialize_i64(i), - N::Float(f) => serializer.serialize_f64(f), + N::Float(f) => serializer.serialize_f64(f.into_inner()), } } @@ -461,7 +462,7 @@ macro_rules! deserialize_any { match self.n { N::PosInt(u) => visitor.visit_u64(u), N::NegInt(i) => visitor.visit_i64(i), - N::Float(f) => visitor.visit_f64(f), + N::Float(f) => visitor.visit_f64(f.into_inner()), } } @@ -625,7 +626,11 @@ impl From for Number { ParserNumber::F64(f) => { #[cfg(not(feature = "arbitrary_precision"))] { - N::Float(f) + N::Float(unsafe { + // This is safe because `ParserNumber` ensures + // that `f` is not `NaN`. + NotNan::new_unchecked(f) + }) } #[cfg(feature = "arbitrary_precision")] { @@ -736,7 +741,7 @@ impl Number { match self.n { N::PosInt(u) => Unexpected::Unsigned(u), N::NegInt(i) => Unexpected::Signed(i), - N::Float(f) => Unexpected::Float(f), + N::Float(f) => Unexpected::Float(f.into_inner()), } } From be2f5738bacf9e149af276bcc702533700d0be51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 13:31:18 +0200 Subject: [PATCH 02/11] Do not show `NotNan` in `Number`'s `Debug` impl. --- src/number.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/number.rs b/src/number.rs index 80dc528bc..b3ae205cc 100644 --- a/src/number.rs +++ b/src/number.rs @@ -283,7 +283,7 @@ impl Debug for Number { debug.field(&i); } N::Float(f) => { - debug.field(&f); + debug.field(&f.into_inner()); } } debug.finish() From b59c0c0e556adc69d0d7981fec0dee77ee8be0e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 13:41:42 +0200 Subject: [PATCH 03/11] Do not import always `NotNan`. Skip it when `arbitrary_precision` is enabled. --- src/number.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/number.rs b/src/number.rs index b3ae205cc..97a17458e 100644 --- a/src/number.rs +++ b/src/number.rs @@ -1,7 +1,6 @@ use crate::de::ParserNumber; use crate::error::Error; use crate::lib::*; -use ordered_float::NotNan; use serde::de::{self, Unexpected, Visitor}; use serde::{ forward_to_deserialize_any, serde_if_integer128, Deserialize, Deserializer, Serialize, @@ -13,6 +12,9 @@ use crate::error::ErrorCode; #[cfg(feature = "arbitrary_precision")] use serde::de::{IntoDeserializer, MapAccess}; +#[cfg(not(feature = "arbitrary_precision"))] +use ordered_float::NotNan; + #[cfg(feature = "arbitrary_precision")] pub(crate) const TOKEN: &str = "$serde_json::private::Number"; From eec4e9d272d2b30943d05705118d37da06169fac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 13:42:01 +0200 Subject: [PATCH 04/11] Do not import `ordered_float` default features. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 057a2297c..aaa8aeca6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,7 +18,7 @@ serde = { version = "1.0.100", default-features = false } indexmap = { version = "1.5", optional = true } itoa = { version = "0.4.3", default-features = false } ryu = "1.0" -ordered-float = "2.8" +ordered-float = { version = "2.8", default-features = false } [dev-dependencies] automod = "1.0" From d153b886e8370c03c39fe1ae937487d337abc455 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 14:02:21 +0200 Subject: [PATCH 05/11] Discard `ordered_float`, use custom `Hash` impl. --- Cargo.toml | 1 - src/number.rs | 47 +++++++++++++++++++++++++++-------------------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index aaa8aeca6..0b09caeae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,7 +18,6 @@ serde = { version = "1.0.100", default-features = false } indexmap = { version = "1.5", optional = true } itoa = { version = "0.4.3", default-features = false } ryu = "1.0" -ordered-float = { version = "2.8", default-features = false } [dev-dependencies] automod = "1.0" diff --git a/src/number.rs b/src/number.rs index 97a17458e..be6a1ece8 100644 --- a/src/number.rs +++ b/src/number.rs @@ -12,9 +12,6 @@ use crate::error::ErrorCode; #[cfg(feature = "arbitrary_precision")] use serde::de::{IntoDeserializer, MapAccess}; -#[cfg(not(feature = "arbitrary_precision"))] -use ordered_float::NotNan; - #[cfg(feature = "arbitrary_precision")] pub(crate) const TOKEN: &str = "$serde_json::private::Number"; @@ -25,13 +22,31 @@ pub struct Number { } #[cfg(not(feature = "arbitrary_precision"))] -#[derive(Copy, Clone, PartialEq, Eq, Hash)] +#[derive(Copy, Clone, PartialEq)] enum N { PosInt(u64), /// Always less than zero. NegInt(i64), /// Always finite. - Float(NotNan), + Float(f64), +} + +// Implementing Eq is fine since any float values are always finite. +#[cfg(not(feature = "arbitrary_precision"))] +impl Eq for N {} + +#[cfg(not(feature = "arbitrary_precision"))] +impl std::hash::Hash for N { + fn hash(&self, h: &mut H) { + match self { + Self::PosInt(i) => i.hash(h), + Self::NegInt(i) => i.hash(h), + Self::Float(f) => { + // Using `f64::to_bits` here is fine since any float values are always finite. + f.to_bits().hash(h) + } + } + } } #[cfg(feature = "arbitrary_precision")] @@ -207,7 +222,7 @@ impl Number { match self.n { N::PosInt(n) => Some(n as f64), N::NegInt(n) => Some(n as f64), - N::Float(n) => Some(n.into_inner()), + N::Float(n) => Some(n), } #[cfg(feature = "arbitrary_precision")] self.n.parse::().ok().filter(|float| float.is_finite()) @@ -231,11 +246,7 @@ impl Number { let n = { #[cfg(not(feature = "arbitrary_precision"))] { - N::Float(unsafe { - // This is safe because `f.is_finite()` ensures - // that `f` is not `NaN`. - NotNan::new_unchecked(f) - }) + N::Float(f) } #[cfg(feature = "arbitrary_precision")] { @@ -285,7 +296,7 @@ impl Debug for Number { debug.field(&i); } N::Float(f) => { - debug.field(&f.into_inner()); + debug.field(&f); } } debug.finish() @@ -310,7 +321,7 @@ impl Serialize for Number { match self.n { N::PosInt(u) => serializer.serialize_u64(u), N::NegInt(i) => serializer.serialize_i64(i), - N::Float(f) => serializer.serialize_f64(f.into_inner()), + N::Float(f) => serializer.serialize_f64(f), } } @@ -464,7 +475,7 @@ macro_rules! deserialize_any { match self.n { N::PosInt(u) => visitor.visit_u64(u), N::NegInt(i) => visitor.visit_i64(i), - N::Float(f) => visitor.visit_f64(f.into_inner()), + N::Float(f) => visitor.visit_f64(f), } } @@ -628,11 +639,7 @@ impl From for Number { ParserNumber::F64(f) => { #[cfg(not(feature = "arbitrary_precision"))] { - N::Float(unsafe { - // This is safe because `ParserNumber` ensures - // that `f` is not `NaN`. - NotNan::new_unchecked(f) - }) + N::Float(f) } #[cfg(feature = "arbitrary_precision")] { @@ -743,7 +750,7 @@ impl Number { match self.n { N::PosInt(u) => Unexpected::Unsigned(u), N::NegInt(i) => Unexpected::Signed(i), - N::Float(f) => Unexpected::Float(f.into_inner()), + N::Float(f) => Unexpected::Float(f), } } From 10655c76395e24297754bac8fd63839a5ebc8efe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 14:05:45 +0200 Subject: [PATCH 06/11] Use `core::hash` instead of `std::hash`. --- src/number.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/number.rs b/src/number.rs index be6a1ece8..93ac2a54f 100644 --- a/src/number.rs +++ b/src/number.rs @@ -36,8 +36,8 @@ enum N { impl Eq for N {} #[cfg(not(feature = "arbitrary_precision"))] -impl std::hash::Hash for N { - fn hash(&self, h: &mut H) { +impl core::hash::Hash for N { + fn hash(&self, h: &mut H) { match self { Self::PosInt(i) => i.hash(h), Self::NegInt(i) => i.hash(h), From c3ac6c00770934a0f0d655f941e97caedfeed504 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 14:13:14 +0200 Subject: [PATCH 07/11] Manual implementation of `Number`'s `PartialEq`. To please clippy. --- src/number.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/number.rs b/src/number.rs index 93ac2a54f..381dfb728 100644 --- a/src/number.rs +++ b/src/number.rs @@ -22,7 +22,7 @@ pub struct Number { } #[cfg(not(feature = "arbitrary_precision"))] -#[derive(Copy, Clone, PartialEq)] +#[derive(Copy, Clone)] enum N { PosInt(u64), /// Always less than zero. @@ -31,6 +31,18 @@ enum N { Float(f64), } +#[cfg(not(feature = "arbitrary_precision"))] +impl PartialEq for N { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (Self::PosInt(a), Self::PosInt(b)) => a == b, + (Self::NegInt(a), Self::NegInt(b)) => a == b, + (Self::Float(a), Self::Float(b)) => a == b, + _ => false, + } + } +} + // Implementing Eq is fine since any float values are always finite. #[cfg(not(feature = "arbitrary_precision"))] impl Eq for N {} From 9046e3fa869c04554c39f46a5ed571420d86cf10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 14:16:49 +0200 Subject: [PATCH 08/11] Avoid `Self::` for older Rust versions. Also fix a clippy warning. --- src/number.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/number.rs b/src/number.rs index 381dfb728..b3685f397 100644 --- a/src/number.rs +++ b/src/number.rs @@ -35,9 +35,9 @@ enum N { impl PartialEq for N { fn eq(&self, other: &Self) -> bool { match (self, other) { - (Self::PosInt(a), Self::PosInt(b)) => a == b, - (Self::NegInt(a), Self::NegInt(b)) => a == b, - (Self::Float(a), Self::Float(b)) => a == b, + (N::PosInt(a), N::PosInt(b)) => a == b, + (N::NegInt(a), N::NegInt(b)) => a == b, + (N::Float(a), N::Float(b)) => a == b, _ => false, } } @@ -51,11 +51,11 @@ impl Eq for N {} impl core::hash::Hash for N { fn hash(&self, h: &mut H) { match self { - Self::PosInt(i) => i.hash(h), - Self::NegInt(i) => i.hash(h), - Self::Float(f) => { + N::PosInt(i) => i.hash(h), + N::NegInt(i) => i.hash(h), + N::Float(f) => { // Using `f64::to_bits` here is fine since any float values are always finite. - f.to_bits().hash(h) + f.to_bits().hash(h); } } } From 48393aba4075ea1cb7e08e1a7b0bd03a78bd47e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 14:24:58 +0200 Subject: [PATCH 09/11] Better justification for the use of `f64::to_bits` --- src/number.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/number.rs b/src/number.rs index b3685f397..4c9dad682 100644 --- a/src/number.rs +++ b/src/number.rs @@ -54,7 +54,7 @@ impl core::hash::Hash for N { N::PosInt(i) => i.hash(h), N::NegInt(i) => i.hash(h), N::Float(f) => { - // Using `f64::to_bits` here is fine since any float values are always finite. + // Using `f64::to_bits` here is fine since any float values are never `Nan`. f.to_bits().hash(h); } } From d12fbb9520bc6d3a5aac6d233bb0ffaae831f956 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Wed, 20 Oct 2021 14:25:51 +0200 Subject: [PATCH 10/11] Fix `NaN` typo. --- src/number.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/number.rs b/src/number.rs index 4c9dad682..ad59ae6dd 100644 --- a/src/number.rs +++ b/src/number.rs @@ -54,7 +54,7 @@ impl core::hash::Hash for N { N::PosInt(i) => i.hash(h), N::NegInt(i) => i.hash(h), N::Float(f) => { - // Using `f64::to_bits` here is fine since any float values are never `Nan`. + // Using `f64::to_bits` here is fine since any float values are never `NaN`. f.to_bits().hash(h); } } From f53ae31df61f7e321276bfcfffe3c1562013d338 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Haudebourg?= Date: Fri, 5 Nov 2021 12:57:41 +0100 Subject: [PATCH 11/11] Use the same hash for +0 and -0. --- src/number.rs | 9 ++++++++- tests/test.rs | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/number.rs b/src/number.rs index ad59ae6dd..b39e164aa 100644 --- a/src/number.rs +++ b/src/number.rs @@ -55,7 +55,14 @@ impl core::hash::Hash for N { N::NegInt(i) => i.hash(h), N::Float(f) => { // Using `f64::to_bits` here is fine since any float values are never `NaN`. - f.to_bits().hash(h); + if *f == 0.0f64 { + // The IEEE 754 standard has two representations for zero, +0 and -0, + // such that +0 == -0. + // In both cases we use the +0 hash so that hash(+0) == hash(-0). + 0.0f64.to_bits().hash(h); + } else { + f.to_bits().hash(h); + } } } } diff --git a/tests/test.rs b/tests/test.rs index c8df23dba..ea7759ce7 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -2284,3 +2284,19 @@ fn test_value_into_deserializer() { let outer = Outer::deserialize(map.into_deserializer()).unwrap(); assert_eq!(outer.inner.string, "Hello World"); } + +#[test] +fn hash_positive_and_negative_zero() { + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; + + fn hash(obj: impl Hash) -> u64 { + let mut hasher = DefaultHasher::new(); + obj.hash(&mut hasher); + hasher.finish() + } + + let k1 = serde_json::from_str::("0.0").unwrap(); + let k2 = serde_json::from_str::("-0.0").unwrap(); + assert!(k1 != k2 || hash(k1) == hash(k2)); +}