Skip to content

Drop the outer slot mapping when a let shadows it in a register - #64

Merged
wtholliday merged 2 commits into
mainfrom
fix-56-shadowed-let-slot
Aug 28, 2026
Merged

Drop the outer slot mapping when a let shadows it in a register#64
wtholliday merged 2 commits into
mainfrom
fix-56-shadowed-let-slot

Conversation

@wtholliday

Copy link
Copy Markdown
Collaborator

Fixes #56.

A let binding inside a while body that shadowed an outer struct var of the same name read the outer variable on the vm and asm backends.

Root cause

A pointer-represented let whose copy is elided (let q = mk(7) where q: P) binds the name to a register holding the value's address — it gets no local slot of its own. That branch in src/vm_codegen.rs updated variables and variable_types but left local_slots alone, so when the name shadowed an outer var q: P the outer variable's slot mapping stayed live for the duration of the block.

The Expr::Id read path re-emits LocalAddr { dst: reg, slot } for any local_slots hit, to keep the register correct across calls that may have clobbered it. With the stale mapping in scope that re-emit overwrote the binding's address register with the outer variable's address, so every read of the shadowing name loaded the outer value. In the bytecode for the issue's repro:

    12: Call { func: 1, args_start: 2, arg_count: 2 }
    13: LocalAddr { dst: 1, slot: 1 }   // inner q — the call's sret slot
    14: LocalAddr { dst: 1, slot: 0 }   // clobbered with outer q
    15: Load32Off { dst: 4, base: 1, offset: 0 }

jit and stack don't use that name-to-slot re-materialization and were correct; asm matched vm because it runs the same bytecode. The for / var / non-struct / non-shadowing variants listed in the issue all avoided this branch, which is why they agreed across backends.

Fix

Drop the stale local_slots entry wherever a let or var binds a name to a register instead of a slot — the two register branches in Expr::Let and the scalar branch in Expr::Var. Block scope already saves and restores local_slots, so the outer variable's mapping is back at block exit and a post-loop print(q.x) still reads 99.

Testing

New golden test tests/cases/structs/shadowed_let_in_while.lyte covers the one-iteration repro, a three-iteration version whose shadowed reads feed only an assignment, one where the read also feeds a call, the outer variable after the loop, and the same shape under for. Pre-fix it prints the issue's 99 and 297 on vm; post-fix all four backends agree.

cargo test --workspace: 380 lib tests plus the golden suite on jit, vm, asm, and stack, all passing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DeNzkFkzMbdH3XPzitDmVa

wtholliday and others added 2 commits August 27, 2026 21:23
A pointer-represented `let` whose copy is elided binds the name to a
register holding the value's address, with no local slot of its own. That
branch updated `variables` and `variable_types` but left `local_slots`
alone, so when the name shadowed an outer `var` of the same name the outer
variable's slot mapping stayed live for the duration of the block.

The `Expr::Id` read path re-emits `LocalAddr { dst: reg, slot }` for any
`local_slots` hit, to keep the register correct across calls that may have
clobbered it. With the stale mapping in scope that re-emit overwrote the
binding's address register with the *outer* variable's address, so every
read of the shadowing name loaded the outer value. jit and stack don't use
that name-to-slot re-materialization and were correct; asm matched vm
because it runs the same bytecode.

Drop the stale entry wherever a `let` or `var` binds a name to a register
instead of a slot. Block scope already saves and restores `local_slots`,
so the outer variable's mapping is back at block exit.

Fixes #56.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeNzkFkzMbdH3XPzitDmVa
Review of the first commit found that dropping only the slot mapping left
the other name-keyed state behind, which turned a miscompile into a
compiler panic: `Expr::Id`, `get_var_address` and `translate_assign` all
check `captured_vars` before `local_slots` and then unwrap the slot, so a
`let` shadowing a lambda-captured name hit `unwrap()` on `None`.

Give both code generators a `shadow_outer_binding` helper that forgets
every name-keyed fact about the outer binding, and call it from each
binding branch of `let` and `var` — after the initializer is translated,
so an initializer that reads the outer binding still resolves to it. The
vm/asm generator clears `local_slots`, `reg_promoted`, `reference_vars`
and `captured_vars`; the stack generator clears `captured_vars` and
`captured_slots` (its reference bindings live in the variable map itself,
so they already shadowed correctly). Block scope now saves and restores
all of them, so the outer binding's state comes back at block exit.

This also fixes two cases that were already broken on main: a binding
shadowing a lambda-captured name (wrong value on stack, out-of-bounds
access on vm/asm) and one shadowing a reference parameter (same
out-of-bounds). Both get golden tests, and both were verified to fail on
main and pass here on all four backends.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DeNzkFkzMbdH3XPzitDmVa
@wtholliday

Copy link
Copy Markdown
Collaborator Author

Addressed the review in 0c17f9d.

The regression it caught was real. Dropping only local_slots left the other name-keyed state behind, which turned a miscompile into a compiler panic: Expr::Id, get_var_address and translate_assign all check captured_vars before local_slots and then unwrap() the slot, so a let shadowing a lambda-captured name hit unwrap() on None. Confirmed the panic locally, and confirmed it is gone.

The fix is now the general one. Both code generators get a shadow_outer_binding helper that forgets every name-keyed fact about the outer binding, called from each binding branch of let and var — after the initializer is translated, so an initializer that reads the outer binding still resolves to it. vm_codegen clears local_slots, reg_promoted, reference_vars and captured_vars; stack_codegen clears captured_vars and captured_slots (its reference bindings live in the variable map itself, so they already shadowed correctly). Block scope now saves and restores all of them, so the outer binding's state comes back at block exit.

That also covers the review's third finding: a binding shadowing a reference parameter used to read through the reference. Two new golden tests, tests/cases/lambdas/shadow_captured_var.lyte and tests/cases/references/shadow_ref_param.lyte, both verified to fail on main (out-of-bounds access on vm/asm, wrong value on stack for the capture case) and pass here on all four backends. Each exercises a read after the shadowing block too, so the scope restore is covered.

The for-variable finding is real but out of scope, so I filed it as #65. It is not limited to the vm/asm generators — jit fails Cranelift verification and stack prints nothing, so there is no all-backend expected output to write a golden test against until the wider fix lands. Behaviour there is identical before and after this PR.

cargo test --workspace: 380 lib tests plus the golden suite on jit, vm, asm and stack, all passing.

@wtholliday
wtholliday merged commit 74bc1c1 into main Aug 28, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

let binding shadowing an outer struct var inside a while body reads the outer var on vm and asm backends

1 participant