Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement Hash for Number. #814

Merged
merged 11 commits into from Nov 5, 2021
Merged

Implement Hash for Number. #814

Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
40ace5a
Impl`Eq+Hash` for `Number` using `ordered_float`.
Oct 20, 2021
be2f573
Do not show `NotNan` in `Number`'s `Debug` impl.
Oct 20, 2021
b59c0c0
Do not import always `NotNan`.
Oct 20, 2021
eec4e9d
Do not import `ordered_float` default features.
Oct 20, 2021
d153b88
Discard `ordered_float`, use custom `Hash` impl.
Oct 20, 2021
10655c7
Use `core::hash` instead of `std::hash`.
Oct 20, 2021
c3ac6c0
Manual implementation of `Number`'s `PartialEq`.
Oct 20, 2021
9046e3f
Avoid `Self::` for older Rust versions.
Oct 20, 2021
48393ab
Better justification for the use of `f64::to_bits`
Oct 20, 2021
d12fbb9
Fix `NaN` typo.
Oct 20, 2021
f53ae31
Use the same hash for +0 and -0.
Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 28 additions & 2 deletions src/number.rs
View file
Open in desktop
Expand Up @@ -16,13 +16,13 @@ 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)]
enum N {
PosInt(u64),
/// Always less than zero.
Expand All @@ -31,10 +31,36 @@ enum N {
Float(f64),
}

#[cfg(not(feature = "arbitrary_precision"))]
impl PartialEq for N {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(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,
}
}
}

// 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 core::hash::Hash for N {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://doc.rust-lang.org/1.56.0/std/hash/trait.Hash.html#hash-and-eq
Hash has a documented requirement that k1 == k2 implies hash(k1) == hash(k2), which this implementation violates.

use serde_json::Number;
use std::hash::{Hash, Hasher};
use std::collections::hash_map::DefaultHasher;

fn main() {
    let k1 = serde_json::from_str::<Number>("0.0").unwrap();
    let k2 = serde_json::from_str::<Number>("-0.0").unwrap();
    dbg!(k1 == k2);
    dbg!(hash(k1) == hash(k2));
}

fn hash(obj: impl Hash) -> u64 {
    let mut hasher = DefaultHasher::new();
    obj.hash(&mut hasher);
    hasher.finish()
}
[src/main.rs:8] k1 == k2 = true
[src/main.rs:9] hash(k1) == hash(k2) = false

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I forgot about that.

fn hash<H: core::hash::Hasher>(&self, h: &mut H) {
match self {
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`.
f.to_bits().hash(h);
}
}
}
}

#[cfg(feature = "arbitrary_precision")]
type N = String;

Expand Down