Add error-handling methods to EitherT#2237
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2237 +/- ##
==========================================
- Coverage 95.05% 94.98% -0.08%
==========================================
Files 333 336 +3
Lines 5789 5846 +57
Branches 211 218 +7
==========================================
+ Hits 5503 5553 +50
- Misses 286 293 +7
Continue to review full report at Codecov.
|
|
We could add redeem and |
| * | ||
| * @see [[upcastR]] | ||
| */ | ||
| @inline def upcastL[AA >: A]: EitherT[F, AA, B] = |
There was a problem hiding this comment.
can we call this leftWiden to mirror widen which we normally use here?
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Bifunctor.scala#L54
| * | ||
| * @see [[upcastL]] | ||
| */ | ||
| @inline def upcastR[BB >: B]: EitherT[F, A, BB] = |
Not sure what you mean, I see there is a I renamed |
|
@johnynek actually I removed |
|
Sorry about the lack of depth on my last comment, was on mobile and on a train. We can create some When cats 2.0 comes around, we can then move them into their respective type classes :) |
|
@LukaJCB that's cool, but the syntax isn't the problem solved here — The real win is the ability to override Related: typelevel/cats-effect#191 |
|
Actually I want to think about this more, given Cats 2.0's timeline I think that's OK, we've got time. Closing it to prevent merging or PR creep, will reopen if the case. |
|
OK, sorry for reopening it — I've carefully considered possibilities, doing benchmarks with alternatives, like a variant that takes a function with an The signature is nice too, it's basically a fold (catamorphism?) that happens in the |
|
Is this PR scheduled to go in for 2.0? If yes can we add the label? |
I hope so 🙂 |
|
Great! |
|
Hey @kailuowang @alexandru, Did we forget to merge this for the 2.0 release? Is it still time? |
|
We didn't forget. Our release plan changed. The original 2.0 release is going to be 2.1, the 2.0 release we are preparing now is different and doesn't allow PRs that break binary compat on Scala 2.11 like this one. |
|
Removing this from the 2.1.0-RC1 milestone in favor of the subset in #3146. I do think we should aim to get the |
armanbilge
changed the title
EitherT
| * | ||
| * @see [[handleErrorF]] for recovering errors thrown in `F[_]` | ||
| */ | ||
| def handleError(f: A => B)(implicit F: Functor[F]): EitherT[F, A, B] = |
There was a problem hiding this comment.
I kind of think this should take a new type parameter for A:
def handleError[A1](f: A => B)... : EitherT[F, A1, B]
Since we know that there is no A left in the result.
| * | ||
| * @see [[handleError]] for recovering errors expressed via `EitherT` | ||
| */ | ||
| def handleErrorF[E](f: E => B)(implicit F: ApplicativeError[F, E]): EitherT[F, A, B] = |
There was a problem hiding this comment.
I don't think we can change this one. How do you make an EitherT(F.pure(Left(a)) into a EitherT(F.pure(Left(a1))) ?
There was a problem hiding this comment.
I guess we can do A1 >: A or something.
| * | ||
| * @see [[handleErrorWithF]] for recovering errors thrown in `F[_]` | ||
| */ | ||
| def handleErrorWith(f: A => EitherT[F, A, B])(implicit F: Monad[F]): EitherT[F, A, B] = |
There was a problem hiding this comment.
Seems this could also benefit from a type parameter A1 in f result type.
| * | ||
| * @see [[handleErrorWith]] for recovering errors expressed via `EitherT` | ||
| */ | ||
| def handleErrorWithF[E](f: E => EitherT[F, A, B])(implicit F: ApplicativeError[F, E]): EitherT[F, A, B] = |
| import cats.data._ | ||
|
|
||
| EitherT.right[String](Option(1)).handleErrorWith((_: String) => EitherT.pure(2)) | ||
| EitherT.right[String](Option(1)).handleErrorWith((_: String) => EitherT.pure[Option, String](2)) |
There was a problem hiding this comment.
@johnynek note that adding an A1 to handleErrorWith seems to break inference. So this would be source-breaking wrt the old implicit syntax way.
| { | ||
| implicit val me: MonadError[EitherT[Option, String, *], Unit] = | ||
| EitherT.catsDataMonadErrorFForEitherT[Option, Unit, String] | ||
| EitherT.right[String](Option(1)).handleErrorWith((_: Unit) => EitherT.pure(2)) | ||
| } |
There was a problem hiding this comment.
Now that EitherT implements handleErrorWith this would no longer work. However, for these sorts of use cases there is the better alternative handleErrorWithF.
8 participants
Addresses #2161 by adding
redeemandredeemWithtoMonadError.These are derived by
attempt.mapandattempt.flatMaprespectively:As mentioned in #2161, this represents an optimization on usage of
attemptbecause for certain datatypes (e.g.IO,Task,Future):Eitherboxingmap/flatMapoperationThe benchmark results in that issue speak for themselves and note that I've measured
IO/Task, which have fairly efficientmapandflatMapoperations. For a data type likeFutureworking withattemptfor error recovery is a disaster.My use-case for
redeem/redeemWithis inmonix.tail.Iterantthat now makes heavy use ofattemptfor error recovery. This is a streaming abstraction, soattempt.flatMapoperations can pile up and add quite significant overhead.Given that we have
Futureinstances provided by Cats, it would have been a shame to not also provide optimizedredeemWithandattemptoperations — because a new, abstractFuture#transformWithhas been introduced in Scala 2.12 as the base function for all other operations (e.g.flatMap,recoverWith).The difficulty here is to keep compatibility with Scala 2.11- so this commit introduces
FutureShimsprovided with different implementations for different Scala versions. Compatibility is thus maintained even with Scala 2.10.I also went ahead and enhanced
EitherTwith the following operations, straight on the class definition itself:widenleftWidenattemptattemptFhandleErrorhandleErrorFhandleErrorWithhandleErrorWithFrecoverFrecoverWithrecoverWithFredeemredeemFredeemWithredeemWithFSo given
EitherT[F, A, B], the functions suffixed withFoperate on anyF: MonadError[F, E]in scope, thus recovering fromEerrors thrown in theF[_]context.Two reasons for it:
Fsuffixed methods are provided because working with the providedMonadError[EitherT[F, L, ?], E]instance sucks (see make sure that EitherT MonadError syntax works the old way #2029)On performance, the JVM is able to do some limited escape analysis to avoid short-term allocations in certain situations and the garbage collector is usually good at dealing with short-term junk, but irregardless, the JVM has a limited budget for optimizations and the way to think about it is that obvious optimizations at some call sites detract from doing obvious optimizations at other call-sites.
In order to optimize memory allocations and the byte-code of those functions, I've rewritten some of them to do more explicit pattern matching, with a default
cast _ =>branch.Benchmarks are a marketing scheme and an implementation that's easier on memory and possibly performance beats an implementation that's elegant. Libraries don't have to be elegant, libraries have to be efficient such that the user's code can be elegant.
Have also written ScalaDocs — which triggered the Scalastyle check for "maximum file size", which I had to disable completely, since it makes no sense to have a restriction that discourages the writing of ScalaDocs.
Unfortunately using a provided
MonadError[EitherT[F, L, ?], E]that's explicitly imported in scope no longer works for those functions.E.g. this sample will trigger an error, hence that particular test is now gone:
This is one reason for why I don't like type-class driven syntax. Such overloads should not be possible or desirable, being much worse than Java-driven overloads which I also prefer to avoid.
Another argument for doing this is that the previous implementation had inconsistent definitions for
recoverandrecoverWithstraight on thecase classdefinition, but not forhandleErrorandhandleErrorWith. So you could do the above forhandleError, but not forrecover.This means that this PR has a source incompatibility with 1.0.0, but this is not a binary incompatibility 😉 and is in my opinion a valid fix.
We've got the following incompatibilities:
Don't know what the current binary compatibility policy is, but due to adding methods with default implementations on traits, these are not backward compatible on Scala 2.11, because it targets Java 7, which doesn't support interfaces with "default implementations".
Scala 2.12 doesn't have such problems though.
But I'm guessing that due to this change, such a PR can only target Cats 2.0, is that correct?