meta: type signature for CryptoRngs

[tarcieri]

In #1087 we discussed various possible type signatures to use for RNGs.

We currently use impl CryptoRng + RngCore, leveraging a blanket impl of RngCore for &mut R where R: RngCore.

This has some ergonomic problems in that confuses users who aren't aware of the blanket impl about what exactly to pass. The flipside is it permits some nice syntax like directly passing OsRng instead of &mut OsRng as a caller.

However, for the receiver it requires reborrowing the rng as &mut rng everywhere if the rng is used more than once. The mut also needs to be present on the local variable binding, so really the only thing it gets rid of is &.

rand_core v0.6.4 introduced a CryptoRngCore marker trait with supertrait bounds on CryptoRng + RngCore, as well as a blanket impl for any types which impl CryptoRng + RngCore.

This allows writing impl CryptoRngCore instead of impl (CryptoRng + RngCore).

In #1147 I'm trying out &mut impl CryptoRngCore which minimizes syntax as well as the drawbacks for the receiver (this will be in a prerelease with the option to tweak it prior to a final v2.0 release).

Regardless of what we pick as a new convention, I think we should use the same convention everywhere.

[tarcieri]

There's an additional problem with &mut impl CryptoRngCore: it doesn't permit RNGs passed as trait objects.

For that to work, we need to do one of:

1. &mut (impl CryptoRngCore + ?Sized)
2. &mut dyn CryptoRngCore
3. <R: CryptoRngCore + ?Sized>(rng: &mut R)

Of these options, I'm now leaning towards number 2, and now understand why the rand/rand_core crates documented their APIs this way a little better.

I think this is one of those cases where the overhead of virtual dispatch is insignificant compared to the operation being invoked, which will involve something like a system call (for getrandom) or a cryptographic operation (for thread_rng).

[newpavlov]

Personally, I prefer the third option and would be surprised to find a &mut dyn signature in code which does not depend on dynamic dispatch in any way.

But I think, the root issue is on the rand side and it should be fixed in the upcoming v0.9 release. Though I haven't done the necessary design work yet.

[tarcieri]

Yeah I'd be fine with the third option too.

We need to make a call before we release signature v2.0

[cbeck88]

FWIW I also prefer (1) or (3) over (2), because what I'm concerned of is:

• Some libraries are using signatures like (2)
• Some libraries are using signatures like (1) or (3)

What is the result if A calls B calls C calls D, where A and C are using (1) or (3) and B and D are using (2)

1. When A calls B, an impl CryptoRngCore + ?Sized is not necessarily a &mut dyn CryptoRngCore so some kind of wrapper will be introduced, and this wrapper will have a vtable and virtual dispatch overhead for this call
2. When B calls C, it takes a &mut dyn CryptoRngCore, and this can bind directly to impl CryptoRngCore + ?Sized. But we are effectively type-erasing the fact that we have a &mut dyn CryptoRngCore.
3. When C calls D, it takes impl CryptoRngCore + ?Sized, and it needs to convert it to a &mut dyn CryptoRngCore. So a wrapper is introduced again and virtual dispatch overhead occurs again.

So it seems to me that this overhead can accumulate without bound, due to repeatedly introducing wrappers and then type erasing them. And there are a lot of libraries today that just have bounds like impl (RngCore + CryptoRng), not just in RustCrypto, but downstream.

I have not looked at actual generated assembly, and I think there are some devirtualization optimizations that can be performed by LTO. But it seems better not to depend on it.

My feeling is trait bounds like impl CryptoRngCore + ?Sized are simpler than dyn CryptoRngCore, even if it's a few more tokens, dyn is itself complicated.

It seems better for APIs to only declare requirements that they actually need -- if they are okay with a reference to a possibly unsized RNG, which most things are, then why not make that the trait bound, instead of forcing the construction of a trait object.

That's my 2 cents, thanks for starting this discussion

[tarcieri]

@garbageslam I don't understand your preference for 1/A over 3/C. Reading your bullet point 2 it seems like you're describing B calling A? I'm a little confused.

My feeling is trait bounds like impl CryptoRngCore + ?Sized are simpler than dyn CryptoRngCore, even if it's a few more tokens

Except with a &mut reference it becomes &mut (impl CryptoRngCore + ?Sized) which is a somewhat onerous syntax.

Granted <R: CryptoRngCore + ?Sized>(&mut R) is an equal or even worse amount of syntax, but it's also a lot more familiar than a parenthesized &mut (impl ...).

[cbeck88]

I'm just trying to imagine that there are a series of alternating calls to functions following option 2 (&mut dyn ...) and functions not following option 2 (no dyn).

Every time we call an option 2 function, we have to create a trait object if we don't have one already, and every time an option 2 function calls into a non-option 2 function, we're forgetting / type-erasing that there is a trait object in there already. So it seems like we could create a lot of trait objects on the stack this way and have a lot of virtual dispatch introduced.

Here I'm assuming that we have a large project, and some functions are using &mut dyn ... and some others are not. Maybe some libraries are changed to use &mut dyn and others are not, and there are lots of dependencies between both types of libraries.

Maybe it doesn't matter for RustCrypto, maybe RustCrypto is fairly self-contained and you would try to switch all the function signatures at once to &mut dyn at the time that you do this, and whether downstream follows suit is up to them, but there would be at most one trait object on the stack that is forced by what RustCrypto did.

The scenario where there's multiple trait objects is like, RustCrypto does this, then out of a collection of a hundred crates.io crates downstream of you, 50% of them switch to using dyn CyptoRngCore pretty much at random, and so sometimes we call out from libs using dyn CryptoRngCore back into libs using impl (RngCore + CryptoRng), and then back again.

[tarcieri]

But aren’t all of those problems the same regardless of if a named generic parameter R is used, or impl Trait?

It feels like six of one, half dozen of another to me there.

[cbeck88]

Yeah I think that's right, it doesn't matter if a named parameter R is used or impl Trait is used.

If I understand right then the impl Trait syntax (1) desugars to a named parameter (3): https://doc.rust-lang.org/reference/types/impl-trait.html#anonymous-type-parameters

[tarcieri]

I just realized we can have both: a much more lightweight syntax and support for trait objects.

use rand_core::CryptoRngCore;

pub fn foo(mut rng: &mut dyn CryptoRngCore) {
    bar(&mut rng);
}

pub fn bar(_rng: &mut impl CryptoRngCore) {
}

That leverages the blanket impl of RngCore for &mut RngCore.

[newpavlov]

@tarcieri
The new design of CryptoRng should work with &mut impl CryptoRng. It becomes a sub-trait of RngCore, so mixing-in dynamic dispatch should not cause any issues.

[tarcieri]

I opened #1183 to move signature back to using &mut impl CryptoRngCore

[burdges]

I now pass &mut impl RngCore+CryptoRng everywhere. Rust folks know what &mut means.

[tarcieri]

@burdges check out CryptoRngCore (per above)!

Hopefully the next release makes RngCore a supertrait bound of CryptoRng so even that isn't necessary.

[newpavlov]

In the upcoming releases we will use &mut impl CryptoRngCore, so I think we can close this issue?