Repository navigation
fix(stim-parser): address proptest findings - #74
Merged
Merged
Conversation
Resolves the two issues surfaced by the fuzz suite in #73 by fixing the underlying parser/AST, not the tests. The workarounds that were present in the proptest files are removed. **Finding 1: stack overflow on deeply nested REPEAT.** The chumsky grammar uses `recursive(...)` for REPEAT bodies and the interpret pass recurses through `Repeat` bodies as well. Both pieces descend recursively at parse time, and the default thread stack (≈2–8 MiB) overflowed somewhere past 24 nesting levels in debug builds. Fixed by running `parse` and `parse_extended` on a dedicated 16 MiB parser thread via `std::thread::scope`. This covers both the chumsky combinator stack and the interpret pass without touching the grammar itself. The proptest cap on `nested_repeats_never_panic` is raised from 16 to 128. **Finding 2: `Raw(MPad)` never round-trips through `parse_extended`.** `parse_extended` always lowers `MPAD …` into the typed `ExtendedInstruction::MPad`, so `ExtendedInstruction::Raw(RawInstruction::MPad{..})` and `Raw(RawInstruction::Repeat{..})` were states that the parser never produced but the type system still allowed — causing runtime `unreachable!()` paths in `prepare` / `executor` and a workaround in the proptest generator. Fixed by introducing `RawPassthrough` (a sub-enum of `RawInstruction` covering only `Gate`/`Noise`/`Measure`/`Annotation`) and changing `ExtendedInstruction::Raw(RawInstruction)` to `Raw(RawPassthrough)`. The bad states are now compile-time unrepresentable: - `prepare` drops the `ExecError::Malformed` variant — it can no longer be reached. - `executor` drops the two `unreachable!()` arms. - `proptest_ast.rs` drops the `ext_raw_passthrough` workaround and generates `Raw(RawPassthrough::*)` directly across all four passthrough variants. The stale regression seed is removed (the case it captured no longer compiles). - `tests/extended.rs` pattern-matches are updated to the new variant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes STIM parser fuzz findings by avoiding parser stack overflows on deeply nested REPEAT blocks and making invalid extended raw states unrepresentable.
Changes:
- Runs
parse/parse_extendedon a dedicated 16 MiB parser stack. - Introduces
RawPassthroughfor extended raw instructions and updates parser, display, executor, preparation, and tests. - Updates/removes proptest workarounds and regression seed tied to the old invalid AST state.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
crates/stim-parser/src/parser.rs |
Adds dedicated parser-stack execution and splits parse_impl. |
crates/stim-parser/src/extended/parser.rs |
Runs parse + interpret on the parser stack. |
crates/stim-parser/src/extended/ast.rs |
Adds RawPassthrough and updates extended measurement counting. |
crates/stim-parser/src/extended/interpret.rs |
Converts raw AST variants into RawPassthrough. |
crates/stim-parser/src/extended/mod.rs |
Re-exports RawPassthrough. |
crates/stim-parser/src/lib.rs |
Adds RawPassthrough to the prelude. |
crates/stim-parser/src/display.rs |
Prints RawPassthrough through raw formatting. |
crates/ppvm-stim/src/prepare.rs |
Updates validation for RawPassthrough and removes malformed raw states. |
crates/ppvm-stim/src/executor.rs |
Updates execution matches for RawPassthrough. |
crates/stim-parser/tests/extended.rs |
Updates extended parser assertions to match RawPassthrough. |
crates/stim-parser/tests/proptest_ast.rs |
Updates extended AST generators and line normalization for RawPassthrough. |
crates/stim-parser/tests/proptest_parse.rs |
Raises nested repeat fuzz depth. |
crates/stim-parser/tests/proptest_ast.proptest-regressions |
Removes stale regression seed for no-longer-representable AST. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match i { | ||
| ExtendedInstruction::Raw(r) => return fmt_raw(r, f, depth), | ||
| ExtendedInstruction::Raw(r) => { | ||
| let owned: RawInstruction = r.clone().into_raw(); |
| /// program. | ||
| #[test] | ||
| fn nested_repeats_never_panic(depth in 0usize..16) { | ||
| fn nested_repeats_never_panic(depth in 0usize..128) { |
| args: vec![p], | ||
| targets, | ||
| line: 0, | ||
| }), |
Collaborator
Author
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/QuEraComputing/ppvm/sessions/3ac90f00-66d8-4d7f-9f7d-f2227bde1f48 Co-authored-by: Roger-luo <8445510+Roger-luo@users.noreply.github.com>
Implemented the requested thread follow-ups in commit
No UI changes were made, so there is no UI screenshot to share. |
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.
Summary
PR #73 added a proptest suite that surfaced two issues, both of which were worked around in the tests rather than fixed in the code. This PR removes the workarounds by fixing the underlying parser/AST.
Finding 1 — stack overflow on deeply nested REPEAT
The chumsky grammar uses
recursive(...)for REPEAT bodies, and the interpret pass recurses throughRepeatbodies as well. Both pieces descend recursively at parse time, and the default thread stack (~2–8 MiB) overflowed somewhere past 24 nesting levels in debug builds.Fix: run `parse` and `parse_extended` on a dedicated 16 MiB parser thread via `std::thread::scope`. Covers both the chumsky combinator stack and the interpret pass without touching the grammar itself.
`proptest_parse.rs`'s `nested_repeats_never_panic` cap is raised from 16 to 128.
Finding 2 — `Raw(MPad)` never round-trips through `parse_extended`
`parse_extended` always lowers `MPAD …` into the typed `ExtendedInstruction::MPad`, so `Raw(RawInstruction::MPad{..})` and `Raw(RawInstruction::Repeat{..})` were states the parser never produced but the type system still allowed — causing runtime `unreachable!()` paths in `prepare` / `executor` and a workaround in the proptest generator.
Fix: introduce `RawPassthrough` (sub-enum of `RawInstruction` covering only `Gate` / `Noise` / `Measure` / `Annotation`) and change `ExtendedInstruction::Raw(RawInstruction)` to `Raw(RawPassthrough)`. The bad states are now compile-time unrepresentable.
Downstream impact:
Test plan
🤖 Generated with Claude Code