Refactor of sieve in Gen

bench/src/main/scala/org/scalacheck/bench/GenBench.scala

@@ -3,6 +3,7 @@ package org.scalacheck.bench
 import java.util.concurrent.TimeUnit
 import org.openjdk.jmh.annotations._
 
+import org.scalacheck.Arbitrary.arbitrary
 import org.scalacheck.Gen
 import org.scalacheck.rng.Seed
 
@@ -110,6 +111,10 @@ class GenBench {
   def asciiPrintableStr(): List[String] =
     seeds.map(s => Gen.asciiPrintableStr.pureApply(params, s))
 
+  @Benchmark
+  def arbitraryString(): List[String] =
+    seeds.map(s => arbitrary[String].pureApply(params, s))
+
   private val gens = Vector.fill(10)(genInt)
 
   @Benchmark
@@ -146,4 +151,12 @@ class GenBench {
       val gf = Gen.frequency(1 -> g, 2 -> g, 3 -> g, 4 -> g, 5 -> g)
       gf.pureApply(params, s)
     }
+
+  private def fg = Gen.choose(1, 100).filter(_ >= 3)
+
+  @Benchmark
+  def testFilter(): List[List[Int]] =
+    seeds.map { s =>
+      Gen.listOfN(100, fg).pureApply(params, s)
+    }
 }

project/codegen.scala

@@ -84,20 +84,13 @@ object codegen {
 
   def zip(i: Int) = {
     val gens = flatMappedGenerators(i, idents("t",i) zip idents("g",i))
-
-    def sieveCopy = idents("g",i) zip idents("t",i) map { case (g,t) => s"$g.sieveCopy($t)" } mkString " && "
     s"""
         |  /** Combines the given generators into one generator that produces a
         |   *  tuple of their generated values. */
         |  def zip[${types(i)}](
         |    ${wrappedArgs("Gen",i)}
-        |  ): Gen[(${types(i)})] = {
-        |    val g = $gens
-        |    g.suchThat {
-        |      case (${vals(i)}) =>
-        |        ${sieveCopy}
-        |    }
-        |  }
+        |  ): Gen[(${types(i)})] =
+        |    $gens
         |""".stripMargin
   }
 

src/main/scala/org/scalacheck/Gen.scala

@@ -33,12 +33,12 @@ sealed abstract class Gen[+T] extends Serializable { self =>
   /** Just an alias */
   private type P = Gen.Parameters
 
-  /** Should be a copy of R.sieve. Used internally in Gen when some generators
-   *  with suchThat-clause are created (when R is not available). This method
-   *  actually breaks covariance, but since this method will only ever be
-   *  called with a value of exactly type T, it is OK. */
+  // This is no longer used but preserved here for binary compatibility.
   private[scalacheck] def sieveCopy(x: Any): Boolean = true
 
+  // If you implement new Gen[_] directly (instead of using
+  // combinators), make sure to use p.initialSeed or p.useInitialSeed
+  // in the implementation, instead of using seed directly.
   private[scalacheck] def doApply(p: P, seed: Seed): R[T]
 
   //// Public interface ////
@@ -104,16 +104,14 @@ sealed abstract class Gen[+T] extends Serializable { self =>
    *  the generator fails (returns None). Also, make sure that the provided
    *  test property is side-effect free, e.g. it should not use external vars.
    *  This method is identical to [Gen.filter]. */
-  def suchThat(f: T => Boolean): Gen[T] = new Gen[T] {
-    def doApply(p: P, seed: Seed) =
-      p.useInitialSeed(seed) { (p0, s0) =>
-        val res = self.doApply(p0, s0)
-        res.copy(s = { (x:T) => res.sieve(x) && f(x) })
-      }
-    override def sieveCopy(x: Any) =
-      try self.sieveCopy(x) && f(x.asInstanceOf[T])
-      catch { case _: java.lang.ClassCastException => false }
-  }
+  def suchThat(f: T => Boolean): Gen[T] =
+    new Gen[T] {
+      def doApply(p: P, seed: Seed): Gen.R[T] =
+        p.useInitialSeed(seed) { (p0, s0) =>
+          val r = self.doApply(p0, s0)
+          r.copy(r = r.retrieve.filter(f))
+        }
+    }
 
   case class RetryUntilException(n: Int) extends RuntimeException(s"retryUntil failed after $n attempts")
 
@@ -182,7 +180,6 @@ sealed abstract class Gen[+T] extends Serializable { self =>
         val r = self.doApply(p0, s0)
         r.copy(l = r.labels + l)
       }
-    override def sieveCopy(x: Any) = self.sieveCopy(x)
   }
 
   /** Put a label on the generator to make test reports clearer */
@@ -215,23 +212,21 @@ object Gen extends GenArities with GenVersionSpecific {
 
   private[scalacheck] trait R[+T] {
     def labels: Set[String] = Set()
-    def sieve[U >: T]: U => Boolean = _ => true
+    // sieve is no longer used but preserved for binary compatibility
+    final def sieve[U >: T]: U => Boolean = (_: U) => true
     protected def result: Option[T]
     def seed: Seed
 
-    def retrieve: Option[T] = result.filter(sieve)
+    def retrieve: Option[T] = result
 
     def copy[U >: T](
       l: Set[String] = this.labels,
+      // s is no longer used but preserved for binary compatibility
       s: U => Boolean = this.sieve,
       r: Option[U] = this.result,
       sd: Seed = this.seed
     ): R[U] = new R[U] {
       override val labels = l
-      override def sieve[V >: U] = { (x: Any) =>
-        try s(x.asInstanceOf[U])
-        catch { case _: java.lang.ClassCastException => false }
-      }
       val seed = sd
       val result = r
     }
@@ -245,7 +240,7 @@ object Gen extends GenArities with GenVersionSpecific {
           r(None, seed).copy(l = labels)
         case Some(t) =>
           val r = f(t)
-          r.copy(l = labels ++ r.labels, sd = r.seed)
+          r.copy(l = labels | r.labels, sd = r.seed)
       }
   }
 
@@ -477,7 +472,6 @@ object Gen extends GenArities with GenVersionSpecific {
   private[scalacheck] def failed[T](seed0: Seed): R[T] =
     new R[T] {
       val result: Option[T] = None
-      override def sieve[U >: T]: U => Boolean = _ => false
       val seed = seed0
     }
 
@@ -581,7 +575,7 @@ object Gen extends GenArities with GenVersionSpecific {
   /** Picks a random generator from a list */
   def oneOf[T](g0: Gen[T], g1: Gen[T], gn: Gen[T]*): Gen[T] = {
     val gs = g0 +: g1 +: gn
-    choose(0,gs.size-1).flatMap(gs(_)).suchThat(x => gs.exists(_.sieveCopy(x)))
+    choose(0, gs.size - 1).flatMap(i => gs(i))
   }
 
   /** Makes a generator result optional. Either `Some(T)` or `None` will be provided. */
@@ -609,9 +603,7 @@ object Gen extends GenArities with GenVersionSpecific {
         builder += ((total, value))
       }
       val tree = builder.result
-      choose(1L, total).flatMap(r => tree.rangeFrom(r).head._2).suchThat { x =>
-        gs.exists(_._2.sieveCopy(x))
-      }
+      choose(1L, total).flatMap(r => tree.rangeFrom(r).head._2)
     }
   }
 
@@ -635,11 +627,31 @@ object Gen extends GenArities with GenVersionSpecific {
    *  complete container generator will also fail. */
   def buildableOfN[C,T](n: Int, g: Gen[T])(implicit
     evb: Buildable[T,C], evt: C => Traversable[T]
-  ): Gen[C] =
-    sequence[C,T](Traversable.fill(n)(g)) suchThat { c =>
-      // TODO: Can we guarantee c.size == n (See issue #89)?
-      evt(c).forall(g.sieveCopy)
+  ): Gen[C] = {
+    require(n >= 0, s"invalid size given: $n")
+    new Gen[C] {
+      def doApply(p: P, seed0: Seed): R[C] = {
+        var seed: Seed = p.initialSeed.getOrElse(seed0)
+        val bldr = evb.builder
+        val allowedFailures = Gen.collectionRetries(n)
+        var failures = 0
+        var count = 0
+        while (count < n) {
+          val res = g.doApply(p, seed)
+          res.retrieve match {
+            case Some(t) =>
+              bldr += t
+              count += 1
+            case None =>
+              failures += 1
+              if (failures >= allowedFailures) return r(None, res.seed)
+          }
+          seed = res.seed
+        }
+        r(Some(bldr.result), seed)
+      }
     }
+  }
 
   /** Generates a container of any Traversable type for which there exists an
    *  implicit [[org.scalacheck.util.Buildable]] instance. The elements in the
@@ -648,10 +660,8 @@ object Gen extends GenArities with GenVersionSpecific {
   def buildableOf[C,T](g: Gen[T])(implicit
     evb: Buildable[T,C], evt: C => Traversable[T]
   ): Gen[C] =
-    sized(s => choose(0, Integer.max(s, 0)).flatMap(n => buildableOfN[C, T](n, g)(evb, evt)))
-    .suchThat { c =>
-      if (c == null) g.sieveCopy(null) else evt(c).forall(g.sieveCopy)
-    }
+    sized(s => choose(0, Integer.max(s, 0)))
+      .flatMap(n => buildableOfN(n, g)(evb, evt))
 
   /** Generates a non-empty container of any Traversable type for which there
    *  exists an implicit [[org.scalacheck.util.Buildable]] instance. The
@@ -661,17 +671,17 @@ object Gen extends GenArities with GenVersionSpecific {
   def nonEmptyBuildableOf[C,T](g: Gen[T])(implicit
     evb: Buildable[T,C], evt: C => Traversable[T]
   ): Gen[C] =
-    sized(s => choose(1, Integer.max(s, 1)).flatMap(n => buildableOfN[C, T](n, g)(evb, evt))).suchThat(c => evt(c).size > 0)
+    buildableOf(g)(evb, evt).suchThat(c => evt(c).size > 0)
 
   /** A convenience method for calling `buildableOfN[C[T],T](n,g)`. */
   def containerOfN[C[_],T](n: Int, g: Gen[T])(implicit
     evb: Buildable[T,C[T]], evt: C[T] => Traversable[T]
-  ): Gen[C[T]] = buildableOfN(n, g)(evb, evt)
+  ): Gen[C[T]] = buildableOfN[C[T], T](n, g)(evb, evt)
 
   /** A convenience method for calling `buildableOf[C[T],T](g)`. */
   def containerOf[C[_],T](g: Gen[T])(implicit
     evb: Buildable[T,C[T]], evt: C[T] => Traversable[T]
-  ): Gen[C[T]] = buildableOf(g)(evb, evt)
+  ): Gen[C[T]] = buildableOf[C[T], T](g)(evb, evt)
 
   /** A convenience method for calling `nonEmptyBuildableOf[C[T],T](g)`. */
   def nonEmptyContainerOf[C[_],T](g: Gen[T])(implicit
@@ -706,17 +716,27 @@ object Gen extends GenArities with GenVersionSpecific {
    *  is equal to calling <code>containerOfN[Map,(T,U)](n,g)</code>. */
   def mapOfN[T,U](n: Int, g: Gen[(T, U)]) = buildableOfN[Map[T, U],(T, U)](n, g)
 
-  /** Generates an infinite stream. */
+  /**
+   * Generates an infinite stream.
+   *
+   * Failures in the underlying generator may terminate the stream.
+   * Otherwise it will continue forever.
+   */
   def infiniteStream[T](g: => Gen[T]): Gen[Stream[T]] = {
-    def unfold[A, S](z: S)(f: S => Option[(A, S)]): Stream[A] = f(z) match {
-      case Some((h, s)) => h #:: unfold(s)(f)
-      case None => Stream.empty
-    }
-    gen { (p, seed0) =>
-      new R[Stream[T]] {
-        val result: Option[Stream[T]] = Some(unfold(seed0)(s => Some(g.pureApply(p, s) -> s.next)))
-        val seed: Seed = seed0.next
+    val attemptsPerItem = 10
+    def unfold(p: P, seed: Seed, attemptsLeft: Int): Stream[T] =
+      if (attemptsLeft <= 0) {
+        Stream.empty
+      } else {
+        val r = g.doPureApply(p, seed)
+        r.retrieve match {
+          case Some(t) => t #:: unfold(p, r.seed, attemptsPerItem)
+          case None => unfold(p, r.seed, attemptsLeft - 1)
+        }
       }
+    gen { (p, seed0) =>
+      val stream = unfold(p, seed0, attemptsPerItem)
+      r(Some(stream), seed0.slide)
     }
   }
 
@@ -757,7 +777,7 @@ object Gen extends GenArities with GenVersionSpecific {
           buf += t
         } else {
           val (x, s) = seed.long
-          val i = (x & 0x7fffffff).toInt % count
+          val i = (x & Long.MaxValue % count).toInt
           if (i < n) buf(i) = t
           seed = s
         }
@@ -1003,4 +1023,8 @@ object Gen extends GenArities with GenVersionSpecific {
     1 -> const(Duration.Undefined),
     1 -> const(Duration.Zero),
     6 -> finiteDuration)
+
+  // used to calculate how many per-item retries we should allow.
+  private def collectionRetries(n: Int): Int =
+    Integer.max(10, n / 10)
 }

src/main/scala/org/scalacheck/Prop.scala

@@ -779,7 +779,7 @@ object Prop {
     }
 
     def shrinker(x: T, r: Result, shrinks: Int, orig: T): Result = {
-      val xs = shrink(x).filter(gr.sieve)
+      val xs = shrink(x)
       val res = r.addArg(Arg(labels,x,shrinks,orig,pp(x),pp(orig)))
       if(xs.isEmpty) res else getFirstFailure(xs) match {
         case Right((x2,r2)) => res

src/main/scala/org/scalacheck/util/Buildable.scala

@@ -22,39 +22,14 @@ trait Buildable[T,C] extends Serializable {
 
 object Buildable extends BuildableVersionSpecific {
   import java.util.ArrayList
-  implicit def buildableArrayList[T]: Buildable[T, ArrayList[T]] = new Buildable[T,ArrayList[T]] {
-    def builder = new ArrayListBuilder[T]
-  }
+  implicit def buildableArrayList[T]: Buildable[T, ArrayList[T]] =
+    new Buildable[T, ArrayList[T]] {
+      def builder = new ArrayListBuilder[T]
+    }
 
   implicit def buildableSeq[T]: Buildable[T, Seq[T]] =
     new Buildable[T, Seq[T]] {
       def builder: mutable.Builder[T, Seq[T]] =
         Seq.newBuilder[T]
     }
 }
-
-/*
-object Buildable2 {
-
-  implicit def buildableMutableMap[T,U] = new Buildable2[T,U,mutable.Map] {
-    def builder = mutable.Map.newBuilder
-  }
-
-  implicit def buildableImmutableMap[T,U] = new Buildable2[T,U,immutable.Map] {
-    def builder = immutable.Map.newBuilder
-  }
-
-  implicit def buildableMap[T,U] = new Buildable2[T,U,Map] {
-    def builder = Map.newBuilder
-  }
-
-  implicit def buildableImmutableSortedMap[T: Ordering, U] = new Buildable2[T,U,immutable.SortedMap] {
-    def builder = immutable.SortedMap.newBuilder
-  }
-
-  implicit def buildableSortedMap[T: Ordering, U] = new Buildable2[T,U,SortedMap] {
-    def builder = SortedMap.newBuilder
-  }
-
-}
-*/