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

Re-export rand-core members in rand? #15

Closed
dhardy opened this issue Oct 21, 2017 · 13 comments
Closed

Re-export rand-core members in rand? #15

dhardy opened this issue Oct 21, 2017 · 13 comments

Comments

@dhardy
Copy link
Owner

dhardy commented Oct 21, 2017

Since rand-core is intended to be a separate crate, but with rand depending on it, should some parts of be re-exported in rand?

I argue that the following should be re-exported in rand so that "normal users" do not have to depend on rand-core:

  • Rng, CryptoRng, SeedableRng, SeedFromRng traits
  • Error struct and ErrorKind enum

The rand_core::impls module is aimed at implementations of Rng so does not need to be re-exported (in my opinion).

There is one other item: the mock module. Probably it is useful to re-export this from rand.

@dhardy
Copy link
Owner Author

dhardy commented Oct 21, 2017

It is also worth mentioning the story for crates implementing Rng / CryptoRng: the intention is that such crates can depend on rand-core but don't need to use rand.

@pitdicker
Copy link

Not re-exporting rand_core::impls would almost force crates implementing Rng to depend on rand-core instead of rand, guiding them to the right dependency. 👍

Could you explain why the mock module should be in rand_core?

@dhardy
Copy link
Owner Author

dhardy commented Oct 21, 2017

I put mock in there to allow tests on the Rng trait. It doesn't really need to there of course.

@dhardy dhardy added this to the rand-core RFC milestone Oct 22, 2017
@dhardy
Copy link
Owner Author

dhardy commented Oct 23, 2017

Ok, I moved mock from rand_core to rand, so that's not a problem any more. I think the rest is resolved?

@pitdicker
Copy link

The re-exports seem fine to me.

I still like to look over rand_core::impls. I now have a branch with 45% wins for fill_bytes with Isaac, and suspect a much smaller win for something like Xorshift should be possible to. And it needs testing. But that is not relevant for this issue...

@burdges
Copy link

burdges commented Nov 10, 2017

I know this messes up the crate separation, but did you consider

pub trait Rng : Sample { ... }
pub trait Sample { ... }
impl<R: Rng> Sample for R { ... }

Would that eliminate needing use rand::Sample? I know that a minor thing, and probably not even a good thing for reading/auditing, but it's vaguely relevant to backwards compatibility. It might also create a bunch of double references in trait objects, which maybe slows down &mut R : Rng slightly.

@dhardy
Copy link
Owner Author

dhardy commented Nov 10, 2017

I don't believe it would eliminate the need to use rand::Sample, no.

Good point about this being a breaking change; I don't see an obvious way around it though (also the crate separation is quite important IMO).

@burdges
Copy link

burdges commented Nov 10, 2017

Right, if that existed it'd require an extra pub like pub trait Rng : pub Sample or something.

@dhardy
Copy link
Owner Author

dhardy commented Nov 10, 2017

Like class extension in classic OO languages? Yes, but I don't think anything like that exists in Rust.

@nixpulvis
Copy link

nixpulvis commented Nov 16, 2017

I think it would be a really good exercise to write down the complete desired interfaces for both rand and rand_core in once place. Might make discussions like this a bit easier (at least for me).

Edit: Public interface. I don't care about internal details for the purposes of this comment.

@dhardy
Copy link
Owner Author

dhardy commented Nov 17, 2017

Check out the documentation: https://dhardy.github.io/rand/

@dhardy
Copy link
Owner Author

dhardy commented Nov 24, 2017

Important to this topic are: which other parts of rand should be moved to other crates?

  • Rng and seeding traits, and error handling code, are being moved to rand-core
  • To provide (or not) specific PRNGs? #58 suggests moving all PRNG implementations to external crates
  • We could move external RNGs (OsRng and JitterRng) to another crate; the rationale is similar to rand-core: cryptographic code may but able to make use of these without needing other rand code; this implies they could also be moved to rand-core

There are more subdivisions possible, although these appear less useful:

Finally, rand::random is the most derived feature: it depends on Sample and distribution code for conversion, and on thread_rng which uses StdRng and ReseedingRng with entropy from the external RNGs — about the only things it doesn't depend on are the sequences module, weak_rng, and the Xorshift and ChaCha generators.

Conclusion

Re-exports

rand is dependent on rand-core, the external RNGs and some PRNGs. To save users depending on multiple crates, we should import anything from these crates which (1) is likely to be useful (includes most stuff, except perhaps rand-core::impls) and (2) is unlikely to be replaced (this may rule out PRNGs as noted in #58).

@dhardy
Copy link
Owner Author

dhardy commented Mar 18, 2018

I think this topic is either decided or will be reviewed later, so closing the issue.

@dhardy dhardy closed this as completed Mar 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants