Rename duplicate argument variables internally - #355
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 |
|
👋 Hey there! It looks like the changelog might need an update. Please take a moment to edit the
|
|
@ryanhill1 I definitely don't think this is the most elegant way to handle this but the variable's name must be changed before the call to |
There was a problem hiding this comment.
Thanks for taking this on, @micpap25 — and for the root-cause work on #311. Identifying the parameter shadowing was the hard part, and the reported case does now work.
Most of what follows is in the _visit_function_call machinery you mentioned being unfamiliar with. Specifics are inline; two general things:
The suite is red. 10 tests fail here that pass on main — 5 in tests/cli, 5 in tests/qasm3/subroutines. pytest tests reproduces most of the inline findings.
No tests or CHANGELOG entry. A few regression tests would have caught most of this. Worth covering: the #311 repro, an array-typed shadowed parameter (qubit[2] q), reset/barrier bodies, a nested call with differing parameter names, and the q/q_arg collision. tests/qasm3/subroutines/test_subroutines.py is the natural home.
Your diagnosis was right — the gap is between fixing this program and fixing it without disturbing the others, and the four call sites in the last inline comment aren't discoverable without going looking. Happy to pair or split it, and no rush.
| # If our actual variable is the same as the function argument, | ||
| # give the function argument a temporary name for internal use | ||
| if actual_arg_name == formal_reg_name: | ||
| formal_reg_name += "_arg" |
There was a problem hiding this comment.
This name reaches the user. It flows into error messages, so a size mismatch reports Expected 3 qubits in variable 'q_arg' when the user wrote q. Breaks the 5 test_subroutines.py tests.
Suggestion: use a separate internal_reg_name for the map keys and leave formal_reg_name alone for messages.
| formal_reg_name = formal_arg.name.name | ||
| # If our actual variable is the same as the function argument, | ||
| # give the function argument a temporary name for internal use | ||
| if actual_arg_name == formal_reg_name: |
There was a problem hiding this comment.
_arg can collide with a name already in use:
qubit[2] q;
qubit[2] r;
def f(qubit q, qubit q_arg) { h q; x q_arg; }
f(q[0], r[1]);main emits h q[0]; x r[1];. Here it's ValueError: Variable 'q_arg' already exists in current scope — and a raw ValueError, not a ValidationError.
Suggestion: use a name that can't appear in user code. INTERNAL_QUBIT_REGISTER in elements.py ("__PYQASM_QUBITS__") is the existing pattern for this.
| ) | ||
| except KeyError: | ||
| for qubit in operation.qubits: | ||
| assert isinstance(qubit.name, str) |
There was a problem hiding this comment.
This fires on real programs — it breaks tests/cli/resources/valid1.qasm and with it all 5 CLI tests:
def create_bell_state(qubit[2] q) { hgate q[0]; cxgate q[0], q[1]; }
create_bell_state(q[0:2]);A qubit operand has two shapes: for Identifier, .name is a str; for IndexedIdentifier (q[0]), .name is an Identifier and .name.name is the string. The assert only holds for the first.
Also worth knowing: assertions compile out under python -O, where this becomes TypeError: unsupported operand type(s) for +=: 'Identifier' and 'str'. Control flow is better as a real branch handling both shapes.
| operation, transform_map, size_map | ||
| ) | ||
| ) | ||
| except KeyError: |
There was a problem hiding this comment.
This is compensated here, but needed in three places. The rename happens in subroutines.py, so qubit_transform_map is keyed on q_arg while the function body still says q. This try/except patches the mismatch at one use site — but there are three:
visitor.py:754 _visit_reset <- not patched
visitor.py:871 _visit_barrier <- not patched
visitor.py:1504 _visit_generic_gate_operation
So these still fail, now with a raw KeyError instead of the located ValidationError they gave before:
def f(qubit q) { reset q; } // KeyError: ('q', 0) (barrier too)
def inner(qubit p) { h p; }
def outer(qubit q) { inner(q); }
outer(q[1]); // KeyError: 'q' — works on mainSuggestion: keep the keys and the body consistent from the start — register the transform map under the name the body uses, or rename the body's references when you rename the parameter. Then no call site needs a retry and all three fix together.
Worth noting too: except KeyError catches it from anywhere inside transform_function_qubits, including genuine bugs.
|
@ryanhill1 I'm going to start by building tests so that we do not revert back to failing the original #311 case. |
|
Code has been updated so it's not failing any of the (existing) tests except for one which cares about the specific error ( |
2 participants
Summary of changes
Fixes #311
Renames the variables created internally to handle quantum arguments in functions, preventing variable lookup issues caused by redefinition / shadowing.