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

Arbitrary functions fail when the Gen instance on their return type sometimes fails #300

Closed
nrinaudo opened this issue Nov 7, 2016 · 5 comments · Fixed by #301
Closed

Comments

@nrinaudo
Copy link
Contributor

nrinaudo commented Nov 7, 2016

This means, among other things, that any Arbitrary instance that relies on Gen.suchThat cannot be used to generate arbitrary functions.

As an example:

import org.scalacheck._

implicit val nonZero: Arbitrary[Int] = Arbitrary(Arbitrary.arbitrary[Int].suchThat(_ != 0))
val prop = Prop.forAll { (f: String => Int) => f("foo"); true }

And then:

scala> prop.check
java.lang.StackOverflowError
  ...

See alexarchambault/scalacheck-shapeless#50 for a far more insidious example.

@non
Copy link
Contributor

non commented Nov 7, 2016

So, just to be clear, the generator has to fail many times (by default 100) before the function generator fails. See pureApply and doPureApply here: https://github.com/rickynils/scalacheck/blob/master/src/main/scala/org/scalacheck/Gen.scala#L73.

I think this is the only reasonable behavior for a generator that can be filtered (it is roughly the same behavior as the original QuickCheck's). By contrast, @xuwei-k's ScalaProps library does not even include suchThat or filter, which is a cleaner design (it removes the possibility of generator exhaustion during tests and also ensures function generators can't fail).

My sense is that ScalaCheck-Shapeless wasn't just failing some of the time, but a significant fraction of the time (e.g. 50% or more) which seems like it needed to be fixed anyway.

@alexarchambault
Copy link
Contributor

alexarchambault commented Nov 7, 2016

@non Per the source, doApply is only called once. The retries parameters is useless here.

@alexarchambault
Copy link
Contributor

alexarchambault commented Nov 7, 2016

As @nrinaudo's example illustrates, it only takes one failure to make Gen.function1 fail:

import org.scalacheck._

implicit val nonZero: Arbitrary[Int] = Arbitrary(Arbitrary.arbInt.arbitrary.suchThat(_ != 0))
val prop = Prop.forAll { (f: String => Int) => f("foo"); true }

prop.check

gives

! Exception raised on property evaluation.
> ARG_0: <function1>
> Exception: org.scalacheck.Gen$RetrievalError: couldn't generate value
org.scalacheck.Gen.loop$1(Gen.scala:57)
org.scalacheck.Gen.doPureApply(Gen.scala:58)
org.scalacheck.Gen.pureApply(Gen.scala:72)
org.scalacheck.GenArities$$anonfun$function1$1$$anonfun$1.apply(GenArities.
  scala:15)

@non
Copy link
Contributor

non commented Nov 7, 2016

OK! Yes, there is a bug/typo, the method should read:

  def doPureApply(p: Gen.Parameters, seed: Seed, retries: Int = 100): Gen.R[T] = {
    @tailrec def loop(r: Gen.R[T], i: Int): Gen.R[T] = {
      if (r.retrieve.isDefined) r
      else if (i > 0) loop(doApply(p, r.seed), i - 1)
      else throw new Gen.RetrievalError()
    }
    loop(doApply(p, seed), retries)
  }

I've confirmed that with this change the property passes.

I'll open a PR to fix this. Thanks for being persistent and reporting it!

non pushed a commit to non/scalacheck that referenced this issue Nov 8, 2016
This commit fixes a bug pointed out by @alexarchambault. When
constructing an `A => B` value from `Cogen[A]` and `Gen[B]`, we need
to be able to reliably generate a `B` value given a `Seed`. The
`doPureApply` method was given the ability to retry -- but
unfortunately, it used the same result value (with the same seed)
instead of trying a new one, defeating the retry code.

This commit fixes that problem. It adds tests to ensure that filtered
generators that can also produce real values can be used with
Gen.function1. (If a generator can never produce values it will still
be a problem.)

Fixes typelevel#300.
non pushed a commit to non/scalacheck that referenced this issue Nov 8, 2016
This commit fixes a bug pointed out by @alexarchambault. When
constructing an `A => B` value from `Cogen[A]` and `Gen[B]`, we need
to be able to reliably generate a `B` value given a `Seed`. The
`doPureApply` method was given the ability to retry -- but
unfortunately, it used the same result value (with the same seed)
instead of trying a new one, defeating the retry code.

This commit fixes that problem. It adds tests to ensure that filtered
generators that can also produce real values can be used with
Gen.function1. (If a generator can never produce values it will still
be a problem.)

Fixes typelevel#300.
@peter-empen
Copy link

I'd appreciate your publishing this fix for Scala 2.11. Thanks a lot.

sellout pushed a commit to sellout/scalacheck that referenced this issue Feb 27, 2017
This commit fixes a bug pointed out by @alexarchambault. When
constructing an `A => B` value from `Cogen[A]` and `Gen[B]`, we need
to be able to reliably generate a `B` value given a `Seed`. The
`doPureApply` method was given the ability to retry -- but
unfortunately, it used the same result value (with the same seed)
instead of trying a new one, defeating the retry code.

This commit fixes that problem. It adds tests to ensure that filtered
generators that can also produce real values can be used with
Gen.function1. (If a generator can never produce values it will still
be a problem.)

Fixes typelevel#300.
sellout pushed a commit to sellout/scalacheck that referenced this issue Mar 13, 2017
This commit fixes a bug pointed out by @alexarchambault. When
constructing an `A => B` value from `Cogen[A]` and `Gen[B]`, we need
to be able to reliably generate a `B` value given a `Seed`. The
`doPureApply` method was given the ability to retry -- but
unfortunately, it used the same result value (with the same seed)
instead of trying a new one, defeating the retry code.

This commit fixes that problem. It adds tests to ensure that filtered
generators that can also produce real values can be used with
Gen.function1. (If a generator can never produce values it will still
be a problem.)

Fixes typelevel#300.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants