async icon indicating copy to clipboard operation
async copied to clipboard

Make `Async a` `Hashable`

Open kirelagin opened this issue 7 years ago • 8 comments

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 avatar Dec 19 '17 22:12 kirelagin

Yes!

simonmar avatar Dec 21 '17 12:12 simonmar

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.

sol avatar Jan 06 '18 15:01 sol

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.

kirelagin avatar Jan 06 '18 17:01 kirelagin

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 avatar Jan 08 '18 08:01 simonmar

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.

kirelagin avatar Jan 08 '18 09:01 kirelagin

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

On Jan 8, 2018 1:07 AM, "Kirill Elagin" [email protected] wrote:

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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/simonmar/async/issues/73#issuecomment-355913344, or mute the thread https://github.com/notifications/unsubscribe-auth/AABwJ--fj0EIovZUNtpdbHUMFtdYnLovks5tIdrggaJpZM4RHqPT .

LeifW avatar Jan 08 '18 09:01 LeifW

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.

simonmar avatar Jan 08 '18 10:01 simonmar

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

sol avatar Feb 10 '18 10:02 sol