fix(codegen): refresh local_types on var redeclaration - #10627
proggeramlug wants to merge 2 commits into
Conversation
A hoisted var reaches lower_let as two Stmt::Lets sharing one local id (a body-entry predefine, then the real declaration); the second takes the #1803 redeclaration early return before ctx.local_types is updated. proven_local_types (consulted by is_numeric_expr) IS refreshed on redeclaration, but local_types (consulted by expr_may_return_boxed_value_from_raw_f64_fallback) was not, so the two predicates disagreed about the same local: a strict-equality compare against an out-of-bounds/hole read of a var-declared number array took the bare-fcmp numeric fast path, which cannot represent the NaN-boxed undefined tag such a read can produce. Refresh local_types on the redeclaration path too.
proggeramlug
added
the
package-audit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe compiler now refreshes ChangesHoisted var comparison handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable current-head risk remains; the new regression fixture will detect divergent strict-comparison results against Node. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
|
Landed via merge train #10710 (v0.5.1597). All source commits preserve authorship; merged main matches the validated train exactly. |
1 participant
Summary
arr[i] === void 0(and any other undefined-valued strict-equality compare)against an out-of-bounds or hole read of a
var-declared number array alwayscompiled
false, and!==always compiledtrue— regardless of whetherthe read is actually out of bounds.
let/constarrays were unaffected.This blocked
decimal.js'stoHexadecimal/toBinary/toOctal(
convertBase's carry-slot initialization check,arr[j + 1] === void 0).Root cause
A hoisted
varreachescrates/perry-codegen/src/stmt/let_stmt.rs'slower_letas TWOStmt::Lets sharing one local id: a body-entry predefine(
Any = undefined, frompredefine_var_bindings_in_function_body), then thereal declaration (here,
Array(Number) = [0]). The SECOND one hits the#1803 "hoisted var redeclaration" branch (reuse the already-allocated slot
via
LocalSetinstead of re-allocating) and returns early — before thenormal declaration path's
ctx.local_types.insert(id, refined_ty)runs.ctx.proven_local_types(a separate map, consulted byis_numeric_expr/stable_local_type_proofwhen deciding whether an arrayelement read is provably numeric) IS refreshed on every
Let, includingredeclarations.
ctx.local_types(consulted byexpr_may_return_boxed_value_from_raw_f64_fallback'sstatic_type_of,which is supposed to veto the numeric fast path whenever an element read
could still come back boxed/undefined) was NOT. So after the redeclaration,
is_numeric_exprsaid "yes, proven Array(Number)" while thefallback-hazard guard — reading the stale
Any— said "no hazard, safe totreat as a raw double." The comparison then took the numeric fast path
(
crates/perry-codegen/src/expr/compare.rs::lower_strict_eq_against_number,a bare
fcmp), which cannot represent the NaN-boxedundefinedtag anout-of-bounds/hole read actually produces (the tag IS a NaN, so
fcmpisunconditionally unordered — always false for
oeq, always true forune).let/constnever hit the redeclaration branch, so theirlocal_typesentry was always fresh and the guard already worked correctly.
Fix
crates/perry-codegen/src/stmt/let_stmt.rs: refreshctx.local_typesonthe
var-redeclaration path too (only in theSome(init_expr)arm — abare
var x;redeclaration with no initializer keeps whatever the localalready knows, matching "keep the prior value" semantics), so both
predicates agree about the local's current type at every point in the
function.
Tests added
test-files/test_gap_10488_var_array_void_compare.ts: every failingvariant from the issue (
void 0, anundefined-holding local, aproperty holding
undefined, another out-of-bounds read, a negativeindex, a hole after
.length =growth, a function-parameter index) plusthe issue's controls (
=== undefinedliteral,let, an in-bounds read)and the
decimal.jsconvertBaserepro verbatim (var, not: any—the earlier draft of this test used
arr: anyand did not reproduce thebug; TypeScript's own inference to
number[]is what triggers it).Validated byte-for-byte against
node --experimental-strip-types(Node26.5.1).
forked from with only the test file added (no fix code) — every
boolean variant inverted (
varVoid falsevs Node'strue, etc.) andconvertBasereturned[null,15](the carry slot never initializes,so
undefined += 15gives NaN, printed asnull). On this branch: alllines match Node exactly, including
convertBase [15,15].crates/perry-codegen/src/stmt/let_stmt_var_redeclare_tests.rs(2 newunit tests, registered in
stmt/mod.rs): hand-builds the exacttwo-
Lets-sharing-one-id HIR shape and asserts the compiled IR for thevoid 0compare calls the boxedjs_eqhelper and contains neitherfcmp une doublenorfcmp oeq double; alet-only control confirms thenon-redeclared case already took the boxed path.
Validation
cargo test --release -p perry-codegen --tests: 2098 passed, 0failed (all prior tests plus the 2 new ones; the new test file's first
assertion draft over-scoped — it initially rejected ANY
fcmpin thecompiled IR, but the boxed
js_eqcomparison path has its OWN internal,runtime-guarded
fcmpfast path for the case where both operands proveout to be genuine untagged doubles at runtime, which is correct and
expected; narrowed the assertion to check for the
js_eqcall instead offcmp's absence). Ran with
CARGO_PROFILE_RELEASE_CODEGEN_UNITS=16.Lint (
SKIP_COMPILE_GATES=1 ./scripts/run_lint_gates.sh): 76/77 gatespassed; pre-existing red: "Public benchmark evidence freshness" (red on
every PR in this repo, unrelated).
cargo fmt --all: clean.python3 scripts/check_test_registration.py: OK.Gap test:
PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_10488→ 1/1 PASS, 100% parity on this branch. Fails on thepristine baseline (
fix/10488-wip's parent commit + only the test fileadded): every boolean variant inverted (
varVoid falsevs Node'strue, etc.) andconvertBasereturned[null,15]instead of[15,15].Perf (
perf stat -e instructions,task-clock, 3 runs each, median shown;baseline binary built from a sibling clone with none of this PR's
changes,
PERRY_NO_AUTO_OPTIMIZE=1for both arms):benchmarks/suite/04_array_read.ts(general array-read workload, does not hit thevar-redeclaration pattern)arr[1] === void 0× 20M,var-declared number array)The repro-loop case is FASTER after the fix, not slower: the baseline's
bare
fcmppath and the fix'sjs_eqboxed-dispatch path are not simply"fast vs. slow" versions of the same work —
js_eq's own internalruntime-guarded fast path (an
icmprange check proving both operandsare genuine untagged doubles, then a plain
fcmp) apparently codegensmore efficiently for this loop shape than whatever the old
always-numeric static path emitted. The general case
(
04_array_read.ts, novarredeclaration in play) shows no measurablechange either way. Node wall time on the repro-loop workload: ~55ms
(JIT-optimized loop unrolling/elision Perry's AOT codegen does not
attempt); pre-existing gap, not something this PR changes.
Package check: verified the issue's exact
decimal.jsconvertBaserepro (the carry-slot
if (arr[j + 1] === void 0) arr[j + 1] = 0;check)directly — did not install the
decimal.jspackage itself; see "What Idid not verify" below.
What I did not verify
decimal.jspackage install/build end-to-end(
perry.compilePackages: ["decimal.js"]) — verified the exactconvertBaserepro from the issue body directly.var-redeclaration path inlower_let, not a hot shared lowering pathused by most programs — relying on CI's sharded gap suite per the
standard process.
a instanceof Dec === falsedecimal.jsfinding is explicitly a different bug ("covered elsewhere" per the issue
body) — not touched here.
Fixes #10488
Summary by CodeRabbit
Bug Fixes
undefinedvalues fromvar-declared numeric arrays.trueorfalse.Tests