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 Compiler.Target extend MonadCancelThrow #2612

Merged
merged 4 commits into from Sep 27, 2021

Conversation

bplommer
Copy link
Contributor

This is intended to facilitate using Target as a context bound when you need MonadCancelThrow but don't specifically need either Sync or Concurrent.

@kubukoz
Copy link
Member

kubukoz commented Sep 17, 2021

Personal take, I wish Target didn't extend either of these traits at all... but I guess this avoids the conflict since you can no longer require Target and a typeclass with only partial overlap?

@bplommer
Copy link
Contributor Author

bplommer commented Sep 17, 2021

Personal take, I wish Target didn't extend either of these traits at all...

Why's that?

but I guess this avoids the conflict since you can no longer require Target and a typeclass with only partial overlap?

Yeah - as things stand, if you need both Target and MonadCancelThrow you either have ambiguous instances of MonadThrow or you pick one of Sync or Concurrent - then if you have dependencies that made both choices you end up using Async to satisfy both, when one or the other should have sufficed. (That's currently the situation with http4s, for example, where Target is derived in some cases via Sync and in others via Concurrent.)

@kubukoz
Copy link
Member

kubukoz commented Sep 17, 2021

Why's that?

Conflicts with other context bounds when you want to use extension methods for a common subset (e.g. .map), thus requiring PRs like this one :D

I guess there's a reason why I use Compiler[F, F] instead

@bplommer
Copy link
Contributor Author

bplommer commented Sep 17, 2021

Linking this http4s PR to use Target as a context bound in case anyone wants to weigh in on why the whole thing is a bad idea.

@mpilquist
Copy link
Member

I pushed an additional commit to remove some of the intermediate classes.

@mpilquist mpilquist merged commit a2a4740 into typelevel:main Sep 27, 2021
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.

None yet

3 participants