Improve integer shrinking

project/MimaSettings.scala

@@ -18,6 +18,7 @@ object MimaSettings {
   )
 
   private def removedPrivateMethods = Seq(
+    "org.scalacheck.ShrinkIntegral.skipNegation",
     "org.scalacheck.util.Buildable.buildableSeq"
   )
 

src/main/scala/org/scalacheck/Shrink.scala

@@ -238,26 +238,31 @@ object Shrink extends ShrinkLowPriority with ShrinkVersionSpecific with time.Jav
 }
 
 final class ShrinkIntegral[T](implicit ev: Integral[T]) extends Shrink[T] {
-  import ev.{ fromInt, gteq, quot, negate, equiv, zero, one }
+  import ev.{ fromInt, quot, negate, equiv, zero, lt, minus }
 
   val two = fromInt(2)
 
-  // see if T supports negative values or not. this makes some
-  // assumptions about how Integral[T] is defined, which work for
-  // Integral[Char] at least. we can't be sure user-defined
-  // Integral[T] instances will be reasonable.
-  val skipNegation = gteq(negate(one), one)
+  // We shrink x to ceil(x * (1 - 1/2^i)) for i = 0,1,….  We also shrink x
+  // to -x if x < 0 < -x (implying x != MinValue for two's complement types).
 
-  // assumes x is non-zero.
-  private def halves(x: T): Stream[T] = {
-    val q = quot(x, two)
-    if (equiv(q, zero)) Stream(zero)
-    else if (skipNegation) q #:: halves(q)
-    else q #:: negate(q) #:: halves(q)
+  // We assume that x - ((((x/2)/2)/...)/2) = x for some repetition count;
+  // otherwise shrinking may diverge.  It holds if x - 0 = x and 0 is closer
+  // to x/2 than to x and there are finitely many values y such that 0 is
+  // closer to y than to x: then the sequence x, x/2, (x/2)/2, ... eventually
+  // arrives at 0, but then (x - x/2/2/.../2) = x - 0 = x.
+
+  private def bisectFromZeroToX(x: T, current: T): Stream[T] = {
+    val head = minus(x, current)
+
+    if (equiv(head, x)) Stream.empty
+    else head #:: bisectFromZeroToX(x, quot(current, two))
   }
 
-  def shrink(x: T): Stream[T] =
-    if (equiv(x, zero)) Stream.empty[T] else halves(x)
+  def shrink(x: T): Stream[T] = {
+    lazy val approach = bisectFromZeroToX(x, x)
+    if (lt(x, zero) && lt(zero, negate(x))) negate(x) #:: approach
+    else approach
+  }
 }
 
 final class ShrinkFractional[T](implicit ev: Fractional[T]) extends Shrink[T] {

src/test/scala/org/scalacheck/ShrinkSpecification.scala

@@ -124,44 +124,61 @@ object ShrinkSpecification extends Properties("Shrink") {
 
   /* Ensure that shrink[T] terminates. (#244)
    *
-   * Let's say shrinking "terminates" when the stream of values
-   * becomes empty. We can empirically determine the longest possible
-   * sequence for a given type before termination. (Usually this
-   * involves using the type's MinValue.)
+   * Shrinks must be acyclic, otherwise the shrinking process loops.
    *
-   * For example, shrink(Byte.MinValue).toList gives us 15 values:
+   * A cycle is a set of values $x_1, x_2, ..., x_n, x_{n+1} = x_1$ such
+   * that $shrink(x_i).contains(x_{i+1})$ for all i.  If the shrinking to a
+   * minimal counterexample ever encounters a cycle, it will loop forever.
    *
-   *   List(-64, 64, -32, 32, -16, 16, -8, 8, -4, 4, -2, 2, -1, 1, 0)
+   * To prove that a shrink is acyclic you can prove that all shrinks are
+   * smaller than the shrinkee, for some strict partial ordering (proof: by
+   * transitivity conclude that x_i < x_i which violates anti-reflexivity.)
+   *
+   * Shrinking of numeric types is ordered by magnitude and then sign, where
+   * positive goes before negative, i.e. x may shrink to -x when x < 0 < -x.
+   *
+   * For unsigned types (e.g. Char) this is the standard ordering (<).
+   * For signed types, m goes before n iff |m| < |n| or m = -n > 0.
+   * (Be careful about abs(MinValue) representation issues.)
+   *
+   * Also, for each shrinkee the stream of shrunk values must be finite.  We
+   * can empirically determine the length of the longest possible stream for a
+   * given type.  Usually this involves using the type's MinValue in the case
+   * of fractional types, or MinValue for integral types.
+   *
+   * For example, shrink(Byte.MinValue).toList gives us 8 values:
+   *
+   *   List(0, -64, -96, -112, -120, -124, -126, -127)
    *
    * Similarly, shrink(Double.MinValue).size gives us 2081.
    */
 
   property("shrink[Byte].nonEmpty") =
-    forAllNoShrink((n: Byte) => Shrink.shrink(n).drop(15).isEmpty)
+    forAllNoShrink((n: Byte) => Shrink.shrink(n).drop(8).isEmpty)
 
   property("shrink[Char].nonEmpty") =
     forAllNoShrink((n: Char) => Shrink.shrink(n).drop(16).isEmpty)
 
   property("shrink[Short].nonEmpty") =
-    forAllNoShrink((n: Short) => Shrink.shrink(n).drop(31).isEmpty)
+    forAllNoShrink((n: Short) => Shrink.shrink(n).drop(16).isEmpty)
 
   property("shrink[Int].nonEmpty") =
-    forAllNoShrink((n: Int) => Shrink.shrink(n).drop(63).isEmpty)
+    forAllNoShrink((n: Int) => Shrink.shrink(n).drop(32).isEmpty)
 
   property("shrink[Long].nonEmpty") =
-    forAllNoShrink((n: Long) => Shrink.shrink(n).drop(127).isEmpty)
+    forAllNoShrink((n: Long) => Shrink.shrink(n).drop(64).isEmpty)
 
   property("shrink[Float].nonEmpty") =
-    forAllNoShrink((n: Float) => Shrink.shrink(n).drop(2081).isEmpty)
+    forAllNoShrink((n: Float) => Shrink.shrink(n).drop(289).isEmpty)
 
   property("shrink[Double].nonEmpty") =
     forAllNoShrink((n: Double) => Shrink.shrink(n).drop(2081).isEmpty)
 
   property("shrink[FiniteDuration].nonEmpty") =
-    forAllNoShrink((n: FiniteDuration) => Shrink.shrink(n).drop(2081).isEmpty)
+    forAllNoShrink((n: FiniteDuration) => Shrink.shrink(n).drop(64).isEmpty)
 
   property("shrink[Duration].nonEmpty") =
-    forAllNoShrink((n: Duration) => Shrink.shrink(n).drop(2081).isEmpty)
+    forAllNoShrink((n: Duration) => Shrink.shrink(n).drop(64).isEmpty)
 
   // make sure we handle sentinel values appropriately for Float/Double.
 
@@ -191,4 +208,172 @@ object ShrinkSpecification extends Properties("Shrink") {
 
   property("shrink(Duration.Undefined)") =
     Prop(Shrink.shrink(Duration.Undefined: Duration).isEmpty)
+
+  // That was finiteness of a single step of shrinking.  Now let's prove that
+  // you cannot shrink for infinitely many steps, by showing that shrinking
+  // always goes to smaller values, ordered by magnitude and then sign.
+  // This is a strict partial ordering, hence there can be no cycles.  It is
+  // also a well-ordering on BigInt, and all other types we test are finite,
+  // hence there can be no infinite regress.
+
+  def numericMayShrinkTo[T: Numeric](n: T, m: T): Boolean = {
+    val num = implicitly[Numeric[T]]
+    import num.{abs, equiv, lt, zero}
+    lt(abs(m), abs(n)) || (lt(n, zero) && equiv(m, abs(n)))
+  }
+
+  def twosComplementMayShrinkTo[T: Integral: TwosComplement](n: T, m: T): Boolean =
+  {
+    val lowerBound = implicitly[TwosComplement[T]].minValue
+    val integral = implicitly[Integral[T]]
+    import integral.{abs, equiv, lt, zero}
+
+    // Note: abs(minValue) = minValue < 0 for two's complement signed types
+    require(equiv(lowerBound, abs(lowerBound)))
+    require(lt(abs(lowerBound), zero))
+
+    // Due to this algebraic issue, we have to special case `lowerBound`
+    if (n == lowerBound) m != lowerBound
+    else if (m == lowerBound) false
+    else numericMayShrinkTo(n, m) // simple algebra Just Works(TM)
+  }
+
+  case class TwosComplement[T](minValue: T)
+  implicit val minByte: TwosComplement[Byte] = TwosComplement(Byte.MinValue)
+  implicit val minShort: TwosComplement[Short] = TwosComplement(Short.MinValue)
+  implicit val minInt: TwosComplement[Int] = TwosComplement(Integer.MIN_VALUE)
+  implicit val minLong: TwosComplement[Long] = TwosComplement(Long.MinValue)
+
+  // Let's first verify that this is in fact a strict partial ordering.
+  property("twosComplementMayShrinkTo is antireflexive") =
+    forAllNoShrink { (n: Int) => !twosComplementMayShrinkTo(n, n) }
+
+  val transitive = for {
+    a <- Arbitrary.arbitrary[Int]
+    b <- Arbitrary.arbitrary[Int]
+    if twosComplementMayShrinkTo(a, b)
+    c <- Arbitrary.arbitrary[Int]
+    if twosComplementMayShrinkTo(b, c)
+  } yield twosComplementMayShrinkTo(a, c)
+
+  property("twosComplementMayShrinkTo is transitive") =
+    forAllNoShrink(transitive.retryUntil(Function.const(true)))(identity)
+
+  // let's now show that shrinks are acyclic for integral types
+
+  property("shrink[Byte] is acyclic") = forAllNoShrink { (n: Byte) =>
+    shrink(n).forall(twosComplementMayShrinkTo(n, _))
+  }
+
+  property("shrink[Short] is acyclic") = forAllNoShrink { (n: Short) =>
+    shrink(n).forall(twosComplementMayShrinkTo(n, _))
+  }
+
+  property("shrink[Char] is acyclic") = forAllNoShrink { (n: Char) =>
+    shrink(n).forall(numericMayShrinkTo(n, _))
+  }
+
+  property("shrink[Int] is acyclic") = forAllNoShrink { (n: Int) =>
+    shrink(n).forall(twosComplementMayShrinkTo(n, _))
+  }
+
+  property("shrink[Long] is acyclic") = forAllNoShrink { (n: Long) =>
+    shrink(n).forall(twosComplementMayShrinkTo(n, _))
+  }
+
+  property("shrink[BigInt] is acyclic") = forAllNoShrink { (n: BigInt) =>
+    shrink(n).forall(numericMayShrinkTo(n, _))
+  }
+
+  property("shrink[Float] is acyclic") = forAllNoShrink { (x: Float) =>
+    shrink(x).forall(numericMayShrinkTo(x, _))
+  }
+
+  property("shrink[Double] is acyclic") = forAllNoShrink { (x: Double) =>
+    shrink(x).forall(numericMayShrinkTo(x, _))
+  }
+
+  property("shrink[Duration] is acyclic") = forAllNoShrink { (x: Duration) =>
+    shrink(x).forall(y => twosComplementMayShrinkTo(x.toNanos, y.toNanos))
+  }
+
+  property("shrink[FiniteDuration] is acyclic") =
+    forAllNoShrink { (x: FiniteDuration) =>
+      shrink(x).forall(y => twosComplementMayShrinkTo(x.toNanos, y.toNanos))
+    }
+
+  // Recursive integral shrinking stops at a success/failure boundary,
+  // i.e. some m such that m fails and m-1 succeeds if 0 < m and m+1
+  // succeeds if m < 0, or shrinks to 0.
+  //
+  // Test that shrink(n) contains n-1 if positive or n+1 if negative.
+  //
+  // From this our conclusion follows:
+  //  - If 0 < n and n fails and n-1 fails then we can shrink to n-1.
+  //  - If n < 0 and n fails and n+1 fails then we can shrink to n+1.
+  // In neither case do we stop shrinking at n.
+  //
+  // Since shrinking only stops at failing values, we stop shrinking at:
+  //  - Some n such that 0 < n and n fails and n-1 succeeds
+  //  - Some n such that n < 0 and n fails and n+1 succeeds
+  //  - 0
+  // which is exactly what we wanted to conclude.
+
+  def stepsByOne[T: Arbitrary: Numeric: Shrink]: Prop = {
+    val num = implicitly[Numeric[T]]
+    import num.{equiv, lt, negate, one, plus, zero}
+    val minusOne = negate(one)
+
+    forAll {
+      (n: T) => (!equiv(n, zero)) ==> {
+        val delta = if (lt(n, zero)) one else minusOne
+        shrink(n).contains(plus(n, delta))
+      }
+    }
+  }
+
+  property("shrink[Byte](n).contains(n - |n|/n)") = stepsByOne[Byte]
+  property("shrink[Short](n).contains(n - |n|/n)") = stepsByOne[Short]
+  property("shrink[Char](n).contains(n - |n|/n)") = stepsByOne[Char]
+  property("shrink[Int](n).contains(n - |n|/n)") = stepsByOne[Int]
+  property("shrink[Long](n).contains(n - |n|/n)") = stepsByOne[Long]
+  property("shrink[BigInt](n).contains(n - |n|/n)") = stepsByOne[BigInt]
+
+  // As a special case of the above, if n succeeds iff lo < n < hi for some
+  // pair of limits lo <= 0 <= hi, then shrinking stops at lo or hi.  Let's
+  // test this concrete consequence.
+
+  def minimalCounterexample[T: Shrink](ok: T => Boolean, x: T): T =
+    shrink(x).dropWhile(ok).headOption.fold(x)(minimalCounterexample(ok, _))
+
+  def findsBoundary[T: Arbitrary: Numeric: Shrink]: Prop = {
+    val num = implicitly[Numeric[T]]
+    import num.{lt, lteq, zero}
+
+    def valid(lo: T, hi: T, start: T): Boolean =
+      lteq(lo, zero) && lteq(zero, hi) && (lteq(start, lo) || lteq(hi, start))
+
+    forAll(Arbitrary.arbitrary[(T, T, T)].retryUntil((valid _).tupled)) {
+      case (lo, hi, start) => valid(lo, hi, start) ==> {
+        val ok = (n: T) => lt(lo, n) && lt(n, hi)
+        val stop = minimalCounterexample[T](ok, start)
+        s"($lo, $hi, $start) => $stop" |: (stop == lo || stop == hi)
+      }
+    }
+  }
+
+  property("shrink finds the exact boundary: Byte")   = findsBoundary[Byte]
+  property("shrink finds the exact boundary: Short")  = findsBoundary[Short]
+  property("shrink finds the exact boundary: Int")    = findsBoundary[Int]
+  property("shrink finds the exact boundary: Long")   = findsBoundary[Long]
+  property("shrink finds the exact boundary: BigInt") = findsBoundary[BigInt]
+
+  // Unsigned types are one-sided.  Test on the range (0 until limit).
+  property("shrink finds the exact boundary: Char")   = forAll {
+    (a: Char, b: Char) =>
+    val (limit, start) = (a min b, a max b)
+    require(limit <= start)
+    val result = minimalCounterexample[Char](_ < limit, start)
+    s"(${limit.toInt}, ${start.toInt}) => ${result.toInt}" |: result == limit
+  }
 }