Skip to content

Commit d20af15

Browse files
bobzhangclaude
andauthored
fix(quickcheck): produce shrink candidates without recursing per block (#4211)
* test(quickcheck): pin the shrink block sequence, show it exhausting the stack Two test files, both against the current implementations. `removes_wbtest.mbt` pins what `removes_array` and `removes_list` produce — one candidate per block offset that still leaves a whole block, ascending for the array and descending for the list — along with their `size_hint`, for every `(n, k)` pair up to 24 elements. It passes as things stand, so a later change to how the candidates are produced has to keep producing the same ones. `deep_recursion_wbtest.mbt` shows the bug. Both functions recurse once per removed block, and the recursive call sits in argument position, so the entire descent runs while the iterator is being built — before it can yield anything. With `k = 1` that is one stack frame per element. `Shrink for @list.List` reaches `k = 1` first (it iterates `[n/2, .., 1]` with `rev_iter`), so shrinking a large list crashes on the first candidate: a property test whose counterexample is big reports a stack overflow instead of the counterexample. `Shrink for Array` iterates the same list forward, so it reaches `k = 1` only at the end of the candidate sequence — same depth, later. Those three tests fail on js, wasm and wasm-gc (`RangeError: Maximum call stack size exceeded`, frames at `quickcheck/shrink/utils.mbt:49`) and pass on native, whose stack holds 20,000 frames. The next commit fixes them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jtBJHK749cpStav5ZMNCr * 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 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9d086a3 commit d20af15

4 files changed

Lines changed: 176 additions & 20 deletions

File tree

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// Copyright 2026 International Digital Economy Academy
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
///|
16+
/// The size these tests use. It has to clear the js and wasm-gc stack limits
17+
/// (both are in the low tens of thousands of frames) while staying small
18+
/// enough that building one candidate is cheap.
19+
const DEEP : Int = 20000
20+
21+
///|
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.
27+
test "removes_list builds its candidates without recursing per element" {
28+
let xs : @list.List[Int] = List(Array::makei(DEEP, i => i))
29+
guard removes_list(1, DEEP, xs).next() is Some(candidate) else {
30+
abort("expected a candidate")
31+
}
32+
guard candidate.length() == DEEP - 1 else {
33+
abort("expected \{DEEP - 1} elements, got \{candidate.length()}")
34+
}
35+
}
36+
37+
///|
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.
42+
test "removes_array builds its candidates without recursing per element" {
43+
let xs = Array::makei(DEEP, i => i)
44+
guard removes_array(1, DEEP, xs).next() is Some(candidate) else {
45+
abort("expected a candidate")
46+
}
47+
guard candidate.length() == DEEP - 1 else {
48+
abort("expected \{DEEP - 1} elements, got \{candidate.length()}")
49+
}
50+
}
51+
52+
///|
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.
56+
test "shrinking a large list yields a candidate" {
57+
let xs : @list.List[Int] = List(Array::makei(DEEP, i => i))
58+
guard Shrink::shrink(xs).next() is Some(candidate) else {
59+
abort("expected a candidate")
60+
}
61+
guard candidate.length() == DEEP - 1 else {
62+
abort("expected \{DEEP - 1} elements, got \{candidate.length()}")
63+
}
64+
}
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// Copyright 2026 International Digital Economy Academy
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
///|
16+
/// The block offsets `removes_array` and `removes_list` remove, ascending:
17+
/// every multiple of `k` that still leaves a whole block inside `n`.
18+
fn block_offsets(k : Int, n : Int) -> Array[Int] {
19+
let offsets = []
20+
for start = 0; start + k <= n; start = start + k {
21+
offsets.push(start)
22+
}
23+
offsets
24+
}
25+
26+
///|
27+
/// Pins the candidate sequence of `removes_array` so that a change to how the
28+
/// candidates are produced cannot quietly change which candidates come out, or
29+
/// in what order. Every `(n, k)` pair up to 24 elements is covered.
30+
test "removes_array removes one block per offset, ascending" {
31+
for n = 0; n <= 24; n = n + 1 {
32+
let xs = Array::makei(n, i => i)
33+
for k = 1; k <= n; k = k + 1 {
34+
let expected = block_offsets(k, n).map(start => {
35+
[..xs[:start], ..xs[start + k:]]
36+
})
37+
let candidates = removes_array(k, n, xs)
38+
guard candidates.size_hint() is Some(hint) && hint == expected.length() else {
39+
abort(
40+
"n=\{n} k=\{k}: wrong size_hint for \{expected.length()} candidates",
41+
)
42+
}
43+
guard candidates.collect() == expected else {
44+
abort("n=\{n} k=\{k}: candidates differ")
45+
}
46+
}
47+
}
48+
}
49+
50+
///|
51+
/// The same for `removes_list`, whose offsets run the other way.
52+
test "removes_list removes one block per offset, descending" {
53+
for n = 0; n <= 24; n = n + 1 {
54+
let xs : @list.List[Int] = List(Array::makei(n, i => i))
55+
for k = 1; k <= n; k = k + 1 {
56+
let expected = block_offsets(k, n)
57+
.rev()
58+
.map(start => xs.take(start).concat(xs.drop(start + k)))
59+
let candidates = removes_list(k, n, xs)
60+
guard candidates.size_hint() is Some(hint) && hint == expected.length() else {
61+
abort(
62+
"n=\{n} k=\{k}: wrong size_hint for \{expected.length()} candidates",
63+
)
64+
}
65+
guard candidates.collect() == expected else {
66+
abort("n=\{n} k=\{k}: candidates differ")
67+
}
68+
}
69+
}
70+
}
71+
72+
///|
73+
/// `k` larger than the collection leaves nothing to remove.
74+
test "removing more than the collection holds yields nothing" {
75+
let xs = Array::makei(4, i => i)
76+
let ls : @list.List[Int] = List(xs)
77+
guard removes_array(5, 4, xs).collect() is [] else { abort("array") }
78+
guard removes_list(5, 4, ls).collect() is [] else { abort("list") }
79+
}

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)