feat!: Implement error handling for modified variables outside modifiers#1712
Conversation
|
| Branch | na/1700-raise-an-error-when-variables-are-used-outside-a-modifier-block |
| Testbed | Linux |
Click to view all benchmark results
| Benchmark | hugr_bytes | Benchmark Result bytes x 1e3 (Result Δ%) | Upper Boundary bytes x 1e3 (Limit %) | hugr_nodes | Benchmark Result nodes (Result Δ%) | Upper Boundary nodes (Limit %) |
|---|---|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | 📈 view plot 🚷 view threshold | 158.77 x 1e3(0.00%)Baseline: 158.77 x 1e3 | 160.36 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 6,641.00(0.00%)Baseline: 6,641.00 | 6,707.41 (99.01%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 27.53 x 1e3(0.00%)Baseline: 27.53 x 1e3 | 27.81 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 1,074.00(0.00%)Baseline: 1,074.00 | 1,084.74 (99.01%) |
| tests/benchmarks/test_queue_push_pop.py::test_queue_push_benchmark_compile | 📈 view plot 🚷 view threshold | 10.91 x 1e3(0.00%)Baseline: 10.91 x 1e3 | 11.02 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 308.00(0.00%)Baseline: 308.00 | 311.08 (99.01%) |
| tests/benchmarks/test_queue_push_pop.py::test_queue_push_pop_benchmark_compile | 📈 view plot 🚷 view threshold | 14.84 x 1e3(-0.01%)Baseline: 14.84 x 1e3 | 14.99 x 1e3 (99.00%) | 📈 view plot 🚷 view threshold | 435.00(0.00%)Baseline: 435.00 | 439.35 (99.01%) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1712 +/- ##
==========================================
- Coverage 93.47% 93.00% -0.47%
==========================================
Files 133 135 +2
Lines 12697 13128 +431
==========================================
+ Hits 11868 12210 +342
- Misses 829 918 +89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses #1700 by detecting when classical/copyable variables captured by a modifier block are assigned within that block, and raising an error if the stale outer binding is used afterward.
Changes:
- Track captured variables that are assigned inside modifier blocks (
CheckedModifiedBlock.modified_captured), including nested modifier bodies. - Extend linearity checking to mark such variables as unusable after the modifier and emit a dedicated diagnostic with a note pointing to the in-modifier assignment.
- Add new error fixtures covering basic, sequential, nested, multiple-variable, and branching scenarios.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/error/modifier_errors/captured_classical_modified.py | New error fixture: modified captured variable used after a modifier block. |
| tests/error/modifier_errors/captured_classical_modified.err | Expected diagnostic output for the above scenario. |
| tests/error/modifier_errors/captured_classical_modified_sequential.py | New error fixture: sequential modifier blocks reusing a modified captured variable. |
| tests/error/modifier_errors/captured_classical_modified_sequential.err | Expected diagnostic output for sequential modifier case. |
| tests/error/modifier_errors/captured_classical_modified_nested.py | New error fixture: modification occurs inside nested modifiers. |
| tests/error/modifier_errors/captured_classical_modified_nested.err | Expected diagnostic output for nested modifier case. |
| tests/error/modifier_errors/captured_classical_modified_multiple.py | New error fixture: multiple captured variables modified; verifies first reported. |
| tests/error/modifier_errors/captured_classical_modified_multiple.err | Expected diagnostic output for multiple modified captures. |
| tests/error/modifier_errors/captured_classical_modified_branch.py | New error fixture: usage in a branch after a modifier. |
| tests/error/modifier_errors/captured_classical_modified_branch.err | Expected diagnostic output for branch usage case. |
| tests/error/errors_on_usage/branch_in_modifier.py | New error fixture ensuring “not defined” behavior remains for vars only defined inside modifier branches. |
| tests/error/errors_on_usage/branch_in_modifier.err | Expected output for “Variable not defined” regression case. |
| guppylang-internals/src/guppylang_internals/nodes.py | Add modified_captured field to CheckedModifiedBlock for later diagnostics/linearity. |
| guppylang-internals/src/guppylang_internals/checker/stmt_checker.py | Minor rename cleanup: return the checked modified block variable consistently. |
| guppylang-internals/src/guppylang_internals/checker/modifier_checker.py | Compute modified_captured by scanning CFG BB assignments, recursing into nested modifier blocks. |
| guppylang-internals/src/guppylang_internals/checker/linearity_checker.py | Introduce UseKind.DEFINED_IN_MODIFIER and raise ModifiedVariableUsedError on later uses; mark modified copyable captures as stale. |
| guppylang-internals/src/guppylang_internals/checker/errors/linearity.py | Add new ModifiedVariableUsedError diagnostic with note/help. |
| guppylang-internals/src/guppylang_internals/checker/cfg_checker.py | Adds commented-out debug print (should be removed). |
| guppylang-internals/src/guppylang_internals/cfg/cfg.py | Adds cfg_as_string() helper (currently unused except for commented debug line). |
| .gitignore | Ignore AGENTS.md. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
I had a initial look through, but I'm not convinced that a new UseKind is the best way to implement this. Currently, you're marking variables as used that are actually not used in the block, which doesn't feel good. In particular, what we actually care about is assignments, so presumably the assignment should track whether it happened in modifier block, not the use?
Furthermore, I think it would be better to check this before linearity checking and maybe even before type checking. This would be more consistent with how we currently catch undefined variable errors (see check_bb in cfg_checker.py). Any way we can include this logic at that point?
@guppy
def foo[T: Copy](x: T) -> tuple[T]:
with power(2):
x = x
return xThe error in this case should be improved, but maybe in a different PR? I added the example in #1597 |
f6fda46 to
8f85e4a
Compare
…ier_block for clarity
closes #1700
This PR add checks to raise an error when we use variables outside a modifier block when their value was assigned inside that modifier block. Moreover it improve the error rendering for undefined variables when the variables are only defined inside the modifier block.
Variable assigned in modifier blockwhen a previously defined variable is assigned in a modifier and then used outside it.BREAKING CHANGE: Using a variable outside a modifier block after it was assigned inside that modifier block now raises a compilation error.