Add CryptoRngCore to support CryptoRng trait objects

[vks]

Fixes #1143.

[newpavlov]

I think we should introduce BlockRngCore and CryptoBlockRngCore marker traits (i.e. I don't think we need the upcast method) as was proposed here and recommend using them instead of CryptoRng. We may even mark the latter as #[deprecated], although I am not 100% sure about that.

[vks]

@newpavlov Would CryptoBlockRngCore replace CryptoRngCore from the current PR?

[newpavlov]

@vks
Whoops. Sorry, I haven't linked the comment properly in my previous comment. Fixed.

No, it will not replace it. I suggest introducing both CryptoBlockRngCore (marker sub-trait of BlockRngCore) and CryptoRngCore (marker sub-trait of RngCore):

#[deprecated(since="0.8.5", note="use `CryptoRngCore` or `CryptoBlockRngCore` instead")]
trait CryptoRng {}

trait CryptoBlockRngCore: BlockRngCore {}
trait CryptoRngCore: RngCore {}

impl<T: BlockRngCore> RngCore for BlockRng<T> { .. }
impl<T: CryptoBlockRngCore> CryptoRngCore for BlockRng<T> {}

// now we have two options for blanket impls
// first:
impl<T: CryptoRng + BlockRngCore> CryptoBlockRngCore for T {}
impl<T: CryptoRng + RngCore> CryptoRngCore for T {}
// second:
impl<T: CryptoBlockRngCore> CryptoRng for T {}
impl<T: CryptoRngCore> CryptoRng for T {}

With the first blanket impls, existing crates would implement the new traits automatically, but RNG crates would have to keep the CryptoRng impl to prevent breaking changes, thus with such impl we probably should not mark CryptoRng as deprecated. With the second blanket impls RNG crates would not support the new traits automatically, but we would be able to mark CryptoRng as deprecated, thus incentivizing users to migrate early. It would make them ready for v0.9 in which we probably should remove CryptoRng completely. We could make it a marker sub-trait of Rng, but I don't see how it could be useful. Personally, I lean towards the second option.

[vks]

@newpavlov I think your approach has the disadvantage that it's not possible to have a trait object that supports both CryptoRngCore and CryptoBlockRngCore, which kind of defeats the original motivation for this PR.

[newpavlov]

I think your approach has the disadvantage that it's not possible to have a trait object that supports both CryptoRngCore and CryptoBlockRngCore

It's not a disadvantage, but an intentional design choice. Why do you think we need a trait object which supports both? In my opinion, ideally RngCore and BlockRngCore should be marked as mutually exclusive traits, but unfortunately Rust lacks them.

Motivation of the original issue is fully covered by the CryptoRngCore trait (after RNG crates will migrate to it).

[vks]

@newpavlov

Why do you think we need a trait object which supports both? In my opinion, ideally RngCore and BlockRngCore should be marked as mutually exclusive traits, but unfortunately Rust lacks them.

You are right, they are used in a mutually exclusive way at the moment, having a trait object for both does not make sense. Let me reframe:

Relative to the current PR, you are proposing to remove the as_rngcore method and introduce an additional trait CryptoBlockRngCore: BlockRngCore. What is the purpose of the second trait? It would us enable to have dyn CryptoBlockRngCore, but this is not very useful compared to dyn CryptoRngCore.

[dhardy]

I think we should introduce BlockRngCore and CryptoBlockRngCore marker traits (i.e. I don't think we need the upcast method) as was proposed here and recommend using them instead of CryptoRng. We may even mark the latter as #[deprecated], although I am not 100% sure about that.

@newpavlov first I presume you meant to write CryptoRngCore and CryptoBlockRngCore. Second, we definitely do need an upcast method to allow method f(csrng: &mut CryptoRngCore) to call g(rng: &mut RngCore), until the Rust language supports trait-object-upcast (don't hold your breath). Third, I don't see much use for CryptoBlockRngCore; we don't expect users to write methods taking a block rng trait object.

From today's perspective, CryptoRng is not a good choice of name for its usage, and it is questionable whether we should have used the same trait for types implementing BlockRngCore. But now we must choose:

1. Add a new CryptoBlockRngCore: BlockRngCore marker trait and make CryptoRng: RngCore. As above, we need a CryptoRng::upcast method, so it's a breaking change for every current implementor of CryptoRng but probably not for users of the trait. It also requires more code for users to implement CryptoRng than other methods.
2. Add CryptoRngCore: RngCore and keep CryptoRng as a marker trait. This achieves essentially the same thing without any breaking changes.
3. Add a new CryptoThing marker trait and make CryptoRng: RngCore, telling users to implement CryptoThing and have an auto impl of CryptoRng for T: CryptoThing + RngCore (essentially the same as 2 with different names and more breakage).

Only one of these options is not a breaking change for all crypto-RNGs, so is there really a good reason to choose any other option?

[vks]

until the Rust language supports trait-object-upcast (don't hold your breath)

It already works on nightly, but who knows when it will be stabilized (see rust-lang/rust#65991 ).

[newpavlov]

What is the purpose of the second trait?

I don't see much use for CryptoBlockRngCore

The only purpose of the CryptoBlockRngCore is the blanket impl of CryptoRngCore for BlockRng<T> where T: CryptoBlockRngCore. I don't think we should reuse CryptoRng for that. I think we should deprecate it and remove completely in the next breaking release.

we definitely do need an upcast method to allow method f(csrng: &mut CryptoRngCore) to call g(rng: &mut RngCore)

Ah, yes. You are right. I forgot about it since I never needed something similar myself. Personally I think such case will be overwhelmingly rare in practice, but I agree it's a valid use-case.

As I've outlined earlier, I would prefer to introduce CryptoRngCore and CryptoBlockRngCore marker sub-traits (with the upcast method for the former), add blanket impls of CryptoRng for their implementors, and mark CryptoRng as deprecated. As far as I can see, this approach does not contain any breaking changes for both users and implementation crates. The only downside is that users who have migrated to CryptoRngCore will not be able to use older RNG implementation crates which only implement CryptoRng.

[dhardy]

but I agree it's a valid use-case

Rare, yes, but it's the use-case behind #1143.

I would prefer ...

But why? As I said above, is there really a reason to choose anything other than the least-breaking option? Your option implies people must either put up with deprecation warnings or update and be incompatible with downstream crates which haven't updated from CryptoRng.

[newpavlov]

it's the use-case behind #1143

I have a different understanding. The author himself has proposed the sub-trait approach without any mention of upcasting. It was mentioned first by you in the reply to @burdges' comment. But either way, I agree that it's worth to have the upcast method just in case.

But why?

Because I think it's a more consistent API which we should have in rand_core v0.7 (and in hindsight we probably should've started with it) and it would be great if gradual migration was possible. But on a second thought, my proposal has the following drawback: changing impl RngCore + CryptoRng to impl CryptoRngCore in user crates will be a breaking change, since it may break downstream code which uses RNGs which have not migrated to CryptoRngCore/CryptoBlockRngCore. I guess your second option will be a fine stop-gap solution for rand_core v0.6.

[dhardy]

I don't think we need a stop-gap since there is no pressing need to solve #1143, hence from my point of view we can either implement your solution in rand_core v0.7 or we can implement mine without bumping the version (I don't recall another reason we need to).

Also from my point of view: it is nicer for users to have a CryptoThing pure-marker trait. The only reason to use something else is where a trait object is needed, so most users will never need to touch CryptoRngCore. But renaming CryptoRng to CryptoWhateverWeWantToCallTheNewMarkerTrait is an extra breaking change, hence why I don't really advocate this option.

So, maybe we need some more perspectives here?

[vks]

I agree that the benefits of consistency don't outweigh the churn due to breaking changes and the additional required boilerplate, especially for such a niche feature. Less users having to touch the new API is also an advantage.

If we really want to make everything more consistent anyway, we can still do that later with a breaking change.

[newpavlov]

Also from my point of view: it is nicer for users to have a CryptoThing pure-marker trait.

I disagree. AFAIK currently most (if not all) users of CryptoRng use impl RngCore + CryptoRng since CryptoRng by itself is useless. With a sub-trait we would simply use impl CryptoRngCore. I guess if sub-traits will be introduced only in rand_core v0.7, then CryptoRng may become a sub-trait of RngCore (thus we would be able to use impl CryptoRng, which is even shorter) and for block RNGs we could introduce CryptoBlockRng, i.e. we would drop the Core part from the crypto trait names.

IIRC the only reason why the current CryptoRng is a marker trait is because we wanted to reuse it for both usual and block-level RNGs (a bad decision in retrospect).

[vks]

A marker trait still has the advantage that it saves users from having to implement the boilerplate for upcasting support. We would lose that advantage by turning CryptoRng into a subtrait of RngCore. Once upcasting is supported by stable Rust, I agree that it would be strictly better to replace CryptoRng with a subtrait (if we ignore the churn from the breakage).

I'm still not convinced that we need CryptoBlockRng, this seems like the wrong level of abstraction to worry about supporting trait objects.

[newpavlov]

@vks
IIUC you only need upcasting in very rare cases when a function takes &mut dyn CryptoRng and wants to use functions which take &mut dyn RngCore. If functions instead take impl CryptoRng and impl RngCore respectively, then everything should work without issues. Same for &mut dyn CryptoRng and impl RngCore.

I'm still not convinced that we need CryptoBlockRng, this seems like the wrong level of abstraction to worry about supporting trait objects.

As I wrote earlier, it's needed solely for the blanket impl on the BockRng wrapper. So we do not care about supporting trait objects with CryptoBlockRng. Note that trait objects will not work with them well either way since BlockRngCore has an associated type, which would have to be fixed in trait objects.

Yes, we could make CryptoBlockRng a marker trait, but since this trait will be only used in combination with BlockRngCore, I think it's logical to make it a sub-trait.

[dhardy]

Considering the arguments, I'm tempted to accept this now, and reconsider breaking changes later (especially after Rust supports trait-object upcast).