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

Add TryRngCore and TryCryptoRng traits #1424

Merged
merged 5 commits into from
May 8, 2024
Merged

Add TryRngCore and TryCryptoRng traits #1424

merged 5 commits into from
May 8, 2024

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Apr 1, 2024

Closes #1418.

@newpavlov
Copy link
Member Author

newpavlov commented Apr 1, 2024

The main issue with this design is that we can not write impl<'a, R: RngCore + ?Sized> RngCore for &'a mut R { ... } since it conflicts with blanket impl for R: TryRngCore<Error = Infallible> (users may write impl TryRngCore for &mut MyType { ... }). This causes wide breakage downstream which assumes that &mut RngCore implements RngCore.

So we probably have to reverse the trait relations and write impl<R: RngCore> TryRngCore for R { type Error = Infallible; ... } instead.

UPD: This no longer relevant, blanket impls are reversed now.

rand_core/src/lib.rs Outdated Show resolved Hide resolved
@newpavlov newpavlov marked this pull request as ready for review April 1, 2024 16:38
@newpavlov newpavlov requested a review from dhardy April 1, 2024 16:40
/// ```
///
/// [getrandom]: https://crates.io/crates/getrandom
#[cfg_attr(doc_cfg, doc(cfg(feature = "getrandom")))]
#[derive(Clone, Copy, Debug, Default)]
pub struct OsRng;

impl CryptoRng for OsRng {}
impl TryRngCore for OsRng {
type Error = getrandom::Error;
Copy link
Member Author

@newpavlov newpavlov Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exposes getrandom::Error as part of rand_core's public API (same with SeedableRng::try_from_entropy). I think it's fine since getrandom is quite stable and we no longer need error boxing provided by rand_core::Error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A caveat is that we can't release rand_core v1.0 before getrandom v1.0, but maybe we shouldn't care about that.

I think otherwise this is perfectly fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will also prevent rand 0.9 from updating its version of getrandom to getrandom 1.0 without a breaking change. We could also just note that the exact type of OsRng::Error isn't considered part of rand's stable API.

All that considered, this seems fine. It's much better to directly expose getrandom::Error than do the wrapping we were doing before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases the semver trick could allow such migration without breaking changes.

rand_core/src/lib.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Apr 2, 2024

Since this includes a bunch of reformatting, would it be easy enough to make a new PR applying rustfmt then rebase this? The only reason I didn't do that yet is because of conflicts with other PRs.

@dhardy dhardy mentioned this pull request Apr 3, 2024
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, though given that it's a very significant change, we shouldn't rush this.

We might also reconsider #1288 afterwards, though I'm pretty sure the renames in that will cause more conflicts than these changes.

rand_core/src/lib.rs Outdated Show resolved Hide resolved
rand_core/src/lib.rs Show resolved Hide resolved
Comment on lines 45 to 50
pub use error::Error;
#[cfg(feature = "getrandom")] pub use os::OsRng;

use core::{convert::Infallible, fmt};

pub mod block;
mod error;
pub mod impls;
pub mod le;
#[cfg(feature = "getrandom")] mod os;

#[cfg(feature = "getrandom")] pub use getrandom;
#[cfg(feature = "getrandom")] pub use os::OsRng;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect most people (including myself) will be happy to see rand_core::Error gone, but I wonder if it will cause problems (for an alloc-less no_std crate which needs to handle errors).

Copy link
Member Author

@newpavlov newpavlov Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should cause any issues. getrandom::Error is a no_std type by default. Defining an IO RNG which uses something like io::Error would make it automatically incompatible with no_std. So everything will depend on implementer crates.

rand_core/src/lib.rs Outdated Show resolved Hide resolved
rand_core/src/lib.rs Outdated Show resolved Hide resolved
/// ```
///
/// [getrandom]: https://crates.io/crates/getrandom
#[cfg_attr(doc_cfg, doc(cfg(feature = "getrandom")))]
#[derive(Clone, Copy, Debug, Default)]
pub struct OsRng;

impl CryptoRng for OsRng {}
impl TryRngCore for OsRng {
type Error = getrandom::Error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A caveat is that we can't release rand_core v1.0 before getrandom v1.0, but maybe we shouldn't care about that.

I think otherwise this is perfectly fine.

src/rng.rs Show resolved Hide resolved
src/rngs/reseeding.rs Show resolved Hide resolved
src/rngs/std.rs Outdated Show resolved Hide resolved
src/rngs/thread.rs Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Apr 3, 2024

Also note: there are some doc link errors to fix.

@MichaelOwenDyer
Copy link
Member

This is a cool change, I think the new design is quite elegant!

@newpavlov
Copy link
Member Author

One open question is whether we need to distinguish between TryRngCore and TryCryptoRng.

Having only TryCryptoRng would allow us to remove the try_next_* methods and the impl_try_rng_from_rng_core! macro. It would mean that non-CSPRINGs would not have bother with the Try* stuff like they do now.

@MichaelOwenDyer
Copy link
Member

One open question is whether we need to distinguish between TryRngCore and TryCryptoRng.

For expressiveness and symmetry I would say keep it. But if it is having a significant impact on ergonomics then maybe there's a better design to be strived for?

@dhardy
Copy link
Member

dhardy commented Apr 5, 2024

An example of a fallible non-crypto RNG could be reading from a file, but we already removed ReadRng due to lack of use.

I don't see much use for try_next_* either.

@newpavlov
Copy link
Member Author

I don't see much use for try_next_* either.

The only case I could think of is RDRAND/RDSEED instructions which produce u32/u64 random values directly.

@dhardy dhardy added the E-question Participation: opinions wanted label Apr 10, 2024
rand_core/src/blanket_impls.rs Outdated Show resolved Hide resolved
rand_core/src/impls.rs Outdated Show resolved Hide resolved
rand_core/src/impls.rs Outdated Show resolved Hide resolved
rand_core/src/lib.rs Outdated Show resolved Hide resolved
rand_core/src/os.rs Show resolved Hide resolved
src/rngs/thread.rs Outdated Show resolved Hide resolved
@dhardy
Copy link
Member

dhardy commented Apr 19, 2024

@josephlr it would be nice to get your thoughts on the usage of getrandom::Error and on the trait design.

Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Having two maker traits for being cryptographically secure is a little weird, but the alternative would be having a marker trait with no methods, and the requiring all users to do T: TryRngCore + CryptoRng which seems worse.

It's also unfortunate that the blanket impl for TryRngCore in terms of RngCore is not currently compatible with Rust's trait system, but I guess that can't be avoided.

rand_core/src/os.rs Show resolved Hide resolved
/// ```
///
/// [getrandom]: https://crates.io/crates/getrandom
#[cfg_attr(doc_cfg, doc(cfg(feature = "getrandom")))]
#[derive(Clone, Copy, Debug, Default)]
pub struct OsRng;

impl CryptoRng for OsRng {}
impl TryRngCore for OsRng {
type Error = getrandom::Error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will also prevent rand 0.9 from updating its version of getrandom to getrandom 1.0 without a breaking change. We could also just note that the exact type of OsRng::Error isn't considered part of rand's stable API.

All that considered, this seems fine. It's much better to directly expose getrandom::Error than do the wrapping we were doing before.

@newpavlov
Copy link
Member Author

newpavlov commented May 6, 2024

It's also unfortunate that the blanket impl for TryRngCore in terms of RngCore is not currently compatible with Rust's trait system, but I guess that can't be avoided.

It could be avoided, but we would need to give up on the blanket impls for &mut R and Box<R>. Personally, I am not sure how useful they are in practice, so maybe we could remove them? At one time we were using rng: impl CryptoRng in RustCrypto (so we could pass OsRng and ThreadRng without &mut), but we decided that rng: &mut impl CryptoRng communicates intent better and is less confusing for users not familiar with blanket impls in rand_core.

@dhardy
Copy link
Member

dhardy commented May 7, 2024

It will also prevent rand 0.9 from updating its version of getrandom to getrandom 1.0 without a breaking change. We could also just note that the exact type of OsRng::Error isn't considered part of rand's stable API.

Also to a hypothetical getrandom v0.3... it should be okay to bump the rand_core version number in concert, however there could be issues for anyone waiting on a third-party RNG crate to get compatibility with the latest rand_core version (so frequent bumps are bad).

We should not rely on documentation for API stability (at least, only as a very last resort). There are always people who don't read it.

It could be avoided, but we would need to give up on the blanket impls for &mut R and Box. Personally, I am not sure how useful they are in practice, so maybe we could remove them?

I don't have specific uses in mind, but I've hit issues where generics don't work as they should too many times. I'm also not going to bet anything on specialization getting implemented soon. So the current approach, which allows all expected types to implement TryRngCore (provided the implementing crate doesn't forget the macro), is the best option IMO.

@dhardy
Copy link
Member

dhardy commented May 7, 2024

@newpavlov could you resolve the conflicts please? Lets merge this!

@newpavlov
Copy link
Member Author

@dhardy Done.

@newpavlov newpavlov changed the title Rework fallability Add TryRngCore and TryCryptoRng traits May 7, 2024
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thing:

[RngCore] and [CryptoRng] should recommend usage of [impl_try_rng_from_rng_core] and [impl_try_crypto_rng_from_crypto_rng] respectively in their documentation.

@newpavlov
Copy link
Member Author

newpavlov commented May 8, 2024

I've updated the rand_core docs a bit. They probably could use a bit more work, but we can do it in a later PR.

I am still not fully convinced on usefulness of &mut R and Box<R> blanket impls, but it's probably better to discuss it in a separate issue.

After merging this PR, I would like to create a PR which would fix formatting and Clippy warnings. It's likely to create a bunch of conflicts in existing PRs, so if you plan to merge some of them in the near future, I can wait for those.

@dhardy dhardy merged commit fba5521 into master May 8, 2024
12 checks passed
@newpavlov newpavlov deleted the try_traits branch May 8, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make RngCore infallible?
4 participants