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.

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

Implement Hash for Number. #814

Merged
merged 11 commits into from
Nov 5, 2021

Conversation

timothee-haudebourg
Copy link

This is the minimal version of my previous PR #790. I implemented the Eq and Hash traits for the Number type, regardless of its definition. Contrarily to my previous PR, this is strictly limited to the Number type.

I was originally going to use the ordered_float crate for that, but it turns out it is much simpler to do it manually. It avoids a dependency, and this library is not compatible with Rust 1.31.0 because it uses const parameters somewhere in the code of an optional feature, which has a syntax that is not supported with this version of Rust.

I had to manually implement PartialEq for Number to please clippy.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks, this mostly looks good.

// 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.

@timothee-haudebourg
Copy link
Author

I've added a test in the hash function to use the +0 hash for both +0 and -0. I've also added a test that cover this case.

With the arbitrary_precision feature, +0 and -0 are not equals so no problem here.

@timothee-haudebourg timothee-haudebourg force-pushed the hash-numbers-only branch 2 times, most recently from 68f9c65 to 7a8c433 Compare November 5, 2021 11:53
@timothee-haudebourg
Copy link
Author

Oh right, my test does not work with arbitrary_precision of course, let me fix that.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay dtolnay merged commit a28b1b1 into serde-rs:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants