Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

LukeMathWalker
Copy link

@LukeMathWalker LukeMathWalker commented Jan 13, 2021

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 #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).

…ional (disabled by default) feature flag - `rand_core_0_6`.

Structure the features flags 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
Copy link
Owner

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 BurntSushi closed this Jan 13, 2021
@BurntSushi
Copy link
Owner

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

@LukeMathWalker
Copy link
Author

Understood 👌🏼

@pickfire
Copy link

So the possible workarounds as of now is to pin quickcheck to 0.9 I believe as in LukeMathWalker/zero-to-production#34 (comment).

@BurntSushi
Copy link
Owner

BurntSushi commented Feb 22, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants