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

Arbitrary for arrays using just stabilized minimal const generics #282

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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions src/arbitrary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::env;
use std::ffi::{CString, OsString};
use std::hash::{BuildHasher, Hash};
use std::iter::{empty, once};
use std::mem::MaybeUninit;
use std::net::{
IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6,
};
Expand Down Expand Up @@ -248,6 +249,49 @@ impl_arb_for_tuples! {
(H, 7),
}

impl<T: Arbitrary + Sized, const N: usize> Arbitrary for [T; N] {
#[allow(unused_variables)] // for [T; 0]
fn arbitrary(g: &mut Gen) -> [T; N] {
// The following code was adopted from the corresponding example provided for `MaybeUninit`:
// https://doc.rust-lang.org/1.50.0/core/mem/union.MaybeUninit.html#initializing-an-array-element-by-element

// SAFETY: Create an uninitialized array of `MaybeUninit`. The `assume_init` is
// safe because the type we are claiming to have initialized here is a
// bunch of `MaybeUninit`s, which do not require initialization.
let mut maybe_uninit: [MaybeUninit<T>; N] =
unsafe { MaybeUninit::uninit().assume_init() };

// SAFETY: Dropping a `MaybeUninit` does nothing. Thus using raw pointer
// assignment instead of `ptr::write` does not cause the old
// uninitialized value to be dropped. Also if there is a panic during
// this loop, we have a memory leak, but there is no memory safety
// issue.
Copy link
Owner

Choose a reason for hiding this comment

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

This wording is a little confusing to me. It says "Thus using raw pointer assignment instead of ptr::write" and that makes me think that the code below is going to be using raw pointer assignment. But it in fact uses ptr::write.

It looks like the wording was copied from the example in std's docs, but the actual code here is using ptr::write instead of just *elem = MaybeUninit::new(T::arbitrary(g)). It seems like the latter would be preferrable?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

for elem in &mut maybe_uninit[..] {
*elem = MaybeUninit::new(T::arbitrary(g));
}

// SAFETY: Everything is initialized (i.e. safe).
// Transmute the array to the initialized type.
// (We need to use `transmute_copy` here as `transmute`
// has some limitations around generic types:
// https://github.com/rust-lang/rust/issues/47966)
unsafe { std::mem::transmute_copy::<_, [T; N]>(&maybe_uninit) }
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be possible to spell out in more detail why transmute_copy is needed here?

Copy link
Author

Choose a reason for hiding this comment

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

The issue is described in this issue and the recommendation to use transmute_copy instead was taken from here.

Should I just add a link to the rust issue in the code comment?

Copy link
Owner

Choose a reason for hiding this comment

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

We should at least add a link, yes. But if we can't grok and regurgitate the specific reasoning for why it's safe in this case, then that makes me worried.

I don't have time to dig into this now unfortunately though.

}

fn shrink(&self) -> Box<dyn Iterator<Item = [T; N]>> {
let cloned = self.clone();
let iter = (0..N).flat_map(move |index| {
let cloned = cloned.clone();
cloned[index].shrink().map(move |shr_value| {
let mut result = cloned.clone();
result[index] = shr_value.clone();
result
})
});
Box::new(iter)
}
}

impl<A: Arbitrary> Arbitrary for Vec<A> {
fn arbitrary(g: &mut Gen) -> Vec<A> {
let size = {
Expand Down Expand Up @@ -1410,6 +1454,29 @@ mod test {
eq(Wrapping(0i32), vec![]);
}

#[test]
fn arrays() {
eq([true], vec![[false]]);

eq([false, false], vec![]);
eq([true, false], vec![[false, false]]);
eq([true, true], vec![[false, true], [true, false]]);

eq([false, false, false], vec![]);
eq([true, false, false], vec![[false, false, false]]);
eq(
[true, true, false],
vec![[false, true, false], [true, false, false]],
);

eq([false, false, false, false], vec![]);
eq([true, false, false, false], vec![[false, false, false, false]]);
eq(
[true, true, false, false],
vec![[false, true, false, false], [true, false, false, false]],
);
}

#[test]
fn vecs() {
eq(
Expand Down