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

Add hash implementation #329

Closed
wants to merge 1 commit into from

Conversation

fabianboesiger
Copy link

There were several requests to implement Hash for IndexMap and IndexSet. The problem with implementing Hash is that it should be consistent with the Eq implementation. The Eq implementation is based on containing equivalent items regardless of order, requiring the hash to stay the same regardless of order. To fix this issue, one comment #67 (comment) suggests using a commutative operation to combine the hashes of the individual entries.

This draft is an implementation of this proposal.

@cuviper
Copy link
Member

cuviper commented Jun 11, 2024

That comment also linked to rust-lang/rust#48366, which was closed due to that hash approach being rather weak.

What is your use-case? Would you be able to work with a newtyped key that does consider the order of items?

@fabianboesiger
Copy link
Author

Yes, I'm aware that the solution is not perfect, but as many people seem to request it, it might still make sense to consider it.

I'm also aware that the .as_slice() API exists where Slice implements Hash, however, this is still very inconvenient when we want to derive Hash for a struct that contains an IndexMap.

It is also possible to add an implementation for newtyped keys that do consider the order of items, I can look into that if sou decide to add this feature.

@cuviper
Copy link
Member

cuviper commented Jun 12, 2024

but as many people seem to request it

Not so many that we need to do something, IMO, especially since the standard HashMap did not.

[...] when we want to derive Hash for a struct that contains an IndexMap.

I am still skeptical that it makes sense to do that. We can figure out ways to create a Hash, ordered or not, but that doesn't necessarily make it useful. Do you have a concrete use-case that you can share?

@fabianboesiger
Copy link
Author

My use case is I'd like to create sets of sets. I could use BTreeSets for that, but this is a bit too slow.

I think I will close this PR for now and create my own wrapper struct that I can use for my use case. Feel free to reopen this in case it is needed in the future.

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.

2 participants