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

Document additional guarantee of Arg's Hashable instance #243

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Document additional guarantee of Arg's Hashable instance #243

wants to merge 1 commit into from

Conversation

josephcsible
Copy link

Fixes #242.

phadej
phadej reviewed Feb 3, 2022
View reviewed changes
src/Data/Hashable/Class.hs
@@ -906,6 +906,8 @@ instance Hashable a => Hashable (Max a) where
hashWithSalt p (Max a) = hashWithSalt p a

-- | __Note__: Prior to @hashable-1.3.0.0@ the hash computation included the second argument of 'Arg' which wasn't consistent with its 'Eq' instance.
-- Additionally, as of @hashable-1.3.0.0@, and going forward as part of the API guarantee, the hash of @'Arg' a _@ is the same value as the hash of @a@.
-- This means that it's safe to add and remove 'Arg' from elements stored in hash-indexed data structures without changing any hashes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so this is actually not true.

hashWithSalt salt (Arg x _) = hashWithSalt x by definitiuon, but hash (Arg x _) /= hash x`:

> hash (Nothing :: Maybe Int)
0
> hash (Arg (Nothing :: Maybe Int) 'x')
-5808590958014384161

That is an unfortunate "oversight".

@josephcsible josephcsible marked this pull request as draft February 3, 2022 17:47
@josephcsible
Copy link
Author

Oof, that's not good, especially since the thing I wanted to use this for does use hash and not hashWithSalt. I assume fixing that would need another major version bump, right?

@phadej
Copy link
Contributor

phadej commented Feb 4, 2022

Oof, that's not good, especially since the thing I wanted to use this for does use hash and not hashWithSalt. I assume fixing that would need another major version bump, right?

Yes

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

@phadej phadej phadej left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document guarantee that hash (Arg x _) == hash x
2 participants
@josephcsible @phadej