Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhances stack safety for Eval. #1888

Merged
merged 5 commits into from
Sep 28, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions core/src/main/scala/cats/Eval.scala
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,15 @@ object Eval extends EvalInstances {
* they will be automatically created when needed.
*/
sealed abstract class Call[A](val thunk: () => Eval[A]) extends Eval[A] {
def memoize: Eval[A] = new Later(() => value)
def value: A = Call.loop(this).value

def memoize: Eval[A] =
new Call[A](thunk) {
override def memoize: Eval[A] = this
override lazy val value: A = Call.loop(this).value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this be lost if you flatMap? Isn't it ignore in that case. Is that okay?

}

def value: A =
Call.loop(this).value
}

object Call {
Expand Down Expand Up @@ -294,12 +301,19 @@ object Eval extends EvalInstances {
* trampoline are not exposed. This allows a slightly more efficient
* implementation of the .value method.
*/
sealed abstract class Compute[A] extends Eval[A] {
sealed abstract class Compute[A] extends Eval[A] { self =>
type Start
val start: () => Eval[Start]
val run: Start => Eval[A]

def memoize: Eval[A] = Later(value)
def memoize: Eval[A] =
new Compute[A] {
type Start = self.Start
val start: () => Eval[Start] = self.start
val run: Start => Eval[A] = self.run
override def memoize: Eval[A] = this
override lazy val value: A = self.value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, this lazy val is lost on flatMap no?
Maybe memoise only applies to an outer flatMap?

I have used it when forking into two paths so I don't evaluate something twice, but this wouldn't actually work with this change I think.

}

def value: A = {
type L = Eval[Any]
Expand Down
6 changes: 3 additions & 3 deletions laws/src/main/scala/cats/laws/discipline/Arbitrary.scala
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ object arbitrary extends ArbitraryInstances0 {

implicit def catsLawsArbitraryForEval[A: Arbitrary]: Arbitrary[Eval[A]] =
Arbitrary(Gen.oneOf(
getArbitrary[A].map(Eval.now(_)),
getArbitrary[A].map(Eval.later(_)),
getArbitrary[A].map(Eval.always(_))))
getArbitrary[A].map(a => Eval.now(a)),
getArbitrary[() => A].map(f => Eval.later(f())),
getArbitrary[() => A].map(f => Eval.always(f()))))

implicit def catsLawsCogenForEval[A: Cogen]: Cogen[Eval[A]] =
Cogen[A].contramap(_.value)
Expand Down
90 changes: 89 additions & 1 deletion tests/src/test/scala/cats/tests/EvalTests.scala
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package cats
package tests

import scala.math.min
import cats.laws.ComonadLaws
import cats.laws.discipline.{BimonadTests, CartesianTests, ReducibleTests, SerializableTests}
import cats.laws.discipline.arbitrary._
import cats.kernel.laws.{GroupLaws, OrderLaws}
import org.scalacheck.{Arbitrary, Cogen, Gen}
import org.scalacheck.Arbitrary.arbitrary
import scala.annotation.tailrec
import scala.math.min

class EvalTests extends CatsSuite {
implicit val eqThrow: Eq[Throwable] = Eq.allEqual
Expand Down Expand Up @@ -140,4 +143,89 @@ class EvalTests extends CatsSuite {
isEq.lhs should === (isEq.rhs)
}
}

// the following machinery is all to faciliate testing deeply-nested
// eval values for stack safety. the idea is that we want to
// randomly generate deep chains of eval operations.
//
// there are three ways to construct Eval[A] values from expressions
// returning A (and which are generated by Arbitrary[Eval[A]]):
//
// - Eval.now(...)
// - Eval.later(...)
// - Eval.always(...)
//
// there are four operations that transform expressions returning
// Eval[A] into a new Eval[A] value:
//
// - (...).map(f)
// - (...).flatMap(g)
// - (...).memoize
// - Eval.defer(...)
//
// the O[A] ast represents these four operations. we generate a very
// long Vector[O[A]] and a starting () => Eval[A] expression (which
// we call a "leaf") and then compose these to produce one
// (deeply-nested) Eval[A] value, which we wrap in DeepEval(_).

case class DeepEval[A](eval: Eval[A])

object DeepEval {

sealed abstract class O[A]

case class OMap[A](f: A => A) extends O[A]
case class OFlatMap[A](f: A => Eval[A]) extends O[A]
case class OMemoize[A]() extends O[A]
case class ODefer[A]() extends O[A]

implicit def arbitraryO[A: Arbitrary: Cogen]: Arbitrary[O[A]] =
Arbitrary(Gen.oneOf(
arbitrary[A => A].map(OMap(_)),
arbitrary[A => Eval[A]].map(OFlatMap(_)),
Gen.const(OMemoize[A]),
Gen.const(ODefer[A])))

def build[A](leaf: () => Eval[A], os: Vector[O[A]]): DeepEval[A] = {

def restart(i: Int, leaf: () => Eval[A], cbs: List[Eval[A] => Eval[A]]): Eval[A] =
step(i, leaf, cbs)

@tailrec def step(i: Int, leaf: () => Eval[A], cbs: List[Eval[A] => Eval[A]]): Eval[A] =
if (i >= os.length) cbs.foldLeft(leaf())((e, f) => f(e))
else os(i) match {
case ODefer() => Eval.defer(restart(i + 1, leaf, cbs))
case OMemoize() => step(i + 1, leaf, ((e: Eval[A]) => e.memoize) :: cbs)
case OMap(f) => step(i + 1, leaf, ((e: Eval[A]) => e.map(f)) :: cbs)
case OFlatMap(f) => step(i + 1, leaf, ((e: Eval[A]) => e.flatMap(f)) :: cbs)
}

DeepEval(step(0, leaf, Nil))
}

// we keep this low in master to keep travis happy.
// for an actual stress test increase to 200K or so.
val MaxDepth = 100

implicit def arbitraryDeepEval[A: Arbitrary: Cogen]: Arbitrary[DeepEval[A]] = {
val gen: Gen[O[A]] = arbitrary[O[A]]
Arbitrary(for {
leaf <- arbitrary[() => Eval[A]]
xs <- Gen.containerOfN[Vector, O[A]](MaxDepth, gen)
} yield DeepEval.build(leaf, xs))
}
}

// all that work for this one little test.

test("stack safety stress test") {
forAll { (d: DeepEval[Int]) =>
try {
d.eval.value
succeed
} catch { case (e: StackOverflowError) =>
fail(s"stack overflowed with eval-depth ${DeepEval.MaxDepth}")
}
}
}
}