Fix classical register visibility inside box scope#306
Conversation
WalkthroughResolve scope lookup so classical registers declared in enclosing scopes are visible inside Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Box blocks were treated the same as function/gate scopes in `get_from_visible_scope()`, restricting access to only constants and qubits from the global scope. This caused measurement statements inside box blocks to fail with "Missing clbit register declaration" even when the classical register was declared at the top level. Per the OpenQASM 3.0 spec, `box` is a timing directive that executes in the enclosing scope, not an isolated scope. Move `in_box_scope()` from the restricted group (function/gate/pulse) to the block group which has full parent scope visibility. Closes #302 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ryanhill1
force-pushed
the
fix/box-scope-classical-registers
branch
from
27ad8f6 to
0ba9da2
Compare
Remove deprecated `suggestion-mode` option from .pylintrc (removed in pylint 3.3.0) and reformat multi-line condition in scope.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pyqasm/scope.py (2)
196-221:⚠️ Potential issue | 🟡 MinorRun Black on this file before merge.
black --checkis still failing onsrc/pyqasm/scope.py, so CI will remain red until this file is reformatted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pyqasm/scope.py` around lines 196 - 221, Run the Python formatter Black on src/pyqasm/scope.py to satisfy CI: reformat the file (e.g., run black src/pyqasm/scope.py or black .) and commit the changes so the function blocks involving in_function_scope, in_gate_scope, in_pulse_scope, in_block_scope, in_box_scope and the loop over self._scope / self._context (Context.BLOCK) are reformatted per Black's style; no logic changes required, just apply Black and update the commit.
196-221:⚠️ Potential issue | 🔴 CriticalDo not stop the scope walk at
Context.BOX.
src/pyqasm/visitor.py, Lines 2912-2945 always pushes a freshContext.BOX, so this only fixesGLOBAL -> BOX. In a reachable stack likeFUNCTION -> BOXorBLOCK -> BOX, Line 212 breaks on the BOX frame immediately, so parent locals stay invisible and Line 220 falls back straight to global scope, bypassing the real enclosing-scope rules. It also leavesget_from_visible_scope()inconsistent withcheck_in_scope()on Lines 152-162 for the same symbol. Please make BOX transparent in the ancestor walk and share that rule across both visibility helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pyqasm/scope.py` around lines 196 - 221, The ancestor-walk must treat Context.BOX as transparent: in the scope-walking logic inside get_from_visible_scope (the function containing the shown loop over reversed(self._scope)/reversed(self._context)), do not break when encountering a BOX frame—skip BOX frames and continue searching parent scopes for var_name instead of immediately falling back to global; make this behavior identical to check_in_scope (lines ~152-162) by sharing the BOX-transparency rule (extract a small helper or reuse the check_in_scope logic) so both functions treat Context.BOX the same and parent locals (e.g., FUNCTION->BOX or BLOCK->BOX) remain visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/qasm3/test_box.py`:
- Around line 237-258: Add a test that exercises BOX visibility when the
enclosing scope is non-global: create a new test (e.g.,
test_box_measurement_with_enclosing_block_scope) which defines a non-global
parent scope (like a named gate/block or a nested block/function) that declares
a classical register (bit[...] c;) and inside that parent scope place a box that
performs c[...] = measure q[...]; then call loads(qasm_str) and
result.validate() to assert no 'Missing clbit register' error. This complements
the existing test and ensures the visitor behavior (see src/pyqasm/visitor.py
handling that always pushes Context.BOX) allows BOX to see classical registers
from an enclosing BLOCK/FUNCTION scope as well as GLOBAL.
---
Outside diff comments:
In `@src/pyqasm/scope.py`:
- Around line 196-221: Run the Python formatter Black on src/pyqasm/scope.py to
satisfy CI: reformat the file (e.g., run black src/pyqasm/scope.py or black .)
and commit the changes so the function blocks involving in_function_scope,
in_gate_scope, in_pulse_scope, in_block_scope, in_box_scope and the loop over
self._scope / self._context (Context.BLOCK) are reformatted per Black's style;
no logic changes required, just apply Black and update the commit.
- Around line 196-221: The ancestor-walk must treat Context.BOX as transparent:
in the scope-walking logic inside get_from_visible_scope (the function
containing the shown loop over reversed(self._scope)/reversed(self._context)),
do not break when encountering a BOX frame—skip BOX frames and continue
searching parent scopes for var_name instead of immediately falling back to
global; make this behavior identical to check_in_scope (lines ~152-162) by
sharing the BOX-transparency rule (extract a small helper or reuse the
check_in_scope logic) so both functions treat Context.BOX the same and parent
locals (e.g., FUNCTION->BOX or BLOCK->BOX) remain visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d49b00dc-ed0d-48f6-a74b-ed8bd2311af5
📒 Files selected for processing (3)
CHANGELOG.mdsrc/pyqasm/scope.pytests/qasm3/test_box.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Test that a box nested inside a non-global scope (e.g. an if-block) can still access classical registers from the enclosing global scope. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pyqasm/scope.py (2)
152-168:⚠️ Potential issue | 🟡 MinorBOX scope handling is inconsistent between
check_in_scopeandget_from_visible_scope.
check_in_scope(lines 152-161) treats BOX like function/gate/pulse scope, restricting global scope access to constants and qubits. However,get_from_visible_scope(line 205) treats BOX like BLOCK scope and allows unrestricted access to all global variables.This inconsistency can cause subtle bugs:
check_in_scopereturnsFalsefor a classical register in global scope inside BOX, butget_from_visible_scopesuccessfully returns the same variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pyqasm/scope.py` around lines 152 - 168, check_in_scope treats Context.BOX like function/gate/pulse (restricting global access) but get_from_visible_scope does not; update get_from_visible_scope to handle Context.BOX the same way as in_function_scope/in_gate_scope/in_pulse_scope: when iterating reversed(self._scope)/reversed(self._context) in get_from_visible_scope, detect Context.BOX and if a var_name is found in global_scope only return it when global_scope[var_name].is_constant or global_scope[var_name].is_qubit (same logic used in check_in_scope), otherwise continue/search or raise as appropriate so BOX visibility behavior is consistent with check_in_scope.
205-217:⚠️ Potential issue | 🟠 MajorLoop breaks prematurely when BOX is nested inside a BLOCK scope, preventing access to BLOCK-scoped variables.
When the current context is
BOX, the first loop iteration encounterscontext=BOXwhich is!= Context.BLOCK, causing an immediate break. This prevents the scope walker from finding variables declared in intermediateBLOCKscopes (e.g., classical variables declared inside if-blocks).For example, with
GLOBAL → BLOCK → BOX:
- Reversed contexts:
[BOX, BLOCK, GLOBAL]- 1st iteration:
context=BOX, condition!= BLOCKis True → breaks immediatelyblock_scopeis never searchedThe existing test
test_box_measurement_with_enclosing_block_scope()passes because classical registers are always inGLOBALscope (handled by fallback). The bug manifests only when accessing variables declared in intermediateBLOCKscopes.Additionally, the same loop logic issue exists in
check_in_scope()(line 164) andset_to_visible_scope()(line 284) methods.🔧 Proposed fix
if self.in_block_scope(): for scope, context in zip(reversed(self._scope), reversed(self._context)): - if context != Context.BLOCK: + if context not in (Context.BLOCK, Context.BOX): var_found = scope.get(var_name, None) breakApply the same fix to
check_in_scope()(line 164) andset_to_visible_scope()(line 284).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pyqasm/scope.py` around lines 205 - 217, The loop that walks reversed self._scope/_context has its conditions reversed: it breaks on the first non-BLOCK context (e.g., BOX) before checking intermediate BLOCK scopes, so swap the branches so BLOCK scopes are inspected first (if context == Context.BLOCK: if var_name in scope: return scope[var_name]; else continue), and only when encountering a non-BLOCK context fall back to grabbing scope.get(var_name) and breaking; apply the same change to check_in_scope() and set_to_visible_scope() so all three methods handle nested BOX inside BLOCK correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pyqasm/scope.py`:
- Around line 152-168: check_in_scope treats Context.BOX like
function/gate/pulse (restricting global access) but get_from_visible_scope does
not; update get_from_visible_scope to handle Context.BOX the same way as
in_function_scope/in_gate_scope/in_pulse_scope: when iterating
reversed(self._scope)/reversed(self._context) in get_from_visible_scope, detect
Context.BOX and if a var_name is found in global_scope only return it when
global_scope[var_name].is_constant or global_scope[var_name].is_qubit (same
logic used in check_in_scope), otherwise continue/search or raise as appropriate
so BOX visibility behavior is consistent with check_in_scope.
- Around line 205-217: The loop that walks reversed self._scope/_context has its
conditions reversed: it breaks on the first non-BLOCK context (e.g., BOX) before
checking intermediate BLOCK scopes, so swap the branches so BLOCK scopes are
inspected first (if context == Context.BLOCK: if var_name in scope: return
scope[var_name]; else continue), and only when encountering a non-BLOCK context
fall back to grabbing scope.get(var_name) and breaking; apply the same change to
check_in_scope() and set_to_visible_scope() so all three methods handle nested
BOX inside BLOCK correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b347744-c2c2-4646-b579-f364a72f47f4
📒 Files selected for processing (3)
.pylintrcsrc/pyqasm/scope.pytests/qasm3/test_box.py
💤 Files with no reviewable changes (1)
- .pylintrc
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/qasm3/test_box.py`:
- Around line 261-284: Add an additional optional test function named
test_box_measurement_with_block_scoped_register that mirrors the pattern in
test_box_measurement_with_enclosing_block_scope but declares the classical
register inside the if-block (i.e., move the "bit[3] c;" declaration into the if
(true) block) and then performs c[1] = measure q[1]; inside the nested box;
ensure the new test calls loads(qasm_str) and result.validate() just like the
existing test to explicitly cover the immediate parent block scope case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1915a621-9b0d-4501-bbac-9b58a968598b
📒 Files selected for processing (1)
tests/qasm3/test_box.py
The scope walker now traverses through both BLOCK and BOX contexts when resolving variables, allowing a box nested inside a block (e.g. an if-statement) to see classical registers declared in that block. Non-constant, non-qubit globals remain restricted inside box scope.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pyqasm/scope.py`:
- Around line 157-165: The traversal/visibility logic for BOX is inconsistent
between check_in_scope() and get_from_visible_scope(): extract a shared helper
that walks reversed(self._scope) and reversed(self._context), sets an in_box
flag when encountering Context.BOX, and applies the same rule for
non-(Context.BLOCK, Context.BOX) frames: if in_box and var_name in scope then
allow only if scope[var_name].is_constant or scope[var_name].is_qubit, otherwise
return var_name in scope; then have both check_in_scope() and
get_from_visible_scope() call this helper (referencing Context.BOX,
Context.BLOCK, self._scope, self._context, and the var_name lookup) so BOX
visibility is unified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2b1ec21-9861-442b-a17f-69ed2c4fdb07
📒 Files selected for processing (2)
src/pyqasm/scope.pytests/qasm3/test_box.py
3 participants
Summary
boxblocks being treated as isolated scopes (likegate/function), preventing access to classical registers from the enclosing scopec[1] = measure q[1];inside aboxblock now correctly resolves the classical registercdeclared at global scopeDetails
In
scope.py,get_from_visible_scope()groupedin_box_scope()within_function_scope()/in_gate_scope()/in_pulse_scope(), which only allows access to constants and qubits from global scope. Classical registers were invisible, causing "Missing clbit register declaration" errors.Per the OpenQASM 3.0 spec,
boxis a timing directive that executes in the enclosing scope — not an isolated scope like a gate or function definition. This PR movesin_box_scope()to the block scope group, which has full parent scope visibility.Closes #302
Summary by CodeRabbit