rust-random / rand

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

Require SeedableRng::Seed to impl Clone #1491

Closed clarfonthey closed 2 months ago

clarfonthey commented 2 months ago

Summary

Adds a Clone bound to SeedableRng::Seed.

Motivation

Simply put: it's already possible to clone these seeds based upon the bounds already provided, but it's really annoying without a proper Clone bound. You can just as easily create a new one with Default and write all of the data from the old seed into the new one with AsMut, but this now requires me to do this explicitly instead of just being able to derive(Clone) on a struct that contains a seed.

Details

This is also a breaking change, but I don't think people will have a problem including it in the next breaking release.

clarfonthey commented 2 months ago

Actually considering adding AsRef to this too, since I had no idea AsMut didn't imply it. It's more work for implementors, but is it really?

Honestly, I kinda wish that the seed parameter were just N to indicate the number of bytes needed, and we forced it to just be an appropriately sized array. But I dunno how people feel about that.

dhardy commented 2 months ago

Honestly, I kinda wish that the seed parameter were just N to indicate the number of bytes needed, and we forced it to just be an appropriately sized array. But I dunno how people feel about that.

This design decision comes from a while back... but it appears that we still need an incomplete nightly-only feature to do this:

#![feature(generic_const_exprs)]

pub trait Seedable {
    const BYTES: usize;
    fn from_seed(seed: [u8; Self::BYTES]) -> Self;
}

struct MyRng;
impl Seedable for MyRng {
    const BYTES: usize = 12;
    fn from_seed(seed: [u8; 12]) -> Self {
        todo!()
    }
}
clarfonthey commented 2 months ago

Yeah, the best workaround would be to use something like generic-array, which honestly seems preferable here.

dhardy commented 2 months ago

I don't think we want to use generic-array — we've had plenty enough people complain about rand being too complicated or having too many dependencies. But once we get equivalent functionality in stable Rust we should consider revisiting this.

In the mean-time, please do add an AsRef bound. It will affect Seed512 but that's probably it.

clarfonthey commented 2 months ago

Finally got to this, but now AsRef is in the list.

I updated the description slightly since Default is the only trait that isn't implemented for [T; N], although you will have to manually implement AsRef and AsMut since it isn't derivable.

dhardy commented 2 months ago

autoimpl supports AsRef and AsMut. But this is fine.