quickcheck icon indicating copy to clipboard operation
quickcheck copied to clipboard

add back a way to put a bound on numbers generated

Open BurntSushi opened this issue 5 years ago • 14 comments

It turns out that the tests inside byteorder were designed around the ability to control the bounds of integers generated and there is no easy way to adapt them to quickcheck 1.0 without this ability. I think we can just add a bound mutator on Gen that is by default not set.

BurntSushi avatar Jan 08 '21 15:01 BurntSushi

Would this bring back gen_range for the new Gen struct, or offer an equivalently powerful solution?

Previously the Gen trait inherited gen_range from the underlying rand::Rng, but now that it's a struct its gen and gen_range methods are private. Would it be possible to make them public? The usefulness of gen_range in particular is evident given how many of the provided Arbitrary impls for std types utilize it.

dmit avatar Jan 08 '21 22:01 dmit

The intent is to add back methods yes. But I'm not making rand a public dependency.

However, this issue is about something different. This is a configuration knob on Gen itself.

If you would like more methods on Gen, please file new issues with the desired method signatures.

BurntSushi avatar Jan 08 '21 22:01 BurntSushi

As far as I can tell, making existing private methods on Gen public doesn't expose rand as a dependency. They mirror rand's signatures, but don't leak any implementation details.

But will do!

dmit avatar Jan 08 '21 22:01 dmit

It does expose rand. Their type signatures include things from the rand crate.

BurntSushi avatar Jan 08 '21 22:01 BurntSushi

Looking through upgrading to 1.0, and this would be great to have.

Presumably it would be possible to accept impl RangeBounds as a parameter in the same way rand does? That wouldn't expose any implementation details, but would allow for using a similar API.

jhpratt avatar Jan 09 '21 03:01 jhpratt

quickcheck seems severely limited without gen_range and gen_bool.

dvc94ch avatar Jan 19 '21 18:01 dvc94ch

You don't need gen_bool. Arbitrary::bool should work just fine.

It would probably be good to add gen_range, but it doesn't seem particularly difficult to use modulus instead.

BurntSushi avatar Jan 19 '21 19:01 BurntSushi

And once again, this issue is unrelated to adding helper methods in Gen. This is about bounding values generated by Arbitrary impls for number types.

BurntSushi avatar Jan 19 '21 19:01 BurntSushi

I also need this to update some of my code to quickcheck 1.0, specifically to generate floating point numbers in a specific range.

sdroege avatar Feb 13 '21 14:02 sdroege

It would probably be good to add gen_range, but it doesn't seem particularly difficult to use modulus instead.

Floating point values don't have such a convenient hack, unfortunately.

Ralith avatar Mar 01 '21 06:03 Ralith

It would probably be good to add gen_range, but it doesn't seem particularly difficult to use modulus instead.

Modulus breaks shrinking. QuickCheck shrinks the generated number, but your number after % can go from MIN_VALUE to MAX_VALUE. In such case, shrinking the number creates a bigger number. This is not at all helpful or useful.

As an example, the values generated in this manner look like this in failures:

assertion failed: `(left == right)`
  left: `(8, 4, 32, 6)`,
 right: `(8, 4, 32, 8)`

The actual failing case, after shrinking, would be either (2, 0, 0, 2) or (1, 59, 59, 29). Other QuickCheck implementations that allow controlling bounds find these cases immediately.

Modulus is completely wrong for this purpose.

ghost avatar May 22 '21 03:05 ghost

@adam-becker I don't know what you're talking about. I was of course referring to the implementation of Arbitrary::arbitrary, not shrinking. This issue has nothing to do with shrinking, which doesn't even get access to a Gen: https://github.com/BurntSushi/quickcheck/blob/defde6fb0ce20b0c8c4e672aa9ae821f7d1f5b38/src/arbitrary.rs#L732-L768

BurntSushi avatar May 22 '21 11:05 BurntSushi

What is the status of this issue? The number of issues from other projects referencing this one is a clear sign people want this resolved.

P.S. A quick check (no pun intended) shows at least one project implemented this functionality on top of QuickCheck, https://github.com/libp2p/rust-libp2p/pull/2857, https://github.com/libp2p/rust-libp2p/blob/master/misc/quickcheck-ext/src/lib.rs. However, I am not sure their implementation is sound when shrinking, and it only works for unsigned numbers.

justinlovinger avatar Oct 23 '22 16:10 justinlovinger

Please add back gen_range or equivalent

Colossus avatar May 23 '23 17:05 Colossus