Avoid empty generators when possible (take 2) #258
Merged
Conversation
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 commit is mostly preparatory -- it doesn't do any major changes other than some basic clean up.
The previous code has to jump through a lot of hoops to try to support both fractional and integral code with one class (based on Numeric). Splitting the problem into two classes (and using the appropriate division/quotient operation for each) makes it a lot easier to see what is going on in each case.
This commit adds a new method to Gen: pureApply. The idea is that this method repeatedly calls doApply until some value is found, or until the number of retries allowed is exhausted. This functionality is especially useful in the function generators, although it's possible to imagine third-party code that would take advantage of this method as well.
When generating generators whose parameters make valid values impossible (e.g. choose(10, 0)), throw an exception rather than returning an empty generator. This commit also improves the logic of the Choose instances for numbers, to make them able to generate values more efficiently when generating the "entire range" of a number type. Several of the tests have to be changed after this, to check for exceptions when impossible parameter values are used.
non
changed the title
|
@rickynils Let me know if this is what you wanted, or if there's anything else I can do to help. |
|
I have a follow-on PR coming that adds some missing cogen instances (including the function instances that @xuwei-k asked for months ago). |
This was referenced Sep 3, 2016
|
Great stuff @non, merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(This PR is a reworked version of #257. I also tried to reduce the
amount of code churn in this PR, even though there's a lot of code
that I'd like to see reformatted at some point.)
This PR was inspired by #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 PR 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.