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

netsize: Hash implementation to match PartialEq #186

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

Alextopher
Copy link
Contributor

Hash and PartialEq implementations on NetworkSize are mutually incompatible and don't follow the std::hash::Hash constraints. This will cause issues if you use NetworkSize in a HashMap or HashSet.

It is required that the keys implement the Eq and Hash traits, although this can frequently be achieved by using #[derive(PartialEq, Eq, Hash)]. If you implement these yourself, it is important that the following property holds:

k1 == k2 -> hash(k1) == hash(k2)

In other words, if two keys are equal, their hashes must be equal. Violating this property is a logic error.

Here is a playground link demonstrating the issue. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d030b1654e0abd2515c16ffe83fcd9a0

This PR fixes this issue by writing a custom implementation of Hash which only use the u128 logical value of the network size, same as PartialEq.

@Alextopher Alextopher changed the title netsize: hash implementation to match PartialEq netsize: Hash implementation to match PartialEq Apr 29, 2024
@Alextopher
Copy link
Contributor Author

@Alextopher
Copy link
Contributor Author

Alextopher commented Apr 29, 2024

Further changes here:

  • Added some documentation to NetworkSize explaining how comparisons work.
  • Wrote private NetworkSize::as_u128() function to simplify conversion
  • Removed Into<T> implementations preferring From<T> which derives Into<T> https://doc.rust-lang.org/rust-by-example/conversion/from_into.html
  • Added the PartialEq, Hash test from the playground as a test in this crate.

@Alextopher
Copy link
Contributor Author

This fixes a bug introduced in #175

@achanda achanda merged commit 241b1dc into achanda:master Apr 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants