BurntSushi / quickcheck

Automated property based testing for Rust (with shrinking).
The Unlicense
2.32k stars 144 forks source link

Arbitrary for arrays using just stabilized minimal const generics #282

Closed regexident closed 1 month ago

regexident commented 3 years ago

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:

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.

jakoschiko commented 3 years ago

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.

jakoschiko commented 3 years ago

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?

BurntSushi commented 3 years ago

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?

Oh good point. it would just tear down the entire testing infrastructure. Which probably makes "leaks may happen on panic" more tolerable.

regexident commented 2 years ago

@BurntSushi is there anything I can do to get this merged, or should I close it?