Add trait SampleRng: Rng and related doc

[dhardy]

Move high-level Rng functionality to an extension trait. See documentation here.

This change is necessary to keep a similar interface for users while allowing a separate rand-core crate. Note that users often won't need to import Rng at all, as many of the examples and the seq module show.

Related:

• This was called Sample, but using SampleRng is consistent with other extension-traits and perhaps less confusing
• The plan was to remove next_f32 and next_f64 from Rng. That may or may not happen; it's not necessary for this change or the rand-core split, and those are definitely low-level methods.
• Rand, Sample and IndepedentSample all use Rng, not SampleRng still. That could change but would be a breaking change (since all the above may be replaced by a single Distribution trait, we can make the change then). I'm not certain which is best; possibly SampleRng since this eliminates the need to import Rng in many cases?

Also, @burdges said recently he was against this trait and the rand-core split. I don't think usage will be much different for end users (if anything, documentation is improved by separating front- and back-end functionality) and several people have requested rand-core so I think overall this is a win.

[dhardy]

Wacky idea: we could rename Rng to rand_core::Rng and then SampleRng to just rand::Rng. We could potentially do a similar thing to SeedableRng and NewRng, perhaps including NewRng::from_seed as a wrapper around SeedableRng::from_seed.

This would provide a nice split between front-end and back-end functionality, but I think overall would be confusing and not so useful (I generally disapprove of using the same name for different things in different namespaces). As I said, wacky idea.

[burdges]

Is that helpful? Afaik, specialization cannot violate crate boundaries, right? I.e. rand cannot say impl<R: Rng> rand_core::Rng for R { ... } and doing so might prove problematic anyways.

Afaik, there are no sensible arguments why separating rand_core beats using some no_std feature, only people who like small crates for aesthetic reasons. I think the two clear arguments against are that separate crates makes reading code and documentation harder, and extension traits add boilerplate and direct dependencies to quick special purpose Rng.

There might be arguments in favor of a slimmer rand crate containing anything in rand_core now, including the Rng::sample method, but only uniform distribution variants and add a separate rand-distributions crate.

[pitdicker]

After seeing your commit, but before looking at it closely I thought the extra trait provides a nice separation between front- and back-end. There really is a difference between the methods that make sense for an RNG to implement, and the methods that are best for user code.

I would also like to see rand-core being separated out. We really should not go overboard and split rand up into 10 tiny crates. But a core that is useful for implementing RNGs, and for crates that deal directly with RNGs, seems useful to me.

The changes and documentation look good to me.

Is SampleRng really the nicest name we can come up with? The three most interesting methods in it are gen, gen_iter and gen_range. Does it make sense to pick something with "gen" in the name, like GenRandom? Although I do not care much for the trait name to be honest.

[pitdicker]

@burdges If the rand-core split is only for aesthetic reasons, like preferring small crates, I would be against it also. Actually I was against it at first. But now I think it provides a nice, clean separation. rand-core is only necessary to include as a dependency for RNG implementations. rand is for everyone else.

You do make two good arguments, boilerplate and documentation. I think gen in SampleRng is already often the easier interface compared to next_u32 and next_u64. Most user code should just use SampleRng, and not care much for Rng. Maybe we can also provide a more generic solution for filling slices here than fill_bytes? If SampleRng can really be seen as (or become) the more convenient interface instead of Rng, I think there is no problem with boilerplate and documentation.

[dhardy]

@burdges the wacky idea was two separate traits, both named Rng, but in different namespaces. So no specialisation, just different names.

If we were to move some distributions code out to rand-distributions but keep a Distribution trait in rand, I think that distribution trait should still be implemented for the basic stuff (uniform, range), so the only things to move would be the statistical distributions (exponential, normal and gamma modules) and maybe some of the sequence sampling stuff. Hardly seems enough to bother.

I'm not convinced about whether or not we need a separate rand-core crate, but even without I think the extension trait helps with documentation. It would be good to get more opinions on this.

Is SampleRng really the nicest name we can come up with? The three most interesting methods in it are gen, gen_iter and gen_range.

There will also be a sample function (sampling from a Distribution) which I didn't add here. I'm not convinced of any name, but this is the best I came up with. (GenRng? UseRng? GenWithRng? SampleFromRng?)

There have already been a couple of suggestions to add a fill function; we certainly could add that to SampleRng.

[burdges]

Your statement that "rand_core [is] only necessary to include as a dependency for RNG implementations" is only a defense of the aesthetic argument, not actually an argument in favor of separation.

I'd almost agree the separation stops being quite so harmful for documentation if users almost never want functions from rand_core::Rng anyways, except.. Rust documentation frequently requires clicking through the [src] links, so if the source references another crate then that still complicates reading the documentation.

Are we clear on the relationship between rand_core::Rngs and rand::Rngs yet? Are we doing impl<R: rand_core::Rng> rand::Rng for R?

We must remember that impl<'a,R: Rng> Rng for &'a mut R { ... } is essential. If it does not exist for rand_core::Rng too then rand_core::Rng becomes much less useful as a stand along crate. I suspect all original separation proponents actually envisioned users using rand_core::Rng directly, so this sounds unworkable.

If the &mut trick exists for both rand_core::Rng and rand::Rng then we've no impl<R: rand_core::Rng> rand::Rng for R { .. } and all Rngs must provide both manually. It's true impl rand::Rng for MyRng { } might suffice after first doing impl rand_core::Rng for MyRng { ... }, but crates that provide Rngs do have both dependencies.

Instead, if we have impl<R: rand_core::R> Rng for R { .. }, then the &mut trick could exist only for rand_core::Rng, which maybe works best. I'm vaguely worried about Rng trait objects behaving strangely and/or increasing code size in all scenarios too.

I still think even if crate separation works it brings only needless constraints and pain. It's easiest to design without crate separation and then ask if your design splits cleanly afterwards.

[dhardy]

If documentation requires clicking through to the source, that's a deficiency in the documentation, in my opinion. That does sometimes happen but I'm not sure it's something we need to design around?

Are we doing impl<R: rand_core::Rng> rand::Rng for R?

That's what's in this PR (plus the "wacky renames"), yes.

I see your point about impl<'a,R: SampleRng> SampleRng for &'a mut R conflicting — except, is it necessary? Any &mut R where R: SampleRng is in fact &mut R where R: Rng (because SampleRng: Rng), and we already have impl<'a,R: Rng> Rng for &'a mut R, plus impl<R: Rng> SampleRng for R. I think that covers it?

I'm not really sure whether users should prefer to use (core) Rng or SampleRng everywhere; using the latter does appear to work fine.

I still think even if crate separation works it brings only needless constraints and pain. It's easiest to design without crate separation and then ask if your design splits cleanly afterwards.

It fits the constraints we already have, except for this PR and the separate NewRng: SeedableRng extension trait. But it is good advice, so I won't merge this just yet. On the other hand, don't forget the "divide and conquer" development strategy. I've worked on some projects with lots of complex components all intricately connected, and it can be a real pain (but at scales somewhat larger than rand). By using extension traits now we effectively allow crate separation later, which has some value.

[dhardy]

I tried adding a fill function to Rng:

fn try_fill<T: AsByteSlice + ?Sized>(&mut self, dest: &mut T) -> Result<(), Error> { ... }

and found:

error[E0038]: the trait `Rng` cannot be made into an object                                                                            
    --> src/lib.rs:1289:37                                                                                                             
     |                                                                                                                                 
1289 |             let mut r = &mut rng as &mut Rng;                                                                                   
     |                                     ^^^^^^^^ the trait `Rng` cannot be made into an object                                      
     |                                                                                                                                 
     = note: method `try_fill` has generic type parameters                                                                                 

The same method can be added to SampleRng however.

Does this count as motivation for this PR? Of course a generic fill is not necessary, especially if fill_bytes is available in the end-user Rng trait, but would be nice.

[dhardy]

My mistake, the above is because try_fill doesn't have a where Self: Sized bound, thus the compiler applies that bound to the whole of Rng.

But this brings us to a more important point: generic methods like fn gen<T: Rand> cannot support dynamic dispatch. However, they can be implemented in an extension trait over an inner Rng type accessed via dynamic dispatch. I believe this gives us good motivation for use of an extension trait.

(Note: gen also cannot support dynamic dispatch right now because Rand doesn't support it, but there are plans to fix that.)

[dhardy]

To confuse this topic even more, test_rng_trait_object does appear to be calling gen on r: &mut Rng, via this syntax:

(&mut r).gen::<i32>();

I don't understand how this works; presumably it forces gen to be called via static-dispatch (or inlined) over an Rng type accessed via dynamic dispatch. The following does not work:

r.gen::<i32>();

error: the `gen` method cannot be invoked on a trait object
    --> src/lib.rs:1259:15
     |
1259 |             r.gen::<i32>();
     |               ^^^

However, this syntax does work with this PR (also in my dhardy/master branch which combines an extension trait with a distribution trait supporting dynamic dispatch).

[dhardy]

After some more investigation, I finally understand how (&mut r).gen::<i32>(); works without the extension trait in this PR. It comes down to this impl:

impl<'a, R: ?Sized> Rng for &'a mut R where R: Rng { ... }

Which can be specialised as:

impl<'a> Rng for &'a mut Rng { ... }

This provides a Sized impl of Rng for (&mut r). gen has the prototype fn gen<T: Rand>(&mut self) -> T where Self: Sized, i.e. requires a &mut R parameter where R: Rng and R: Sized, so we must pass in &mut r where r itself is Sized — hence the double &mut required in test_rng_trait_object.

Note that we could change gen to fn gen<T: Rand>(mut self) -> T where Self: Sized. This would directly allow r.gen() for r: &mut Rng — but since gen() now consumes self, all users who don't wish to destroy their rng now need to create references themselves: (&mut rng).gen(), excepting where the rng is Copy! We could add another method fn gen_ref<T: Rand>(mut self) -> T where Self: Sized { (&mut self).gen() } but now things are getting ridiculous.

---

Conclusion: if we do not wish to add this extension trait, it appears reasonable to forgo direct support for dynamic dispatch on Rngs, and let users to write (&mut r).gen() (or a second reference) where required.

[dhardy]

Rebasing #256 without this PR is very easy. So the question here appears to be is it worth adding an extension trait (a) to simplify syntax with dynamic dispatch and (b) to allow Rng to be moved to a separate rand-core trait?

I think I agree with @burdges now that unless we have a real need for a separate rand-core, there's no need for this PR.

[KodrAus]

One reason to split out a rand_core crate could be for stability. If the fundamental trait Rng { fn next_u32(); ... } can be split and stabilised then it means the front-end rand API can be churned with less impact on consumers of the Rng trait. But as @burdges has pointed out, blanket impls across crate boundaries can get problematic. And if most consumers will be on the front-end rather than the back-end then you might not gain much from a stable back-end.

[dhardy]

A stable back-end would allow stable PRNG/entropy source crates to be published sooner — but most consumers are indeed on the front-end.

In reply to your other post about Serde's type erasure, I don't think adding an ObjectSafeRng type would actually add much. We already have direct support for dynamic dispatch on Box<Rng> and indirect support for &mut Rng (just requires double referencing); see test_rng_trait_object.

[dhardy]

@vks brings up the point that the error message is far from clear if double-referencing is not used — this could confuse users and is a legitimate reason to consider this PR — although not necessarily sufficient. See here and the following two posts.

[vks]

One reason to split out a rand_core crate could be for stability.

I think rand_core is also nicer for implementing RNGs in separate crates, because you don't have to depend on the rest of rand.

[dhardy]

That's a moot point because rand compiles fast and almost any project using Rng is going to use the rest of rand anyway — at least, I don't see much advantage of separate rand-core, even if it is a nice separation.

[dhardy]

I'm now tempted to rename SampleRng → RngExt (extension), because it better describes what the trait is.

I still like the idea of an extension trait because it separates what RNGs are supposed to implement and what they are not (e.g. backends should not be modifying Rng::gen).

[KodrAus]

Hmm, it's true RngExt is a more accurate description of the trait than SampleRng in relation to the Rng trait, but I think it would be common to use SampleRng independently of Rng. In that case it's almost more confusing because it's an extension you need for a trait you don't touch.

Bikeshedding the name aside I think the trait separation is fine too because it simplifies the mechanics of the Rng trait.

[dhardy]

Oh, good, sounds like we have mostly positive support for this change.

To go back to the bike-shedding, I'm tempted to go with @newpavlov's suggestion: trait Rng: RngCore.

[KodrAus]

As far as colours for bikesheds go, Rng: RngCore sounds good to me 👍

[dhardy]

Then I will close in favour of #265.