Fix #1733: make Kleisli.flatMap stack safe

[alexandru]

Close #1733.

This is badly needed in cats-effect in order to define a Sync[Kleisli[F, R, ?]] implementation.

I'm already pushing one right now, but with a patched flatMap implementation that would no longer preserve coherence with the official flatMap of Kleisli. So it would be nice if both these PRs happen, see: typelevel/cats-effect#144

The way to fix it is to define a flatMap implementation that triggers the F[_] context earlier. Like this:

def flatMap[C](f: B => Kleisli[F, A, C])(implicit F: Monad[F]): Kleisli[F, A, C] =
  Kleisli(a => F.pure(a).flatMap(a => F.flatMap[B, C](run(a))((b: B) => f(b).run(a))))
  //                     ^^^^^^^                      ^^^^^^
  //              Suspended execution in F[_], no longer increases the stack size

Unfortunately this solution doesn't work directly because we have a FlatMap restriction on that operation, not Monad. And so the trick is to discriminate between plain FlatMap and Monad via inheritance and the subtyping encoding that we're all doing, this being a subtitute for Kleisli.apply:

private[data] def shift[F[_], A, B](run: A => F[B])
  (implicit F: FlatMap[F]): Kleisli[F, A, B] = {

  F match {
    case ap: Applicative[F] @unchecked =>
      Kleisli(r => F.flatMap(ap.pure(r))(run))
    case _ =>
      Kleisli(run)
  }
}

And now the flatMap definition can be:

def flatMap[C](f: B => Kleisli[F, A, C])(implicit F: FlatMap[F]): Kleisli[F, A, C] =
  Kleisli.shift(a => F.flatMap[B, C](run(a))((b: B) => f(b).run(a)))

As proven by the tests with Eval, this works. And it will work splendidly with cats-effect.

---

FAQ

In preparation for the upcoming discussions, I can think of the following ...

Is there any other solution possible?

No, this is the only fix possible for Cats version 1.x, given the A => F[B] encoding.

We could end up with a different internal encoding, say F[A => F[B]], but note that such an encoding doesn't guarantee a fix either. In spite of popular belief note that StateT isn't currently stack safe for left binds either, see: typelevel/cats-effect#139

Doesn't this assume that a stack safe F.pure(a).flatMap(f) is lazy?

Yes, however it is my firm belief that no stack safe flatMap can be defined with strict evaluation, not even when the source is F.pure. This is because you can always define a recursive structure that's already evaluated (in memory) and that triggers stack overflows.

In other words if F[_] really has a stack safe flatMap (that could pass the laws of Sync in cats-effect for example, or that could describe tailRecM), then the behavior of F.pure(a).flatMap(f) is necessarily lazy.

For details and a working example: typelevel/cats-effect#92

Isn't usage of isInstanceOf a hack?

A much more elaborate signature would do something like the Priority pattern in Algebra.

However consider that:

1. we cannot break compatibility
2. this is cleaner, lets not forget the scary CanBuildFrom signatures
3. the cases where this will yield problems (i.e. those cases where the Monad definition comes from a different place than FlatMap) are totally uninteresting; the Scala compiler will pick Monad if in scope because it has a preference for sub-types or in case they happen to have the same "score", it yields a conflict anyway

Most importantly is that this PR solves a problem and without it we cannot solve that problem.

Perfect is the enemy of good 😉

[codecov-io]

Codecov Report

Merging #2185 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2185      +/-   ##
==========================================
+ Coverage   94.75%   94.75%   +<.01%     
==========================================
  Files         330      330              
  Lines        5568     5570       +2     
  Branches      201      204       +3     
==========================================
+ Hits         5276     5278       +2     
  Misses        292      292

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala	97.93% <100%> (+0.04%)	⬆️

---

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 d39b856...47a10a7. Read the comment docs.

[rossabaker]

A couple practical observations:

• I have never been bitten by stack-unsafe Kleisli, and now all our taxes are going up.
• But Sync[Kleisli[F, A, ?]] is useful in practice.

I don't have any benchmarks from real projects, but my gut feeling is that this is a good trade.

[alexandru]

@rossabaker thanks.

As said in the issue that this solves, the stack unsafety is for "left-associated binds", which for our purposes are less important than "right-associated binds", which are those that happen in recursive loops.

I suspect the taxes are going up due to an extra F.flatMap, however if our F is an IO / Task data type, then it's probably not a big deal. If there's concern, I guess we can always do a benchmark.

I would be interested about Kleisli usage in the wild and what data types people use for F.

[rossabaker]

In descending order of F frequency for me: IO, OptionT[IO, ?], Id, Future. Of these, OptionT and Future have the heavy flatMaps. Where I use OptionT, I'm doing much heavier things. And Future is something I'm happy to support but not happy to hold back progress to optimize.

[LukaJCB]

These kinds of type system hacks always make me a bit weary (though tbh, I played around with implementing stack-safe function composition by using a big fat List[Any => Any] 😅), but from what I can see in this case, it seems to make a lot of sense, some quick benchmarks would be nice of course, but I understand if you don't have the time. After thinking about this for a while I'm 👍