diff --git a/CHANGELOG.md b/CHANGELOG.md index 3aa68efb..cd9cff36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,22 @@ version number before tagging the release. ## [current] +### Fixed + +- **A closure's own body local sharing a name with a promoted capture of the + enclosing function emitted undeclared C.** When an enclosing function had a + local that one closure captured-and-mutated — so codegen promoted it to a heap + cell — a *later* closure whose own body declared a local of the **same name** + had that local 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 existed, so gcc rejected + the generated C with `'' undeclared` (in code the author never sees, + with a whole-graph blast radius for a fan-out build). A parent-promoted name is + now inherited into a closure's promoted-capture set only if the closure + actually captures it; an uncaptured same-named name is a shadowing own local + and declares normally. Landed in 0.675; verified fixed against a minimal + reproducer. `tests/regression/test_closure_local_shadows_promoted_capture.ae`. + ## [0.696.0] ### Added diff --git a/asks/closure-locals-unified-with-outer-block-names-emit-undeclared-c.md b/asks/closure-locals-unified-with-outer-block-names-emit-undeclared-c.md new file mode 100644 index 00000000..160da8bd --- /dev/null +++ b/asks/closure-locals-unified-with-outer-block-names-emit-undeclared-c.md @@ -0,0 +1,117 @@ +# A closure's own locals are unified with same-named locals in an earlier closed block, and emitted as captures outside their declaring block + +> **STATUS: live on ae 0.696.0.** Worked around downstream in aeb +> (`6af17aa`, renames the colliding locals), so the symptom is currently hidden +> — but the compiler bug is unfixed and will bite the next person who reuses a +> name. Verified still-broken on 0.696 by reverting only the rename. +> +> Companion ask on the aeb side: +> `aeb/asks/closure-var-collides-with-function-body-name.md`. That one describes +> the trigger as "the transform/inline path"; the mechanism below supersedes it. + +## Symptom + +gcc rejects the generated C for `aeb`'s dotnet SDK: + +``` +lib/dotnet/module.ae: In function 'dotnet_build_project': +lib/dotnet/module.ae:764:126: error: 'idx' undeclared (first use in this function) +lib/dotnet/module.ae:764:290: error: 'entry' undeclared (first use in this function) +aeb-link: FATAL — failed to link the fan-out orchestrator +``` + +**Blast radius is the whole graph, not one node.** aeb's fan-out orchestrator is +a single binary for every node, so nothing in any graph containing the offending +module can run — a full presubmit dies before executing a single test. + +Note the reported location is misleading: line 764 is +`if string.length(line_r) > 0 {`, which has no column 290. The `#line` mapping +back to `.ae` is off, so chasing that line wastes time. Chase the generated C. + +## Bisect + +One fixed aeb (dotnet SDK byte-identical across v0.311–v0.319), varying only the +compiler: + +| ae | result | +|----|--------| +| 0.668.0 | clean | +| 0.675.0 | **`idx`/`entry` undeclared** | +| 0.677.0 | **undeclared** | +| 0.681.0 | **undeclared** | +| 0.696.0 | **undeclared** (re-confirmed 2026-09-19 by reverting only the rename) | + +So it landed in **0.675** and is still present in **0.696**. + +## Mechanism, from the generated C + +In `dotnet_build_project`, the names `idx`/`entry` are assigned in **two** places: +once in an earlier nested block (a pkgrefs loop, `.ae` ~724) and once as the +*own locals* of a later closure passed to `string.seq_each` (`.ae` ~766). + +The emitted C for that one function: + +```c +/* ~12739 — inside the EARLIER block */ +int* idx = (int*)_aether_cell_new(sizeof(int)); +const char** entry = (const char**)_aether_cell_new(sizeof(const char*)); +... +/* ~12775 — that block ENDS; the cells are released */ +_aether_cell_release_str(entry); +_aether_cell_release(idx); +... +/* ~12827 — closure-construction site, OUTSIDE that block */ +_e->idx = (int*)_aether_cell_retain(idx); /* 'idx' undeclared here */ +_e->entry = (const char**)_aether_cell_retain(entry); +``` + +So codegen: + +1. unifies the closure's own locals with the enclosing function's same-named + locals purely by name, +2. concludes the pair is captured-and-mutated and promotes the **earlier** ones + to heap cells, +3. emits the capture at a construction site that sits **outside the C block + where those cells were declared and released**. + +The closure's locals are not captures at all — they are declared in the closure +body. A name assigned inside a closure should not be unified with a same-named +local of the enclosing function, least of all one in an already-closed block. + +## What did NOT reproduce it (so you don't redo this) + +Five reduced cases all compile clean on 0.677 and 0.696, so the shape alone is +not sufficient: + +1. a closure local declared inside a nested `if`; +2. the same plus tuple-destructuring assignment (`a, b = f(...)`) in the closure; +3. the same with string interpolation over captured vars; +4. the closure inside a `while` loop; +5. the same name in an earlier closed block **and** in the closure — the shape + described above, in a small function. + +The trigger needs something the small cases lack — plausibly the size of +`dotnet_build_project` (~200 lines, several closures) tipping an inlining or +scope-flattening decision. `aeb/lib/dotnet/module.ae:698-893` at the pre-`6af17aa` +revision is the reliable reproducer. + +## Confirming the diagnosis + +Renaming *only* the closure's own locals (`idx`→`vr_idx`, `entry`→`vr_entry`), +changing nothing else, makes it compile and pass. That is what shipped in aeb +`6af17aa`. It confirms name-unification as the mechanism, and is the reason the +symptom is currently invisible. + +## Why it is worth fixing rather than leaving worked around + +Two unrelated locals sharing a name in one long function is ordinary code, not a +smell anyone would flag in review. The failure is a C compile error naming a +variable that does not exist at the reported line, in generated code the author +never sees — with a whole-graph blast radius. The next occurrence will cost +someone the same day it cost here. + +--- + +Reported from `servirtium-vcr` on CachyOS (session sv-co), with the bisect and +generated-C evidence; the aeb-side workaround and the presubmit blast-radius +detail came from the selenium side (session se-co). diff --git a/compiler/codegen/codegen_expr.c b/compiler/codegen/codegen_expr.c index ad1483fc..d1ba411a 100644 --- a/compiler/codegen/codegen_expr.c +++ b/compiler/codegen/codegen_expr.c @@ -2104,6 +2104,23 @@ void emit_closure_definitions(CodeGenerator* gen) { for (int p = 0; p < parent_promoted_count; p++) { if (!parent_promoted[p]) continue; if (is_closure_param(closure, parent_promoted[p])) continue; + // A parent-promoted name is a promoted capture of THIS + // closure only if the closure actually captures it — i.e. + // it has the `T* name = _env->name;` prologue alias emitted + // above. A parent-promoted name the closure does NOT capture + // is either unused here or SHADOWED by a same-named local of + // this closure's own body; inheriting it would (a) put a + // dereferencing `*name` promoted-write on that own local and + // (b) mark it pre-declared, so the local is never minted and + // the emitted C references an undeclared name. Exclude it so + // the own local declares normally. + int captured = 0; + for (int c = 0; c < cap_count; c++) { + if (captures[c] && strcmp(captures[c], parent_promoted[p]) == 0) { + captured = 1; break; + } + } + if (!captured) continue; body_promoted[body_promoted_count++] = parent_promoted[p]; } for (int p = 0; p < own_promoted_count; p++) { @@ -2123,9 +2140,20 @@ void emit_closure_definitions(CodeGenerator* gen) { // Mark promoted captures as already-declared in this local scope // so writes in the body hit the reassignment branch (emits // *name = ...) rather than trying to declare+malloc again. - // The prologue alias `T* name = _env->name;` is the declaration. + // The prologue alias `T* name = _env->name;` is the declaration — + // so only names this closure actually captures are pre-declared. + // A parent-promoted name the closure does NOT capture has no alias + // and may be shadowed by a same-named own local, which must declare + // normally (see the body_promoted filter above). for (int p = 0; p < parent_promoted_count; p++) { - if (parent_promoted[p]) mark_var_declared(gen, parent_promoted[p]); + if (!parent_promoted[p]) continue; + int captured = 0; + for (int c = 0; c < cap_count; c++) { + if (captures[c] && strcmp(captures[c], parent_promoted[p]) == 0) { + captured = 1; break; + } + } + if (captured) mark_var_declared(gen, parent_promoted[p]); } /* A closure body is its own C function — it needs the same * heap-string lifecycle as a top-level function, or heap diff --git a/tests/leaks_known.txt b/tests/leaks_known.txt index 40f635dd..a61633c2 100644 --- a/tests/leaks_known.txt +++ b/tests/leaks_known.txt @@ -35,3 +35,13 @@ test_rsa_pkcs1 130 # transient-extern annotation to opt in). Bounded (does not scale with element # count), and unrelated to the value-correctness this test asserts. test_closure_local_alloc_capture 20 + +# test_closure_local_shadows_promoted_capture: the one residual leak is the +# promoted heap cell for the earlier block's captured-and-mutated local (a +# `bump` closure captures it, forcing capture promotion). That 20-byte cell is +# reclaimed a generation late by capture-promotion's RCU discipline and is +# INHERENT to any promoted-capture program — a bump-only program with no name +# collision leaks the identical cell. It is independent of the shadowing bug +# this test guards (a C-compile failure, fixed in codegen), which allocates +# nothing itself. macOS-only gate (Linux doesn't leak-check this). +test_closure_local_shadows_promoted_capture 1 diff --git a/tests/regression/test_closure_local_shadows_promoted_capture.ae b/tests/regression/test_closure_local_shadows_promoted_capture.ae new file mode 100644 index 00000000..66a3871f --- /dev/null +++ b/tests/regression/test_closure_local_shadows_promoted_capture.ae @@ -0,0 +1,61 @@ +// Regression: a closure's OWN body local must not be unified with a +// same-named PROMOTED capture of the enclosing function. +// +// closure-locals-unified-with-outer-block-names-emit-undeclared-c.md +// (live on 0.696, landed 0.675). When an enclosing function has a local +// (here `idx`/`entry`) captured-and-mutated by one closure — so codegen +// promotes it to a heap cell — a LATER closure whose own body declares +// locals of the SAME NAME had those locals wrongly treated as promoted +// captures. The closure 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 the +// generated C referenced an undeclared `idx`/`entry` (a gcc hard error in +// code the author never sees). The fix: a parent-promoted name is inherited +// into a closure's promoted set only if the closure actually captures it; an +// uncaptured same-named name is a shadowing own local and declares normally. +// +// Compiling IS the assertion (the bug was a C-compile failure); the value +// check guards that both the promoted-capture path and the own-local path +// still compute correctly. No heap collections/strings are allocated so the +// only leak is the promoted cell itself (inherent to capture promotion, +// independent of this bug — see tests/leaks_known.txt). + +import std.string + +build(n: int) -> int { + total = 0 + + // Earlier block: idx/entry are captured AND mutated by `bump`, so the + // compiler promotes them to heap cells. bump: idx 0 -> 1, len("bumped")=6 + // => returns 7. + if n > 0 { + idx = 0 + entry = "seed" + bump = || { + idx = idx + 1 + entry = "bumped" + return idx + string.length(entry) + } + total = total + bump() + } + + // Later closure: `idx`/`entry` here are its OWN locals, NOT captures of + // the promoted names above. each: 7 + length("local")=5 => 12. + each = | it: string | { + idx = 7 + entry = "local" + return idx + string.length(entry) + } + total = total + each("x") + return total // 7 + 12 = 19 +} + +main() { + got = build(1) + println("got=${got}") + if got != 19 { + println("FAIL: expected 19, got ${got}") + exit(1) + } + println("PASS: closure own-local shadows promoted capture") +}