Fix #2186: make IndexedStateT stack safe

[alexandru]

Close #2186.

This is very similar in spirit with the PR #2185 that fixes Kleisli, but this one is for IndexedStateT and with a different solution.

For IndexedStateT instead of piggybacking on an F: Monad for the stack safety of repeated left binds, this time we are introducing an AndThen implementation.

AndThen was first introduced in cats-effect for describing functions that are stack safe in andThen and compose, the original cats.effect.IO implementation being based on it.

Introduced as an internal helper (thus not exposed to the public), this is now used to provide stack safety for IndexedStateT.

Note that an implementation similar to that of PR #2185 is also possible, deferring this responsibility to F[_], except that this version proposed here works for any F[_] when it comes to the stack safety of left associative binds.

---

Benchmarks

I've added a benchmark (see StateTBench) that I used to compare the new implementation with master. The results are:

Benchmark	Master	This PR	Units
single flatMap	3924997	3832845	ops/s
repeated left binds	353197	343995	ops/s
repeated right binds	346440	337696	ops/s

There is a difference but is imo insignificant.

FAQ

In preparation for the upcoming discussion:

Could this solution be used for Kleisli?

I do not see a solution, because andThen swallows the input and you need it a second time for triggering the instance returned by the mapping function:

def flatMap[C](f: B => Kleisli[F, A, C])(implicit F: FlatMap[F]): Kleisli[F, A, C] =
  Kleisli(AndThen(run).andThen { b =>
    f(b).run(a)
    //   ^^^^^^
    // We've lost `A` here, so this won't work :-(
  })

Unless I'm missing something, the answer is no — but it would have been awesome 😞

Is AndThen reusable?

Yes, we might end up using it for fixing the stack safety of other data types too.

Should we expose AndThen?

It's a pretty useful data type, maybe. Currently it's private[cats].

[codecov-io]

Codecov Report

Merging #2187 into master will increase coverage by 0.03%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master    #2187      +/-   ##
==========================================
+ Coverage   94.75%   94.78%   +0.03%     
==========================================
  Files         330      332       +2     
  Lines        5568     5697     +129     
  Branches      201      214      +13     
==========================================
+ Hits         5276     5400     +124     
- Misses        292      297       +5

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/IndexedStateT.scala	89.24% <100%> (ø)	⬆️
core/src/main/scala/cats/data/AndThen.scala	96.96% <96.96%> (ø)
core/src/main/scala/cats/data/WriterT.scala	91.37% <0%> (-1.76%)	⬇️
core/src/main/scala/cats/data/package.scala	87.5% <0%> (ø)	⬆️
core/src/main/scala/cats/data/NonEmptySet.scala	97.29% <0%> (ø)
core/src/main/scala/cats/data/Kleisli.scala	97.93% <0%> (+0.04%)	⬆️
core/src/main/scala/cats/data/EitherT.scala	97.65% <0%> (+0.05%)	⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	100% <0%> (+1.28%)	⬆️

---

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...439b785. Read the comment docs.

[johnynek]

I wonder checking benchmarks. This might not be measurably slower with the technique it is using.

[johnynek]

This actually seems like something to move to kernel since it is not higher kinded very useful.

I see it is private, but lots of use cases could exist for an optimized AndThen.

[alexandru]

It is not very clear to me what the boundaries of Cats's sub-projects and packages are.

I can move it to cats-kernel if needed, however I only see a bunch of type classes in cats-kernel, whereas I am seeing data types that aren't type classes in cats.data.

[alexandru]

@johnynek I've added benchmark results, compared with current master, see the description.

[alexandru]

Also let me know what you think about AndThen — should we make it public and if yes in what package?

[kailuowang]

Nitpick, can we make this 127 (and at line 32) a constant with a sensible name?

[alexandru]

Thanks for the suggestion. I've updated the code with a documented constant.

[kailuowang]

AndThen definitely looks useful. But my vote is to keep internal for a bit before we release it and lock the binary compatibility, (the same approach we are taking with new type)

[johnynek]

If we look at the API of AndThen, from a binary compatibility point of view it is just an alias for Function1. I'm not sure how we could even make a change that would break binary compatibility, yet still be an instance of Function1.

Even the apply method on the companion just takes a Function1 and returns and AndThen.

So as long as we keep the subclasses private and all the support methods private, I think we should make it available. If for no other reason than cats-effect can use it at that point.

[kailuowang]

@johnynek this API is basically fixed to Function1. What I was worrying about was that, outside of IndexedStateT, is this encoding (i.e. extending the Function1 type), best suited for public usage for stacksafe compose of functions? That being said, I just realized that if there turns out to be a better encoding user can just switch to that one, this AndThen is still needed by the StateT. So now I am more neutral on whether to open it up.

[johnynek]

+1

[kailuowang]

fixed scalastyle issues, will merge after build green.