Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CryptoRngCore trait imply CryptoRng as well #1230

Merged

Conversation

cbeck88
Copy link

@cbeck88 cbeck88 commented May 14, 2022

Hi,

I recently had a case where I needed to make an API that takes &mut dyn (CryptoRng + RngCore). I didn't want to make my trait generic because it would greatly complicate some FFI situations.

Since rust doesn't currently allow dyn (CryptoRng + RngCore), I needed to make an extension trait that implies both of these, and, naturally, is implied for any object with both of these traits.

It turns out that rand crate already does almost the same thing with CryptoRngCore (which is actually what I named my extension trait).

However the implementation in rand crate is slightly different: CryptoRngCore implies RngCore but not CryptoRng.

I can't see right now that there's a compelling use-case for it the way it's implemented right now: any place where I would write &mut dyn CryptoRngCore my code would also work if my function takes &mut dyn RngCore, since CryptoRngCore provides no additional functionality other than a function that converts self to &mut dyn RngCore. (But I could have started with that...)

OTOH if CryptoRngCore implies both RngCore and CryptoRng then it's quite useful, because it works around the inability to make dyn (X + Y) in rust right now. Then I would just use the upstream version and drop my version.

Let me know what you think. Thank you!

@dhardy
Copy link
Member

dhardy commented May 16, 2022

See also #1187. Looks good to me but I'll give others a chance to take a look.

@cbeck88
Copy link
Author

cbeck88 commented May 16, 2022

(I guess this is also described in the alternatives section of this RFC: #1143)

@dhardy dhardy merged commit 3543f4b into rust-random:master May 19, 2022
@cbeck88 cbeck88 deleted the crypto-rng-core-requires-crypto-rng branch May 19, 2022 18:08
@dhardy dhardy mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants