-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Don't depend on random sampling to determine function equivalence (#2577
) * Don't depend on random sampling to determine function equivalence This is a work in progress and there is a bit more work that should probably be done before merging this. However, I have already put a fair amount of time into this and wanted to see what people thought about it before pushing through to do all of the relevant work. Cats has a lot of instances for function-like types. When we go to check the laws for these types, we are required to supply an `Eq` instance. But defining equality for functions is awkward. So far we've been approaching this by generating a bunch of values and passing them into both functions and ensuring that we get the same results from both. This can produce false positives if two functions differ but we just happen to sample values that produce the same output in both. For some purposes, it isn't a big deal if we get some occasional false positives, because over many runs of the tests with different RNG seeds, we should eventually catch any discrepancies. But here be dragons. Some tests use the results of these equality checks on the left side of an implication, so a false positive in the equality check becomes a false _negative_ (and thus a build failure) in the test result. See [here](#1666 (comment)) for further discussion. This is where my adventure with this PR begins. Cats builds have been timing out quite a bit recently, so I tried to reduce the number of random values that we sample when comparing two functions for equality. While this did speed up the build a little bit, it started leading to a much higher frequency of build failures due to false negatives in tests. So I started to rethink how we determine function equivalence. Instead of relying on nondeterministic behavior for equality, we can only provide function equality for functions whose domains are small enough to exhaustively check. If two functions produce the same output for the entirety of their domain, they are equivalent. I've introduced an `ExhaustiveCheck[A]` type class that is similar to `Gen[A]` but produces a `Stream[A]` of the entire domain of `A`. I made the name somewhat specific to tests as opposed to something like `Finite[A]`, because types like `Int` have a finite domain but would be prohibitively expensive to exhaustively check in tests and therefore shouldn't have an instance for this type class. I also added some `Eq` instances for function-like types whose domains have `ExhaustiveCheck` instances. For the sake of compatibility, I didn't remove the old `Eq` instances, but I've put them in a lower implicit priority scope, and I've changed the sites that were using them to use the new instances (well not quite all of them yet -- that's why this PR isn't quite complete yet). The main benefits of this change as I see it are: 1. Remove some nondeterministic behavior from the build. 2. Allow for shrinking of the number of values checked to improve build times without triggering build failures. 3. Allow for future deprecation of some problematic instances that are exposed through cats-laws but that users should probably not depend on. The main potential downside that I can think of is that we might be checking 15 examples where we were checking 50 before, which could be considered a reduction in test coverage. However, I think that all of the places where this sort of approach is used are parametric on the type, so I don't think that it should matter much that the domain for this type is much smaller. Let me know what you think. If people like this approach then I can switch over the remaining bits. * Remove ExhaustiveCheck.map method * Fix InvariantMonoidal tests * Fix tests with failing implicit resolution
- Loading branch information
1 parent
1205af2
commit 276a948
Showing
38 changed files
with
1,091 additions
and
606 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
62 changes: 62 additions & 0 deletions
62
laws/src/main/scala/cats/laws/discipline/ExhaustiveCheck.scala
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
package cats | ||
package laws | ||
package discipline | ||
|
||
/** | ||
* An `ExhuastiveCheck[A]` instance can be used similarly to a Scalacheck | ||
* `Gen[A]` instance, but differs in that it generates a `Stream` of the entire | ||
* domain of values as opposed to generating a random sampling of values. | ||
*/ | ||
trait ExhaustiveCheck[A] extends Serializable { self => | ||
def allValues: Stream[A] | ||
} | ||
|
||
object ExhaustiveCheck { | ||
def apply[A](implicit A: ExhaustiveCheck[A]): ExhaustiveCheck[A] = A | ||
|
||
def instance[A](values: Stream[A]): ExhaustiveCheck[A] = new ExhaustiveCheck[A] { | ||
val allValues: Stream[A] = values | ||
} | ||
|
||
implicit val catsLawsExhaustiveCheckForBoolean: ExhaustiveCheck[Boolean] = | ||
instance(Stream(false, true)) | ||
|
||
implicit val catsLawsExhaustiveCheckForSetBoolean: ExhaustiveCheck[Set[Boolean]] = | ||
forSet[Boolean] | ||
|
||
/** | ||
* Warning: the domain of (A, B) is the cross-product of the domain of `A` and the domain of `B`. | ||
*/ | ||
implicit def catsLawsExhaustiveCheckForTuple2[A, B](implicit A: ExhaustiveCheck[A], | ||
B: ExhaustiveCheck[B]): ExhaustiveCheck[(A, B)] = | ||
instance(A.allValues.flatMap(a => B.allValues.map(b => (a, b)))) | ||
|
||
/** | ||
* Warning: the domain of (A, B, C) is the cross-product of the 3 domains. | ||
*/ | ||
implicit def catsLawsExhaustiveCheckForTuple3[A, B, C](implicit A: ExhaustiveCheck[A], | ||
B: ExhaustiveCheck[B], | ||
C: ExhaustiveCheck[C]): ExhaustiveCheck[(A, B, C)] = | ||
instance( | ||
for { | ||
a <- A.allValues | ||
b <- B.allValues | ||
c <- C.allValues | ||
} yield (a, b, c) | ||
) | ||
|
||
implicit def catsLawsExhaustiveCheckForEither[A, B](implicit A: ExhaustiveCheck[A], | ||
B: ExhaustiveCheck[B]): ExhaustiveCheck[Either[A, B]] = | ||
instance(A.allValues.map(Left(_)) ++ B.allValues.map(Right(_))) | ||
|
||
implicit def catsLawsExhaustiveCheckForOption[A](implicit A: ExhaustiveCheck[A]): ExhaustiveCheck[Option[A]] = | ||
instance(Stream.cons(None, A.allValues.map(Some(_)))) | ||
|
||
/** | ||
* Creates an `ExhaustiveCheck[Set[A]]` given an `ExhaustiveCheck[A]` by computing the powerset of | ||
* values. Note that if there are `n` elements in the domain of `A` there will be `2^n` elements | ||
* in the domain of `Set[A]`, so use this only on small domains. | ||
*/ | ||
def forSet[A](implicit A: ExhaustiveCheck[A]): ExhaustiveCheck[Set[A]] = | ||
instance(A.allValues.toSet.subsets.toStream) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
package cats | ||
package laws | ||
package discipline | ||
|
||
import cats.kernel.{BoundedSemilattice, CommutativeGroup, CommutativeMonoid} | ||
import cats.instances.int._ | ||
|
||
/** | ||
* Similar to `Int`, but with a much smaller domain. The exact range of [[MiniInt]] may be tuned from time to time, so | ||
* consumers of this type should avoid depending on its exact range. | ||
* | ||
* `MiniInt` has integer overflow characteristics similar to `Int` (but with a smaller radix), meaning that its addition | ||
* and multiplication are commutative and associative. | ||
*/ | ||
final class MiniInt private (val intBits: Int) extends AnyVal with Serializable { | ||
import MiniInt._ | ||
|
||
def unary_- : MiniInt = this * negativeOne | ||
|
||
def toInt: Int = intBits << intShift >> intShift | ||
|
||
def +(o: MiniInt): MiniInt = wrapped(intBits + o.intBits) | ||
def *(o: MiniInt): MiniInt = wrapped(intBits * o.intBits) | ||
def |(o: MiniInt): MiniInt = wrapped(intBits | o.intBits) | ||
def /(o: MiniInt): MiniInt = wrapped(intBits / o.intBits) | ||
|
||
override def toString: String = s"MiniInt(toInt=$toInt, intBits=$intBits)" | ||
} | ||
|
||
object MiniInt { | ||
val bitCount: Int = 4 | ||
val minIntValue: Int = -8 | ||
val maxIntValue: Int = 7 | ||
private val intShift: Int = 28 | ||
val minValue: MiniInt = unsafeFromInt(minIntValue) | ||
val maxValue: MiniInt = unsafeFromInt(maxIntValue) | ||
val zero: MiniInt = unsafeFromInt(0) | ||
val one: MiniInt = unsafeFromInt(1) | ||
val negativeOne: MiniInt = unsafeFromInt(-1) | ||
|
||
def isInDomain(i: Int): Boolean = i >= minIntValue && i <= maxIntValue | ||
|
||
def fromInt(i: Int): Option[MiniInt] = if (isInDomain(i)) Some(unsafeFromInt(i)) else None | ||
|
||
def wrapped(intBits: Int): MiniInt = new MiniInt(intBits & (-1 >>> intShift)) | ||
|
||
def unsafeFromInt(i: Int): MiniInt = | ||
if (isInDomain(i)) { | ||
new MiniInt(i << intShift >>> intShift) | ||
} else throw new IllegalArgumentException(s"Expected value between $minIntValue and $maxIntValue but got $i") | ||
|
||
val allValues: Stream[MiniInt] = (minIntValue to maxIntValue).map(unsafeFromInt).toStream | ||
|
||
implicit val catsLawsEqInstancesForMiniInt: Order[MiniInt] with Hash[MiniInt] = new Order[MiniInt] | ||
with Hash[MiniInt] { | ||
def hash(x: MiniInt): Int = Hash[Int].hash(x.intBits) | ||
|
||
def compare(x: MiniInt, y: MiniInt): Int = Order[Int].compare(x.toInt, y.toInt) | ||
} | ||
|
||
implicit val catsLawsExhuastiveCheckForMiniInt: ExhaustiveCheck[MiniInt] = | ||
ExhaustiveCheck.instance(allValues) | ||
|
||
val miniIntAddition: CommutativeGroup[MiniInt] = new CommutativeGroup[MiniInt] { | ||
val empty = MiniInt.zero | ||
def combine(x: MiniInt, y: MiniInt): MiniInt = x + y | ||
def inverse(x: MiniInt): MiniInt = -x | ||
} | ||
|
||
val miniIntMultiplication: CommutativeMonoid[MiniInt] = new CommutativeMonoid[MiniInt] { | ||
val empty = MiniInt.one | ||
def combine(x: MiniInt, y: MiniInt): MiniInt = x * y | ||
} | ||
|
||
val miniIntOr: BoundedSemilattice[MiniInt] = new BoundedSemilattice[MiniInt] { | ||
val empty = MiniInt.zero | ||
def combine(x: MiniInt, y: MiniInt): MiniInt = x | y | ||
} | ||
} |
Oops, something went wrong.