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

Will anything break if Hashable got Eq as a superclass? #318

Closed
phadej opened this issue Aug 30, 2021 · 18 comments
Closed

Will anything break if Hashable got Eq as a superclass? #318

phadej opened this issue Aug 30, 2021 · 18 comments

Comments

@phadej
Copy link
Contributor

phadej commented Aug 30, 2021

Hashable and Eq have to be compatible for unordered-containers to work properly, so having Eq as superclass of Hashable seems natural to me.

My question: is there any non-contrived use case for Hashable which will be ruled out by having Eq superclass? Bonus points if that use case is in realm of hashing-based data structures, because "This class (Hashable) exists for the benefit of hashing-based data structures."

EDIT: I'm asking here first, for smaller audience. Will later ask on libraries list & other fora.

@treeowl
Copy link
Collaborator

treeowl commented Aug 30, 2021

It's hard to imagine one applicable to hashing-based data structures. For one that isn't, maybe something like computable reals?

@phadej
Copy link
Contributor Author

phadej commented Aug 30, 2021

computable reals

You mean that they cannot be Eqd? But can they be hashed either?

@treeowl
Copy link
Collaborator

treeowl commented Aug 30, 2021 via email

@treeowl
Copy link
Collaborator

treeowl commented Aug 30, 2021 via email

@phadej
Copy link
Contributor Author

phadej commented Aug 30, 2021

But what that hash would be useful for? Specifically what algorithm (polymorphic in Hashable type) would be applicable to this (hashing N first digits of computable reals) (because if it's not polymorhic, you don't need to use Hashable).

@treeowl
Copy link
Collaborator

treeowl commented Aug 30, 2021 via email

@phadej
Copy link
Contributor Author

phadej commented Oct 31, 2021

https://hackage.haskell.org/package/hashable-1.4.0.0 is on Hackage.

If the bounds are simply relaxed to allow hashable-1.4.0.0 everything compiles fine and all tests pass.

However another alternative or additional option is to make a new release with hashable >=1.4.0.0 lower bound and cleaned up type-signatures.

@phadej
Copy link
Contributor Author

phadej commented Oct 31, 2021

I was imprecise, wigh GHC-7:

% cabal build -w ghc-7.10.3 --constraint='hashable >=1.4'
Resolving dependencies...
Build profile: -w ghc-7.10.3 -O1
In order, the following will be built (use -v for more details):
 - unordered-containers-0.2.14.0 (lib) (first run)
Configuring library for unordered-containers-0.2.14.0..
Preprocessing library for unordered-containers-0.2.14.0..
Building library for unordered-containers-0.2.14.0..
[1 of 9] Compiling Data.HashMap.Internal.List ( Data/HashMap/Internal/List.hs, /code/other-haskell/unordered-containers/dist-newstyle/build/x86_64-linux/ghc-7.10.3/unordered-containers-0.2.14.0/build/Data/HashMap/Internal/List.o )
[2 of 9] Compiling Data.HashMap.Internal.Unsafe ( Data/HashMap/Internal/Unsafe.hs, /code/other-haskell/unordered-containers/dist-newstyle/build/x86_64-linux/ghc-7.10.3/unordered-containers-0.2.14.0/build/Data/HashMap/Internal/Unsafe.o )
[3 of 9] Compiling Data.HashMap.Internal.Array ( Data/HashMap/Internal/Array.hs, /code/other-haskell/unordered-containers/dist-newstyle/build/x86_64-linux/ghc-7.10.3/unordered-containers-0.2.14.0/build/Data/HashMap/Internal/Array.o )
[4 of 9] Compiling Data.HashMap.Internal ( Data/HashMap/Internal.hs, /code/other-haskell/unordered-containers/dist-newstyle/build/x86_64-linux/ghc-7.10.3/unordered-containers-0.2.14.0/build/Data/HashMap/Internal.o )

Data/HashMap/Internal.hs:536:10-48:
    Could not deduce (transformers-0.4.2.0:Data.Functor.Classes.Eq1
                        (HashMap k))

as Eq1 instance is given only for base-4.9.

I'd suggest just dropping GHC-7 support. It's the simplest option.

@sjakobi
Copy link
Member

sjakobi commented Oct 31, 2021

I'd suggest just dropping GHC-7 support. It's simplest option.

Dropping support for GHC < 8 sounds good to me. If it turns out that some users really need continued support for GHC-7, we can consider a more complex solution.

@treeowl
Copy link
Collaborator

treeowl commented Oct 31, 2021

I'm missing something. What's the purpose of this change? And how does an Eq superclass for Hashable relate to Eq1 for HashMap k?

@phadej
Copy link
Contributor Author

phadej commented Oct 31, 2021

analogously Eq1 is a superclass of Hashable1. HashMap is Hashable1, but not always (GHC <8) Eq1.

The proposed change is just restrict the unordered-containers' supported GHC versions so Eq1 (HashMap k) always exists.

@phadej
Copy link
Contributor Author

phadej commented Oct 31, 2021

Alternatives are:

  • adding a dependency on transformers (and transormers-compat) to have Eq1 always.
  • removing Hashable1 instances (breaking change)

@sjakobi
Copy link
Member

sjakobi commented Oct 31, 2021

I've started working on removing the compat code for GHC < 8. So far, this looks like a very nice cleanup. We can even get rid of an entire module: https://github.com/haskell-unordered-containers/unordered-containers/blob/master/Data/HashMap/Internal/Unsafe.hs

@treeowl
Copy link
Collaborator

treeowl commented Oct 31, 2021

I'm not using 7.x anymore. If y'all think other people won't mind, let's go ahead.

@treeowl
Copy link
Collaborator

treeowl commented Oct 31, 2021

And getting rid of hideously dangerous code is always good.

sjakobi added a commit that referenced this issue Nov 1, 2021
This prepares u-c for supporting hashable-1.4, as discussed in
#318.
sjakobi added a commit that referenced this issue Nov 1, 2021
This prepares u-c for supporting hashable-1.4, as discussed in
#318.
@sjakobi
Copy link
Member

sjakobi commented Nov 1, 2021

Support for GHC < 8 was removed in #323. #324 will bump our bound on hashable.

@sjakobi sjakobi closed this as completed Nov 1, 2021
@phadej
Copy link
Contributor Author

phadej commented Nov 7, 2021

Is there an ETA for a release?

@sjakobi
Copy link
Member

sjakobi commented Nov 7, 2021

@phadej I'll upload a release once #327 is merged.

So, probably Monday or maybe Tuesday.

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

No branches or pull requests

3 participants