Add MonadRec for Future and reinstate Future tests for JVM #1261
Conversation
| def pure[A](x: A): Future[A] = Future.successful(x) | ||
|
|
||
| def flatMap[A, B](fa: Future[A])(f: A => Future[B]): Future[B] = fa.flatMap(f) | ||
|
|
||
| final def tailRecM[B, C](b: B)(f: B => Future[(B Xor C)]): Future[C] = |
There was a problem hiding this comment.
@kailuowang This implementation won't actually compile with @tailrec, since the recursive call is inside the flatMap. It is stack-safe, though, thanks to the way Future#flatMap works.
There was a problem hiding this comment.
@travisbrown is this stack safe on all versions of scala? I thought scala futures could have issues with this (at least in 2.10 or something. This is just a vague recollection).
There was a problem hiding this comment.
@johnynek I'm pretty sure that was only an issue with the original Akka Future implementation, and that by the time SIP-14 landed flatMap was stack-safe (thanks to pressure from Twitter and inspired by c.t.u.Future). I'll double-check that, though.
There was a problem hiding this comment.
@johnynek I'm not finding the discussion I was thinking of at the moment, but I did try this on 2.9.3:
import scala.concurrent.{ Await, Future }, scala.concurrent.duration._
import scala.concurrent.ExecutionContext.Implicits.global
def test(i: Int)(f: Int => Future[Int]): Future[Int] = f(i).flatMap {
case x if x > 100000 => Future.successful(x)
case x => test(x)(f)
}
val f1 = test(0)(x => Future(x + 1))
val f2 = test(0)(x => Future(x))Neither blows the stack, and f1 succeeds as expected.
There was a problem hiding this comment.
Yeah, looks legit. Thanks (I know Twitter's is stack safe, I didn't know scala was).
There was a problem hiding this comment.
Maybe a code comment mentioning that this construct is actually stack-safe would be appropriate here?
There was a problem hiding this comment.
@fthomas Makes sense—done.
|
Oops sorry about that, it looks like I did remove some tests that I didn't mean to remove. Thanks for catching this @travisbrown. I'm a bit confused though. Why is it that these tests need to be jvm-only? One explanation that I could imagine is that the |
Current coverage is 90.32% (diff: 100%)@@ master #1261 diff @@
==========================================
Files 243 243
Lines 3285 3286 +1
Methods 3231 3231
Messages 0 0
Branches 51 52 +1
==========================================
+ Hits 2961 2968 +7
+ Misses 324 318 -6
Partials 0 0
|
|
@ceedubs The |
| @@ -47,4 +47,5 @@ class FutureTests extends CatsSuite { | |||
|
|
|||
| checkAll("Future[Int]", MonadErrorTests[Future, Throwable].monadError[Int, Int, Int]) | |||
| checkAll("Future[Int]", ComonadTests[Future].comonad[Int, Int, Int]) | |||
| checkAll("Future", MonadRecTests[Future].monadRec[Int, Int, Int]) | |||
There was a problem hiding this comment.
is this test a reliable test of stack safety?
There was a problem hiding this comment.
@johnynek I don't believe so—it looks like it just checks for consistency with flatMap.
|
👍 Love some MonadRec! |
|
👍 thanks a lot ! |
|
@travisbrown thanks for the explanation regarding 👍 once the build passes. Thanks! |
This PR adds a new
MonadRecinstance forFuture(as discussed on Gitter). The implementation oftailRecMis not literally tail recursive, but it is stack-safe.I've also reinstated the
jvmmodule that was removed in #1018, since it looks like the removal of theMonadErrorlaws checking there wasn't intentional, and since I need a place to put theMonadReclaws checking forFuture.