Distribution sample prototype and sized-ness of `Rng`

[dhardy]

Currently we have:

pub trait RngCore { ... }
pub trait Rng: RngCore { ... }

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

pub trait Distribution<T> {
    fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> T;
}

impl Distribution<f64> for Exp {
    fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> f64 {
        let n: f64 = rng.sample(Exp1);
        n * self.lambda_inverse
    }
}

In my master experimental branch the Distribution trait is slightly different:

pub trait Distribution<T> {
    fn sample<R: RngCore + ?Sized>(&self, rng: &mut R) -> T;
}

impl Distribution<f64> for Exp {
    fn sample<R: RngCore + ?Sized>(&self, rng: &mut R) -> f64 {
        let n: f64 = rng.sample(Exp1);
        n * self.lambda_inverse
    }
}

Are they equivalent? Almost, but:

• What does R: Rng + ?Sized mean when Rng: Sized and why does this work? The constraint is relaxed.
• Why does Uniform.sample(&mut r) where r: &mut RngCore work in the latter case? This does work in both cases which is the main thing. Rng is implemented for every RngCore so effectively r: &mut Rng.
• It is necessary to import both Rng and RngCore to implement Distribution in the latter
• It is often necessary to write mut rng when implementing Distribution in the latter

Are there any subtleties I'm missing? The former is slightly more convenient, but I don't understand it as well as the latter. @burdges do you understand this better?

[burdges]

1. You want pub trait Rng: RngCore + ?Sized { .. }, no? Afaik, there is never a reason to write + Sized because rust assumes + Sized unless you say + ?Sized. I have not looked up the rules recently though. We should retain support unsized Rngs like the current crate if possible.

2. Are you actually using the trait object &mut RngCore? If so, that sounds fishy. I'm not too surprised if &mut rng works when rng: &mut R and R: RngCore but see 4 below.

3. I'm missing some context to answer this one perhaps, but presumably it's bouncing some call through Rng that you imagine should go through RngCore.

4. I think your mut rng: &mut R arises because you call rng.sample(Exp1) which takes a &mut self. It'll likely disappear if you call Exp1::sample(rng) or if you do (*rng).sample(Exp1).

[burdges]

I think 4 sounds like a rustc bug for which we should minimize and raise an issue. It's clear &mut R provides a &mut self but maybe it's picking the wrong impl for whatever reason.

[dhardy]

1. All methods on Rng require Self: Sized — I thought putting this restriction on the trait itself made more sense, but maybe not
2. Yes. Why is that fishy?
3. Actually I expected everything to go through Rng; we discourage using any of the RngCore methods directly
4. That might explain why I was confused about needing mut rng.

Thanks. But do you recommend R: Rng + ?Sized or R: RngCore + ?Sized in methods, or doesn't it matter? In the case of SeedableRng::from_rng we have to use RngCore (crate separation) but for the distributions we could use either.

[burdges]

I donno.. At this point, you've written more distributions than anyone, so did you prefer writing them with the higher or lower level method? :) It might not matter much actually if distributions will mostly build directly on other distributions.

I've no idea why Sized appears everywhere in both the original rand and your redesign, but rand still passes all tests if you remove them : https://github.com/burdges/rand/tree/unsized

It's possible the Sized restriction merely forces using the impls for &mut R: Rng, which costs nothing if the compiler removes all the double references. If so, I've no idea why that helps. I did not compare benchmarks or compile times with the Sized bounds removed.

It's also likely almost all Rngs are all Sized so the Sized restriction never actually comes into play.

[pitdicker]

But do you recommend R: Rng + ?Sized or R: RngCore + ?Sized in methods, or doesn't it matter?

It works, but using Rng in the distributions code bugs me a bit. It creates a kind of circular dependency between Rng and the distributions, although one distribution is always implemented using another so it works out.

[dhardy]

True; conceptually fn sample doesn't belong in Rng, but it is convenient. I'm not quite sure about it; also fn shuffle (in my master branch you write v[..].shuffle(&mut rng); instead).

[dhardy]

Contrast (trait from my branch but similar to Distribution::sample case):

pub trait Choose<T> {
    fn choose<R: RngCore>(self, rng: R) -> Option<T>;
}

    #[test]
    fn dyn_dispatch() {
        let mut r: &mut RngCore = &mut ::test::rng(813);
        // here we take a second reference to r only so that we don't consume:
        assert_eq!([7, 7][..].choose(&mut r), Some(&7));
        // ... (including more uses of r)
    }

with:

pub trait Choose<T> {
    fn choose<R: RngCore + ?Sized>(self, rng: &mut R) -> Option<T>;
}

    #[test]
    fn dyn_dispatch() {
        let r: &mut RngCore = &mut ::test::rng(813);
        
        assert_eq!([7, 7][..].choose(r), Some(&7));
        // the above does not consume r ...
   }

Both support usage of type-erased RngCore. The first requires a second level of reference to r formally (I don't know whether it gets optimised away; see rust-lang/rfcs#1403 for why we can't just copy the reference), the second presumably doesn't.

I guess the conclusion is that the first version is nicer to write the impls for, but the second is nicer to use, and avoids the &mut reborrow problem, so should be our choice (which is a shame; if &mut R were copy but with proper lifetime tracking we would get nicer code, i.e. eventually the first option should be the best choice).