Scope the for variable to its loop on every backend - #66
Conversation
A `for` variable that shadowed an outer binding of the same name went wrong on all five code generators, each in its own way: `jit` and `llvm` failed to compile the function, `vm` and `asm` read the outer variable's memory as the loop counter, and `stack` segfaulted. Two bugs, one per half of the binding. The name was bound and never unbound. Every generator did `variables.insert(var, counter)` before the loop and left it there, so after the loop the name still resolved to the counter — reading a struct field off an i32 on jit and llvm, dereferencing an integer on stack, and silently reading the wrong storage on vm and asm. Nested loops binding the same name lost the outer counter for the rest of the outer body, and that one was wrong the same way on all five. And the binding was a mix of two bindings. Like the `let`/`var` case in #56, `translate_for` never cleared the outer binding's name-keyed state, so reads *inside* the loop still went through it: on vm and asm the outer struct's `local_slots` entry was live, and the Expr::Id read path prefers a slot to the register, so `s = s + i` summed the struct's memory. Both generators that keep such state now snapshot it before binding the loop variable and restore it after the body, and rebind the name with the same `shadow_outer_binding` helper `let` and `var` use. Since jit, llvm and stack keep only their variable maps, they just save and restore those. The bounds are still translated before the binding, so `for i in 0 .. i.x` reads the outer `i`. The vm generator also marks the counter `reg_promoted`, which is what it is — a scalar held in a register. Without it, taking the counter's address found neither a slot nor a promoted register and panicked with "variable has no storage" whenever a lambda captured a loop variable; that now spills to a slot the way any other register-held scalar does. Bytecode for the biquad benchmark is unchanged. Blocks and `for` loops save the same state, so both call one `save_bindings`/`restore_bindings` pair per generator rather than repeating the field list — the divergence between those lists is what made this a five-backend bug. Fixes #65. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Khv7uRSSSAEEirZKnVvhMV
…ety checker
Review of the first commit found two more places the `for` variable's
binding was incomplete.
A counter a lambda captures is shared by address, so it needs storage of
its own. The vm generator marked it `reg_promoted` and left the rest to
the lazy spill in `get_var_address`, which allocates the slot and stores
to it *at the capture site* — and that site can sit on a
conditionally-executed path. With the capture in an `else if` condition,
iterations that take the `if` branch never run the store, so later reads
of the counter loaded an unwritten slot: vm, asm and stack printed 2052
where jit and llvm printed 2062. Worse, on main those two backends
rejected the program outright with "variable has no storage", so the
first commit turned a loud compiler error into a silent miscompile.
Allocate the counter's storage when the loop binds it, the way `let` and
`var` already do for a `lambda_referenced` scalar, and refresh it from
the counter register at the top of every iteration. The loop variable is
immutable, so nothing writes back the other way. Bytecode for the biquad
benchmark is still unchanged: a loop whose counter no lambda mentions
takes the register path exactly as before.
The safety checker had the same unscoped-variable bug, where it costs
memory safety rather than a wrong number. Its snapshot was taken *after*
the loop variable's interval, LenBound and VarBound were pushed, so the
restore after the body kept them: `0 <= i < 3` outlived the loop, and
when the loop variable shadowed an outer one of the same name it proved
a bound for the outer variable. `var i = 100; for i in 0 .. 3 {}; a[i] = 7`
was accepted against a `[i32; 4]` and ran, writing 400 bytes past the
array on every backend. Take the snapshot before the binding instead;
both later uses of it want the pre-binding state. The `vars` push is now
popped too, matching how blocks handle it.
Both get golden tests, verified to fail on main and pass here on all five
backends.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Khv7uRSSSAEEirZKnVvhMV
|
Addressed the review findings in 7c38988. All three were real; I reproduced each before fixing. 1 & 2 — a lambda-captured counter had no storage of its own (vm, asm, stack). Marking the counter jit/llvm 3 — the same bug in This compiled and ran on all five backends. Renaming the loop variable, or dropping the loop, correctly gives Tests: 🤖 Generated with Claude Code |
7c38988 landed the two golden tests without the source changes they cover: a `git checkout HEAD -- src/` while HEAD was still the first commit reverted the working tree before it was committed, so CI ran the new tests against unfixed generators and a unfixed safety checker. The three fixes described in that commit message are here. vm and stack code generators: allocate storage for a `for` counter a lambda captures when the loop binds it, and refresh it from the counter register at the top of every iteration, rather than leaving it to the lazy spill in `get_var_address`/`emit_var_address`, whose store lands at the capture site and can sit on a conditionally-executed path. Safety checker: take the snapshot restored after the loop body *before* binding the loop variable, so its interval and bounds don't outlive the loop and prove a bound for an outer variable of the same name. Verified with `cargo test --workspace` and `--features llvm`: 380/382 unit tests and 364 golden tests on all five backends. Biquad benchmark bytecode is still byte-identical to main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Khv7uRSSSAEEirZKnVvhMV
A
forvariable that shadowed an outer binding of the same name went wrong on all five code generators, each in its own way:jitandllvmfailed to compile the function,vmandasmread the outer variable's memory as the loop counter, andstacksegfaulted.Two bugs, one per half of the binding.
The name was bound and never unbound. Every generator did
variables.insert(var, counter)before the loop and left it there, so after the loop the name still resolved to the counter — reading a struct field off an i32 on jit and llvm, dereferencing an integer on stack, and silently reading the wrong storage on vm and asm. Nested loops binding the same name lost the outer counter for the rest of the outer body, and that one was wrong the same way on all five.And the binding was a mix of two bindings. Like the
let/varcase in #56,translate_fornever cleared the outer binding's name-keyed state, so reads inside the loop still went through it: on vm and asm the outer struct'slocal_slotsentry was live, and theExpr::Idread path prefers a slot to the register, sos = s + isummed the struct's memory.Both generators that keep such state now snapshot it before binding the loop variable and restore it after the body, and rebind the name with the same
shadow_outer_bindinghelperletandvaruse. Since jit, llvm and stack keep only their variable maps, they just save and restore those. The bounds are still translated before the binding, sofor i in 0 .. i.xreads the outeri.The vm generator also marks the counter
reg_promoted, which is what it is — a scalar held in a register. Without it, taking the counter's address found neither a slot nor a promoted register and panicked withget_var_address: variable "i" has no storagewhenever a lambda captured a loop variable; that now spills to a slot the way any other register-held scalar does. Bytecode for the biquad benchmark is unchanged.Blocks and
forloops save the same state, so both call onesave_bindings/restore_bindingspair per generator rather than repeating the field list — the divergence between those lists is what made this a five-backend bug.Testing
tests/cases/loops/shadowed_for_var.lytecovers the reported repro, a bound expression reading the outer binding, shadowing a scalarvarthat stays assignable afterwards, nested same-name loops, shadowing a reference parameter, shadowing a name captured from an enclosing scope, and a lambda capturing the counter. It fails onmainon all four backends the issue lists (jit: verifier errors; vm:606then an out-of-bounds trap; asm:126; stack: no output) and passes on all five here.cargo test --workspaceandcargo test --workspace --features llvmare green: 382 unit tests and the golden suite on jit, llvm, vm, asm and stack.Fixes #65.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Khv7uRSSSAEEirZKnVvhMV