Add Cont alias to ContT #3403
Add Cont alias to ContT #3403
Conversation
| @@ -82,4 +82,17 @@ package object data extends ScalaVersionSpecificPackage { | |||
| def apply[S, A](f: S => A, s: S): Store[S, A] = | |||
| RepresentableStore[S => *, S, A](f, s) | |||
| } | |||
|
|
|||
| type Cont[A, B] = ContT[Id, A, B] | |||
There was a problem hiding this comment.
I think this needs to be Eval instead of Id in order to preserve stack safety. But otherwise, big 👍 on this!
There was a problem hiding this comment.
So I tried to verify the stack safety of ContT[Eval, A, B]:
def add[R](a: Int, b: Int): ContT[Eval, Int, Int] =
ContT(f => f(a + b))
val seed = add[Int](3, 2)
val f = (0 until 10000).foldLeft(seed)((acc, _) => acc.flatMap(x => Cont.pure(x + 1)))
println(f.run(x => Eval.later(x)).value)But I seem to be getting stack overflows for this as well. Excerpt of the stack trace:
[error] java.lang.StackOverflowError
[error] at cats.data.ContT$.apply(ContT.scala:153)
[error] at cats.data.ContT$.defer(ContT.scala:132)
[error] at cats.data.package$Cont$.defer(package.scala:96)
[error] at org.simpleapp.Playground$.$anonfun$f$2(Playground.scala:11)
[error] at org.simpleapp.Playground$.$anonfun$f$2$adapted(Playground.scala:11)
[error] at scala.Function1.$anonfun$andThen$1(Function1.scala:57)
[error] at cats.data.AndThen.runLoop(AndThen.scala:97)
[error] at cats.data.AndThen.apply(AndThen.scala:67)
[error] at cats.data.ContT$.$anonfun$defer$1(ContT.scala:133)
[error] at cats.data.AndThen.runLoop(AndThen.scala:97)
[error] at cats.data.AndThen.apply(AndThen.scala:67)
[error] at cats.data.ContT.$anonfun$flatMap$2(ContT.scala:43)
[error] at scala.Function1.$anonfun$andThen$1(Function1.scala:57)
[error] at cats.data.AndThen.runLoop(AndThen.scala:97)
[error] at cats.data.AndThen.apply(AndThen.scala:67)
[error] at cats.data.ContT$.$anonfun$defer$1(ContT.scala:133)Do you think there might be a deficiency in the ContT#flatMap implementation?
There was a problem hiding this comment.
Finally got a chance to poke at this a bit. Have you tried the same test with tailRecM? I haven't looked into extreme detail about what's going on here, but I remember tailRecM being very meticulously verified as stack-safe, but I don't think the same work was done on flatMap (in fact, I remember specifically making arguments that you can't make a stack-safe flatMap for ContT).
There was a problem hiding this comment.
Yeah, no more stack overflows with tailRecM. I'll need to convince myself why it won't work for flatMap, but otherwise I think this is all ready. :) I noticed we don't have any inductive class instances for ContT in cats-effect, might take a look and see if those can be added
There was a problem hiding this comment.
I'd like to add some, but I think the problem is the only one we could add in CE2 is Bracket. All Sync instances are constrained to be stack-safe in their flatMap, which obviously ContT isn't.
As an aside, the main reason why flatMap on ContT isn't stack-safe is similar to the reason why flatMap on Option isn't stack-safe: there's nothing to trampoline through. With tailRecM, you're forced to pass back to the trampoline loop with each iteration, while flatMap has no such guarantee. With Option, you need to have a recursive flatMap in order to really see the damage. With ContT, a long chain of flatMaps is recursive because it's effectively just a very long chain of andThens. Since flatMap on ContT corresponds directly to the CPS transformation, we can see it more directly in imperative form:
def direct(i: Int): String = i.toString
def cps[A](i: Int)(k: String => A): A = k(i.toString)Now imagine chaining 10,000 cpses together. Naturally, the result will be a stack overflow.
It's worth noting that even IO would have this issue if someone worked entirely in terms of async.
Codecov Report
@@ Coverage Diff @@
## master #3403 +/- ##
==========================================
- Coverage 92.70% 92.65% -0.06%
==========================================
Files 379 379
Lines 7981 7991 +10
Branches 230 237 +7
==========================================
+ Hits 7399 7404 +5
- Misses 582 587 +5
Continue to review full report at Codecov.
|
Let me know if I need tests or documentation of any kind.