Handle minimum signed values in QuickCheck shrinking - #4151
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes QuickCheck shrinking for signed minimum values without overflowing abs().
Changes:
- Terminate signed shrinking loops when the delta reaches zero.
- Add boundary-value regression tests for
IntandInt64.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Summary | Review status |
|---|---|---|
quickcheck/shrink/shrink.mbt |
Updates signed integer shrink candidate generation. | No findings |
quickcheck/shrink/shrink_test.mbt |
Adds minimum-value regression tests. | Moderate issue: assert the complete candidate sequence or expected length; the current checks can miss skipped intermediate candidates. |
Suppressed comments (1)
quickcheck/shrink/shrink_test.mbt:72
- This regression test samples only the first two candidates and the final zero, so an iterator that skips intermediate halving candidates can still pass. Please assert the complete sequence or at least its expected length as well (the
Int64minimum produces 64 candidates including zero).
(candidates[0], candidates[1], candidates[candidates.length() - 1]),
content="(-9223372036854775807, -9223372036854775806, 0)",
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`Int64::min`, `Int64::max`, `Int64::clamp`, `UInt::min`, `UInt::max` and `UInt::clamp` carried one-line comments while their documented siblings follow the full structure. Bring all six to the same level: semantics, parameters, return behavior (including the abort when `min > max` in `clamp`) and executable examples. Continues moonbitlang#623
…ang#4142) * perf(bytes): skip redundant initialization in concatenation * Limit Bytes benchmark setup to benchmark runs
…onbitlang#4099) * refactor(bigint): read each limb with a u32be pattern in from_octets The inner loop was a 4-iteration big-endian shift-accumulate, which is exactly one u32be read (RADIX_BIT_LEN = 32). Measured: native -34% at n=1024 / -24% at n=64, wasm-gc -46% / -42%. js is unaffected (bigint_js.mbt has its own from_octets). * bench(bigint): add from_octets benchmarks Covers the limb-fill path at n=64 and n=1024 so the u32be conversion in the previous commit is reproducible. Signed-off-by: Codex CLI <codex@openai.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(bigint): pin RADIX_BIT_LEN so the u32be limb read can't drift Addresses the review note that `from_octets`'s tail now hard-codes a 32-bit read while `byte_per_limb` is still derived from `RADIX_BIT_LEN`. Rather than branch on the constant at runtime, the assumption is pinned by a whitebox test and named in the comment above the loop: narrowing `RADIX_BIT_LEN` now fails a test instead of silently mis-decoding. The pin uses `@test.assert_eq`, not `inspect`. A snapshot would be rewritten by `moon test --update`, which would retire the guard at exactly the moment it is supposed to fire; `assert_eq` is not auto-updatable. Verified both halves by temporarily setting the expected value to 16: the test fails with `32 != 16`, and `moon test --update` leaves it failing rather than rewriting it. A runtime fallback was considered and rejected. Restoring the old shift-accumulate loop as the "generic" arm would not actually be generic: `byte_per_limb` is `RADIX_BIT_LEN / 8`, so for the documented-legal widths 4, 12, 20 and 28 (the file only requires a multiple of 4, at most 32) that loop truncates and mis-decodes just as badly. It would trade a loud failure for a quiet one, plus a permanently dead branch that no test can reach. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Signed-off-by: Codex CLI <codex@openai.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep wasm-gc on the existing 32-bit limb implementation because wide arithmetic multi-value results regress there.
CAIMEOX
approved these changes
Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Avoids calling
abs()while generatingIntandInt64shrink candidates.abs()wraps for the minimum signed value, which previously left only zero as a candidate and prevented further shrinking when zero passed the property.The loop now terminates when its signed delta reaches zero. This preserves the existing candidate order while producing the full sequence for
Int::MIN_VALUEandInt64::MIN_VALUE.Tests cover both minimum values on wasm, wasm-gc, JavaScript, and native targets.
Validation:
moon test quickcheck/shrink --target all,moon check --target all, andmoon info.