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

Issue #1316: Add the MonadDefer type-class #1552

Closed
wants to merge 7 commits into from
Closed

Issue #1316: Add the MonadDefer type-class #1552

wants to merge 7 commits into from

Conversation

alexandru
Copy link
Member

@alexandru alexandru commented Mar 9, 2017

This pull request is about issue #1316. We are adding one new type class:

MonadDefer: for monadic types which are necessarily for lazy, as it repeats side-effects on each evaluation and have a stack safe flatMap. Implementing types: cats.Eval, IO, Task, etc.

This is needed for abstracting over applicatives and monads that can capture effects (e.g. Eval, Task) and libraries are doing their own thing:

  1. FS2 has Suspendable
  2. Doobie has Capture
  3. Monix is evolving to just do pure(()).map(_ => a) and pure(()).flatMap(_ => fa) and hope for the best

Please checkout the described laws.

UPDATE: Originally this PR was introducing a second type class which was removed, leaving the description here for historical purposes:

ApplicativeEval, which would add an eval operation that lifts values and evaluates side-effects in the F[_] context and is optionally lazy. This type-class does not require implementing types to be lazy, therefore it can be implemented for eager evaluation as well: Try, Future, etc.

See the below discussion for why ApplicativeEval was removed.

@alexandru alexandru changed the title Fixes #1316: add ApplicativeEval and MonadDefer typeclasses Issue #1316: Add ApplicativeEval and MonadDefer type-classes Mar 9, 2017
@kailuowang
Copy link
Contributor

I will dive into the code later, but meanwhile, do you think adding a brief doc here https://github.com/typelevel/cats/tree/master/docs/src/main/tut/typeclasses with some example use cases to demonstrate the motivation would be helpful?
Of course that doesn't necessarily have to happen in this PR.

@tpolecat
Copy link
Member

tpolecat commented Mar 9, 2017

@alexandru will you be at NEScala? I'd like to get a group together to discuss this. I don't think it should be part of cats because fs2 has its own and scalaz doesn't have one. Would be nice for this to be its own thing so we can all use it once and for all.

@alexandru
Copy link
Member Author

alexandru commented Mar 9, 2017

@tpolecat FS2 has its own Monad too and this MonadDefer is basically their Suspendable, with the novelty that ApplicativeEval has less requirements. In building this, I took a look at both FS2 and Doobie. Should be in Cats because it is foundational IMO.

Yes, I'll probably be at NEScala - tried to submit a talk too, but some fine folks came ahead of me :-)

*
* Alias for `eval(always(a))`.
*/
final def delay[A](a: => A): F[A] = eval(always(a))
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be final? Many types, have their own implementation of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

eval and delay are redundant, imo we should have either one or the other. I added delay basically for familiarity purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but it is a shame to have to box to Eval just to get a method Task already has.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would have preferred eval to be described in terms of delay. This can go either way.

* Lifts any value into the `F[_]` applicative context, where the
* evaluation is controlled by [[Eval]] and can be optionally lazy.
*/
def eval[A](a: Eval[A]): F[A]
Copy link
Contributor

Choose a reason for hiding this comment

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

what about fromEval?

Copy link
Contributor

Choose a reason for hiding this comment

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

would pure(a).map(_.value) be a legit implementation of this? If not, can you add a comment to help people understand the purpose of the type.

If this is not legit, can we point to exactly what law rules it out?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there's consensus, sure, though I like eval because it's just one verb.


trait ApplicativeEvalLaws[F[_]] extends ApplicativeLaws[F] {
implicit override def F: ApplicativeEval[F]

Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need something like:

var i = 0
val asF = F.eval(later { i += 1; i })
i == 0 // we have not incremented i yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because the purpose of ApplicativeEval is evaluation in the F[_] context, but it doesn't necessarily have to be lazy, such that Try and Future can implement it.

What you're thinking of is the other type class called MonadDefer.

val lh = F.delay(tr(state1))
val rh = F.pure(state2).map(tr)

lh.flatMap(_ => lh) <-> rh.flatMap(_ => rh)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you comment on the intent of running these effects each twice and comparing them? I'd rather have an effectful thing on one side, and a non-effectful thing on the other so I can more easily read intent.

Copy link
Member Author

Choose a reason for hiding this comment

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

The triggered code is side-effectul and thought it would be a good idea to test that delay(a) is equivalent with pure.map(_ => a) in the presence of side-effects. This doesn't necessarily have to be so. Consider ...

implicit object EvalInstances extends MonadDefer[Eval] {
  def eval[A](a: Eval[A]) = a.memoize
}

And now this law is violated.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it helps to add negative tests to be clear exactly what we are ruling out.

@@ -25,6 +24,9 @@ class TryTests extends CatsSuite {
checkAll("Try", MonadTests[Try].monad[Int, Int, Int])
checkAll("Monad[Try]", SerializableTests.serializable(Monad[Try]))

checkAll("Try[Int]", ApplicativeEvalTests[Try].applicativeEvalWithError[Int, Int, Int])
checkAll("ApplicativeEval[Try]", SerializableTests.serializable(ApplicativeEval[Try]))
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a negative test to show that ApplicativeEval[Try] and MonadDefer[Try] will fail the laws?

I think Try should not be lawful if this typeclass is really representing anything. Since any Applicative can do it if Try can, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also added this law (notice applicativeEvalWithError):

eval(always(throw ex)) == raiseError(ex)

It's an optional law that happens in case F[_] also has an ApplicativeError[A, Throwable]. Mentioning this because Try follows this law, whereas Eval does not.

@tpolecat
Copy link
Member

tpolecat commented Mar 9, 2017

@alexandru but fs2 is not going to add a cats dependency, which means there will need to be another shim to support this. It's propagating the exact problem it's trying to solve.

@alexandru
Copy link
Member Author

@tpolecat I think the problem here is that people want to support both Cats and Scalaz, in order to not upset anybody, I have the same problem in @monix, including my own Monad. It sucks, but eventually one will win.

I think it should be in Cats also because it needs Applicative, ApplicativeError and Monad to describe some laws. I know you're a fan of laws, would appreciate some feedback on those.

But anyway, we can further discuss this at NEScala. I wanted to do this PR for quite some time and finally got it out of my system :-)

@codecov-io
Copy link

codecov-io commented Mar 9, 2017

Codecov Report

Merging #1552 into master will decrease coverage by 0.01%.
The diff coverage is 91.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1552      +/-   ##
==========================================
- Coverage   92.34%   92.33%   -0.02%     
==========================================
  Files         247      250       +3     
  Lines        3907     3940      +33     
  Branches      132      140       +8     
==========================================
+ Hits         3608     3638      +30     
- Misses        299      302       +3
Impacted Files Coverage Δ
core/src/main/scala/cats/MonadDefer.scala 0% <0%> (ø)
core/src/main/scala/cats/Eval.scala 97.36% <100%> (+0.07%) ⬆️
...n/scala/cats/laws/discipline/MonadDeferTests.scala 100% <100%> (ø)
laws/src/main/scala/cats/laws/MonadDeferLaws.scala 94.44% <94.44%> (ø)

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 18c906b...605482f. Read the comment docs.

@edmundnoble
Copy link
Contributor

👍 goddamn it, finally. Maybe this means stack-safety work can continue.

* applicative context, but without the repeating side-effects
* requirement.
*/
@typeclass trait MonadDefer[F[_]] extends Monad[F] with ApplicativeEval[F] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth providing a default instance of MonadDefer implemented as pure(()).map(_ => a) and pure(()).flatMap(_ => fa) for any Monad M? This way we could have a MonadDefer instance even for Id which can be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I've added laws that eval should indeed be equivalent with pure(()).map(_ => a) and defer should be equivalent with pure(()).flatMap(_ => fa).

However Id cannot implement MonadDefer, because for this type-class we are specifying that any side effects should be repeated on each evaluation, with defer building a factory of F[A].

This means that MonadDefer can be implemented for cats.Eval, Task, IO, Coeval, Observable or other monads that handle side-effects, but cannot be implemented for Id. It would fail the specified laws - tried to come up with something meaningful there, would appreciate feedback on those.

Id can implement ApplicativeEval, which doesn't have such a restriction. Try and Future are other candidates for ApplicativeEval and that can't implement MonadDefer. I find MonadDefer cool because it describes laziness.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on Id not having a MonadDefer instance. It wouldn't be really lazy and stackoverflow errors would happen (just got one while trying to use Id as my "simplest MonadDefer monad").

@johnynek
Copy link
Contributor

johnynek commented Mar 10, 2017 via email

@alexandru
Copy link
Member Author

alexandru commented Mar 10, 2017

@johnynek because you took it out :-) See: #1150

Indeed, any Applicative can be an ApplicativeEval, although I've added an optional law for ApplicativeError that says: eval(always(throw ex)) <-> raiseError(ex). This extra law for example addresses this concern: #1150 (comment)

Note that not any Monad can be a MonadDefer. This one specifies laziness.

@johnynek
Copy link
Contributor

johnynek commented Mar 10, 2017 via email

@johnynek
Copy link
Contributor

johnynek commented Mar 10, 2017 via email

@alexandru
Copy link
Member Author

alexandru commented Mar 10, 2017

@johnynek

Why should the Applicative function not be lazy, but Monad be lazy?

I can think of two reasons:

  1. Because monads are used for handling side-effects, as side-effects need ordering / sequencing and the monad is in fact the type-class that models sequencing due to creating a data dependency between the source and its continuation. E.g. Task, IO, Eval, Free are all monads.
  2. We need Monad in order to make MonadDefer lawful, otherwise we can't specify those laws, because we can't ensure ordering of operations. I actually tried specifying those laws with map2 and it doesn't work.

if we want to add a synonym for pure(()).map(_ => a) I guess that's fine

It's not a perfect synonym for pure(()).map(_ => a) because ApplicativeErrorLaws does not specify pure(()).map(_ => throw ex) <-> raiseError(ex).

This type-class specifies that if there is an ApplicativeError[F, Throwable] defined, then exceptions should also get caught in the F[_] context.

@mpilquist
Copy link
Member

Note that in FS2, we optimize pure(a).flatMap(f) patterns in to f(a) to avoid unnecessary trampolines. Hence, pure(()).map(_ => a) is not a safe way to delay evaluation and similarly, pure(()).flatMap(_ => fa) is not a safe way to suspend evaluation.

cats.free.Free does the same optimization, which means you can't safely use Free.suspend for effect capture.

In FS2, we solve this by implementing suspend in a way that's distinct from pure(()).flatMap(_ => fa). The former is never eagerly stepped whereas the latter is always eagerly stepped.

@johnynek
Copy link
Contributor

Okay, now I see more clearly. Can we separate into two PRs possibly?

  1. @alexandru why do we need the ApplicativeEval over catchNonFatal on ApplicativeError: https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/ApplicativeError.scala#L83 ? It seems duplicative to me in that it seems exactly like the law you are proposing is the current implementation. Should we just add more laws? I'm concerned about adding optional laziness. When do I want a type-class to optionally have a behavior? If I need laziness, I probably need it. So why not have that required or not?

  2. The MonadDefer I think it totally fine. If it were me, I would lift the methods from ApplicativeEval into MonadDefer and require that they all be lazy or in the by-name versions have always-like behavior.

I think we are actually talking about two or more concerns here:

a. a typeclass about exception capture into a monad (more than ApplicativeError since we want some laws around throw).
b. one or more typeclasses about deferred evaluation. I think we want Applicative and Monad flavors of this, but we can certainly start with Monad where we are almost sure we want a flavor of this.

@alexandru
Copy link
Member Author

alexandru commented Mar 10, 2017

@johnynek

why do we need the ApplicativeEval over catchNonFatal on ApplicativeError? It seems duplicative to me

It's not. ApplicativeEval is meant to let F[_] evaluate the expression, instead of passing a strict parameter in pure. This means that it abstracts over these functions:

  • Eval.apply (Eval.always)
  • Task.delay (could be Task.apply)
  • Try.apply
  • Future.apply

What do they have in common? I can see 2 things:

  1. All of them take a by-name parameter, which gets evaluated as they see fit
  2. All of them are equivalent with pure(()).map(_ => a)

We had this in Applicative.pureEval but it was removed.

And here I must mention that the Scalaz equivalent Applicative.point takes a by-name parameter and so it doesn't have this problem. In other words, in Scalaz we can abstract over the above apply functions, but with Cats we cannot.

As for ApplicativeError, thecatchNonFatal function I feel that it is missing the point. When you evaluate an expression, you want F[_] to be in charge of the evaluation. What happens next, error handling or not, is up to the user. And I know this is totally subjective, but I'll never use catchNonFatal to abstract over Future.apply simply because it has a scary and long name.
And if these 2 operations really are redundant, then imo catchNonFatal is the one that should go.

@johnynek
Copy link
Contributor

My concern is that these four things are not similar in any way except that they take a by-name parameter.

  1. Eval: does not catch exceptions. Eval.always, later have different semantics. Used to create a trampolined Id[T]
  2. Task: does (I believe) catch exceptions and has always semantics? Eventual execution is async.
  3. Future.apply( ) catches exceptions. In scala futures async execution using an ExecutionContext, has later semantics (not always).
  4. Try.apply catches exceptions except with .now like semantics.
  5. Twitter Future.apply exactly like Try except in a Future (not async, catches exceptions).

So, if we have a typeclass, the idea, to me, is that it has some usable property that I only need to depend on. I don't see what it is here since there are several things it could be:

  1. catching exceptions in evaluation
  2. always-like semantics (side-effects will be re-run).
  3. later-like semantics (we know something will be run 0 or 1 times)
  4. async evaluation

I think we should be clear what we want. But right now we seem to be saying, I want to unify the above 4 properties such that at least one of them is true. I don't see how I can usefully program with that.

@alexandru
Copy link
Member Author

@johnynek Yes, they take a by-name parameter AND they are consistent with pure for pure expressions.

If you're looking that much into it, you might reach the same concerns about map and flatMap. After all Try.flatMap is eager, whereas Task.flatMap is lazy, has always semantics and is possibly async.

At this point I think we should talk use-cases. Here's the kind of code I'm dealing with right now:

sealed abstract class Iterant[F[_], +A] { self =>
  // ..
  final def foldLeftL[S](seed: => S)(op: (S,A) => S)(implicit F: Monad[F]): F[S] = {
    def loop(self: Iterant[F,A], state: S): F[S] = {
      try self match {
        case Next(a, rest, stop) =>
          // Side-effects and exceptions possible here!!!
          val newState = op(state, a)
          rest.flatMap(loop(_, newState))
        case Halt(None) =>
          F.pure(state)
        case Halt(Some(ex)) =>
          throw ex
      }
      catch {
        case NonFatal(ex) =>
          // Closing resources!
          earlyStop.flatMap(_ => (throw ex) : F[S])
      }
    }

    val init = F.eval {
      // Exception handling for the seed
      try seed catch {
        case NonFatal(ex) =>
          earlyStop.flatMap(_ => (throw ex) : F[S])
      }
    }

    init.flatMap(loop(self, _))
  }
}

For this piece of code I don't want to work with ApplicativeError because it would make it unnecessarily restrictive. Without ApplicativeError I can work with Eval just fine. I also don't care much about strict or lazy semantics, or about synchronous versus async evaluation. Maybe I have a use-case for using Future.

The whole point of higher-kinded polymorphism, the way I see it, is the ability to plug-in an F[_] that changes the nature of your operation according to F's capabilities. This sample does that, if only we had an eval.

So how can this operation be described with the current Cats type-classes and what should the solution look like?

@johnynek
Copy link
Contributor

Thank you for taking the time to make a concrete example. I think it helps not talk past one another.

First, Functor and Monad have well defined laws (composition of map and associativity of bind, etc...). While it is true some have lazy semantics, you get no promises and can't rely on it.

To me, it looks like you do want MonadError since you explicitly want to handle the possibility of non fatal exceptions in java/scala code. You also appear to want an "always"-like pure, but maybe you don't care if it is always, but could actually be Now.

Can you explain why you don't want MonadError? You are reimplementing catchNonFatal nearly exactly here.

Lastly, I suspect you may say you don't want MonadError because you want to support Eval, but as your code demonstrates you can nearly hack Eval to make it support MonadError. What is missing is recover. I think we can add a recover node to Eval without hurting performance or stack safety of existing code. As you point out, on the JVM an exception may come at any time, so currently evaluating Eval may throw. It would be nice to have a method like:

Eval.handleErrorWith[A](e: Eval[A])(fn: Throwable => Eval[A]): Eval[A]

If Eval supported MonadError, would you feel that MonadError covered your use case?

By the way, iteratee.io has many very similar cases and seems to use MonadError without it being a problem.

I think there is still the separate suspend use case which must be lazy (or maybe have always semantics). That seems like something we don't have currently. But I still don't see a new typeclass that includes Try, Eval and Task other than Applicative.

Maybe we could have a weaker thing than ApplicativeError where you can raise but not handle errors. Like ApplicativeRaise. Eval and Try can both be ApplicativeRaise with an error type of Throwable, it's just that Eval can't (yet?) handle the errors.

I think we could maybe make a case class TryT[F[_], A](unwrap: F[Try[A]]) that we could use with Eval to make a lazy Try also.

@edmundnoble
Copy link
Contributor

The reason that MonadDefer needs to exist is to abstract over free monad implementations that provide stack-safety through some kind of deferred evaluation. These free monad implementations include Eval, Free[Function0, A], and every kind of Task. Without MonadDefer, it is impossible to safely perform monadic recursion and still abstract over the monad in question. This is the killer usecase of MonadDefer. Injections from Eval are not sufficient to implement MonadDefer. Just try it yourself: given a function that yields a Task[A] and recurses on itself, you need to convert the Task into an Eval so it can be suspended and then back out of Eval to return. That's not possible.

@johnynek
Copy link
Contributor

To be clear, I am all for MonadDefer as long as we can be clear about the laws. There seem to be at least two threads:

  1. we need defer for stack safe recursion
  2. we need defer to control when side-effects occur (i.e. we call some side-effecting method in a defer( ... pure( )).

I don't know if both of those cases should be covered by a single typeclass.

@edmundnoble
Copy link
Contributor

@johnynek My position on this is that we should provide a separate side-effect capture typeclass, potentially in a different PR. I don't think it necessarily needs any superclasses. What we will provide to users there is purely an abstraction that allows people to easily switch side-effect types, by pushing the decision to the call site. This is invaluable for libraries performing side effects that should not need to provide clones of their methods for different cats-compatible effect-capture libraries. I think the only law that is necessarily useful there is that whatever is suspended is not evaluated eagerly.

@alexandru
Copy link
Member Author

Hi guys,

So the current consensus is that we need MonadDefer, but for ApplicativeEval the need isn't immediately clear, because ApplicativeError is fine for the use-cases I had in mind, am I correct?

If this is so, then I'll create another PR with just MonadDefer. I think some of us happen to be at NEScala as well and maybe we'll also talk about this.

@alexandru
Copy link
Member Author

alexandru commented Mar 25, 2017

The history of this issue is interesting, for now I have not created a new PR, just updated this one. Changes:

  • dropped ApplicativeEval from this PR
  • updated the description of this PR
  • removed the "optional" laws for ApplicativeError
  • added law for flatMap stack safety

Please re-review. Thanks!

@alexandru alexandru changed the title Issue #1316: Add ApplicativeEval and MonadDefer type-classes Issue #1316: Add the MonadDefer type-class Mar 25, 2017
@johnynek
Copy link
Contributor

Can Free and FreeT implement this? It seems like they could.

Also I think EitherT can if the F there can implement it (maybe any transformer can, ReaderT, WriterT, ... )

@edmundnoble
Copy link
Contributor

@johnynek Free and FreeT can only implement this if F =:= Function0. Otherwise yeah, transformers preserve it.

@johnynek
Copy link
Contributor

@edmundnoble I don't follow why Free.suspend is not a valid defer. What am I missing?

@edmundnoble
Copy link
Contributor

@johnynek try Free[Id, A]. Then suspend does nothing for stack safety.

@johnynek
Copy link
Contributor

johnynek commented Mar 25, 2017 via email

@johnynek
Copy link
Contributor

johnynek commented Mar 25, 2017 via email

@mpilquist
Copy link
Member

@johnynek Re: Free.suspend and MonadDefer -- #1277

@johnynek
Copy link
Contributor

johnynek commented Mar 25, 2017 via email

@mpilquist
Copy link
Member

I haven't used FreeT so I can't say without investigating. Note that we fixed it in FS2 by introducing a flag on the internal Pure constructor indicating whether it can be eagerly stepped. We could certainly do the same in cats: https://github.com/functional-streams-for-scala/fs2/blob/46a1281813f487064aab5d8ad2747bb299e28eeb/core/shared/src/main/scala/fs2/util/Free.scala#L52-L59

@alexandru
Copy link
Member Author

@johnynek btw, the current definition for catchNonFatal is this:

  def catchNonFatal[A](a: => A)(implicit ev: Throwable <:< E): F[A] =
    try pure(a)
    catch {
      case NonFatal(e) => raiseError(e)
    }

And in the instances defined in Cats, the only override I'm seeing is in FutureInstances. In other words, catchNonFatal really is not what I need, because:

  1. it's an afterthought that people don't override much
  2. I don't want to impose the ApplicativeError restriction when Applicative would be fine with both Scalaz and Haskell

I remember mentioning this before, can't find the comment, but Cats is the only one with the problem because all the others have lazy behavior in pure.

@kailuowang kailuowang added this to the cats-1.0.0 milestone Apr 6, 2017
@kailuowang
Copy link
Contributor

@alexandru
Copy link
Member Author

I have included a Deferrable type-class and an Evaluable type-class in my proposal for the Typelevel Schrodinger project.

This addresses two concerns:

  1. this project does not depend on cats-core and these type-classes do not inherit from Applicative or Monad, hence avoiding MTL issues
  2. the provided laws are describing side-effects and so they are kept out of Cats, where purity is preferred

If everybody agrees, I would like to close this pull request and this new project instead.

@alexandru
Copy link
Member Author

Hello, I feel this issue has reached a deadlock. I don't want this to stop progress on alternative proposals, therefore I'm closing it.

@alexandru alexandru closed this Apr 8, 2017
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants