forked from typelevel/cats
-
Notifications
You must be signed in to change notification settings - Fork 0
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
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](typelevel#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.
- Loading branch information
Showing
33 changed files
with
572 additions
and
430 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
55 changes: 55 additions & 0 deletions
55
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,55 @@ | ||
| 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] | ||
|
|
||
| def map[B](f: A => B): ExhaustiveCheck[B] = new ExhaustiveCheck[B] { | ||
| def allValues: Stream[B] = self.allValues.map(f) | ||
| } | ||
| } | ||
|
|
||
| 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)) | ||
|
|
||
| /** | ||
| * 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(_)))) | ||
| } |
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,36 @@ | ||
| package cats | ||
| package laws | ||
| package discipline | ||
|
|
||
| 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. | ||
| */ | ||
| final class MiniInt private (val asInt: Int) extends AnyVal with Serializable | ||
|
|
||
| object MiniInt { | ||
| val minIntValue: Int = -7 | ||
| val maxIntValue: Int = 7 | ||
|
|
||
| def isInDomain(i: Int): Boolean = i >= minIntValue && i <= maxIntValue | ||
|
|
||
| def apply(i: Int): Option[MiniInt] = if (isInDomain(i)) Some(new MiniInt(i)) else None | ||
|
|
||
| def unsafeApply(i: Int): MiniInt = | ||
| if (isInDomain(i)) new MiniInt(i) | ||
| else throw new IllegalArgumentException(s"Expected value between $minIntValue and $maxIntValue but got $i") | ||
|
|
||
| val allValues: Stream[MiniInt] = (minIntValue to maxIntValue).map(unsafeApply).toStream | ||
|
|
||
| implicit val catsLawsEqInstancesForMiniInt: Order[MiniInt] with Hash[MiniInt] = new Order[MiniInt] | ||
| with Hash[MiniInt] { | ||
| def hash(x: MiniInt): Int = Hash[Int].hash(x.asInt) | ||
|
|
||
| def compare(x: MiniInt, y: MiniInt): Int = Order[Int].compare(x.asInt, y.asInt) | ||
| } | ||
|
|
||
| implicit val catsLawsExhuastiveCheckForMiniInt: ExhaustiveCheck[MiniInt] = | ||
| ExhaustiveCheck.instance(allValues) | ||
| } |
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
Oops, something went wrong.