fix: detect gate definition cycles of any length, not just self-calls - #376
Conversation
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
81e4018 to
41069c6
Compare
A cycle of two or more gate definitions recursed until the interpreter's stack limit and surfaced as a bare RecursionError naming nothing. The guard in _visit_custom_gate_operation compared the body's gate name against the single name being expanded, so it matched only a gate calling itself. Track the chain of gates currently being expanded and test membership of that chain instead. A cycle of any length now raises a ValidationError at the call that closes it, naming the path, e.g. (a -> b -> a). Membership of the chain, rather than of every gate seen, keeps a diamond -- one gate reached twice down separate paths -- expanding normally. Fixes #369 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6dfffb0 to
4da00a9
Compare
|
Looks good, the error was simple |
82fc04b
into
fix-nested-external-gate-depth
…ngth (#375) * fix: record a nested external gate's own depth, not its skipped body's An external custom gate whose body calls another custom gate reported the depth of the decomposition it never emitted: depth() == 13 for a single emitted statement with external_gates=["outer"], and 2 with both gates named external. _visit_custom_gate_operation assigned _recording_ext_gate_depth without saving it and cleared it to False on exit. Descending into a non-external inner gate therefore re-enabled recording for the body the outer gate was skipping, and the clear on the inner gate's exit left the outer gate unable to record its own single depth. Save the flag, restore it in a finally block, keep an inner gate suppressed inside an enclosing external gate, and record the depth once from the outermost external gate only. Fixes #367 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: detect gate definition cycles of any length, not just self-calls (#376) A cycle of two or more gate definitions recursed until the interpreter's stack limit and surfaced as a bare RecursionError naming nothing. The guard in _visit_custom_gate_operation compared the body's gate name against the single name being expanded, so it matched only a gate calling itself. Track the chain of gates currently being expanded and test membership of that chain instead. A cycle of any length now raises a ValidationError at the call that closes it, naming the path, e.g. (a -> b -> a). Membership of the chain, rather than of every gate seen, keeps a diamond -- one gate reached twice down separate paths -- expanding normally. Fixes #369 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor: inline the cyclic-gate error at its one call site The helper had a single caller, so the indirection cost a method and a docstring without earning anything. The message and span are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #369.
The bug
pyqasm reported a gate that calls itself cleanly, but a cycle between two definitions exhausted the Python stack:
The
RecursionErrornamed nothing, so there was no indication of which gates were cycling.Cause
The guard in
_visit_custom_gate_operationcompared the body's gate name against one name — the gate currently being expanded:A cycle of length one matches. A cycle of length two or more passes the check and recurses through gate expansion until the interpreter's stack limit.
The fix
Track the chain of gates currently being expanded (
_gate_expansion_chain, pushed on entry and popped in afinally) and test membership of that chain instead of equality with a single name. One mechanism now covers both cases, so the direct-recursion check is replaced rather than duplicated.The error names the path it closes, as the issue requested:
The check stays at the same point in the body scan as the old one, so the reported span is still the call inside the definition that closes the cycle.
Chain, not a seen-set. Membership of the chain is what distinguishes a cycle from a diamond. With a set of every gate seen,
acalling bothbandc, where both calld, would be misreported as recursion. Popping on return keeps that legal.Behaviour that does not change
gate a q { a q; }ValidationError(a -> a)bnever definedUnsupported / undeclared QASM operation: ba → {b, c} → dVerification
tox -e format-check: pylint 10.00/10, isort, black, mypy and headers all clean.RecursionError; the two diamond/repeat guards pass onmain, as they should.Tests added
CUSTOM_GATE_INCORRECT_TESTSgainsindirect_recursive_definition(a → b → a) andthree_gate_recursive_definition(a → b → c → a). Both pin the full message including the path, and both inherit the existing line/column assertions.test_shared_gate_definition_is_not_recursion— the diamond must expand, not raise.test_repeated_gate_call_in_one_body_is_not_recursion— the chain entry must be popped when an expansion returns.test_indirect_recursion_does_not_exhaust_the_stack— pins that the failure is aValidationErrorand never aRecursionError, which was the reported symptom.Note on the base branch
This is stacked on #375, because both PRs edit
_visit_custom_gate_operation— reviewing them independently would hand you a conflict. Merge #375 first; this diff then reduces to its own four files. The commits are separate and each stands on its own.Follow-up not taken
The issue notes this is likely to be hit by anyone fixing #54 / #370 (
opaquedeclarations), since giving Quantinuum's opaque primitives real bodies creates a mutual cycle withhqslib1'sUandCX. That case now reports the cycle by name instead of blowing the stack, which is the whole of what this issue asked for.