Skip to content

Commit bf18321

Browse files
bobzhangclaude
andcommitted
fix(quickcheck): produce shrink candidates without recursing per block
Replace the recursion in `removes_array` and `removes_list` with a cursor over the block offsets. Both now walk `0, k, 2k, ..` directly — ascending for the array, descending for the list — so building the iterator is O(1) and yielding a candidate uses no stack beyond the call itself. The candidate sequences and `size_hint`s are the ones the previous commit pinned, which is why that test file is unchanged here. This fixes the three stack-overflow tests from the previous commit on js, wasm and wasm-gc, so a property test whose counterexample is a large collection now shrinks instead of dying with `RangeError: Maximum call stack size exceeded`. It is also much faster on the `List` side, because the old form rebuilt the candidate through one `concat` per level on the way back up, where the new one does a single `take`/`drop`/`concat`. First candidate from a 10,000-element collection, native release: | Collection | Before | After | | ---------- | --------: | --------: | | Array | 240.69 ns | 288.55 ns | | List | 1.32 ms | 213.65 µs | The array case is a hair slower — it now allocates the candidate through an array spread instead of returning a shared empty literal — and in exchange the `k == n` special case added in #4208 is gone, since taking a zero-length prefix already costs nothing. The benchmark goes back to 10,000 elements; #4208 had to lower it to 1,000 precisely because of the bug this fixes. The comments in `deep_recursion_wbtest.mbt` move to the past tense here, so that they describe the failure the tests guard against rather than the code as it now stands. Reviewed by Codex CLI together with the preceding test commit: "Verified ordering, exact size hints, non-divisible lengths, `k == n` without element copying, and cursor compatibility with single-use iterators. Independent old/new models agree for 5,050 input pairs." Signed-off-by: Codex CLI <codex@openai.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jtBJHK749cpStav5ZMNCr
1 parent 6d03c2a commit bf18321

3 files changed

Lines changed: 45 additions & 29 deletions

File tree

quickcheck/shrink/deep_recursion_wbtest.mbt

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,11 @@
1919
const DEEP : Int = 20000
2020

2121
///|
22-
/// `removes_list` recurses once per removed block, and the recursive call sits
23-
/// in argument position, so the whole descent runs while the iterator is being
24-
/// built -- before it can yield anything. With `k = 1` that is one frame per
25-
/// element.
22+
/// `removes_list` used to recurse once per removed block, with the recursive
23+
/// call in argument position, so the whole descent ran while the iterator was
24+
/// being built -- before it could yield anything. With `k = 1` that was one
25+
/// stack frame per element, and this test overflowed the stack on js, wasm and
26+
/// wasm-gc.
2627
test "removes_list builds its candidates without recursing per element" {
2728
let xs : @list.List[Int] = List(Array::makei(DEEP, i => i))
2829
guard removes_list(1, DEEP, xs).next() is Some(candidate) else {
@@ -34,9 +35,10 @@ test "removes_list builds its candidates without recursing per element" {
3435
}
3536

3637
///|
37-
/// `removes_array` has the same shape. Its candidate order reaches `k = 1`
38-
/// only at the end of the sequence, so a property test hits this later than it
39-
/// hits the `List` case, but the recursion depth is the same.
38+
/// `removes_array` had the same shape and overflowed the same way. Its
39+
/// candidate order reaches `k = 1` only at the end of the sequence, so a
40+
/// property test would have hit this later than it hits the `List` case, but
41+
/// the depth was the same.
4042
test "removes_array builds its candidates without recursing per element" {
4143
let xs = Array::makei(DEEP, i => i)
4244
guard removes_array(1, DEEP, xs).next() is Some(candidate) else {
@@ -48,8 +50,9 @@ test "removes_array builds its candidates without recursing per element" {
4850
}
4951

5052
///|
51-
/// The user-visible symptom: a property test whose counterexample is a large
52-
/// list crashes while shrinking instead of reporting the counterexample.
53+
/// The user-visible symptom this guards against: a property test whose
54+
/// counterexample was a large list crashed while shrinking instead of
55+
/// reporting the counterexample.
5356
test "shrinking a large list yields a candidate" {
5457
let xs : @list.List[Int] = List(Array::makei(DEEP, i => i))
5558
guard Shrink::shrink(xs).next() is Some(candidate) else {

quickcheck/shrink/utils.mbt

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,29 +25,42 @@ fn[T] deferred(f : () -> Iter[T]) -> Iter[T] {
2525
}
2626

2727
///|
28-
/// `n` must be `xs.length()`; the recursive call maintains that, and both
29-
/// guards below rely on it.
28+
/// Yields `xs` with one block of `k` consecutive elements removed: one
29+
/// candidate per block offset `0, k, 2k, ..`, up to the last offset that still
30+
/// leaves a whole block, in ascending order.
31+
///
32+
/// `n` must be `xs.length()`.
3033
fn[T] removes_array(k : Int, n : Int, xs : Array[T]) -> Iter[Array[T]] {
3134
guard k <= n else { [||] }
32-
// Dropping every element leaves the empty array; taking the `k`-element
33-
// prefix first would copy the whole array only to discard it. Past this
34-
// guard `xs[k:]` is non-empty.
35-
guard k < n else { [|[]|] }
36-
let xs2 = xs[:k].to_owned()
37-
let xs1 = xs[k:].to_owned()
38-
[|xs1|].add(removes_array(k, n - k, xs1).map(x => xs2 + x))
35+
let mut start = 0
36+
Iter::new(
37+
fn() {
38+
guard start + k <= n else { None }
39+
let candidate = [..xs[:start], ..xs[start + k:]]
40+
start += k
41+
Some(candidate)
42+
},
43+
size_hint=(n - k) / k + 1,
44+
)
3945
}
4046

4147
///|
48+
/// The `@list.List` counterpart of `removes_array`, except that the offsets
49+
/// descend: the block nearest the end is removed first.
50+
///
51+
/// `n` must be `xs.length()`.
4252
fn[T] removes_list(k : Int, n : Int, xs : @list.List[T]) -> Iter[@list.List[T]] {
4353
guard k <= n else { [||] }
44-
let xs_drop = xs.drop(k)
45-
if xs_drop.is_empty() {
46-
[|List([])|]
47-
} else {
48-
let xs_take = xs.take(k)
49-
removes_list(k, n - k, xs_drop).map(x => xs_take.concat(x)).add([|xs_drop|])
50-
}
54+
let mut start = (n - k) / k * k
55+
Iter::new(
56+
fn() {
57+
guard start >= 0 else { None }
58+
let candidate = xs.take(start).concat(xs.drop(start + k))
59+
start -= k
60+
Some(candidate)
61+
},
62+
size_hint=(n - k) / k + 1,
63+
)
5164
}
5265

5366
///|

quickcheck/shrink_collection_bench_test.mbt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,13 @@
1313
// limitations under the License.
1414

1515
///|
16-
test "bench shrink Array first candidate size=1000" (it : @bench.T) {
17-
let input = Array::make(1000, ())
16+
test "bench shrink Array first candidate size=10000" (it : @bench.T) {
17+
let input = Array::make(10000, ())
1818
it.bench(fn() { it.keep(@shrink.Shrink::shrink(input).next()) })
1919
}
2020

2121
///|
22-
test "bench shrink List first candidate size=1000" (it : @bench.T) {
23-
let input : @list.List[Unit] = List(Array::make(1000, ()))
22+
test "bench shrink List first candidate size=10000" (it : @bench.T) {
23+
let input : @list.List[Unit] = List(Array::make(10000, ()))
2424
it.bench(fn() { it.keep(@shrink.Shrink::shrink(input).next()) })
2525
}

0 commit comments

Comments
 (0)