-
Notifications
You must be signed in to change notification settings - Fork 538
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
Rewrite PureIO on top of coop #738
Rewrite PureIO on top of coop #738
Conversation
I'm going to just offer two high level thoughts for lack of more time:
This is an interesting point, I can't make up my mind but it really irks me that |
// cancelation is suppressed. | ||
// | ||
// uncancelable(_ => canceled(a)) <-> pure(a) | ||
// (canceled(a) >> never).start.void <-> pure(a).start.flatMap(_.cancel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this 👍
// | ||
// uncancelable(_ => canceled(a)) <-> pure(a) | ||
// (canceled(a) >> never).start.void <-> pure(a).start.flatMap(_.cancel) | ||
def canceled[A](fallback: A): F[A] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be by-name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pure
isn't. The idea here is basically that you're inserting a cancelation if not masked. fallback
is what you get if you are masked. So it's exactly the same as the following (if we could push the .start
et al outside whatever context we're in):
pure(fallback).start.flatMap(_.cancel)
…with the guarantee that the cancel
hits the bind either immediately before or after the pure
. Making the argument by-name would mean that canceled
is more analogous to delay
, with the tighter bound that the cancel is checked before it runs. That seems like unnecessary added complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your sentiment, but I'm not really sure it has to be analogous to delay
which is an FFI entry point and has totally different semantics. In this case it'd be simply about evaluation, staying as lazy as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cats very consistently avoids these kinds of things though in favor of Eval
, except when almost straight-up mandatory (e.g. ifM
). It also breaks the pure
-related laws, since pure
itself is eager. If you can show me a compelling example, I'm open to being convinced (a la ifM
). :-) Absent a really compelling rationale for laziness here though, I'm strongly leaning towards keeping it eager, at the very least to maintain equivalence with pure
.
The main reason I didn't is because I basically need to prism around and interpret them independently. The inner So it would just create a ton of boilerplate throughout the implementation, and would definitely be somewhat tricky in the interpreter due to the split instantiations. Not at all impossible, but is that better than simply having it in two distinct layers? To be clear here, the |
I think I'm convinced yeah |
Splitting this out…
I agree that the code becomes harder to understand in this case, but I literally just can't get past the fact that As an example, think about trying to write laws for Once you get down into In Note that the code complexity issues shouldn't affect |
Your considerations on the hierarchy are why I'm torn yeah |
Agreed. And I think that's a smell unless it's being done for performance reasons. (and even then, I'm tempted to say "don't", since
I'll change it in |
Based on the above, am I appropriately understanding that perhaps Bracket being merged with Concurrent may be appropriate? |
If we did that, then |
Mega-change of doom! This PR does a couple of things:
mask
/poll
combinator (calleduncancelable
for now) as described in Cats Effect 3.0 Interruption Model Proposal #681bracketCase
primitive, despite the slight overlap withuncancelable
. This is mostly because of inversion of control betweenBracket
andConcurrent
. There is no way (that I'm aware of) to definebracketCase
inBracket
in terms ofonCase
andguarantee
without making it aware of cancelability. And if we don't make it aware of cancelability, then we can define it, but we would need to override it in everyConcurrent with Bracket
implementation, which seems very annoying. So instead I madebracketCase
primitive whileguarantee
andonCase
are derived from it, and we'll have a law which codifies the relationship betweenuncancelable
and thebracket
.F[A]
into theCompleted
case ofExitCase
. This has the interesting side-effect of makingExitCase
kind of behave like a monad transformer, but only whenF
forms aTraverse
. When it doesn't form aTraverse
,ExitCase
is onlyApplicative
! Fixes CE3: ExitCase.Completed #643PureIO
asPureConc
(names are hard) on top of https://github.com/djspiewak/coop Eventually, if this works, I'll move coop under the Typelevel orgThe last point is an important one.
PureConc
is now the following:The type seems imposing but it's actually relatively easy to deconstruct. Taking it one stage at a time:
MVar
from coopFreeT
The nesting is actually somewhat arbitrary: we could have put
ThreadT
on the outside, and even merged the twoKleisli
s together into one if we wanted to.Laws are currently not passing, presumably because of bugs. I'm working on that, and in the process working on making
ThreadT
structures a bit more introspectable for debugging purposes, but this is good enough to get some review eyes on it. Also getting it into the upstream branch allows everyone else to pitch in and help out!