fix(codegen): closure own-local shadowing a promoted capture emits valid C - #2114
Open
paul-hammant wants to merge 3 commits into
Open
paul-hammant wants to merge 3 commits into
paul-hammant wants to merge 3 commits into
Conversation
… (live on 0.696) A closure's own locals are unified by name with same-named locals in an earlier, already-closed block of the enclosing function, promoted to heap cells, and then captured at a construction site outside the block where those cells were declared and released — so gcc rejects the generated C with "'idx' undeclared". Measured bisect, holding the downstream code byte-identical and varying only the compiler: clean on 0.668; broken on 0.675, 0.677, 0.681 and 0.696. The 0.696 row was taken by reverting only aeb's workaround rename, so it is the compiler being measured rather than the symptom. Currently invisible: aeb 6af17aa renames the colliding locals, which hides it. That is why this is worth filing anyway — the next person to reuse a name in a long function gets an undeclared-C wall in generated code they never see, naming a variable that does not exist at the reported line, with no clue it is a known compiler bug. The blast radius is the whole graph, because aeb's fan-out orchestrator is one binary for every node. Includes the generated-C evidence (cells declared inside the earlier block, released when it closes, then retained at the later construction site), and the five reductions that did NOT reproduce — nested if, tuple destructuring, interpolation over captures, closure in a while loop, and the same-name-in- earlier-block shape in a small function — so nobody re-derives that the shape alone is insufficient. The hypothesis that function size tips an inlining or scope-flattening decision is labelled as a hypothesis. Found from servirtium-vcr on CachyOS; the aeb-side workaround and the presubmit blast-radius detail came from the selenium side. Companion ask that documents the aeb-level workaround rather than the compiler defect: aeb/asks/closure-var-collides-with-function-body-name.md — both are worth keeping, they answer different questions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013vNUNU6AmTr5SW3hByb2QY
…valid C A closure's own body local that shared a name with a PROMOTED capture of the enclosing function was misclassified as that promoted capture, emitting undeclared C. When an enclosing function had a local captured-and-mutated by one closure (so codegen promotes it to a heap cell), a LATER closure whose own body declared a same-named local had that local treated as the promoted capture: the body emitted a cell-dereferencing write (*idx = ...) and mark_var_declared'd the name, but the closure never captured it and no prologue alias (T* idx = _env->idx;) existed -- so gcc rejected the generated C with "'idx' undeclared", in code the author never sees, with a whole-graph blast radius for a fan-out build. Landed in 0.675, live through 0.696; worked around downstream in aeb by renaming the colliding locals. Root cause in emit_closure_definitions (compiler/codegen/codegen_expr.c): the closure's body_promoted set was built from ALL parent_promoted names (filtered only by is_closure_param), and every parent_promoted name was mark_var_declared'd -- but the declaration for a promoted capture is the prologue alias, which is emitted ONLY for names in captures[]. A parent-promoted name the closure does not capture therefore had no alias. Fix: gate both the body_promoted inheritance and the mark_var_declared loop on captures[] membership -- a parent-promoted name is inherited only if genuinely captured; an uncaptured same-named name is a shadowing own local and declares normally. Verified: the minimal reproducer (earlier block's idx/entry captured+mutated -> promoted; later closure reuses the names as own locals) failed with 'idx'/'entry' undeclared before, builds+runs total=13 after. 94/94 closure/capture regression+syntax tests pass, closure-heavy tinyweb examples build clean, full make test green (410 passed). New regression test fails on the pre-fix compiler and passes after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… cell leak
The macOS ARM64 leaks gate failed the new regression test (4 leaks, allowed 0):
it allocated a std.collections list (never freed) and a string_concat in the
promoted path. Rewrote it to the minimal bug shape — an int gate instead of a
list, no string_concat — which drops it to a single 20-byte leak: the promoted
heap cell for the earlier block's captured-and-mutated local. That cell is
INHERENT to capture promotion (a bump-only program with no name collision leaks
the identical cell, independent of the shadowing bug this test guards) and is
reclaimed a generation late by promotion's RCU discipline. Recorded it in
tests/leaks_known.txt with an explaining comment, at 1, mirroring how the
sibling test_closure_local_alloc_capture (20) is handled. Value assertion
updated (got=19: bump 1+len("bumped")=7, each 7+len("local")=12). Still a genuine
guard — fails on the pre-fix compiler with 'idx'/'entry' undeclared.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a compiler codegen bug (documented in the ask on this branch): a closure's own body local that shares a name with a promoted capture of the enclosing function was emitted as undeclared C.
The bug
When an enclosing function has a local that one closure captures-and-mutates, codegen promotes it to a heap cell. If a later closure's own body then declares a local of the same name, that local was misclassified as the promoted capture: the body emitted a cell-dereferencing write (
*idx = ...) and marked the name pre-declared — but the closure never captured it and no prologue alias (T* idx = _env->idx;) existed, so gcc rejected the generated C with'idx' undeclared. In code the author never sees, with a whole-graph blast radius for a fan-out build.Landed in 0.675, live through 0.696; worked around downstream in aeb by renaming the colliding locals (so the symptom was hidden, the bug unfixed).
Root cause & fix
In
emit_closure_definitions(compiler/codegen/codegen_expr.c): the closure'sbody_promotedset was built from allparent_promotednames (filtered only byis_closure_param), and every one wasmark_var_declared'd — but the declaration for a promoted capture is the prologue alias, emitted only for names incaptures[]. A parent-promoted name the closure doesn't capture therefore had no alias.Fix: gate both the
body_promotedinheritance and themark_var_declaredloop oncaptures[]membership. A parent-promoted name is inherited into the closure's promoted set only if the closure genuinely captures it; an uncaptured same-named name is a shadowing own local and declares normally. Two small hunks, kept in sync with the alias-emission set.Verification
idx/entrycaptured+mutated → promoted; later closure reuses the names as own locals): failed with'idx'/'entry' undeclaredbefore, builds + runstotal=13after.make test: 410 passed, 0 failed. 94/94 closure/capture regression+syntax tests pass; closure-heavycontrib/tinywebexamples build clean.tests/regression/test_closure_local_shadows_promoted_capture.ae— fails on the pre-fix compiler (git stashof the codegen change → the exact undeclared errors), passes after. Genuine guard.CHANGELOG
[current]### Fixedentry added; released sections untouched.🤖 Generated with Claude Code
https://claude.ai/code/session_01KkGcGUJt57vpCGjvCZEVra