Add `TryRngCore` and `TryCryptoRng` traits #1424

newpavlov · 2024-04-01T15:13:54Z

Closes #1418.

newpavlov · 2024-04-01T15:17:42Z

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

newpavlov · 2024-04-01T16:42:26Z

rand_core/src/os.rs

 /// ```
 ///
 /// [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;


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.

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.

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.

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

rand_core/src/lib.rs

dhardy · 2024-04-02T14:51:21Z

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

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

dhardy · 2024-04-03T11:03:30Z

rand_core/src/lib.rs

-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;


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).

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

dhardy · 2024-04-03T11:19:50Z

rand_core/src/os.rs

 /// ```
 ///
 /// [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;


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

src/rngs/reseeding.rs

src/rngs/std.rs

src/rngs/thread.rs

dhardy · 2024-04-03T11:37:11Z

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

MichaelOwenDyer · 2024-04-03T13:22:50Z

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

newpavlov · 2024-04-03T15:04:05Z

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 · 2024-04-05T12:07:20Z

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 · 2024-04-05T15:36:37Z

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 · 2024-04-05T16:27:05Z

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.

rand_core/src/blanket_impls.rs

rand_core/src/impls.rs

rand_core/src/lib.rs

rand_core/src/os.rs

src/rngs/thread.rs

dhardy · 2024-04-19T09:55:46Z

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

josephlr

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

josephlr · 2024-05-06T12:20:54Z

rand_core/src/os.rs

 /// ```
 ///
 /// [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;


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 · 2024-05-06T12:53:43Z

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 · 2024-05-07T07:36:09Z

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 · 2024-05-07T10:28:47Z

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

newpavlov · 2024-05-07T20:06:54Z

@dhardy Done.

dhardy

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 · 2024-05-08T11:32:11Z

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.

newpavlov commented Apr 1, 2024

View reviewed changes

rand_core/src/lib.rs Outdated Show resolved Hide resolved

newpavlov marked this pull request as ready for review April 1, 2024 16:38

newpavlov requested a review from dhardy April 1, 2024 16:40

newpavlov mentioned this pull request Apr 1, 2024

Add rand_core::InfallibleRng marker trait #1412

Closed

newpavlov commented Apr 1, 2024

View reviewed changes

rand_core/src/lib.rs Outdated Show resolved Hide resolved

dhardy mentioned this pull request Apr 3, 2024

Task: rustfmt #1429

Closed

dhardy reviewed Apr 3, 2024

View reviewed changes

dhardy added the E-question Participation: opinions wanted label Apr 10, 2024

dhardy reviewed Apr 19, 2024

View reviewed changes

This was referenced Apr 30, 2024

Add #[track_caller] to methods which panic #1442

Merged

Tracker: rand 0.9 #1165

Open

josephlr approved these changes May 6, 2024

View reviewed changes

Rework fallability

515ad2c

newpavlov force-pushed the try_traits branch from 6c6bcb6 to 515ad2c Compare May 7, 2024 19:39

newpavlov added 3 commits May 7, 2024 22:46

Merge branch 'master' into try_traits

6466cc8

fix benches

cafe56f

Fix warnings in benches

9d5735b

newpavlov changed the title ~~Rework fallability~~ Add TryRngCore and TryCryptoRng traits May 7, 2024

dhardy requested changes May 8, 2024

View reviewed changes

Improve rand_core docs

a28d242

dhardy approved these changes May 8, 2024

View reviewed changes

dhardy merged commit fba5521 into master May 8, 2024
12 checks passed

newpavlov deleted the try_traits branch May 8, 2024 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `TryRngCore` and `TryCryptoRng` traits #1424

Add `TryRngCore` and `TryCryptoRng` traits #1424

newpavlov commented Apr 1, 2024 •

edited

Loading

newpavlov commented Apr 1, 2024 •

edited

Loading

newpavlov Apr 1, 2024 •

edited

Loading

dhardy Apr 3, 2024

josephlr May 6, 2024

newpavlov May 6, 2024

dhardy commented Apr 2, 2024

dhardy left a comment

dhardy Apr 3, 2024

newpavlov Apr 3, 2024 •

edited

Loading

dhardy Apr 3, 2024

dhardy commented Apr 3, 2024

MichaelOwenDyer commented Apr 3, 2024

newpavlov commented Apr 3, 2024

MichaelOwenDyer commented Apr 5, 2024

dhardy commented Apr 5, 2024

newpavlov commented Apr 5, 2024

dhardy commented Apr 19, 2024

josephlr left a comment

josephlr May 6, 2024

newpavlov commented May 6, 2024 •

edited

Loading

dhardy commented May 7, 2024

dhardy commented May 7, 2024

newpavlov commented May 7, 2024

dhardy left a comment

newpavlov commented May 8, 2024 •

edited

Loading

Add TryRngCore and TryCryptoRng traits #1424

Add TryRngCore and TryCryptoRng traits #1424

Conversation

newpavlov commented Apr 1, 2024 • edited Loading

newpavlov commented Apr 1, 2024 • edited Loading

newpavlov Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

dhardy Apr 3, 2024

Choose a reason for hiding this comment

josephlr May 6, 2024

Choose a reason for hiding this comment

newpavlov May 6, 2024

Choose a reason for hiding this comment

dhardy commented Apr 2, 2024

dhardy left a comment

Choose a reason for hiding this comment

dhardy Apr 3, 2024

Choose a reason for hiding this comment

newpavlov Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

dhardy Apr 3, 2024

Choose a reason for hiding this comment

dhardy commented Apr 3, 2024

MichaelOwenDyer commented Apr 3, 2024

newpavlov commented Apr 3, 2024

MichaelOwenDyer commented Apr 5, 2024

dhardy commented Apr 5, 2024

newpavlov commented Apr 5, 2024

dhardy commented Apr 19, 2024

josephlr left a comment

Choose a reason for hiding this comment

josephlr May 6, 2024

Choose a reason for hiding this comment

newpavlov commented May 6, 2024 • edited Loading

dhardy commented May 7, 2024

dhardy commented May 7, 2024

newpavlov commented May 7, 2024

dhardy left a comment

Choose a reason for hiding this comment

newpavlov commented May 8, 2024 • edited Loading

Add `TryRngCore` and `TryCryptoRng` traits #1424

Add `TryRngCore` and `TryCryptoRng` traits #1424

newpavlov commented Apr 1, 2024 •

edited

Loading

newpavlov commented Apr 1, 2024 •

edited

Loading

newpavlov Apr 1, 2024 •

edited

Loading

newpavlov Apr 3, 2024 •

edited

Loading

newpavlov commented May 6, 2024 •

edited

Loading

newpavlov commented May 8, 2024 •

edited

Loading