quickcheck
quickcheck copied to clipboard
Arbitrary for arrays using just stabilized minimal const generics
I won't merge this until const generics have been stable for at least 2 releases, to give folks time to migrate to the new Rust release. Feel free to ping me when that time comes if I don't remember to merge it.
That's alright with me. :)
Also, could you please add a detailed comment for each
unsafe
block justifying why it is safe? It should be in this form:// SAFETY: yadda yadda yadda
Done!
Currently there is no way of collecting an iterator into a fixed-size array, so we're forced to using MaybeUninit
, unless we simply initialize an array with default values and then override those with the actual values.
The latter has two disadvantages, due to which I went with the former:
- we would be initializing a possibly large number of values, which would be discarded immediately after
- the type
T
of[T; N]
might not actually beT: Default
to begin with
Also, I haven't scrutinized the code too carefully yet, but is
unsafe
required here? There is no safe way to create arrays in a generic context?
I feel reasonably confident in the code, but you might want to give the unit test a check as I'm not 100% sure it actually does test what needs to be tested.
Would it be ok to use a single-purpose crate for the array initialization? array_init allows to initialize the array using a closure and also fixes the memory leak.
So when it comes to the leak-on-panic, I think it would only be able to happen when calling T::arbitrary(). Almost certainly, a panic inside of an arbitrary() impl is a bug.
The documentation of Arbitrary::arbitrary
doesn't say that a panic inside the impl is a bug. Maybe this should be added.
On the flip side, quickcheck does specifically catch panics and attempt to shrink the witnesses. So if there is a leak, it could build up. But, unless that leak is gratuitous, quickcheck is going to present some kind of witness as a failure, and ultimately, the panic should be fixed.
If T::arbitrary
panics, there is no value. How can quickcheck catch the panic and shrink the witness? Does quickcheck call T::arbitrary
again in this case?
If
T::arbitrary
panics, there is no value. How can quickcheck catch the panic and shrink the witness? Does quickcheck callT::arbitrary
again in this case?
Oh good point. it would just tear down the entire testing infrastructure. Which probably makes "leaks may happen on panic" more tolerable.
@BurntSushi is there anything I can do to get this merged, or should I close it?