fix(codegen): don't refresh local_types for a box-captured local

[proggeramlug]

What went wrong

0ed806587c (#10488) added a ctx.local_types refresh to the redeclaration branch of lower_let, so is_numeric_expr and static_type_of would stop disagreeing about a hoisted var.

It fixed that desync for hoisted var and introduced a new one for box-captured locals.

A captured local reaches that same branch without any redeclaration in the source: Stmt::PreallocateBoxes registers the id up front, so its one real Stmt::Let finds ctx.locals already populated and lands there. For such an id the refined type describes the value, while the slot holds a box pointer. The local_types readers then lower reads as raw local loads instead of going through js_box_get_bits.

Result: the declaring scope read undefined while a closure over the same binding, which holds the box directly, still saw the real array.

Object.defineProperty(Array.prototype, "11", { configurable: true, get() { return "P"; } });
delete (Array.prototype as any)[11];

function z4(): void {
  const dest: any[] = new Array(12);
  dest[0] = { old: "O" };
  for (let i = 1; i < 12; i++) { dest[i] = i; }
  function S(): any[] { return dest; }        // nested closure capturing `dest`
  console.log(typeof dest, S().length, S() === dest);
}
z4();

Node prints object 12 true. Before this change Perry printed undefined 12 false.

The trigger is rare — something must have put an indexed property on Array.prototype, which arms a monotone deopt latch that deleting the property does not clear. The affected shape is not rare: an array local captured by a closure is ordinary code, and the symptom is a silently empty-looking variable rather than a crash.

The fix

One condition, in the branch that introduced the bug:

if !ctx.boxed_vars.contains(&id) {
    ctx.local_types.insert(id, refined_ty.clone());
}

ctx.boxed_vars is an existing FnCtx field already in scope here — documented as the set whose LocalGet "reads the slot, unboxes, and calls js_box_get_bits", and prealloc_boxes ids are added to it automatically. No plumbing was needed.

A hoisted var is unboxed unless separately captured, so #10488 keeps its fix.

Why not a revert

Measured, not assumed. Building HEAD with the added line simply deleted:

test	plain revert	this PR
test_gap_10727_captured_array_local_proto_index	pass	pass
test_gap_array_side_mask_covers_a_pointer_stored_at_a_late_index	pass	pass
test_gap_10488_var_array_void_compare	FAIL	pass

A revert trades one red gap test for another and leaves pr-gate broken either way.

What this costs, and what it does not

A box-captured local now keeps whatever type the predefine recorded, normally Any, so it can lose a fast path. Three things about that:

• It is the conservative direction — the cost is a fast path, never correctness.
• It is not a regression against any working behaviour. Before 0ed806587c these locals got no refinement at all; after it they got one that described the value while the slot held a box pointer. This declines to record a type that was wrong for this storage class.
• If refinement for captured locals is wanted later, the right shape is a box-aware type, not this line.

What this does NOT touch

No change to capture lowering, PreallocateBoxes, boxed_vars collection, the box runtime helpers, array/indexing*.rs, array/splice_slice.rs, or the collector. The array was never corrupted and the GC was never involved — the fault was entirely in which storage a read was lowered against.

How it was found

The fixture that exposed it is named test_gap_array_side_mask_covers_a_pointer_stored_at_a_late_index and its companions live in gc/tests/, so it read as a GC side-mask bug. It is not one. Reading destination[10] before the gc() showed it already undefined, which removed the collector from the picture in one probe. slice passing while splice failed turned out to be an ordering artefact — the first invocation in a process passes because the latch is not yet armed.

Dated and attributed by bisect, six builds, each with its runtime stamp checked against the commit under test:

commit	verdict
train 218 60922041cd	clean
#6 08bf655af7	clean
#10 e821e10a8b	clean
#12 8f1ad83a8f	clean
#13 324b8ad0bf	changelog only
#14 0ed806587c	first bad
#16 8cbf5bef09	broken
HEAD 4715bc2fa1	broken

Verification

• New fixture proven discriminating: run against unfixed HEAD (v0.5.1598 | git:4715bc2fa1) it fails at line 3 with captured typeof: undefined.
• All three tests pass from one binary (stamp src:1b6c72be…) — new fixture, test_gap_10488_var_array_void_compare, and test_gap_array_side_mask....
• cargo test --release -p perry-codegen: 2145 passed, 0 failed, including both let_stmt_var_redeclare_tests — the unit-level guards on codegen: arr[i] === void 0 is always false for an out-of-bounds read of a var-declared number array (numeric fcmp on the undefined tag) #10488's behaviour.
• cargo check --release -p perry-codegen --all-targets clean (--all-targets because --lib compiles no cfg(test) code).
• cargo fmt --all -- --check clean.
• Full gap suite, 871 fixtures, one invocation (HARNESS_EXIT=0, 1h37m):

Parity Pass:   864
Parity Fail:   7
Compile Fail:  0
Crashed:       0
Skipped:       0
Parity Rate:   99.1%

All 7 failures are pre-existing and none is attributable to this change: the six standing gap_snapshot.json entries (2159_defineproperty_class_prototype, 2514_settracesigint, json_lazy_defineproperty_index, perfhooks_3088_3008_3010_3011, prop_plan_cache_invalidation, v8_2_3680plus) all failing in exactly their recorded state, plus 9592_child_timeout_threads, which is #10730 — a macOS fixture-portability bug where the fixture hardcodes /bin/true and the oracle is the broken half. The snapshot gate is bidirectional, so a listed test that started passing would also be a divergence; none did.

The three tests that matter here came from that same invocation as the other 868, not from separate runs: test_gap_10488_var_array_void_compare at position 46, test_gap_10727_captured_array_local_proto_index at 59, and test_gap_array_side_mask_covers_a_pointer_stored_at_a_late_index at 353 — all PASS. The first two are the two sides of the trade a plain revert could not satisfy, green together in one pass.

The new fixture includes the #10488 hoisted-var-redeclare shape as its own case, so it guards against a future change re-breaking what 0ed806587c fixed.

Note for the next person in this file

crates/perry-codegen/src/stmt/let_stmt.rs is now at exactly 2000 lines, the check_file_size.sh cap. It was at 1999 before this change; the combined #10488/#10727 comment was condensed to fit the guard in at net +1 line. Anything further added here needs the file split first.

Closes #10727

Summary by CodeRabbit

• Bug Fixes

  • Fixed captured array locals incorrectly reading as undefined from their declaring scope after certain array prototype changes.
  • Preserved correct behavior for hoisted variables and captured values across nested closures.

• Tests

  • Added regression coverage for captured arrays, objects, mutations, prototype indexed properties, and hoisted variable redeclarations.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: afd7155b-6742-46ee-b0e2-673c49b4a9ec

📥 Commits

Reviewing files that changed from the base of the PR and between d4ef732 and 56b8009.

📒 Files selected for processing (3)

• changelog.d/10747-box-captured-local-types.md
• crates/perry-codegen/src/stmt/let_stmt.rs
• test-files/test_gap_10727_captured_array_local_proto_index.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

The fix prevents boxed captured locals from receiving value types for pointer storage. Hoisted unboxed variables retain the type refresh. A regression fixture covers captured arrays, control cases, mutations, and hoisted redeclarations.

Changes

Captured local fix

Layer / File(s)	Summary
Guard boxed local type refresh crates/perry-codegen/src/stmt/let_stmt.rs, changelog.d/10747-box-captured-local-types.md	The redeclaration path skips local_types refreshes for ids in ctx.boxed_vars. The changelog records the cause, fix, and verification results.
Regression fixture coverage test-files/test_gap_10727_captured_array_local_proto_index.ts	The fixture tests captured array reads after the array-prototype deopt is armed, plus plain arrays, captured objects, mutations, and hoisted var redeclarations.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main code generation fix: avoiding an incorrect local type refresh for box-captured locals.
Description check	✅ Passed	The description provides a detailed problem statement, fix, issue reference, regression coverage, verification results, and scope. It does not use the template's exact Summary, Changes, Test plan, or …
Linked Issues check	✅ Passed	Issue #10727 requires correct reads for a closure-captured array, correct binding identity, preservation of hoisted-var behavior, and no gap snapshot change. The change guards the ctx.local_types …
Out of Scope Changes check	✅ Passed	The changes are limited to the capture-lowering type guard, regression coverage for #10727 and the related hoisted-var case, and a changelog entry. These changes directly support the linked issue or…

Full details: Docstring Coverage

Explanation

Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Landed in merge train 226 (#10751), released as v0.5.1605 — main is now 91a566c8af.

Closing rather than merging is how trains work here: the four PRs were cherry-picked onto one tree, validated together, and landed under the train's own commit, so GitHub cannot mark this one merged even though your change is on main.

The workspace count triple was re-derived on the assembled tree rather than taken from any PR's recorded value: 78 members / externalize=29 / keep=44. Both #10679 and #10691 correctly recorded 78/29/44 against 053b9ccac4, and whichever landed second would have been wrong — so the number was recomputed here rather than carried.

Validation: all nine cheap gates, cargo check --workspace --all-targets under -D warnings, the release build of all five pinned artifacts, every unit suite, and an 8-area gap sweep with zero unexplained regressions and each area asserted to have run a non-zero number of tests. lint completed its full 6-of-6 compile tier with nothing outside the known-red public-baseline step. Ledger green at 376/326, unrooted_local_shape at 578.