-
-
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
Add error-handling methods to EitherT
#2237
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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] = |
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.
rightWiden?
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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] = |
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.
Some A comment
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.
Still need to add tests.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that EitherT
implements handleErrorWith
this would no longer work. However, for these sorts of use cases there is the better alternative handleErrorWithF
.
Addresses #2161 by adding
redeem
andredeemWith
toMonadError
.These are derived by
attempt.map
andattempt.flatMap
respectively:As mentioned in #2161, this represents an optimization on usage of
attempt
because for certain datatypes (e.g.IO
,Task
,Future
):Either
boxingmap
/flatMap
operationThe benchmark results in that issue speak for themselves and note that I've measured
IO
/Task
, which have fairly efficientmap
andflatMap
operations. For a data type likeFuture
working withattempt
for error recovery is a disaster.My use-case for
redeem
/redeemWith
is inmonix.tail.Iterant
that now makes heavy use ofattempt
for error recovery. This is a streaming abstraction, soattempt.flatMap
operations can pile up and add quite significant overhead.Given that we have
Future
instances provided by Cats, it would have been a shame to not also provide optimizedredeemWith
andattempt
operations — because a new, abstractFuture#transformWith
has 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
FutureShims
provided with different implementations for different Scala versions. Compatibility is thus maintained even with Scala 2.10.I also went ahead and enhanced
EitherT
with the following operations, straight on the class definition itself:widen
leftWiden
attempt
attemptF
handleError
handleErrorF
handleErrorWith
handleErrorWithF
recoverF
recoverWith
recoverWithF
redeem
redeemF
redeemWith
redeemWithF
So given
EitherT[F, A, B]
, the functions suffixed withF
operate on anyF: MonadError[F, E]
in scope, thus recovering fromE
errors thrown in theF[_]
context.Two reasons for it:
F
suffixed 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
recover
andrecoverWith
straight on thecase class
definition, but not forhandleError
andhandleErrorWith
. 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?