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

Make Async a Hashable #73

Open
kirelagin opened this issue Dec 19, 2017 · 8 comments
Open

Make Async a Hashable #73

kirelagin opened this issue Dec 19, 2017 · 8 comments

Comments

@kirelagin
Copy link
Contributor

kirelagin commented Dec 19, 2017

Async a already has instances of Eq and Ord, which makes it possible to store it in a Set, but it has no instance of Hashable, so it is not possible to store it in a HashSet, while the idea of having a HashSet of running threads seems to be quite useful.

Given that ThreadId already has an instance of Hashable and Eq and Ord of Async a delegate to the internal ThreadId, providing such an instance would be trivial, although it would require depending on hashable.

If this is acceptable, I can prepare a PR.

@kirelagin kirelagin changed the title Make Async Hashable Make Async a Hashable Dec 19, 2017
@simonmar
Copy link
Owner

Yes!

@sol
Copy link

sol commented Jan 6, 2018

I'm not too excited about this change for

  • the added dependency on hashable (which e.g. currently does not build with GHC HEAD)
  • the transitive dependency on text, which is (a) heavy weight (b) not a boot library with older versions of GHC

While I understand the virtue of this change, it also induces a cost on every user of async.

How I see it, this is a trade of between cost and functionality. Making a decision either way is valid. I just wanted to voice my preference here, which is not adding the additional dependencies and keeping async as lightweight as possible.

@kirelagin
Copy link
Contributor Author

kirelagin commented Jan 6, 2018

Yeah, I do realise that it is somewhat of a compromise, that’s why I started by asking whether Simon considers adding a dependency on hashable possible.

And I agree that some users may see this as an additional cost they did not ask for. But, on the other hand, I believe that any non-trivial Haskell project is very likely to depend on text and probably on hashable too.

In the end it should be your decision. Another option (rather weird one, but probably easier on the users) would be to have a separate library (e.g. async-hashable) for this sole purpose.

@simonmar
Copy link
Owner

simonmar commented Jan 8, 2018

The alternative is to have hashable depend on async. I don't feel strongly either way, but if hashable already has a large set of dependencies perhaps this is the better way to go. Putting the instance in a separate library is not good because orphans.

@simonmar simonmar reopened this Jan 8, 2018
@kirelagin
Copy link
Contributor Author

if hashable already has a large set of dependencies

It does not :(

  Build-depends:     base >= 4.4 && < 4.11,
                     bytestring >= 0.9 && < 0.11,
                     deepseq >= 1.3
  if impl(ghc)
    Build-depends:   ghc-prim,
                     text >= 0.11.0.5
  if impl(ghc) && flag(integer-gmp)
    Build-depends:   integer-gmp >= 0.2

We can ask what they think nevertheless, but I suspect they won’t be too excited about a transitive dependency on stm either.

@LeifW
Copy link

LeifW commented Jan 8, 2018 via email

@simonmar
Copy link
Owner

simonmar commented Jan 8, 2018

You could add it as an optionally built module + dep via cabal flag?

No, that's even worse. Packages should have a stable API. How would you express a dependency on the version of async with the flag enabled?

FWIW, GHC will have a dependency on text soon, so I don't think a dependency on text is terrible.

@sol
Copy link

sol commented Feb 10, 2018

For hspec/hspec I now include a "vendored" copy of async without the hashable dependency. It's ok with me to close this issue.

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

4 participants