Skip to content

Add Hash derive#532

Open
fredszaq wants to merge 12 commits intoJelteF:masterfrom
fredszaq:hash-derive
Open

Add Hash derive#532
fredszaq wants to merge 12 commits intoJelteF:masterfrom
fredszaq:hash-derive

Conversation

@fredszaq
Copy link
Copy Markdown

@fredszaq fredszaq commented Jan 7, 2026

Synopsis

This PR adds a new derive for the Hash trait. This does mostly the same as the derive bundled with rust std but with a few niceties :

  • no overcontrain of generic types
  • possibility to ignore fields with #[hash(skip)] (also honors #[eq(skip)] and #[partial_eq(skip)] to ensure trait expectations)
  • possibility to use custom hash function on fields with #[hash(with(my_custom_hash_function)]

Solution

I used the PartialEq derive as an example to implement this, didn't have to do exotic things to get things to work

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

@tyranron tyranron added feature New derive or feature k::api Related to API (application interface) labels Jan 8, 2026
@tyranron tyranron added this to the next milestone Jan 8, 2026
Copy link
Copy Markdown
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@fredszaq that's a solid work!

One major concern should be addressed before merging this. Otherwise, the PR makes a very good impression!

Thank you very much for contributing this and being pedantic to the implementation!

UPD: The concern appeared to be not major and non-concern at all. Then, will try to merge it the next few days!

Comment thread tests/hash_and_eq.rs Outdated
Comment thread impl/doc/hash.md
Self { a: self_0, b: self_1, c: self_2 } => {
self_0.hash(state);
self_1.hash(state);
self_2.hash(state);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@fredszaq I think, deriving Hash is a little bit trickier than it seems to be.

For the correctness, we should definitely consider and work correctly with the prefix collisions. They also should have good coverage in tests, since seem to be quite error-prone to edge cases.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmmm... okay, it seems that std impls for tuples doesn't bother with that. It's all up to "leaf" types to bother with such things.

Good, so we don't need to bother about that. However, some tests for such cases still would be good to see.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@tyranron I added a few more tests around possible collisions, see 3fd8115 did you have other use cases in mind ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The rustc also doesn't bother for its derived impls (pass -Zunpretty=expanded to rustc or see playground):

#[derive(Hash)]
struct Test {
    a: u32,
    b: u32,
}

becomes

#![feature(prelude_import)]
extern crate std;
#[prelude_import]
use std::prelude::rust_2024::*;
struct Test {
    a: u32,
    b: u32,
}
#[automatically_derived]
impl ::core::hash::Hash for Test {
    #[inline]
    fn hash<__H: ::core::hash::Hasher>(&self, state: &mut __H) {
        ::core::hash::Hash::hash(&self.a, state);
        ::core::hash::Hash::hash(&self.b, state)
    }
}

@Ten0
Copy link
Copy Markdown

Ten0 commented Mar 3, 2026

I'd like to PR support for with in PartialEq as well, but I see this would conflict with this PR that already adds the attribute for the Hash case. Would love to see this merged so I can build on top! 😃
(Also I would also have uses for this PR 🙂.)

@fredszaq
Copy link
Copy Markdown
Author

fredszaq commented Mar 3, 2026

@Ten0 #535 already open here 😄

@fredszaq
Copy link
Copy Markdown
Author

Hey @tyranron, pinging back on this, anything I can do to help you get this merged ?

@tyranron
Copy link
Copy Markdown
Collaborator

tyranron commented Mar 13, 2026

@fredszaq

anything I can do to help you get this merged ?

Nah... I would like to, but don't see how you could possibly decrease the load on my current job for me to get back on this 😅

Anyway, thanks for asking! I will reall try to merge this on these weekends.

@fredszaq
Copy link
Copy Markdown
Author

No pressure^^ do not hesitate if you have any questions or want me to make changes !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New derive or feature k::api Related to API (application interface)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants