fix Gen.{nat,pos}_split{2,}

[gasche]

This is a fix for several bugs reported in #180.

Those functions (and range_subset), implemented by myself in #114, were utter garbage. I had tested their implementation heavily within a different random library, and when I ported them to QCheck it seems that I messed things up considerably (this is fiddly code full of off-by-one traps etc.).

In the present PR I have gone over the implementation of each function and done some manual testing, and I also wrote automated tests in the existing QCheck testsuite. (This forced me to think about the valid input space for the combinators; I extended pos_split to support the case size=0, n=0, and found a treacherous bug when size=1.)

My recommendation for the future would be to encourage new generators to come with their tests.

Sorry for the mess, and thanks @max-lang for the report!

[vch9]

Maybe we could add these generators in QCheck2 as well?

[gasche]

I don't know what the current status is for QCheck vs. QCheck2, is there some explanation somewhere?

[vch9]

I don't know what the current status is for QCheck vs. QCheck2, is there some explanation somewhere?

I believe we kept QCheck for retro-compatibility issues (ping @c-cube, @sir4ur0n). Tests from "QCheck1" are actually migrated to QCheck2:

qcheck/src/core/QCheck2.mli

Line 1753 in b065a81

(** {1:migration_qcheck2 Migration to QCheck2}

.

Because QCheck2 is new (released ~last week), we can afford breaking changes; such as designs changes, remove/rename generators.

[gasche]

I think it would be nice to (review and) merge this PR without waiting too much, because it fixes real bugs in released code. I don't mind duplicating the logic in QCheck2.ml, but I think this is a somewhat independent question that can be handled in another PR.

In any case, I wish the current status of core/QCheck.ml vs. core/QCheck2.ml was clearly stated somewhere, for example in the README, so that other occasional contributors know what to do.

[vch9]

I think it would be nice to (review and) merge this PR without waiting too much, because it fixes real bugs in released code. I don't mind duplicating the logic in QCheck2.ml, but I think this is a somewhat independent question that can be handled in another PR.

yes

In any case, I wish the current status of core/QCheck.ml vs. core/QCheck2.ml was clearly stated somewhere, for example in the README, so that other occasional contributors know what to do.

and yes

[vch9]

LGTM, I tested the bugs reported in #180 with this fix.

(We (I) will make a followup merge request for changelogs, qcheck2, etc.)

[c-cube]

I don't know what the current status is for QCheck vs. QCheck2, is there some explanation somewhere?

That's a good question actually. I think both still have their place, even if only because QCheck is a bit simpler conceptually. Maybe in the future we'll actually deprecate QCheck but I think it'll take years, and will require good confidence that the shrinking of >>= and others is good. For now, in my mind, both are supported.