search

rust-random / rand

A Rust library for random number generation.
https://crates.io/crates/rand
Other
1.6k stars 423 forks source link

Add `TryRngCore` and `TryCryptoRng` traits #1424

Closed newpavlov closed 1 month ago

newpavlov commented 3 months ago

Closes #1418.

newpavlov commented 3 months ago

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.

dhardy commented 3 months ago

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 commented 3 months ago

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

MichaelOwenDyer commented 3 months ago

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

newpavlov commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

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

dhardy commented 2 months ago

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

newpavlov commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

newpavlov commented 1 month ago

@dhardy Done.

newpavlov commented 1 month ago

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.