Stack overflow regression of recursive arbOption

[ashawley]

The change in #408 broke definitions of Arbitrary[X] where X is a class with a recursive Option value. One potential fix is to rollback #286 that was released in 1.14.0. Alternatively, we can just call-by-name the implicit Arbitrary of arbOption with Gen.delay, in case it is recursive.

[non]

I don't agree this is a legit test case.

Here's a very similar definition that does fine without the code as-is:

case class Recur(option: Option[Recur])

implicit def arbRecur: Arbitrary[Recur] =
  Arbitrary {
    Gen.lzy(Arbitrary.arbitrary[Option[Recur]]).map(Recur(_))
  }

An even more idiomatic version might look like this:

case class Recur(option: Option[Recur])

object Recur {
  implicit val arbRecur: Arbitrary[Recur] = {
    lazy val g: Gen[Recur] = Gen.lzy(Gen.option(g).map(Recur(_)))
    Arbitrary(g)
  }
}

I think it's reasonable to require people to use Gen.lzy when building circular generators. The fact that it happened to work previously without is something of a fluke.

[ashawley]

I agree. That it worked before was a fluke. I guess the motivation was to produce a bug-fix release, and this would avoid a regression for existing test suites.

[ashawley]

Toying around with it further, it seems there is a way to avoid the stackoverflow regression for existing recursive definitions, but also keep the 90/10 weighting of Some/None.

[non]

@ashawley I'd prefer to keep the existing weighting since I think it's more likely to produce interesting data by default. OK to close this?

[ashawley]

Have you looked at the new changes here? This preserves the existing weighting.

[ashawley]

That, or I'm not understanding what you mean by "existing".

[non]

Aha, I see your update. Sorry for the confusion!

I'm still not convinced by this PR. There is a cost to using Gen.delay and I don't think every user of Arbitrary[Option[A]] should pay it so that the recursive definition in the test can avoid it.

As before, you could just rewrite your test to use delay instead:

implicit def arbRecur: Arbitrary[Recur] = Arbitrary {
  Gen.delay(Arbitrary.arbitrary[Option[Recur]])
    .map(Recur(_))
  }

My belief is that these kinds of self-referential Arbitrary instances are quite rare, and that it's fine to require this kind of rewrite. I agree that it's unfortunate that code that used to work may have to be modified, but I think it's likely to be a rare occurrence. The alternatives both seem worse to me: reverting the frequency means that most testing with options will end up being redundant, and adding this delay will slow down all tests that use arbitrary to generate options.

What do you think?

[ashawley]

I'm happy to close. Again, I'm not invested in recursive Option, so I can't defend it. I'm trying to generate goodwill by avoiding a regression in bug fix release, and get ScalaCheck working again in the Community Build.

I'll reopen a new PR that captures the the idiomatic version you want to support going forward.

[non]

Perfect, thanks! I'm sorry to be so demanding, but I'm trying to step up into more of a maintainer-ish role here (to get things back on track).