BurntSushi / quickcheck

Automated property based testing for Rust (with shrinking).
The Unlicense
2.4k stars 149 forks source link

Feature-gated implementation of `rand_core::RngCore` for `quickcheck::Gen` #271

Closed LukeMathWalker closed 3 years ago

LukeMathWalker commented 3 years ago

Rationale

The latest version of quickcheck, 1.x.x, has transformed Gen into a struct which does not implement the rand_core::RngCore trait.
Reading through https://github.com/BurntSushi/quickcheck/issues/241 it seems the main motivation is API stability: rand_core has been publishing minor releases at a much faster pace than quickcheck and the API does not yet seem stable enough to allow quickcheck to cut a 1.0 release without having to plan for a 2.0 shortly afterwards.
The other motivation mentioned in the RFC, dependency bloat, seems to have been archived due to a restructuring in rand's feature flagging system - rand is in fact a dependency of quickcheck in 1.0.

While I appreciate the driver behind the decision the outcome seems to damage interoperability across the ecosystem - with all its issues, rand_core::RngCore is still the reference trait for crates that want to allow a pluggable source of randomness (e.g. fake).
I believe there is path forward that allows quickcheck to avoid unnecessary breaking changes while enabling users that rely on rand_core for interoperability to get a Gen struct that implements RngCore - the present PR.
There is no denying that somebody has to pay the bill for rand_core's frequent breaking changes: this PR adds complexity that quickcheck's maintainers will have to look after going forward, therefore I understand if you don't want to merge it @BurntSushi. But I thought it was worth a shot to flesh it out as a PoC for your evaluation 😁

Changes

Add an implementation of rand_core::RngCore for Gen behind an optional (disabled by default) feature flag - rand_core_0_6.

The features flags are structured to allow backwards-compatible additions of new RngCore implementations if rand_core were to cut a new release with breaking changes (either 0.7 or 1.x).

BurntSushi commented 3 years ago

I'm never making rand (or likely any other crate) a public dependency of quickcheck, whether opt-in or not.

Making it opt-in is arguably even more maintenance work, and does nothing to help with semver. If rand_core does a breaking change release, then I have to do the same. Or otherwise retain support for each breaking change release of rand_core with distinct features.

One thing I would be open to doing is exposing the methods required to implement RngCore. Then one can add their own wrapper type with a trivial implementation. It's not that convenient, but I think it's likely not often needed. I am also willing to add more methods on Gen that re-export parts of rand's API that are commonly used in implementations of Arbitrary. (But without making rand a public dependency.)

BurntSushi commented 3 years ago

Note that I would be open to making rand_core a public dependency if and when it reaches 1.0.

LukeMathWalker commented 3 years ago

Understood 👌🏼

pickfire commented 3 years ago

So the possible workarounds as of now is to pin quickcheck to 0.9 I believe as in https://github.com/LukeMathWalker/zero-to-production/issues/34#issuecomment-767026290.

BurntSushi commented 3 years ago

@pickfire If you need Gen to be trait or want rand_core to be a public dependency, then yes, you'll be stuck on quickcheck 0.9 forever or you'll need to move to some other crate. That doesn't seem like a good long term solution to me.

I suspect #278 would unblock you.