-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Removed and replaced inconsistent Parallel derivation for EitherT #3777
Removed and replaced inconsistent Parallel derivation for EitherT #3777
Conversation
I take it back, the new method has fewer implicits. |
Co-authored-by: Lars Hupel <lars.hupel@mytum.de>
Had a long discussion with @Baccata and he pointed out that it's very important that we enable users to get access to the alternative One option here is to expose the old Basically we need to find an answer for that orphan. |
Oh, just so it's preserved for reference… The first principles argument here is surrounding the nature of what it means to be "an effect", and also what it means to merge the semantics of those effects together. The existing type F[A] = StateT[State[Int, *], Int, A] If we do Monad transformers always apply effects in a single layer (another example is None of this is meant to imply that we shouldn't make the old semantic available, it's still very useful, it's simply the best "first principles" argument for a default semantic that we were able to come up with. |
…default semantics
Requesting review from @Baccata. I added a mechanism for restoring the non-default semantics in a lexical scope, and it will take precedence over the orphan in Cats Effect. You can even abuse package objects to force it into a whole package (recursively!) if you need to. |
Doesn't that mean that only the error-accumulating instance should be available? |
I was hoping no one would notice that. :-P Yes it does imply that. However, this would be… essentially useless. It's also impossible to derive that semantic in terms of |
I'm not sure it would take precedence if both are in scope, at least on Scala 2.
Not sure what you mean 😄 |
@joroKr21 Daniel's change means that the current behaviour is not provided implicitly out of the box anymore, but the user can easily use the explicit package object myPackage {
implicit def overridingBehaviour[F[_] : Parallel, E : Semigroup] : Parallel[EitherT[F, E, *] = EitherT.accumulatingParallel[F, E]
} Because this accumulating behaviour is not default anymore, it means that a What this means is that you and I weirdos that use EitherT's parallel to accumulate errors are essentially being softly sacrificed for the greater good 😄 (aka the people who rightly think that concurrency should imply parallelism, which is a very correct intuition). |
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.
@djspiewak, not that I'm acting as a figure of authority on this in any way, shape or form, but you have my personal blessing (provided the same change is performed on IorT)
@LukaJCB, @larsrh , if this change is approved, could we maybe discuss the addition of a monadic ValidatedT
(or a different name) that'd require both Parallel[F]
and Semigroup[E]
to derive a Parallel, filling the hole that's being created ?
Fine with me.
@djspiewak Can you add that to this PR? |
I'm a bit queasy about the fact that this will silently change the behaviour of an instance, but I also don't have any idea how to alert these users except through release announcements etc, but I fear that cats is so far upstream that most upgrades will be pulled in transitively anyways, so I'm sure this will unfortunately lead to some surprises. :/ |
The original issue was in err;
Well, |
Well, you see, you've established that the MonadTransformers as a concept isn't well outlined . So nothing would prevent ValidatedT to be monadic without being considered "MonadTransformer", and have validated semantics on its Parallel instance, without CE having to support it. Reusing the name |
After hours of deliberation with Daniel I have concluded that I definitely think that this maybe works. So let's ship it! 2.4.0 imminent. |
Fixes #3776
This is source breaking, but silently so. Anyone relying on the error-accumulating behavior in the old instance will be surprised by the new one, which will continue to compile on all the same call-sites, but which will exhibit different semantics than previously.