-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Implement Random
for array
#136732
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
base: master
Are you sure you want to change the base?
Implement Random
for array
#136732
Conversation
Implement `Random` for `[T; N]`, where `Random` is implemented for `T`.
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
r? @joboet |
I think it would be better to implement this with a dedicated method on the |
That doesn't necessarily need specialization. We could add an internal |
Calling fn random_array() -> [u8; 1_000] {
array::from_fn(|_| u8::random(&mut DefaultRandomSource))
}
fn random_array2() -> [u8; 1_000] {
let mut arr = [0; 1_000];
DefaultRandomSource.fill_bytes(&mut arr);
arr
} Calling
|
library/core/src/array/mod.rs
Outdated
let mut buf = [const { MaybeUninit::uninit() }; N]; | ||
for elem in &mut buf { | ||
elem.write(T::random(source)); | ||
} | ||
// SAFETY: all elements of the array were initialized. | ||
unsafe { mem::transmute_copy(&buf) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a version that uses fill_bytes()
.
let mut buf = [const { MaybeUninit::uninit() }; N]; | |
for elem in &mut buf { | |
elem.write(T::random(source)); | |
} | |
// SAFETY: all elements of the array were initialized. | |
unsafe { mem::transmute_copy(&buf) } | |
let mut buf = [const { MaybeUninit::<T>::uninit() }; N]; | |
// SAFETY: this initializes every element in the buffer with random bytes. | |
unsafe { | |
let slice = core::slice::from_raw_parts_mut(&raw mut buf as *mut u8, N * mem::size_of::<T>()); | |
source.fill_bytes(slice); | |
mem::transmute_copy(&buf) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unsound for generic Ts. There's no guarantee that a Type's random
impl shovels those bytes into their memory representation. A trivial example would be NonZero<u8>
which could implement Random
via rejection sampling. Setting it to 0 would be unsound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@the8472 I did notice that, so we would need to restrict this to types that can hold arbitrary bytes (which currently includes every type implementing Random
except bool
). Otherwise, use a slower custom implementation with rejection sampling or modulo.
This comment has been minimized.
This comment has been minimized.
Implements specialized `Random` for the array whose elements are integer primitives. This reduces the number of system calls.
7c195ab
to
5435be8
Compare
library/core/src/array/mod.rs
Outdated
macro_rules! impl_random_for_integer_array { | ||
($t:ty) => { | ||
#[unstable(feature = "random", issue = "130703")] | ||
impl<const N: usize> Random for [$t; N] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specializations should be hidden behind a private trait. https://std-dev-guide.rust-lang.org/policy/specialization.html
Alternatively you can avoid specialization by implementing a buffering random source wrapper as mentioned in #136732 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too would prefer a buffering implementation (in the generic case, the specialization for primitives makes sense to me) – but you can leave that for another PR.
&raw mut buf as *mut u8, | ||
N * (<$t>::BITS as usize / 8), | ||
); | ||
source.fill_bytes(bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unsound: you create a &mut [u8]
and pass it to user code – but nothing guarantees that the user code won't try to read the slice, which would be undefined behaviour. My recommendation would be to use MaybeUninit::zeroed
to create buf
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this needs a trait bound like zerocopy::FromBytes
or bytemuck::Pod
, but we don't have that in std
, so... the macro should be renamed to unsafe_impl_...
since safety depends on the macro caller?
This uses specialization to do what we couldn't do with Any non-default impls of (My view is still that random-value generation should be left to external crates like |
Implement
Random
for[T; N]
, whereRandom
is implemented forT
.rust-lang/libs-team#393 (comment) states:
This PR is also based on #130703 (comment).
Tracking issue: #130703