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

Add MonadError instance for EitherT that recovers from F[_] errors. #1644

Merged
merged 23 commits into from
Jul 24, 2017

Conversation

leandrob13
Copy link
Contributor

@leandrob13 leandrob13 commented May 1, 2017

Adds functions for recovery and mapping functions for context F.

Addressed in #1643

@@ -88,11 +88,22 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {

def map[D](f: B => D)(implicit F: Functor[F]): EitherT[F, A, D] = bimap(identity, f)

def mapF[N[_], D](fe: F[Either[A, B]] => N[Either[A, D]]): EitherT[N, A, D] = EitherT(fe(value))
Copy link
Contributor

@kailuowang kailuowang May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and the leftMapF feels weird to me. The type signature doesn't prevent the user from mapping changing the other side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang I recognize your point. It is mapping F to N. Whatever is inside, could be a Left or a Right (mapped or not) and it is still mapping the Either. If I left the F[] to N[] without mapping the Either inside it would be a transform (F[] ~> N[]) but in EitherT transform is like

def transform[C, D](f: Either[A, B] => Either[C, D])(implicit F: Functor[F]): EitherT[F, C, D] =
    EitherT(F.map(value)(f))

Should it be a transformF?

def transformF[N[_], C, D](fe: F[Either[A, B]] => N[Either[C, D]]): EitherT[F, A, D] = 
  EitherT(fe(value))

Should we make standard what to do in a mapF? In Kleisli it looks like this:

def mapF[N[_], C](f: F[B] => N[C]): Kleisli[N, A, C] =
    Kleisli(run andThen f)

That is why I thought I could add a mapping function from F[] to N[] in EitherT. Also, transform in Kleisli means a transformation from F to N and also maps what is inside of F. Should we fix those semantics in the monad transformers?

Copy link
Contributor

@kailuowang kailuowang May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This transformF sounds like we might need a new higher order type class? like

trait HFunctor[F[_[_], _] {
  def hmap[A[_], B[_], T](fa: F[A, T])(fk: A ~> B): F[B, T]
}

There is some discussion in this gist. It maybe too theoretical, but I think we can see its practial usage here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang I will look into that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang taken your advice I opened #1713 and came to the conclusion to add this to EitherT

def transformF[G[_]](f: FunctionK[F, G]): EitherT[G, A, B] = EitherT(f(value))

There is something confusing about EitherT and OptionT using transform as a bimap/map of the parametrized types respectively. I called it transformF just to avoid the confusion. The issue is opened for the evaluation of your proposal and to address the confusion around transform semantics.

@@ -88,11 +88,22 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {

def map[D](f: B => D)(implicit F: Functor[F]): EitherT[F, A, D] = bimap(identity, f)

def mapF[N[_], D](fe: F[Either[A, B]] => N[Either[A, D]]): EitherT[N, A, D] = EitherT(fe(value))

def recoverF[E](pf: PartialFunction[E, Either[A, B]])(implicit ae: ApplicativeError[F, E]): EitherT[F, A, B] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also feels weird to me because you can "recover" to the left side, which is a different semantics from the original recover. I suspect that these two different sets of recovers and recoverWiths should probably best be organized into two different derivations of MonadError instances. That way the code organization might be clearer and more importantly, we might be able to test with MonadError laws.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang that is exactly what I intended, maybe the names should be changed so the semantics are right? The problem is that if you have an val x = EitherT(Future.failed(new Exception("Error"))) you can recover it with x.recoverFWith[Throwable](ex => Future.successfull(Either.left(ex.getMessage))) or if you want with a Right too. I didn't add a MonadError implicitly because Validated doesn't have one.

What do you mean by organizing them into two different derivations of MonadError instances?

As I discussed in #1643, there is a limitation on recovering for monad transformers like EitherT because you can't recover from F (my example is with a Future). My intention is to keep it practical and since EitherT has its own implementation of recover and recoverWith which are used for overriding the corresponding function of its MonadError instace, I thought we could add these functions without including them in MonadError or some other typeclass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as @djspiewak pointed out in #1643, there are two ways to provide a MonadError instance for a EitherT, the existing one ignores the error in F and recovers from the left side of the Either. We can have a new MonadError instance if theF already has MonadError instance and delegate to that, which is basically what you are doing with recoverF. These two instances might be able to coexist since the Error type is different, which should be true as long as the E in F: MonadError and the EitherT's left type are different, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang right! Let me work it out then, thanks!

@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #1644 into master will increase coverage by 1.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
+ Coverage   94.17%   95.31%   +1.13%     
==========================================
  Files         256      265       +9     
  Lines        4207     4288      +81     
  Branches       93       85       -8     
==========================================
+ Hits         3962     4087     +125     
+ Misses        245      201      -44
Impacted Files Coverage Δ
core/src/main/scala/cats/data/EitherT.scala 98.3% <100%> (+1.78%) ⬆️
.../src/main/scala/cats/data/ReaderWriterStateT.scala 95.78% <0%> (-0.09%) ⬇️
core/src/main/scala/cats/Traverse.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Cokleisli.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/list.scala 100% <0%> (ø) ⬆️
...c/main/scala/cats/laws/discipline/ArrowTests.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/Show.scala 100% <0%> (ø) ⬆️
laws/src/main/scala/cats/laws/ArrowLaws.scala 100% <0%> (ø) ⬆️
...c/main/scala/cats/laws/discipline/SplitTests.scala
core/src/main/scala/cats/arrow/Split.scala
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99b543b...839f1be. Read the comment docs.

@leandrob13 leandrob13 changed the title Add recoverF, recoverFWith, mapF and leftMapF for EitherT Add recoverF, recoverFWith, transformF for EitherT May 30, 2017
@leandrob13
Copy link
Contributor Author

@djspiewak @kailuowang just to validate, I ended up defining the new MonadError instance for EitherT as:

implicit def catsDataMonadErrorFForEitherT[F[_], E, L](implicit FE0: MonadError[F, E]): MonadError[EitherT[F, L, ?], E] =
    new EitherTMonadErrorF[F, E, L] { implicit val F = FE0 }

private[data] trait EitherTMonadErrorF[F[_], E, L] extends MonadError[EitherT[F, L, ?], E] with EitherTMonad[F, L] {
  implicit val F: MonadError[F, E]

  def handleErrorWith[A](fea: EitherT[F, L, A])(f: E => EitherT[F, L, A]): EitherT[F, L, A] =
    EitherT(F.handleErrorWith(fea.value)(f(_).value))

  override def handleError[A](fea: EitherT[F, L, A])(f: E => A): EitherT[F, L, A] =
    EitherT(F.handleError(fea.value)(f andThen(Right(_))))

  def raiseError[A](e: E): EitherT[F, L, A] = EitherT.left(F.raiseError(e)) // I am not so sure about this one

  override def recover[A](fla: EitherT[F, L, A])(pf: PartialFunction[E, A]): EitherT[F, L, A] =
    fla.recoverF(pf)

  override def recoverWith[A](fla: EitherT[F, L, A])(pf: PartialFunction[E, EitherT[F, L, A]]): EitherT[F, L, A] =
    fla.recoverFWith(pf.andThen(_.value))
}

@kailuowang
Copy link
Contributor

@leandrob13 I was thinking you don't need to override methods, and thus you don't need recoverF.

private[data] trait EitherTMonadErrorF[F[_], E, L] extends MonadError[EitherT[F, L, ?], E] with EitherTMonad[F, L] {
  implicit val F: MonadError[F, E]

  def handleErrorWith[A](fea: EitherT[F, L, A])(f: E => EitherT[F, L, A]): EitherT[F, L, A] =
    EitherT(F.handleErrorWith(fea.value)(f(_).value))

  def raiseError[A](e: E): EitherT[F, L, A] = EitherT(F.raiseError(e)) //shouldn't be left or right here I think, `F.raiseError(e)` should return a `F[Either[L, A]]`.   
}


Functor[EitherT[ListWrapper, String, ?]]
Applicative[EitherT[ListWrapper, String, ?]]
Monad[EitherT[ListWrapper, String, ?]]
MonadTrans[EitherT[?[_], String, ?]]

checkAll("EitherT[ListWrapper, String, Int]", MonadErrorTests[EitherT[ListWrapper, String, ?], String].monadError[Int, Int, Int])
checkAll("EitherT[Option, String, Int]", MonadErrorTests[EitherT[Option, String, ?], Unit].monadError[Int, Int, Int])
checkAll("MonadError[EitherT[List, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[ListWrapper, String, ?], String]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to law tests the new instance.

Copy link
Contributor Author

@leandrob13 leandrob13 Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang I really did that by copying and it seems to give the coverage to the new MonadError instance. If it is not this way, can you please give me a hint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang I still think that it is useful to have the recoverF/With functions but if it is a blocker for the PR to not be approved then I could remove them and keep the new MonadError instance and the transformF function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize, @leandrob13, I meant serializable test the instance. Not sure what I was thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang no problem, got it!

@@ -91,6 +92,14 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {

def map[D](f: B => D)(implicit F: Functor[F]): EitherT[F, A, D] = bimap(identity, f)

def transformF[G[_]](fe: FunctionK[F, G]): EitherT[G, A, B] = EitherT(fe(value))
Copy link
Member

@LukaJCB LukaJCB Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In accordance with the corresponding method on Kleisli or ReaderT being called only transform, maybe we should name it this way also? See #1724

Edit: I see now, that EitherT already has a function called transform, so we might need to find a different solution

@kailuowang
Copy link
Contributor

I am thinking that the MonadError instance change is straightforward, the transform thing seems a more complex problem. How about we separate the transform part out of this PR?

@leandrob13
Copy link
Contributor Author

@LukaJCB @kailuowang agreed. I didn't call it transform because there is one already defined for EitherT, so I agree to tackle this issue in another PR so we can agree on the standard semantics for transform along MonadT's.

@leandrob13
Copy link
Contributor Author

@kailuowang BTW, the build is only failing for scala 2.10 and I don't know how to fix it.

@LukaJCB
Copy link
Member

LukaJCB commented Jun 9, 2017

Looks like you have diverging implicits. Don't have time to look in detail right now, but can you build the tests with the -Xlog-implicits flag?

@leandrob13
Copy link
Contributor Author

@LukaJCB sure thing, thanks!

@leandrob13 leandrob13 changed the title Add recoverF, recoverFWith, transformF for EitherT Add recoverF, recoverFWith for EitherT Jun 10, 2017
@leandrob13 leandrob13 changed the title Add recoverF, recoverFWith for EitherT Add MonadError instance for EitherT that recovers from F[_] errors. Jun 17, 2017
@leandrob13
Copy link
Contributor Author

@kailuowang This (et: EitherT[F, L, R]).recover[E] is with the help of the applicative error syntax, right? What i actually intended is for it to be straight forward in the DataType. And yes, there can be that problem with EitherT[Future, Throwable, ?] but this recoverF and recoverFWith functions are property of the EitherT. There is no complete implementation of the MonadError functions natively in any data type that has an instance of it (like future or option). What I intended was to have it natively as part of the features of the EitherT since it was a necessity detected from practice.

I see the need of the extra MonadError instance but I think it will be useful. Another example is that the recover and recoverWith functions on the MonadError instance that recovers from the Either are overriden for the original implementation of EitherT's recover and recoverWith, that is why I thought I could do the same.

What do you think? I just wish this things could be done more straight forward in the data types instead of having a syntax imported for something that should be native. I understand in cases like option and either that are not part of cats, but for those that we have control we can provide that straightforwardness. My argument was similar for Kleisli recovery too, given that we had to define an alias for the Kleisli type in order to get the MonadError instance to work.

I can leave the PR only about the addition of the new ME instance but I hope you can consider these arguments.

@kailuowang
Copy link
Contributor

kailuowang commented Jun 19, 2017

@leandrob13 IMO, in a canonical FP way, when you use an EitherT, you shouldn't even need to know this concrete type, instead, all you need to know is that it is MonadError.
For example, say you have method that takes an EitherT,

def f[F[_], A, B](e: EitherT[F, A, B]):  XXX

If you don't care about the left side, then you don't have to take an EitherT right? You could just do this, making the type more generic (and thus parametric, explained in detail here)

def f[F[_]: Monad, T](e: F[T]): XXX

Now the only reason you care about the left side is that you want to handle the error, in which case, you can do

def f[F[_], E, T](e: F[T])(implicit F: MonadError[F, E]): XXX

This signature is telling everyone that there is only one way to handle the error from F, that is the MonadError instance. They don't need to learn what's native on F. More importantly, there is only one possible error sencrio you need to handle. If we have two sets of methods of error handling on F (one native, one provided by the MonadError), then it will be significanlty more complex to deal with. Are you going to handle the error on the EitherT or the F or both?
In my projects, whenever I was given a Future[Either[E, T] (usually by a 3rd party library), I converts it into an EitherT[Future, E, T] with something like the following

 def throwableToE(t: Throwable) : E = ??? //convert a throwable to my own error type
 def from[T](future: Future[T]): EitherT[Future, E, T] =
   EitherT(f.recover {
        case e: Throwable =>throwableToE(e).asLeft[T]
   })

Then from this point on, if we stick with the never-throw-exception FP paradigm, we don't have to worry about the Future errors. Actually, in my projects I would convert Future immediately into a Task or IO, but that's a different topic.
Maybe what we should also add is a factory method on the EitherT companion method (obviously doesn't have to be in this PR):

private[data] final class FromMonadErrorPartiallyApplied[E](val dummy: Boolean = true) extends AnyVal {
    def apply[F[_], A, B](fa: F[Either[A, B]])(recover : E => A)(implicit F: MonadError[F, E]): EitherT[F, A, B] =
     EitherT(fa.handleError(fa)(e => recover(e).asLeft[B] ))
}
def fromMonadError[E] : FromMonadErrorPartiallyApplied[E] 

Then from this point on we don't have to worry about the inner error on F.

@leandrob13
Copy link
Contributor Author

@kailuowang understood and I like the last idea. I will delete those and leave only the new instance. Thanks for taking the time to explain.

@@ -483,9 +483,13 @@ private[data] abstract class EitherTInstances1 extends EitherTInstances2 {
new EitherTBitraverse[F] {
val F0: Traverse[F] = F
}

implicit def catsDataMonadErrorFForEitherT[F[_], E, L](implicit FE0: MonadError[F, E]): MonadError[EitherT[F, L, ?], E] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: would you add a comment doc here to indicate that this instance delegate to the MonadError of F instead of using Either?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kailuowang sure, I'm on it.


Functor[EitherT[ListWrapper, String, ?]]
Applicative[EitherT[ListWrapper, String, ?]]
Monad[EitherT[ListWrapper, String, ?]]
MonadTrans[EitherT[?[_], String, ?]]

checkAll("EitherT[ListWrapper, String, Int]", MonadErrorTests[EitherT[ListWrapper, String, ?], String].monadError[Int, Int, Int])
checkAll("EitherT[Option, String, String]", MonadErrorTests[EitherT[Option, String, ?], Unit].monadError[String, String, String])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another nitpick, would you add a comment indicating that this is testing the catsDataMonadErrorFForEitherT and the above one is testing the catsDataMonadErrorForEitherT?

@kailuowang
Copy link
Contributor

Thanks so much for this valuable contribution and being so patient with me.

@leandrob13
Copy link
Contributor Author

@kailuowang thanks! No problem, I am learning a lot with this conversations.

checkAll("EitherT[ListWrapper, String, Int]", MonadErrorTests[EitherT[ListWrapper, String, ?], String].monadError[Int, Int, Int])
checkAll("MonadError[EitherT[List, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[ListWrapper, String, ?], String]))
// Tests for catsDataMonadErrorFForEitherT instance, for recovery on errors of F.
checkAll("EitherT[Option, String, String]", MonadErrorTests[EitherT[Option, String, ?], Unit].monadError[String, String, String])
checkAll("MonadError[EitherT[Option, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[Option, String, ?], Unit]))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should separate these MonadError tests using Option in a separate section.

{
  // If a MonadError is defined for F
  val eq = ...
  
  checkAll("EitherT[Option, String, String]", ...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterneyens no problem, I can do that.

@leandrob13
Copy link
Contributor Author

@peterneyens completed your request.

@kailuowang
Copy link
Contributor

@peterneyens got some time to re-review this one?

Copy link
Contributor

@edmundnoble edmundnoble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.

checkAll("EitherT[ListWrapper, String, Int]", MonadErrorTests[EitherT[ListWrapper, String, ?], String].monadError[Int, Int, Int])
checkAll("MonadError[EitherT[List, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[ListWrapper, String, ?], String]))
// Tests for MonadTrans instance.
checkAll("EitherT[ListWrapper, String, Int]]", MonadTransTests[EitherT[?[_], String, ?]].monadTrans[ListWrapper, Int, Int])
checkAll("MonadTrans[EitherT[?[_], String, ?]]", SerializableTests.serializable(MonadTrans[EitherT[?[_], String, ?]]))
}

{
//if a Monad is defined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just noticed here, should it be `if a MonadError is defined"?

checkAll("EitherT[ListWrapper, String, Int]]", MonadTransTests[EitherT[?[_], String, ?]].monadTrans[ListWrapper, Int, Int])
checkAll("MonadTrans[EitherT[?[_], String, ?]]", SerializableTests.serializable(MonadTrans[EitherT[?[_], String, ?]]))
}

{
//if a Monad is defined
// Tests for catsDataMonadErrorFForEitherT instance, for recovery on errors of F.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but should we say "if a MonadError is defined"?

Copy link
Collaborator

@peterneyens peterneyens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart of the comment (which @kailuowang just noticed before me as well) 👍

Thanks @leandrob13!

@leandrob13
Copy link
Contributor Author

@peterneyens @kailuowang done with the comment fix. Thanks guys.

@kailuowang
Copy link
Contributor

merge with 3 sign-offs

@kailuowang kailuowang merged commit ad17ffc into typelevel:master Jul 24, 2017
@kailuowang kailuowang added this to the 1.0.0-MF milestone Jul 24, 2017
@tg44 tg44 mentioned this pull request Nov 26, 2019
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.

6 participants