aead: generate_nonce accept an immutable rng

[alxiong]

I'm a little confused by the latest AeadCore::generate_nonce() API:

in code, it seems to be

    #[cfg(feature = "rand_core")]
    #[cfg_attr(docsrs, doc(cfg(feature = "rand_core")))]
    fn generate_nonce(mut rng: impl CryptoRng + RngCore) -> Nonce<Self>
    where
        Nonce<Self>: Default,
    {
        let mut nonce = Nonce::<Self>::default();
        rng.fill_bytes(&mut nonce);
        nonce
    }

however in rustdoc (latest/v0.5.1), it seem to be accepting an immutable rng: fn generate_nonce(rng: impl CryptoRng + RngCore) -> Nonce<Self>

that's the first confusion.

The second confusion is, why does it accept mut rng instead of &mut rng?

[tarcieri]

The mut keyword is a property of the rng local binding, not the type signature of the trait method.

It's leveraging a blanket impl of RngCore for &mut R: https://docs.rs/rand_core/latest/rand_core/trait.RngCore.html#impl-RngCore-for-%26%27a%20mut%20R

The purpose of using this was to make the type signature a bit cleaner than rng: &mut (impl CryptoRng + RngCore). However, it seems like we may have made things more confusing.

@newpavlov it seems people get really confused by these type signatures because they don't immediately realize they need to pass &mut R. See also #1062

[newpavlov]

In addition to cleaner signatures the current approach also allows us to write Alg::generate_nonce(OsRng) and Alg::generate_nonce(thread_rng()) without redundant &muts.

I am open to changing it in the next breaking releases (together with crypto-common), but I think we should use &mut R and where R: CryptoRng + RngCore. &mut impl Trait looks a bit weird in my opinion.

[tarcieri]

Being able to directly pass OsRng is cool. I didn’t even think of that.

I’d be down to change it back to a named parameter in the next release, though. I think it’s just too confusing as is.

[alxiong]

I see, I was so entrenched in seeing and using function signature like &mut R, now I actually kind of like the current API design. Feel free to close this issue.

Future reference for myself:

trait RngCore {}

#[derive(Default)]
struct OsRng;
impl RngCore for OsRng {}

// blanket implementation, mimicking rand_core: https://docs.rs/rand_core/latest/rand_core/trait.RngCore.html#impl-RngCore-for-%26%27a%20mut%20R
impl<'a, R: RngCore> RngCore for &'a mut R {}

// same as `fn encrypt(_rng: &mut impl RngCore)`, but generic over R looks better.
// passing in an immutable binding of mutable reference: can't change what `rng` points to, can change the value/state that `rng` points to.
fn encrypt<R: RngCore>(_rng: &mut R) {}

// what RustCrypto and Halo2 uses. Cleaner-looking API than `encrypt()`
fn encrypt2(mut _rng: impl RngCore) {}

fn main() {
    let mut rng = OsRng::default();

    // encrypt(rng); // will give clear error:  "expected `&mut _`, consider mutably borrowing here: `&mut rng`"
    encrypt(&mut rng); // only this will works! obvious from function signature

    // encrypt2(&rng); // the trait `RngCore` is not implemented for `&OsRng`
    //                 // will be troublesome if we can pass in `&rng`, because that means inside `encrypt()`, they can mutate the reference and point
    //                 // to some other Rng, which is obviously undesirable. Thankfully, the blanket implementation is only for `&mut R`, not `&R`, not `&R`

    encrypt2(&mut rng); // works! becasue of the blanket impl
    encrypt2(rng); // works! but note that we move `rng` into this function, useful when you only want to use some Rng once without the redundant &mut
}

[tarcieri]

Something else I think is a bit annoying is the need to declare the binding mut and reborrow if you already have a &mut Rng and need to use it more than once, e.g.

pub fn some_fn(mut rng: &mut Rng) -> (Key, Nonce) {
    let key = MyAead::generate_key(&mut rng);
    let nonce = MyAead::generate_nonce(&mut rng);
    (key, nonce)
}

...whereas with <R: RngCore>(rng: &mut R) it's this:

pub fn some_fn(rng: &mut Rng) -> (Key, Nonce) {
    let key = MyAead::generate_key(rng);
    let nonce = MyAead::generate_nonce(rng);
    (key, nonce)
}

[alxiong]

yeah, that's true.

Guess no clear winner between the two options then, and a matter of taste.

[alxiong]

another tangential question, why are we marking rng: RngCore + CryptoRng instead of rng: Rng + CryptoRng in user-facing APIs? @tarcieri

didn't rng_core's doc suggests that only Rng implementations should impl RngCore, otherwise "users should normally use Rng trait, which is automatically implemented for every type implementing RngCore."?

[newpavlov]

Using Rng bound would mean pulling the rand crate and its dependencies, which is clearly redundant for us. Some users already complain that we use "too many crates", so we try to keep our dependency tree as lean as possible and feature gate parts of it in cases of optional functionality.

[tarcieri]

TIL: rand_core added a CryptoRngCore core extension trait in the latest release (v0.6.4):

https://docs.rs/rand_core/latest/rand_core/trait.CryptoRngCore.html

Given that, I think we could move to &mut impl CryptoRngCore in the next breaking release.

(though for whatever reason rand_core docs recommend &mut dyn CryptoRngCore)

[tarcieri]

I think this issue is "solved", and opened #1148 for further discussion.