fix(quickcheck): produce shrink candidates without recursing per block

[bobzhang]

Shrinking a large collection crashes with a stack overflow on js, wasm and wasm-gc: a property test whose counterexample is big reports RangeError: Maximum call stack size exceeded instead of the counterexample.

Two commits, deliberately in this order.

6d03c2ae — tests against the code as it stands

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 — together with their size_hint, for every (n, k) with n <= 24 and 1 <= k <= n. It passes on this commit, before any production code changes, so it is a characterisation of the existing behaviour rather than of the fix.

deep_recursion_wbtest.mbt shows the bug, and its three tests fail on this commit on js, wasm and wasm-gc (frames at quickcheck/shrink/utils.mbt:49) while passing on native.

ff70d6ce — the fix

Both functions recursed once per removed block, with the recursive call in argument position — so the whole descent ran while the iterator was being built, before it could yield anything. With k = 1 that is one stack frame per element. Shrink for @list.List asks for k = 1 first (it iterates [n/2, .., 1] through rev_iter), so a list crashes on its very first candidate; Shrink for Array iterates the same list forward and reaches k = 1 only at the end of the sequence — same depth, later.

They now walk the offsets with a cursor instead:

// removes_array: start = 0, step +k while start + k <= n
let candidate = [..xs[:start], ..xs[start + k:]]
// removes_list: start = (n - k) / k * k, step -k down to 0
let candidate = xs.take(start).concat(xs.drop(start + k))

Building the iterator is O(1) and yielding a candidate uses no stack beyond the call. removes_wbtest.mbt is untouched by this commit, which is the evidence that the sequences and hints did not move.

Where this comes from

removes_array / removes_list arrived with #3955, transliterated from Haskell QuickCheck's shrinkList, where the shape is fine because the recursive call is a thunk and only the consumed elements get forced. In a strict language the same expression means "recurse to the bottom before returning anything". shr_sub_terms in the same package has the same origin; #4208 deferred its construction, and making it iterative needs a different fix — out of scope here.

Numbers

Faster too, because the old form rebuilt each 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 allocates the candidate through an array spread rather than 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 also goes back to 10,000 elements; #4208 had to lower it to 1,000 precisely because of this bug.

Validation

• moon test -p quickcheck/shrink --target all 59/59 and moon test -p quickcheck --target all 163/163 on wasm, wasm-gc, js and native. On 6d03c2ae the first command gives 56/59 on the three non-native backends, failing exactly the three deep-recursion tests.
• moon check --target all, moon fmt --check and moon info clean; no .mbti diff (both functions are private).
• moon bench -p quickcheck passes on all four targets at 10,000 elements.
• Mutation-checked: flipping the list cursor to ascend fails removes_wbtest.mbt:52; size_hint=(n - k) / k + 2 fails removes_wbtest.mbt:30.
• Codex CLI signed off on both commits, having built independent models of the old and new implementations and confirmed they agree on ordering and exact counts for all 5,050 valid (n, k) pairs through n = 100.

🤖 Generated with Claude Code

https://claude.ai/code/session_016jtBJHK749cpStav5ZMNCr

[Copilot]

🟢 Approval recommended

The functional change is well-scoped, preserves existing candidate behavior (pinned by tests), and includes targeted regression coverage for the reported stack overflow.

Pull request overview

Fixes stack overflows on js/wasm backends when shrinking large Arrays/Lists by changing shrink-candidate generation to an iterative cursor-based approach (so iterator construction is O(1) and no longer recurses per removed block), while preserving the exact candidate sequence and size_hint behavior.

Changes:

• Rework removes_array / removes_list to generate candidates iteratively instead of via recursive iterator construction.
• Add whitebox tests that (a) pin the exact candidate sequences and size_hints for many (n, k) pairs and (b) regress the deep-recursion stack overflow scenario.
• Restore collection-shrink benchmark inputs to 10,000 elements now that shrinking no longer overflows the stack.

File summaries

File	Description
quickcheck/shrink/utils.mbt	Makes removes_array and removes_list cursor-iterative to avoid deep recursion during iterator construction.
quickcheck/shrink/removes_wbtest.mbt	Characterizes and pins candidate ordering + size_hint for removes_array/removes_list across many sizes.
quickcheck/shrink/deep_recursion_wbtest.mbt	Regression tests to ensure large collection shrinking yields candidates without stack overflow on js/wasm.
quickcheck/shrink_collection_bench_test.mbt	Increases benchmark input size back to 10,000 elements.

Review details

Suppressed comments (2)

quickcheck/shrink/deep_recursion_wbtest.mbt:39

• This comment also describes the pre-fix recursive behavior in present tense. Updating it to past tense (and noting it’s guarding against the prior failure mode) will keep the test documentation accurate.

/// `removes_array` has the same shape. Its candidate order reaches `k = 1`
/// only at the end of the sequence, so a property test hits this later than it
/// hits the `List` case, but the recursion depth is the same.

quickcheck/shrink/deep_recursion_wbtest.mbt:52

• The test description says shrinking a large list "crashes" while shrinking; after this PR the expected behavior is that it yields a candidate. Rephrasing to describe the historical symptom (as a regression test) avoids confusing readers.

/// The user-visible symptom: a property test whose counterexample is a large
/// list crashes while shrinking instead of reporting the counterexample.

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[coveralls]

Coverage Report for CI Build 6582

Coverage remained the same at 89.27%

Details

• Coverage remained the same as the base build.
• Patch coverage: 8 of 8 lines across 1 file are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	18331
Covered Lines:	16364
Line Coverage:	89.27%
Coverage Strength:	273924.65 hits per line

---

💛 - Coveralls

[bobzhang]

Addressed the Copilot review: the three doc comments in deep_recursion_wbtest.mbt described the recursive implementation in the present tense, which stopped being true at the fix commit. They now read as past tense — what the tests guard against — rather than as a description of the current code. Moved into bf183213 rather than a follow-up commit so that each commit's tree is self-consistent: the test commit keeps the present tense, which is accurate against the code it was written for.

Re-validated: moon test -p quickcheck/shrink --target all 59/59 on wasm, wasm-gc, js and native; moon check --target all and moon fmt --check clean.