Add callCC to ContT

[TimWSpence]

Add "call with current continuation" to ContT

[codecov-io]

Codecov Report

Merging #3747 (f1df637) into master (88ccb7a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3747      +/-   ##
==========================================
+ Coverage   90.24%   90.25%   +0.01%     
==========================================
  Files         390      390              
  Lines        8865     8868       +3     
  Branches      260      267       +7     
==========================================
+ Hits         8000     8004       +4     
+ Misses        865      864       -1     

[johnynek]

I'm trying to see:

1. Why we can't move cb(a) out
2. If we should use Defer[M] anywhere to make this lazier or safer (as is done in tailRecM)

[TimWSpence]

Sorry, what did you mean by move cb(a) out?

The Defer thing is something I was worried about. I am not aware that we need it anywhere but was hoping that somebody with more experience would catch it in review if I'm wrong

[johnynek]

for instance:

apply { cb =>
  val cont = f { a =>
    val cba = cb(a)
    apply(_ => cba)
  }

  cont.run(cb)
}

has potentially different execution behavior, although, if we have purity, I think the same result. It isn't clear to me which is likely to be more efficient.

Secondly, if we have a Defer[M] then we could put the entire cont.run(cb) in a defer: Defer[M].defer(cont.run(cb))

Since we do that on map and flatMap, I assume there are recursive cases that would want that here as well, and if nothing else, for consistency, I guess I'd do it...

But it would be nice to find a recursive example and see if we can show we can do deep recursions.

[TimWSpence]

Hmm it would appear that it is not stack-safe. I pushed a failing test for it but no amount of poking it with defer and AndThen has fixed it yet 😂

[johnynek]

this seems to fix it:

oscar@Patricks-MacBook-Air cats % git diff
diff --git a/core/src/main/scala/cats/data/ContT.scala b/core/src/main/scala/cats/data/ContT.scala
index 4bef4e13c..d4864b80b 100644
--- a/core/src/main/scala/cats/data/ContT.scala
+++ b/core/src/main/scala/cats/data/ContT.scala
@@ -40,7 +40,10 @@ sealed abstract class ContT[M[_], A, +B] extends Serializable {
     // allocate/pattern match once
     val fnAndThen = AndThen(fn)
     ContT[M, A, C] { fn2 =>
-      val contRun: ContT[M, A, C] => M[A] = _.run(fn2)
+      val contRun: ContT[M, A, C] => M[A] =  { c =>
+        M.defer(c.run(fn2))
+      }
+
       val fn3: B => M[A] = fnAndThen.andThen(contRun)
       M.defer(run(fn3))
     }

in the flatMap function.

Can you try that (I pulled your PR and it worked for me).

[TimWSpence]

😍 THANK YOU! Very good call. Just out of curiosity: what made you realize that flatMap was the issue?

I've added an explicit stack-safety test for flatMap as well (which I just assumed already existed hence not even thinking to suspect flatMap)

[johnynek]

Honestly, I just looked at all the places a Defer could go and I tried it.

Afterwords, it seemed more obvious that flatMap needed one there: it was internally doing a run which starts a new stack on whatever M you have. By using a Defer, you should push that evaluation onto the same final heap used to interpret M.

I think a good rule of thumb should be: any time we do a run internally, we should wrap that inside Defer[M].defer.

[TimWSpence]

Thanks @johnynek, just trying to visualize the execution and the interaction between AndThen and Defer. Any insights much appreciated 😂

[arosien]

Use brackets instead of parentheses to make it cleaner looking?

[johnynek]

according to the principal above, I think we should put the run call in callCC inside a defer.

apply { cb =>
  val cont = f { a =>
    apply(_ => cb(a))
  }

  Defer[M].defer(cont.run(cb))
}

[TimWSpence]

👍 done. If you could re-trigger the build that would be great! I created #3762 to track/capture the scalacheck seed for the Order[FiniteDuration] laws failure

[johnynek]

request one more change and then we can merge this I think.

[johnynek]

This is great. Glad we have the flatMap stack safety checks too. Thanks!