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

[WIP] Implement Fill for some MaybeUninit types #1241

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 64 additions & 14 deletions src/rng.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@
// except according to those terms.

//! [`Rng`] trait
use core::num::Wrapping;
use core::{mem, slice};

use rand_core::{Error, RngCore};

use crate::distributions::uniform::{SampleRange, SampleUniform};
use crate::distributions::{self, Distribution, Standard};
use core::num::Wrapping;
use core::{mem, slice};

/// An automatically-implemented extension trait on [`RngCore`] providing high-level
/// generic methods for sampling values and other convenience methods.
Expand Down Expand Up @@ -91,7 +92,9 @@ pub trait Rng: RngCore {
/// [`Standard`]: distributions::Standard
#[inline]
fn gen<T>(&mut self) -> T
where Standard: Distribution<T> {
where
Standard: Distribution<T>,
{
Standard.sample(self)
}

Expand Down Expand Up @@ -129,7 +132,7 @@ pub trait Rng: RngCore {
fn gen_range<T, R>(&mut self, range: R) -> T
where
T: SampleUniform,
R: SampleRange<T>
R: SampleRange<T>,
{
assert!(!range.is_empty(), "cannot sample empty range");
range.sample_single(self)
Expand Down Expand Up @@ -216,7 +219,8 @@ pub trait Rng: RngCore {
/// [`fill_bytes`]: RngCore::fill_bytes
/// [`try_fill`]: Rng::try_fill
fn fill<T: Fill + ?Sized>(&mut self, dest: &mut T) {
dest.try_fill(self).unwrap_or_else(|_| panic!("Rng::fill failed"))
dest.try_fill(self)
.unwrap_or_else(|_| panic!("Rng::fill failed"))
}

/// Fill any type implementing [`Fill`] with random data
Expand Down Expand Up @@ -316,25 +320,25 @@ pub trait Fill {
fn try_fill<R: Rng + ?Sized>(&mut self, rng: &mut R) -> Result<(), Error>;
}

macro_rules! impl_fill_each {
macro_rules! impl_fill_each_uninit {
() => {};
($t:ty) => {
impl Fill for [$t] {
impl Fill for [mem::MaybeUninit<$t>] {
fn try_fill<R: Rng + ?Sized>(&mut self, rng: &mut R) -> Result<(), Error> {
for elt in self.iter_mut() {
*elt = rng.gen();
*elt = mem::MaybeUninit::new(rng.gen());
}
Ok(())
}
}
Comment on lines -322 to 333
Copy link
Member

Choose a reason for hiding this comment

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

Implementing Fill for [MaybeUninit<bool>] (etc.) is okay in itself, but I don't think it very useful. In order to use it with uninitialized memory, assume_init is needed somewhere, which thus requires the user use unsafe code (and based on the assumption that fill succeeded). I mean, users can use this API correctly, but not easily, and it isn't difficult to implement equivalent functionality in userspace anyway.

};
($t:ty, $($tt:ty,)*) => {
impl_fill_each!($t);
impl_fill_each!($($tt,)*);
impl_fill_each_uninit!($t);
impl_fill_each_uninit!($($tt,)*);
};
}

impl_fill_each!(bool, char, f32, f64,);
impl_fill_each_uninit!(bool, char, f32, f64,);

impl Fill for [u8] {
fn try_fill<R: Rng + ?Sized>(&mut self, rng: &mut R) -> Result<(), Error> {
Expand Down Expand Up @@ -392,10 +396,36 @@ macro_rules! impl_fill {
impl_fill!(u16, u32, u64, usize, u128,);
impl_fill!(i8, i16, i32, i64, isize, i128,);

macro_rules! impl_fill_delegate_to_maybe_uninit{
() => {};
($t:ty) => {
impl Fill for [$t] {
fn try_fill<R: Rng + ?Sized>(&mut self, rng: &mut R) -> Result<(), Error> {
Fill::try_fill(
unsafe {
slice::from_raw_parts_mut(self.as_mut_ptr()
as *mut mem::MaybeUninit<$t>,
self.len()
)
},
rng,
)
}
}
};
($t:ty, $($tt:ty,)*) => {
impl_fill_delegate_to_maybe_uninit!($t);
impl_fill_delegate_to_maybe_uninit!($($tt,)*);
};
}

impl_fill_delegate_to_maybe_uninit!(bool, char, f32, f64,);
Comment on lines +399 to +422
Copy link
Member

Choose a reason for hiding this comment

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

This part doesn't make much sense to me: replacing what was a safe impl of Fill for [bool] (etc.) with an unsafe one which lowers to [MaybeUninit<bool>] (etc.).


#[cfg_attr(doc_cfg, doc(cfg(feature = "min_const_gen")))]
#[cfg(feature = "min_const_gen")]
impl<T, const N: usize> Fill for [T; N]
where [T]: Fill
where
[T]: Fill,
{
fn try_fill<R: Rng + ?Sized>(&mut self, rng: &mut R) -> Result<(), Error> {
self[..].try_fill(rng)
Expand Down Expand Up @@ -431,9 +461,10 @@ impl_fill_arrays!(!div 4096, N,N,N,N,N,N,N,);
#[cfg(test)]
mod test {
use super::*;
use crate::test::rng;
use crate::rngs::mock::StepRng;
#[cfg(feature = "alloc")] use alloc::boxed::Box;
use crate::test::rng;
#[cfg(feature = "alloc")]
use alloc::boxed::Box;

#[test]
fn test_fill_bytes_default() {
Expand Down Expand Up @@ -485,6 +516,25 @@ mod test {
assert_eq!(array, gen);
}

#[test]
fn test_gen_range_uninit_same_as_simple() {
let mut r = rng(101);
let mut mu_array: [mem::MaybeUninit<char>; 64] = unsafe {
// Safe to assume init because inner values have uninit type.
mem::MaybeUninit::uninit().assume_init()
Comment on lines +523 to +524
Copy link
Member

Choose a reason for hiding this comment

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

Did you read the doc on assume_init? This is not safe.

It sounds like you want to cast [MaybeUninit<T>; N] to &mut [T]. This is explicitly not allowed by the Rust memory model.

Copy link
Author

Choose a reason for hiding this comment

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

Nope, you are wrong.

This code is same as in MaybeUninit docs actually.
https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#initializing-an-array-element-by-element

And I make an array of MaybeUninits which can be created from uninit array.

Copy link
Member

Choose a reason for hiding this comment

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

Correct. It's just your comment that is wrong then: the reason it is safe is that the outer type is an array of MaybeUninit.

};
r.fill(&mut mu_array);

let mut r = rng(101);
let mut array = ['\0'; 64];
r.fill(&mut array);

assert!(mu_array.iter().zip(&array).all(|(&a, &b)| unsafe {
// This assume_init should trigger Miri if we done something wrong
a.assume_init() == b
}))
}

#[test]
fn test_fill_empty() {
let mut array = [0u32; 0];
Expand Down