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

java.util.NoSuchElementException: None.get #218

Closed
xuwei-k opened this issue Feb 5, 2016 · 12 comments
Closed

java.util.NoSuchElementException: None.get #218

xuwei-k opened this issue Feb 5, 2016 · 12 comments

Comments

@xuwei-k
Copy link
Contributor

xuwei-k commented Feb 5, 2016

scalaz/scalaz#1095

@rickynils
Copy link
Contributor

Steps to reproduce:

git clone https://github.com/xuwei-k/scalaz.git -b scalacheck-1.13.0
cd scalaz
sbt "testOnly scalaz.CofreeTest -- -v 2"

Relevant output:

[info] ! scalaz.CofreeTest.CofreeStream:monad.left identity: Exception raised on property evaluation.
[info] > ARG_0: -502843521
[info] > ARG_1: <function1>
[info] > Exception: java.util.NoSuchElementException: None.get
[info] scala.None$.get(Option.scala:313)
[info] scala.None$.get(Option.scala:311)
[info] org.scalacheck.GenArities$$anonfun$function1$1$$anonfun$1.apply(GenArities.scala:15)
[info] scalaz.CofreeBind$class.bind(Cofree.scala:224)
[info] scalaz.CofreeInstances0$$anon$3.bind(Cofree.scala:170)
[info] scalaz.CofreeInstances0$$anon$3.bind(Cofree.scala:170)
[info] scalaz.Monad$MonadLaw$class.leftIdentity(Monad.scala:80)
[info] scalaz.Monad$$anon$3.leftIdentity(Monad.scala:82)
[info] scalaz.scalacheck.ScalazProperties$monad$$anonfun$leftIdentity$4.apply(ScalazProperties.scala:211)
[info] scalaz.scalacheck.ScalazProperties$monad$$anonfun$leftIdentity$4.apply(ScalazProperties.scala:211)
[info] org.scalacheck.Prop$$anonfun$forAll$10$$anonfun$apply$26.apply(Prop.scala:870)
[info] scala.Function1$$anonfun$andThen$1.apply(Function1.scala:55)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1$$anonfun$3.apply(Prop.scala:712)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1$$anonfun$3.apply(Prop.scala:712)
[info] org.scalacheck.Prop$.secure(Prop.scala:456)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1.org$scalacheck$Prop$$anonfun$$result$1(Prop.scala:712)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1.apply(Prop.scala:749)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1.apply(Prop.scala:706)
[info] org.scalacheck.Prop$$anonfun$apply$5.apply(Prop.scala:291)
[info] org.scalacheck.Prop$$anonfun$apply$5.apply(Prop.scala:290)
[info] org.scalacheck.PropFromFun.apply(Prop.scala:22)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1.org$scalacheck$Prop$$anonfun$$result$1(Prop.scala:713)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1.apply(Prop.scala:749)
[info] org.scalacheck.Prop$$anonfun$forAllShrink$1.apply(Prop.scala:706)
[info] org.scalacheck.Prop$$anonfun$apply$5.apply(Prop.scala:291)
[info] org.scalacheck.Prop$$anonfun$apply$5.apply(Prop.scala:290)
[info] org.scalacheck.PropFromFun.apply(Prop.scala:22)
[info] org.scalacheck.Test$.org$scalacheck$Test$$workerFun$1(Test.scala:294)
[info] org.scalacheck.Test$$anonfun$2.apply(Test.scala:323)
[info] org.scalacheck.Test$$anonfun$2.apply(Test.scala:323)
[info] org.scalacheck.Platform$.runWorkers(Platform.scala:40)
[info] org.scalacheck.Test$.check(Test.scala:323)
[info] org.scalacheck.ScalaCheckRunner$$anon$2$$anonfun$execute$3$$anonfun$apply$2.apply(ScalaCheckFramework.scala:88)
[info] org.scalacheck.ScalaCheckRunner$$anon$2$$anonfun$execute$3$$anonfun$apply$2.apply(ScalaCheckFramework.scala:87)
[info] scala.collection.TraversableLike$WithFilter$$anonfun$foreach$1.apply(TraversableLike.scala:772)
[info] scala.collection.immutable.List.foreach(List.scala:318)
[info] scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:32)
[info] scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:45)
[info] scala.collection.TraversableLike$WithFilter.foreach(TraversableLike.scala:771)
[info] org.scalacheck.ScalaCheckRunner$$anon$2$$anonfun$execute$3.apply(ScalaCheckFramework.scala:87)
[info] org.scalacheck.ScalaCheckRunner$$anon$2$$anonfun$execute$3.apply(ScalaCheckFramework.scala:84)
[info] scala.collection.TraversableLike$$anonfun$flatMap$1.apply(TraversableLike.scala:251)
[info] scala.collection.TraversableLike$$anonfun$flatMap$1.apply(TraversableLike.scala:251)
[info] scala.collection.IndexedSeqOptimized$class.foreach(IndexedSeqOptimized.scala:33)
[info] scala.collection.mutable.ArrayOps$ofRef.foreach(ArrayOps.scala:108)
[info] scala.collection.TraversableLike$class.flatMap(TraversableLike.scala:251)
[info] scala.collection.mutable.ArrayOps$ofRef.flatMap(ArrayOps.scala:108)
[info] org.scalacheck.ScalaCheckRunner$$anon$2.execute(ScalaCheckFramework.scala:84)
[info] sbt.TestRunner.runTest$1(TestFramework.scala:76)
[info] sbt.TestRunner.run(TestFramework.scala:85)
[info] sbt.TestFramework$$anon$2$$anonfun$$init$$1$$anonfun$apply$8.apply(TestFramework.scala:202)
[info] sbt.TestFramework$$anon$2$$anonfun$$init$$1$$anonfun$apply$8.apply(TestFramework.scala:202)
[info] sbt.TestFramework$.sbt$TestFramework$$withContextLoader(TestFramework.scala:185)
[info] sbt.TestFramework$$anon$2$$anonfun$$init$$1.apply(TestFramework.scala:202)
[info] sbt.TestFramework$$anon$2$$anonfun$$init$$1.apply(TestFramework.scala:202)
[info] sbt.TestFunction.apply(TestFramework.scala:207)
[info] sbt.Tests$.sbt$Tests$$processRunnable$1(Tests.scala:239)
[info] sbt.Tests$$anonfun$makeSerial$1.apply(Tests.scala:245)
[info] sbt.Tests$$anonfun$makeSerial$1.apply(Tests.scala:245)
[info] sbt.std.Transform$$anon$3$$anonfun$apply$2.apply(System.scala:44)
[info] sbt.std.Transform$$anon$3$$anonfun$apply$2.apply(System.scala:44)
[info] sbt.std.Transform$$anon$4.work(System.scala:63)
[info] sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
[info] sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
[info] sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
[info] sbt.Execute.work(Execute.scala:235)
[info] sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
[info] sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
[info] sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
[info] sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
[info] java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info] java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[info] java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info] java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
[info] java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
[info] java.lang.Thread.run(Thread.java:745)
[info] Elapsed time: 0.001 sec 

@ceedubs
Copy link
Contributor

ceedubs commented Feb 12, 2016

@non in #171 you mentioned that you were forced to use Option.get. Do you think this is related?

@xuwei-k
Copy link
Contributor Author

xuwei-k commented Feb 12, 2016

xuwei-k@a6066af4447db679f33
xuwei-k/scalaz@8b92943a435b6ea19901fa

https://travis-ci.org/xuwei-k/scalaz/builds/108790554#L4764

[info] ! scalaz.CofreeTest.CofreeStream:monad.left identity: Exception raised on property evaluation.
[info] > ARG_0: -2147483648
[info] > ARG_1: <function1>
[info] > Exception: java.lang.RuntimeException: function Gen error

@rickynils
Copy link
Contributor

cc @non

A minimal way of reproducing this exception:

scala> import org.scalacheck._
import org.scalacheck._

scala> trait A
defined trait A

scala> val g: Gen[A] = Gen.fail
g: org.scalacheck.Gen[A] = org.scalacheck.Gen$$anon$1@3eb98407

scala> implicit val a: Arbitrary[A] = Arbitrary(g)
a: org.scalacheck.Arbitrary[A] = org.scalacheck.ArbitraryLowPriority$$anon$1@1e103add

scala> Arbitrary.arbitrary[Int => A].sample.get(0)
java.util.NoSuchElementException: None.get
  at scala.None$.get(Option.scala:347)
  at scala.None$.get(Option.scala:345)
  at org.scalacheck.GenArities$$anonfun$function1$1$$anonfun$1.apply(GenArities.scala:15)
  ... 42 elided

So, if an arbitrary generator of the function's target type fails generating a value, this exception will be triggered. It is not obvious how we could work around this in the general case as long as generator results are modeled with the Option type. I think @tonymorris actually reached the same conclusion a long time ago.

@non
Copy link
Contributor

non commented May 10, 2016

@rickynils I agree that using Option is unfortunate but QuickCheck has a similar issue despite using exceptions instead. [1] [2]

I have a branch where I removed Option / filter to see if that fixed this issue, but it seemed like the real problem (at least in this case) was that Gen.fail instances were being produced during shrinking. I think that if we stop doing that we'll fix most of the problems people hit. Of course if someone bases a Gen[A => B] on a bad Gen[B] they'll still have issues, but in the long run I think we can limit this possibility by improving the Gen architecture.

[1] https://github.com/nick8325/quickcheck/blob/master/Test/QuickCheck/Gen.hs#L120
[2] https://github.com/nick8325/quickcheck/blob/master/Test/QuickCheck/Gen.hs#L81

@ghost
Copy link

ghost commented May 21, 2016

Hi All - I would really love to help out on this, but.... Anyway, any progress on this, as a fix for this would be useful to get into scalatest 3.0.0 release?

@non
Copy link
Contributor

non commented Jul 2, 2016

Sorry for the long radio silence.

I have thought I understood what was causing this particular crash several times. In fact, it's possible to fix @xuwei-k's branch with the following patch:

diff --git a/scalacheck-binding/src/main/scala/scalaz/scalacheck/ScalazArbitrary.scala b/scalacheck-binding/src/main/scala/scalaz/scalacheck/ScalazArbitrary.scala
index 795221b..c3e045a 100644
--- a/scalacheck-binding/src/main/scala/scalaz/scalacheck/ScalazArbitrary.scala
+++ b/scalacheck-binding/src/main/scala/scalaz/scalacheck/ScalazArbitrary.scala
@@ -213,7 +213,8 @@ object ScalazArbitrary {

   implicit def TreeArbitrary[A: Arbitrary]: Arbitrary[Tree[A]] =
     Arbitrary(Gen.sized(n =>
-      Gen.choose(1, n).flatMap(treeGenSized[A])
+      if (n < 1) treeGenSized[A](0)
+      else Gen.choose(1, n).flatMap(treeGenSized[A])
     ))

   private[scalaz] def treeLocGenSized[A: NotNothing](size: Int)(implicit A: Arbitrary[A]): Gen[TreeLoc[A]] = {

@non
Copy link
Contributor

non commented Jul 2, 2016

I'm working on a patch to ScalaCheck to make this issue harder to hit. As @rickynils says, anytime someone is using Gen.fail to build a Cogen there will be major problems. The big issue here is that it's very easy to misuse Choose or size parameters to create failed generators -- which is the issue in the Scalaz case.

The PR I'm working on is going to try to make invalid sizes and ranges throw errors, which makes it a lot easier to catch these kinds of problems. I also want to make some combinators to make it safer to use the size parameter.

@non
Copy link
Contributor

non commented Jul 2, 2016

We could also choose to return Option[Gen[A]] instead of throwing an exception. Either way, we will need to try to explicitly signal failure, rather than encoding it using things like Gen.fail.

non pushed a commit to non/scalacheck that referenced this issue Aug 30, 2016
This commit was inspired by typelevel#218. It turns out that it ends up being
very easy to accidentally create empty generators (that will never
return Some(_) values) unnecessarily. The particular issue was around
usage of Gen.sized, but there are a number of other combinators that
end up creating empty generators in some cases.

Previously, when the arguments to a combinator make it impossible to
produce a value, ScalaCheck has preferred to produce empty Gen[T]
values. After this change, combinators whose arguments make it
impossible to generate values will instead throw exceptions during
creation. This makes it easier for the author to realize and correct
their mistake.

The commit also tightens up a bunch of the internal logic around
generating collections, numbers in a given range, etc. to minimize the
use of empty generators.

The reason why empty generators are a much bigger problem now than
before is the introduction of Cogen[T]. When we produce an arbitrary
`A => B` value, we do so by using a `Cogen[A]` to permute the PRNG,
and then use a `Gen[B]` to produce a `B` value from that PRNG
state. If our generator is empty (or even if it's a very sparse
partial generator), this process will fail when we try to apply the
function to an `A` value.

Since ScalaCheck allows empty generators (as QuickCheck does) there's
probably no backwards-compatible way to be 100% sure a `Gen[B]` is
safe to use. However, in practice most `Gen` instances *could* be safe
to use this way (as they are in QuickCheck), but are not due to how
easy it is to end up with empty (or mostly empty) generators.

This changes is binary compatible with the last release (check with
MiMA). It does introduce new runtime behavior (in the form of some new
exceptions users might see), but I think the change is worth it to get
faster (and safer) generators, as well as more reliable function
generators.

Review by @rickynils, @xuwei-k, and anyone else interested.
non pushed a commit to non/scalacheck that referenced this issue Aug 30, 2016
This commit was inspired by typelevel#218. It turns out that it ends up being
very easy to accidentally create empty generators (that will never
return Some(_) values) unnecessarily. The particular issue was around
usage of Gen.sized, but there are a number of other combinators that
end up creating empty generators in some cases.

Previously, when the arguments to a combinator make it impossible to
produce a value, ScalaCheck has preferred to produce empty Gen[T]
values. After this change, combinators whose arguments make it
impossible to generate values will instead throw exceptions during
creation. This makes it easier for the author to realize and correct
their mistake.

The commit also tightens up a bunch of the internal logic around
generating collections, numbers in a given range, etc. to minimize the
use of empty generators.

The reason why empty generators are a much bigger problem now than
before is the introduction of Cogen[T]. When we produce an arbitrary
`A => B` value, we do so by using a `Cogen[A]` to permute the PRNG,
and then use a `Gen[B]` to produce a `B` value from that PRNG
state. If our generator is empty (or even if it's a very sparse
partial generator), this process will fail when we try to apply the
function to an `A` value.

Since ScalaCheck allows empty generators (as QuickCheck does) there's
probably no backwards-compatible way to be 100% sure a `Gen[B]` is
safe to use. However, in practice most `Gen` instances *could* be safe
to use this way (as they are in QuickCheck), but are not due to how
easy it is to end up with empty (or mostly empty) generators.

This changes is binary compatible with the last release (check with
MiMA). It does introduce new runtime behavior (in the form of some new
exceptions users might see), but I think the change is worth it to get
faster (and safer) generators, as well as more reliable function
generators.

Review by @rickynils, @xuwei-k, and anyone else interested.
@non
Copy link
Contributor

non commented Sep 4, 2016

I think this particular issue will be fixed by #258. The buggy generator in question would now crash earlier and signal to the author that there was a problem.

@dwijnand
Copy link
Contributor

Assume it's fixed?

@ashawley
Copy link
Contributor

ashawley commented Apr 8, 2021

Closing as fixed. Please reopen if that is a mistaken assumption on my part.

@ashawley ashawley closed this as completed Apr 8, 2021
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

No branches or pull requests

6 participants