Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions changelog.d/10627-var-array-void-compare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
### Fixed

- **`arr[i] === void 0` (and other undefined-valued comparisons) against an
out-of-bounds or hole read of a `var`-declared number array always
compiled `false`, and `!==` always `true`.** A hoisted `var` lowers to two
HIR declarations sharing one local id (a body-entry predefine, then the
real declaration); the codegen redeclaration path refreshed the numeric
type PROOF used by `is_numeric_expr` but left the declared-type map used
by the boxed-fallback hazard guard stale at `Any`. The two disagreed about
the same local, so the hazard guard never caught the case and a strict
equality compare against the array element compiled to a bare `fcmp` —
which cannot represent the NaN-boxed `undefined` tag a hole/out-of-bounds
read actually produces. Both are now kept in sync on every `var`
redeclaration. This was blocking `decimal.js`'s `toHexadecimal`/
`toBinary`/`toOctal` (`convertBase`'s carry-slot initialization check).
16 changes: 16 additions & 0 deletions crates/perry-codegen/src/stmt/let_stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,22 @@ pub(crate) fn lower_let(
ctx,
&perry_hir::Expr::LocalSet(id, Box::new(init_expr.clone())),
)?;
// #10488: a hoisted `var`'s real declaration reaches this
// redeclaration branch (#1803 predefine-then-declare shape) and
// returns below before the fresh-declaration path's
// `ctx.local_types.insert` ever runs. `proven_local_types` a few
// lines up IS refreshed per-site, so `is_numeric_expr` (which
// consults it via `stable_local_type_proof`) sees this
// declaration's more specific type — but `local_types` keeps
// whatever the FIRST (predefine) site declared, normally `Any`.
// That desyncs `is_numeric_expr` from `static_type_of` /
// `expr_may_return_boxed_value_from_raw_f64_fallback`, which both
// read `local_types`: a strict-equality compare against an
// out-of-bounds/hole array read was treated as definitely-numeric
// (a bare `fcmp`, which cannot represent `undefined`) instead of
// falling back to a boxed compare. Refresh `local_types` here too
// so both predicates agree on this local's current type.
ctx.local_types.insert(id, refined_ty.clone());
} else if ctx.tdz_boxes.remove(&id) {
// No-init reuse (`let x;`) of a TDZ-seeded box must still end the
// dead zone by clearing the sentinel to `undefined`; otherwise a
Expand Down
119 changes: 119 additions & 0 deletions crates/perry-codegen/src/stmt/let_stmt_var_redeclare_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
//! #10488: a hoisted `var` reaches `lower_let` as TWO `Stmt::Let`s sharing one
//! local id — a body-entry predefine (`Any = undefined`), then the real
//! declaration (here, `Array(Number) = [0]`) — and the second one takes the
//! #1803 redeclaration early return. `ctx.local_types` must be refreshed on
//! that path too, or `expr_may_return_boxed_value_from_raw_f64_fallback`
//! (which reads it via `static_type_of`) stays desynced from
//! `is_numeric_expr` (which reads the separately-refreshed
//! `proven_local_types`), and a strict-equality compare against an
//! out-of-bounds array read wrongly takes the bare-`fcmp` numeric fast path.

use perry_hir::types::Type;
use perry_hir::{CompareOp, Expr, Stmt};

use crate::temp_root_coverage::main_ir_for as ir_for;

const ARR: u32 = 1;
const R: u32 = 2;

/// A CALL to the boxed comparison helper, not the unconditional `declare`
/// line — mirrors `compare_tests.rs`'s `JS_EQ_CALL`.
const JS_EQ_CALL: &str = "call i64 @js_eq(";

/// Hand-build the exact HIR shape a hoisted `var arr = [0]; ...; arr[1] ===
/// void 0;` lowers to: TWO `Let`s sharing id `ARR` (the predefine, then the
/// real array-literal declaration), followed by a strict-equality compare of
/// an out-of-bounds index read against `Expr::Void`.
#[test]
fn var_redeclared_numeric_array_compare_against_void_stays_boxed() {
let ir = ir_for(
"var_redeclare_void_compare",
vec![
// Body-entry predefine: `var arr;` before the real declaration
// runs — declared `Any`, matching what hoisting emits.
Stmt::Let {
id: ARR,
name: "arr".to_string(),
ty: Type::Any,
mutable: true,
init: Some(Expr::Undefined),
},
// The real declaration: same id, now a proven `Array(Number)`.
// This is the REDECLARATION path in `lower_let` (#1803).
Stmt::Let {
id: ARR,
name: "arr".to_string(),
ty: Type::Array(Box::new(Type::Number)),
mutable: true,
init: Some(Expr::Array(vec![Expr::Integer(0)])),
},
Stmt::Let {
id: R,
name: "r".to_string(),
ty: Type::Any,
mutable: true,
init: Some(Expr::Compare {
op: CompareOp::Eq,
left: Box::new(Expr::IndexGet {
object: Box::new(Expr::LocalGet(ARR)),
index: Box::new(Expr::Integer(1)),
}),
right: Box::new(Expr::Void(Box::new(Expr::Integer(0)))),
}),
},
],
);
assert!(
ir.contains(JS_EQ_CALL),
"an out-of-bounds read of a var-redeclared Array(Number) compared \
against `void 0` must fall back to the boxed js_eq helper (the \
element read can be undefined, which the STATIC numeric fast path \
can't represent); got IR:\n{ir}"
);
// NOT a blanket "no fcmp anywhere" check: `js_eq`'s own dynamic
// comparison lowering has an internal, RUNTIME-guarded fast path (an
// `icmp` range check on the raw bits proves both operands are genuine
// untagged doubles before it dares an `fcmp`) that is safe and expected
// to appear here too — that is a property of the boxed path, not the
// STATIC always-numeric bug this test guards against. The call to
// `js_eq` above is what proves this comparison did NOT take the static
// fast path (`lower_strict_eq_against_number`, which emits an
// UNGUARDED `fcmp` with no dynamic dispatch at all).
}

/// Control: the SAME shape through a `let` (no redeclaration ambiguity)
/// already took the boxed path before this fix and must keep doing so.
#[test]
fn let_numeric_array_compare_against_void_stays_boxed() {
let ir = ir_for(
"let_void_compare",
vec![
Stmt::Let {
id: ARR,
name: "arr".to_string(),
ty: Type::Array(Box::new(Type::Number)),
mutable: false,
init: Some(Expr::Array(vec![Expr::Integer(0)])),
},
Stmt::Let {
id: R,
name: "r".to_string(),
ty: Type::Any,
mutable: true,
init: Some(Expr::Compare {
op: CompareOp::Eq,
left: Box::new(Expr::IndexGet {
object: Box::new(Expr::LocalGet(ARR)),
index: Box::new(Expr::Integer(1)),
}),
right: Box::new(Expr::Void(Box::new(Expr::Integer(0)))),
}),
},
],
);
assert!(
ir.contains(JS_EQ_CALL),
"control case (let, no redeclaration) must already take the boxed \
path; got IR:\n{ir}"
);
}
2 changes: 2 additions & 0 deletions crates/perry-codegen/src/stmt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ mod let_buffer_views;
mod let_object_facts;
mod let_stmt;
mod let_stmt_facts;
#[cfg(test)]
mod let_stmt_var_redeclare_tests;
mod loops;
mod masked_window_region;
#[cfg(test)]
Expand Down
101 changes: 101 additions & 0 deletions test-files/test_gap_10488_var_array_void_compare.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// #10488: `arr[i] === void 0` (and other undefined-valued comparisons)
// against an out-of-bounds/hole read of a `var`-declared number-literal
// array always compiled `false` (and `!==` always `true`).
//
// A hoisted `var` lowers to TWO HIR `Let`s sharing one LocalId: a body-entry
// predefine (`Any = undefined`) and the real declaration (`Array(Number) =
// [0]`, say). `lower_let` (crates/perry-codegen/src/stmt/let_stmt.rs)
// refreshes `ctx.proven_local_types` on every `Let`, but a redeclaration
// (the second `Let`, since the predefine already allocated the slot) took
// an early return that never touched `ctx.local_types`. That left
// `is_numeric_expr` (which consults `proven_local_types`) and
// `expr_may_return_boxed_value_from_raw_f64_fallback`/`static_type_of`
// (which read the now-stale `local_types`, still `Any`) disagreeing about
// the very same local: the fallback-hazard guard never fired, so a
// strict-equality compare against the array element compiled to a bare
// `fcmp` — which can't represent the NaN-boxed `undefined` tag a hole or
// out-of-bounds read actually produces.

function varVoid(): boolean {
var arr = [0];
return arr[1] === void 0;
}
function varNe(): boolean {
var arr = [0];
return arr[1] !== void 0;
}
function varUndefVar(): boolean {
var arr = [0];
var u;
return arr[1] === u;
}
function varTwoReads(): boolean {
var arr = [0];
return arr[1] === arr[2];
}
function varHole(): boolean {
var arr = [0, 1];
arr.length = 5;
return arr[3] === void 0;
}
function varNegIdx(): boolean {
var arr = [0];
return arr[-1] === void 0;
}
function varPropCompare(): boolean {
var arr = [0];
var o: any = {};
return arr[1] === o.v;
}
function varParamIdx(i: number): boolean {
var arr = [0];
return arr[i] === void 0;
}
// Controls: already correct in Perry before this fix; must stay correct.
function varUndefined(): boolean {
var arr = [0];
return arr[1] === undefined;
}
function letVoid(): boolean {
let arr = [0];
return arr[1] === void 0;
}
function varInBoundsVoid(): boolean {
var arr = [0];
return arr[0] === void 0;
}

var NUMERALS = "0123456789abcdef";
// decimal.js convertBase('255', 10, 16) (decimal.mjs:2608), verbatim.
function convertBase(str: string, baseIn: number, baseOut: number) {
var j, arr = [0], arrL, i = 0, strL = str.length;
for (; i < strL; ) {
for (arrL = arr.length; arrL--; ) arr[arrL] *= baseIn;
arr[0] += NUMERALS.indexOf(str.charAt(i++));
for (j = 0; j < arr.length; j++) {
if (arr[j] > baseOut - 1) {
if (arr[j + 1] === void 0) arr[j + 1] = 0;
arr[j + 1] += (arr[j] / baseOut) | 0;
arr[j] %= baseOut;
}
}
}
return arr.reverse();
}

for (const f of [
varVoid,
varNe,
varUndefVar,
varTwoReads,
varHole,
varNegIdx,
varPropCompare,
varUndefined,
letVoid,
varInBoundsVoid,
]) {
console.log(f.name, f());
}
console.log("varParamIdx", varParamIdx(1));
console.log("convertBase", JSON.stringify(convertBase("255", 10, 16)));
Loading