Remove FlatMapRec make all FlatMap implement tailRecM

[johnynek]

Looking for comments on this. This addresses #1278

I think all the non-test code compiles. The test code needs to be updated.

After doing this I am more convinced we should remove FlatMapRec. It seems we didn't implement it in many cases, and everywhere it seems it can be implemented safely in cats.

Given that this gives us the chance to avoid blowing the stack, I think it will be a real advance to just require tailRecM and disallow recursive flatMap in the abstract in favor of calling tailRecM.

I will update the tests next.

[ceedubs]

This is interesting. It would definitely be nice to not have to worry about separate Monad and MonadRec forms of certain methods. I'm definitely interested in any examples of a type that supports flatMap but not tailRecM that anyone might be able to come up with.

Should/could we have a helper method to create an "unsafe" (meaning not stack-safe) version of tailRecM given a flatMap method for people who know that they aren't going to hit stack-safety issues on their type and want a Monad instance for it?

[non]

I really like this! 👍

I think lack of stack safety is one of the biggest barriers to using FP techniques in the large. As @johnynek argued, it's not obvious to me that there are many monads we care about that can't be stack-safe (and if a monad can't be stack safe it's not clear to me how you use it correctly in generic code that needs arbitrary monadic recursion).

@ceedubs I would be fine with allowing people to create their own "unsafe instance" as long as we aren't providing them implicitly. This constructor would be safe for types which are internally stack-safe, and unsafe for types which aren't.

[non]

For context: I hit a situation recently where generic code was using flatMap for monadic recursion (assuming that M[_] was stack-safe) and I wanted to be able to use something like Xor[E, ?] or Try with that code. Both of those can totally support tailRecM but since the original code was written in terms of flatMap (to relax the type class constraints) there was nothing to be done.

The argument I'm making is that any time someone wants to express monadic recursion they should have a method to do so, rather than just using flatMap and hoping for the best.

[adelbertc]

I have an issue with this purely on principle - the definition of Monad is well-defined and adding tailRecM muddles this definition. Is the burden that users will unknowingly require Monad/flatMap instead of MonadRec/tailRecM? I think that's on the user in that case as opposed to on us to do a change like this.

EDIT I haven't thought it through completely/tried it, but I think if you wrote an algebra in finally tagless it would not be stack safe. If this PR went through, would something like: https://github.com/adelbertc/scala-sandbox/blob/master/tagless-stacksafety/src/main/scala/taglessStackSafety/TaglessSet.scala still be stack safe? Where the tailRecM impl just delegates to F's. I think I tried it once and it didn't quite work out.

[adelbertc]

OK I did some experimenting, it looks like this unexpectedly blows stack, though it's also possible I'm doing it wrong:

import cats.{Eval, Monad, MonadRec}
import cats.data.{State, Xor}
import cats.implicits._

trait SafeMonadSet[F[_]] extends MonadRec[F] {
  def add(int: Int): F[Unit]
  def exists(int: Int): F[Boolean]
}

object SafeMonadSet {
  def apply[F[_]](implicit F: SafeMonadSet[F]): SafeMonadSet[F] = F

  implicit val stateInterpreter: SafeMonadSet[State[Set[Int], ?]] = new SafeMonadSet[State[Set[Int], ?]] {
    def add(int: Int): State[Set[Int], Unit] = State.modify(_ + int)
    def exists(int: Int): State[Set[Int], Boolean] = State.inspect(_.contains(int))
    def pure[A](x: A): State[Set[Int], A] = State.pure(x)
    def flatMap[A, B](fa: State[Set[Int], A])(f: A => State[Set[Int], B]): State[Set[Int], B] = fa.flatMap(f)
    def tailRecM[A, B](a: A)(f: A => State[Set[Int], Xor[A, B]]): State[Set[Int], B] =
      MonadRec[State[Set[Int], ?]].tailRecM(a)(f)
  }
}

trait SafeTaglessSet[A] {
  def run[F[_]: SafeMonadSet]: F[A]
}

object SafeTaglessSet {
  implicit val instance: MonadRec[SafeTaglessSet] = new MonadRec[SafeTaglessSet] {
    def pure[A](x: A): SafeTaglessSet[A] = new SafeTaglessSet[A] {
      def run[F[_]: SafeMonadSet]: F[A] = SafeMonadSet[F].pure(x)
    }

    def flatMap[A, B](fa: SafeTaglessSet[A])(f: A => SafeTaglessSet[B]): SafeTaglessSet[B] = new SafeTaglessSet[B] {
      def run[F[_]: SafeMonadSet]: F[B] = SafeMonadSet[F].flatMap(fa.run[F])(a => f(a).run)
    }

    def tailRecM[A, B](a: A)(f: A => SafeTaglessSet[Xor[A, B]]): SafeTaglessSet[B] = new SafeTaglessSet[B] {
      def run[F[_]: SafeMonadSet]: F[B] = SafeMonadSet[F].tailRecM(a)(a => f(a).run)
    }
  }

  def add(int: Int): SafeTaglessSet[Unit] = new SafeTaglessSet[Unit] {
    def run[F[_]: SafeMonadSet]: F[Unit] = SafeMonadSet[F].add(int)
  }

  def exists(int: Int): SafeTaglessSet[Boolean] = new SafeTaglessSet[Boolean] {
    def run[F[_]: SafeMonadSet]: F[Boolean] = SafeMonadSet[F].exists(int)
  }
}

And the usage that blows stack:

package taglessStackSafety

import cats.MonadRec
import cats.data.{Xor, State}
import cats.implicits._

object BenchmarkApp extends App {
  val M = MonadRec[SafeTaglessSet]
  def loop(i: Int): SafeTaglessSet[Xor[Int, Int]] =
    if (i == 0) M.pure(Xor.right(i))
    else M.pure(Xor.left(i - 1))

  val x = M.tailRecM(5000)(loop).run[State[Set[Int], ?]].runA(Set.empty[Int]).value
  println(x)
}

[non]

@adelbertc If you need a monad with broken monadic recursion I think @ceedubs' suggestion of providing an unsafe constructor works fine. I would be surprised if many/most monads can't implement stack-safe monadic recursion in one way or another though.

I guess my point is that figuring out which things are (or aren't) stack-safe is subtle and if we can free our users from having to worry about this it can only help the cause of FP in scala. In the same way that @milessabin convinced me to build a trampoline into Eval (which I am now 100% convinced was the right way to do it) I think we should build the ability to do stack-safe monadic recursion into all type classes with flatMap.

(Whether users will choose to do the same or not is an open question.)

What do other folks think?

[johnynek]

There is at least one subtle issue, for any monad you can implement a (possibly stack unsafe) tailRecM, I can't see a way to do that for any FlatMap[M] (you need both flatMap and pure for the naive implementation). It so happens that the only examples of a FlatMap in cats are Map[K, ?] and WriterT examples and there I can implement tailRecM.

We might more consider a conservative approach:

1. move tailRecM up to Monad and not on FlatMap
2. keep FlatMapRec but make Monad extend FlatMapRec.

Since we have so few examples of FlatMap and they all support tailRecM, I'd say let's be aggressive and keep it as it is in this PR.

[johnynek]

looks like the docs failed. Will look at that.

[adelbertc]

Continuing the discussion from here: https://gitter.im/typelevel/cats?at=57aba485ae838f6f569559dd since I'm not sure how often I'll be on Gitter tonight

Finally tagless aside, on principle MonadRec is about stack safety so the stack safety law should come with it - making it separate seems strange to me. Delineating Monad (like for finally tagless) and MonadRec I think is important in that if a function is written against F[_]: MonadRec it is clearly expecting stack-safe recursion. With this change all such functions would become F[_]: Monad which makes non-stack-safe monads get a rude runtime surprise. The distinction is no longer clear. It's certainly true that you could implement a bogus tailRecM for non-stack-safe monads but that would fail the law check as well as now incurring a cost to the caller in having to check if a function they're about to call that's parameterized over a Monad requires stack safety.

[johnynek]

In my view, the current facts are playing into my thoughts:

1. MonadRec is a pain to use with MonadCombine, MonadError, and MonadError is especially common. (implicit m: MonadError[M, T] with MonadRec[M])
2. All the Monad/FlatMap examples in our repo can be made stack safe.
3. We have only one example (which I have not looked at in detail) that is not yet stack safe (the one above).
4. I don't know of any library that guarantees stack safety. They are best efforts where if you use the wrong monad, you lose it all. At least here we will have a method which says: call this if you are recursing and the monad must give its best effort to be stack safe.
5. Even our codebase uses recursive flatMaps (and see more proposed by WIP: add some missing applicative and monad ops #1216 . If guaranteeing stack safety of a call is our main concern, we can't have any of those recursive flatMaps on general monads, nor could we merge WIP: add some missing applicative and monad ops #1216 without moving all the calls to MonadRec.
6. Without all Monads having tailRecM, my fear is that in practice we will have much worse stack safety because no one will want to implement methods twice (note that @tpolecat pushed back on that when suggested here: WIP: add some missing applicative and monad ops #1216). So this is where "ideal" is becoming the enemy of "much better".

To me the only approach that makes sense is one of the two:

1. since virtually all examples we have support tailRecM, move it to Monad and realize that there may be cases where it is not 100% stack safe (but always equivalent to a recursive flatMap implementation).
2. ban any use of recursive flatMaps (or any recursion that may not be stack-safe), including non-stack safe tailRecM, which pushes it to a dedicated trait.

I don't see how we can not do 2 yet argue that 1 is too risky.

[johnynek]

An alternate argument leading to the same place is: cats only supports stack-safe recursive monads. If your monad is cannot be made stack safe with tailRecM, you must trampoline it. So your above example is simple not a supported monad because it is not safe. You must trampoline it or find a trick to implement tailRecM.

Given how important recursion is, I think this is a fine position to take.

[adelbertc]

MonadRec is a pain to use with MonadCombine, MonadError, and MonadError is especially common.

What are the pain points specifically? Just multiple constraints?

We have only one example (which I have not looked at in detail) that is not yet stack safe (the one above).

The example generalizes to any (perhaps naive) implementation of a finally tagless algebra, which I strongly believe is a perfectly valid way to encode EDSLs in Scala. The Cats encoding of type classes already penalizes finally tagless with the ambigous implicits when using type class syntax (as shown in #1210) - this patchset would penalize it even more.

Between 1 and 2 I would vote for 2 which clearly delineates when stack safety is expected and when it is not. Having used tagless final EDSLs for the past year, I explicitly know that it's not stack safe but I'm OK with it both because I know the trees are very small and I don't want the overhead associated with Free. As you said given that most (all?) data types in the stdlib and Cats have lawful implementations of MonadRec I am fine pushing out recursive flatMaps into using MonadRec.

At the bottom level my main concern is this delineation and my perhaps stubborn unwillingness to break laws.

[tixxit]

Neither solution seems to be particularly idealistic to me.

Having stack-unsafe monads means I can create more lawful monads, or at least create them easily, but it does that by greatly restricting the types of programs I can write with them. Worse, this prohibition can't really be encoded into a law. From the Monad laws, I see no reason why I can't do a recursive flatMap (or have some way of doing it). Instead, this is just some runtime implementation detail I need to keep in my head, along with the existence of MonadRec. Given the number of recursive flatMaps there are, even in cats, it seems like this is not an easy thing to keep in mind.

Having safe Monads means we can't (currently?) create some lawful, but stack unsafe monads. At least not without worrying about what code we're breaking because they assume stack-safe monads. However, I'm able to write much more powerful programs with these monads. I also don't need to worry nearly as much about the particular details of the runtime my program will run on.

Anyways, we're making compromises in either case (MonadRec is very leaky, stack-safe tailRecM limits the monads we can implement) - we're just wondering which compromises are easier to live with. I believe having tailRecM in Monad is probably the better world, from an FP in Scala POV (and, selfishly, for my own code).

[travisbrown]

I think I'd prefer a world where there are clear conventions or whatever that allow me as a library author to say "stack safety is BYOB" for some context, and as a library user to know that I'll need to provide my own trampolining or take some other approach if I need to use a monad with un-stack-safe recursive flatMap in that context.

I don't have any real objections to adding tailRecM here, though.

[adelbertc]

Having stack-unsafe monads means I can create more lawful monads, or at least create them easily, but it does that by greatly restricting the types of programs I can write with them.

This is the same reason why we have Apply and FlatMap in addition to the traditional Applicative and Monad. For instance, we have FlatMap[Map[K, ?]], but no Monad. Similarly here many (all?) monadic finally tagless algebras are lawful Monads, but I've yet to figure out how to make them stack safe and therefore a MonadRec.

Having safe Monads means we can't (currently?) create some lawful, but stack unsafe monads. At least not without worrying about what code we're breaking because they assume stack-safe monads.

This is my worry. We could as Oscar suggested make the stack-safety law a separate check, but that law is the essence of MonadRec.

As Oscar said, most/all data types supported by Cats have a valid MonadRec implementation (unless someone decides to use the MTL type classes as standalone EDSLs), so I think shifting any monadic recursion to MonadRec should be fine from a usability point of view. It also clearly delineates when stack safety is expected and when it is not. Stack safety is an additional requirement on top of the clean definition of a Monad which is why I argue it should be another type class. It effectively is an MTL-style type class I believe, along with MonadReader, MonadState, etc.

[non]

I guess the question we need to ask is whether we think the collection of types which cannot participate in MonadRec is larger than the set of types which cannot support a stack-safe flatMap but can support MonadRec?

We could maintain the current monad/rec split, but if we rewrote all of Cats' recursive flatMap calls in terms of tailRecM and MonadRec (and if downstream libraries started doing the same thing) then my guess is that things like finally tagless algebras will have just as much trouble (they would have to produce bogus MonadRec instances or be unable to participate). So my guess is that the same argument against this change would be made against those proposed changes as well.

I'm somewhat biased, since I haven't really used finally tagless algebras (or similar types from the first collection) whereas I often find myself using types from the second collection (e.g. Try). I also take that @johnynek's point (that composing MonadRec with other monad-derived type classes is ugly).

Stack safety is an additional requirement on top of the clean definition of a Monad...

This is definitely the crux of the matter. I'm not convinced that basing a type class on a clean-but-dangerous definition is the right way to go. For Monoid we usually like to have an aggregate adder (which can be more efficient, minimize error, etc) as well as a simple combine operation. I think there's an analogy to monads and monadic recursion.

If most people prefer to keep the separation, I could imagine another design that trades MonadRec for something that just signals that you can do monadic recursion, but isn't itself a monad. For example:

trait Recurse[F[_]] {
  def recurse[A, B](a: A, f: A => F[A Xor B])(implicit m: Monad[F]): F[B]
}

The (small) advantages of this approach are that you don't have a type class dependency diamond, and that you can more easily specify this as a constraint:

def applicationLogic[F[_]: MonadEmpty: Recurse](...) = ...

[adelbertc]

I also take that @johnynek's point (that composing MonadRec with other monad-derived type classes is ugly).

What are the ugly points?

If it's a similar case to #1210 (comment) I think we should aim to solve that more general problem instead of this specific one. We agreed to punt on that one , but it sounds like any nastiness with using, say, MonadRec with MonadCombine would be just a specific case of using multiple MTL type classes in the same scope.

As for your Recurse example, it sounds like a similar thing was done in scalaz with LiftIO and MonadIO. LiftIO in and of itself has no laws as far as I can tell, but I'm guessing perhaps the reason it was written that way was to avoid exactly the issues with MTL style programming in the encoding of type classes.

Apologies if I'm making things difficult, I just want to make sure we don't muddle fundamental type classes without careful discussion, and want to make sure we're solving the right problem instead of a specific instance of a more general one.

[non]

Yes, I think you're right that we should solve that issue in a general way. I don't think that ugliness is the crux of the argument above though.

[non]

The way I see it, there are two different ways we can talk about possible stack-safety:

1. Callee requires evidence of stack-safety. The calling type either has to have a stack-safe type class, fake it and hope for the best, or lift itself into a new type.
2. Callee uses flatMap recursively, requiring flatMap to be trampolined or similar. The calling type either needs to be changed/lifted, or has to hope for the best.

Cats has traditionally done (2). The problem here is that there's nothing in the type system that lets the caller know they need a stack-safe monad -- and even if documented, it's easy for upstream callers to fail to pass the documentation along.

The proposal here (within Cats at least) is to move to (1), either by adding this requirement to all Monad type classes or by aggressively adding something like Recurse within the library and forbidding recursive flatMap calls in Cats. Either way, this makes it harder to write broken code, and makes it more obvious when there will be a problem (using non-stack safe type class with something that needs safety).

It seems like either of these options will inconvenience types that are currently sneaking through under (1) though.

How do other people feel? Am I exaggerating the scope of the problem?

[adelbertc]

We could maintain the current monad/rec split, but if we rewrote all of Cats' recursive flatMap calls in terms of tailRecM and MonadRec (and if downstream libraries started doing the same thing) then my guess is that things like finally tagless algebras will have just as much trouble (they would have to produce bogus MonadRec instances or be unable to participate). So my guess is that the same argument against this change would be made against those proposed changes as well.

So going back to @johnynek 's two proposals (as I understand them):

1. Keep current split and move all monadic recursion into MonadRec.
2. Merge them together.

In the former case, users of Cats which have non stack-safe can still opt-in to MonadRec-y things, but that is on them and they (should) know that they are lying about the instance. And I think that's OK, we can't control what instances users write - if they want to break ap-bind consistency whatever, but inside Cats a) all instances we provide are lawful b) the separation gives users the option of opting out of certain "features."

In the latter case everything lives in Monad which may look cleaner and improve the user experience a bit, but users can no longer write law abiding monads in some cases - there is no opt out mechanism.

EDIT Just saw your second comment:

or by aggressively adding something like Recurse within the library and forbidding recursive flatMap calls in Cats

I vote for this. Specifically I would like to keep the separation and move uses of monadic recursion in Cats itself to be done through MonadRec. Users who have data types that are not stack-safe (which I think we've ascertained is me :P ) and want that stuff (like untilM, whileM, etc) can write their own bogus instances but that's on them.

[adelbertc]

@non Apologies for constantly getting in the way, just want to make sure we arrive at a decision having explored most possibilities.

What is the meaning of Recurse[F] if not to mark specifically a stack-safe Monad[F]?

[non]

@adelbertc The presence of Recurse[F] says that the Monad[F] you have is stack-safe. It's a marker in the sense that you probably aren't actually using it at runtime.

trait Recurse[F[_]]

def mightFailOnRecursion[F[_]: Monad]: F[Unit] = ...

def wontFailOnRecursion[F[_]: Monad: Recurse]: F[Unit] = ...

The law for Recurse[F] requires a Monad[F] too and might look something like this:

val Limit: Int = ... // some large-ish positive value, possibly F-dependent

F.pure(Limit) <-> F.tailRecM(0, n => F.pure(if (n < Limit) Left(n + 1) else Right(n)))

[adelbertc]

Does doing trait Recurse[F[_]] { self: Monad[F] => ... } take away from that? I see it only as being more faithful to that - it ensures uses of Recurse are tied with Monad while not sitting in the subtype hierarchy.

[non]

@adelbertc Yes, in that it makes it really clunky to use with MonadEmpty, MonadCombine, and so on. This is especially true for derived monads where we already have an explosion of type class instances. Even though Recurse itself isn't in a subtype hierarchy under your prosal, all its instances would have to be, right?

To turn it around, is there a real concern that someone is going to have multiple Monad[F] instances, some of which have stack-safe recursion while others don't?

[adelbertc]

Sorry, I'm not sure I follow where the clunkiness comes in with the self-type versus not having a self-type. Could you show an example with and without the self-type?

[non]

So maybe I'm wrong, but consider any situation where we can derive a monad in some cases but not others. These are situations where different amounts of evidence allow us to derive different types and we have to use prioritization to avoid implicit ambiguities. In those situations deciding when you can (or can't) add Recurse[F] into the mix will probably add a bunch of extra derivations. At least it seems that way to me.

Since Recurse[F] doesn't actually add any implementations it seems cleaner not to have its derivation tied up in the other derivations, except to the degree necessitated by the evidence. In other words, you can use whatever evidence you needed to get a monad + safe recursion, without worrying about any of the other stuff.

If you wanted to only create one instance, you'd be free to mix Recurse in with Monad (or whatever) but you wouldn't need to do so.

[adelbertc]

Oh I see, essentially for any monad transformer that has the equivalent of like:

implicit def monadOptionT[F[_]: Monad]: Monad[OptionT[F, ?]] = ...

and with Recurse you would also have to do

implicit def monadRecOptionT[F[_]: Monad: Recurse]: Monad[OptionT[F, ?]] with Recurse[OptionT[F, ?]] = ...

Is that what you mean?

I think I can be convinced of this. I would like to hear @tpolecat 's thoughts on this as well though.

[non]

Right, that's it exactly.

[tpolecat]

@adelbertc I can't really weigh in much because I haven't had time to mess with it. My instinct is to not complicate Monad since it's so fundamental and MonadRec only appeared in the last year or so and thinking around it is more volatile (as seen here). But I think if we're going to try this out it's certainly not going to get any easier, so maybe it's worth a go.

[adelbertc]

For the record the combinatorial explosion of instances for Recurse in my suggestion is the usual explosion associated with MTL type classes. I remain hesitant with the proposal and reserve the right to complain moving forward, but I suppose worth investigating since, as stated, the important of stack safety in FP on the JVM is quite important.

[johnynek]

okay, I have updated the PR with what I think the suggestions are here. Please take a look.

[non]

We should remove this comment -- implementations are no longer required to do this.

[adelbertc]

Should this file be renamed?

Also can this be added as a law somewhere as opposed to a separate check?

[johnynek]

I agree, but would it be possible that we follow that up in a next PR since this PR and discussion is already getting pretty large? Refactoring the existing tests might be a good line to draw.

[adelbertc]

Sure, I've created #1283 to track it

[codecov-io]

Current coverage is 90.88% (diff: 93.10%)

Merging #1280 into master will increase coverage by 0.33%

@@             master      #1280   diff @@
==========================================
  Files           243        238     -5   
  Lines          3293       3369    +76   
  Methods        3235       3312    +77   
  Messages          0          0          
  Branches         56         54     -2   
==========================================
+ Hits           2982       3062    +80   
+ Misses          311        307     -4   
  Partials          0          0          

Powered by Codecov. Last update ce70923...61de995

[non]

👍

This looks great to me. Looking forward to following it up with improved laws. Thanks everyone!

[adelbertc]

Can we add a comment saying theres a defaultTailRecM that could be used as a stub/template?

[adelbertc]

Modulo the 1 comment, LGTM 👍

[adelbertc]

We should probably write a note somewhere for these tuts why we also have tailRecM on monad instances. I've added this ticket #1284 to track it

[johnynek]

Merged with the two +1s.

Thanks everyone for working together to compromise. I think this brings a simpler hierarchy and more opportunities in practice for avoiding stack overflows, which I'm really excited about.