Re-add Kleisli instances for Sync, Async and Concurrent

[alexandru]

Instances for cats.data.Kleisli where initially added but then removed in #69 due to Kleisli not being stack safe for left associative binds.

There is now a still opened issue in the Cats repo at: typelevel/cats#1733

In this commit I'm providing lawful instances with a non-broken flatMap, the same thing I did for StateT.

However as mentioned in my comment on that issue, the problem with this solution is that the Sync instance now lacks coherence with Kleisli's own Monad implementation.

Therefore the plan is to also push these changes in the Cats repository, as the problem is fixable by essentially triggering the F[_] context earlier than Kleisli#run, so the flatMap could look like this, which is what we are doing:

def flatMap[A, B](fa: Kleisli[F, R, A])(f: A => Kleisli[F, R, B]): Kleisli[F, R, B] =
  Kleisli[F, R, B] { r =>
    F.now(r).flatMap(r => fa.run(r).flatMap(f.andThen(_.run(r))))
  }

And without this, then Kleisli simply cannot be fixed with its current A => F[B] encoding, because you can't avoid growing the stack on flatMap without suspending execution in F[_], if the underlying F[_] is capable.

So the plan is to push this in cats-effect, then open a PR in Cats and when merged then the two implementations will become coherent.

---

Note that if we don't push this in cats-effect, then we have to either change the laws, or remove the StateT instance, because StateT is also stack unsafe on left associative binds, see #139

And to be realistic about expectations, Cats is at 1.0.1 so Kleisli won't have a changed encoding at least until Cats 2.0 and this proposed solution is reasonable for everybody.

---

Update

We now have PRs in Cats as well:

• for Kleisli: Fix #1733: make Kleisli.flatMap stack safe cats#2185
• for StateT: Fix #2186: make IndexedStateT stack safe cats#2187

When these get merged in Cats, then the cats-effect instances will become coherent.

[codecov-io]

Codecov Report

Merging #144 into master will increase coverage by 0.14%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   89.97%   90.11%   +0.14%     
==========================================
  Files          47       47              
  Lines         978     1002      +24     
  Branches       82       78       -4     
==========================================
+ Hits          880      903      +23     
- Misses         98       99       +1

[rossabaker]

I would like this for Frameless. After the Cats change, I am in favor.

The debate is whether it's worth the incoherence if the changes are rejected in Cats. And maybe we can lazily avoid that debate by seeing how the ticket is received in Cats. So I'm in favor of initially focusing the discussion there.

[alexandru]

Thanks @rossabaker - we'll see how the debate goes there and then we can decide here.

It's a tricky business for cats-effect. The laws getting violated have to do with repeated left binds and maps, but when speaking of stack safety we are more concerned about repeated right binds, which are OK for Kleisli. And the problem is that I discovered the same problem for StateT as well and fixed it and now we have the coherence issue for StateT too.

So lets resume the discussion here after we see what happens with those issues in the Cats core repository.

[rossabaker]

What duplication is this referring to? And we're already on cats-1.0.

[alexandru]

TBH I don't remember 😄 I think I copied this from version 0.5, from where I imported AndThen

[alexandru]

I removed it.

[alexandru]

Given that PR typelevel/cats#2185 is now merged and going to be available in Cats 1.1, I think it's now safe to merge this in cats-effect.

We are only doing F.suspend(f) instead of F.pure(a).flatMap(f), which is a simple, but coherent optimization.

[LukaJCB]

👍