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

Move Rng to rand-core crate? #264

Closed
dhardy opened this issue Feb 17, 2018 · 6 comments
Closed

Move Rng to rand-core crate? #264

dhardy opened this issue Feb 17, 2018 · 6 comments
Labels
E-question Participation: opinions wanted

Comments

@dhardy
Copy link
Member

dhardy commented Feb 17, 2018

This has come up many times already, so quick summary:

  • move Rng, SeedableRng, Error, and some related code to a new crate, rand-core
  • move some helper code for implementing Rng (impls and le modules) to rand-core
  • export all but the helper code in rand too
    Do we want to do this?

Note: this has already been tested.

Also note: rand already supports no_std; we don't need rand-core for that!

I think it's time we tried to answer this question. I already know some people would like to see this, but I'm less clear of all the reasons. I know @burdges doesn't, because it divides related logic between two different crates, and is mostly unnecessary.

For:

  • fairly clean separation; we can put backend docs in rand-core and user docs in rand
  • enables us to move PRNG implementations into sub-crates, and use code from there (see below)
  • enables some other crate tricks, e.g. allowing users to replace OsRng (see below)
  • less code to review for crates only needing rand-core

Against:

  • doing this requires Add trait SampleRng: Rng and related doc #244 (or something very similar) — contentious but not a big deal
  • this might restrict specialisation implementations — no currently known issues
  • code and documentation gets split between two crates (though this is also a potential advantage)

PRNG sub-crates: we can for example move XorShiftRng to a rand-small crate, which depends on the rand-core, and have rand expose XorShiftRng as WeakRng. If we tried to do this without a separate rand-core there would be a cyclic dependency randrand-smallrand, so we might have to copy the PRNG code.

User-replaceable OsRng: @pitdicker mentioned this somewhere: move OsRng to a sub-crate, and allow users building a binary on a platform not supported by our OsRng to replace our rand-os crate with their own implementation. It is not practical for us to provide an OS entropy source in all cases, e.g. WASM and embedded platforms, and this would allow users to keep thread_rng etc.

@dhardy dhardy added the E-question Participation: opinions wanted label Feb 17, 2018
@nagisa
Copy link
Contributor

nagisa commented Feb 17, 2018

I’m still waiting on rand-core before upgrading my rdrand crate.

@nagisa
Copy link
Contributor

nagisa commented Feb 17, 2018

Thinking about it now, an alternative implementation of the same thing would work for me, one based on the features.

Namely, the rand crate would have a default feature called impls or something, that would then enable compilation of all the Rng implementations etc, and if not present, would make rand act just like rand-core in content.

Although that would still not allow swapping out the implementations for "built-in" Rngs.

@newpavlov
Copy link
Member

newpavlov commented Feb 17, 2018

I am in support of using rand-core. One not listed pro point for the split is significantly smaller amount of code which must be reviewed in case of CSRNG implementations. (Although there is a contr-point that most of non-trivial crypto applications will have to depend on rand either way.) Now about cons:

doing this requires #244 (or something very similar) — contentious but not a big deal

We can make Rng an extension trait of RngCore which will be defined in the rand-core, so not much will change for most of the users.

this might restrict specialisation implementations — no currently known issues

I think RNG implementations can still depend on the rand directly and specialize over "extended" Rng trait.

code and documentation gets split between two crates (though this is also a potential advantage)

Is it about crate-level documentation or item-level documentation? I don't think that amount of duplicated documentation will be that big, also if it will be deemed necessary we can just re-export traits or the whole rand-core crate into rand.

@dhardy
Copy link
Member Author

dhardy commented Feb 17, 2018

We can make Rng an extension trait of RngCore which will be defined in the rand-core, so not much will change for most of the users.

We could, but there's an issue: the extension trait cannot be object-safe (it consists entirely of generic functions). This means users need to use the base trait where they want a type-erased RNG: fn foo(rng: &mut RngCore). Probably no big deal because it's only necessary in this case. (If you want to discuss this further, use #244 please.)

I don't really see a problem with the documentation, but see this comment.

@vks
Copy link
Collaborator

vks commented Feb 19, 2018

I think the implementation of each RNG and the traits are logically independent, so it makes sense to have them in different crates, like rustcrypto does for its algorithms.
If you implement an RNG in an independent crate and want to profit from the ecosystem, you only need the rand traits and don't really care about distributions and other RNG implementations.

@dhardy
Copy link
Member Author

dhardy commented Mar 8, 2018

Sounds like this is decided. Closing in favour of PR #288.

@dhardy dhardy closed this as completed Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

No branches or pull requests

4 participants