diff --git a/crates/perry-codegen/src/expr/unary.rs b/crates/perry-codegen/src/expr/unary.rs index cc9f4b7440..78db047c08 100644 --- a/crates/perry-codegen/src/expr/unary.rs +++ b/crates/perry-codegen/src/expr/unary.rs @@ -32,7 +32,7 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // Everything else may be a BigInt at runtime — notably an indexed // read from a plain array — and routing it through ToNumber would // silently round large BigInts before `fneg` (#9142). - let dynamic_neg = matches!(op, UnaryOp::Neg) + let _dynamic_neg = matches!(op, UnaryOp::Neg) && !statically_numeric && !is_provably_not_bigint(ctx, operand); let (v, precomputed_truthy) = if matches!(op, UnaryOp::Not) { @@ -44,13 +44,18 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let blk = ctx.block(); match op { UnaryOp::Neg => { - if dynamic_neg { - Ok(blk.call(DOUBLE, "js_dynamic_neg", &[(DOUBLE, &v)])) - } else if numeric { + if numeric { Ok(blk.fneg(&v)) } else { - let coerced = blk.call(DOUBLE, "js_number_coerce", &[(DOUBLE, &v)]); - Ok(blk.fneg(&coerced)) + // Mirrors `Pos` below: anything not statically numeric + // goes through the dynamic helper, which performs a real + // ToNumeric. The old `js_number_coerce` + `fneg` fallback + // answered NaN when the operand's `valueOf` THREW, so + // `-{ valueOf() { throw … } }` silently produced NaN + // where every other operator (`+x`, `~x`, `x * 2`) + // propagated. `js_dynamic_neg` also keeps a BigInt a + // BigInt, which is why `dynamic_neg` folded in here. + Ok(blk.call(DOUBLE, "js_dynamic_neg", &[(DOUBLE, &v)])) } } UnaryOp::Pos => { diff --git a/crates/perry-codegen/src/stmt/loops.rs b/crates/perry-codegen/src/stmt/loops.rs index 8a57441397..4d20d9c912 100644 --- a/crates/perry-codegen/src/stmt/loops.rs +++ b/crates/perry-codegen/src/stmt/loops.rs @@ -5147,9 +5147,25 @@ fn stmt_is_packed_f64_loop_safe( // way `Stmt::Return` checks its value; #9185 admitted any throw at all // and leaned on `stmt_array_length_effect` to reject the constructing // ones, which is a weaker guarantee than stating the requirement here. + // The thrown operand is deliberately NOT required to be + // `expr_is_packed_f64_loop_safe`. That predicate asks "can the clone + // keep running after this", and after a throw it cannot: control + // leaves the loop for a landing pad and never returns, so no later + // iteration can observe a value the operand disturbed. + // + // What the operand still must not do is change `arr.length` before the + // hoisted bound is used, and that IS checked — `stmt_preserves_array_length` + // walks `Stmt::Throw(e)` into `e`, so `throw arr.pop()` is rejected + // there. Keeping the two questions in their own predicates is what lets + // a constructed operand (`new Error(…)`, `"bad " + i`) stay on the fast + // path; requiring loop-safety here costs 8.4x for no correctness gain. + // + // The accumulator writeback this needs is emitted at the throw site by + // `flush_packed_accumulator_locals`; without it this admission is the + // #9185 wrong answer. Stmt::Throw(value) => { packed_loop_abrupt_enabled() - && expr_is_packed_f64_loop_safe(ctx, value, arr_id, counter_id) + && throw_operand_cannot_unwind(ctx, value, arr_id, counter_id) } Stmt::LabeledBreak(_) | Stmt::LabeledContinue(_) @@ -5287,6 +5303,99 @@ fn local_is_int32_value(ctx: &FnCtx<'_>, local_id: u32) -> bool { ) } +/// Can evaluating `expr` as a thrown operand UNWIND before the throw itself? +/// +/// This is the guard on `flush_packed_accumulator_locals`. That flush is +/// emitted at the throw site, after the operand is lowered — so an operand that +/// unwinds on its own leaves the loop-carried accumulators stale, reproducing +/// #9185 one level deeper. `expr_is_packed_f64_loop_safe` is NOT sufficient +/// here: it accepts a bare `Expr::Binary` over locals, and `+` on an object +/// dispatches to a user `valueOf`/`toString` that can throw. +/// +/// Throwing a value coerces nothing, so a bare local is fine. Anything that +/// COERCES — arithmetic, concatenation, an Error message being stringified — +/// must be provably free of user dispatch. +fn throw_operand_cannot_unwind( + ctx: &FnCtx<'_>, + expr: &perry_hir::Expr, + arr_id: u32, + counter_id: u32, +) -> bool { + use perry_hir::Expr; + match expr { + // Thrown as-is: no conversion, so no user code, whatever it holds. + Expr::LocalGet(_) + | Expr::String(_) + | Expr::Number(_) + | Expr::Integer(_) + | Expr::Bool(_) + | Expr::Null + | Expr::Undefined => true, + Expr::ErrorNew(message) => message + .as_ref() + .is_none_or(|m| expr_is_coercion_free_primitive(ctx, m, arr_id, counter_id)), + Expr::TypeErrorNew(message) + | Expr::RangeErrorNew(message) + | Expr::ReferenceErrorNew(message) + | Expr::SyntaxErrorNew(message) => { + expr_is_coercion_free_primitive(ctx, message, arr_id, counter_id) + } + Expr::Binary { left, right, .. } | Expr::Compare { left, right, .. } => { + expr_is_coercion_free_primitive(ctx, left, arr_id, counter_id) + && expr_is_coercion_free_primitive(ctx, right, arr_id, counter_id) + } + Expr::Unary { operand, .. } => { + expr_is_coercion_free_primitive(ctx, operand, arr_id, counter_id) + } + _ => false, + } +} + +/// Can `expr` be stringified WITHOUT running user code? +/// +/// Deliberately syntactic and much narrower than `expr_is_packed_f64_loop_safe`: +/// that predicate accepts a bare `Expr::LocalGet`, which is right for a value +/// that is merely thrown (throwing coerces nothing) but wrong for one that is +/// about to be stringified by an Error constructor, since the local may hold an +/// object with a user `toString`. +/// +/// Admitted: literals, the loop counter, a read of the packed array being +/// iterated (both are numbers by the loop's own guards), and arithmetic or +/// concatenation over those. Anything else — including any other local — is +/// rejected, because proving it is not an object is not this predicate's job. +fn expr_is_coercion_free_primitive( + ctx: &FnCtx<'_>, + expr: &perry_hir::Expr, + arr_id: u32, + counter_id: u32, +) -> bool { + use perry_hir::Expr; + match expr { + Expr::String(_) + | Expr::Number(_) + | Expr::Integer(_) + | Expr::Bool(_) + | Expr::Null + | Expr::Undefined => true, + // The counter is the loop's own i32 induction variable. + Expr::LocalGet(id) => *id == counter_id, + // An element of the packed f64 array is a genuine double: the versioned + // loop only entered the fast clone after proving that layout. + Expr::IndexGet { object, index } => { + matches!(object.as_ref(), Expr::LocalGet(id) if *id == arr_id) + && expr_is_coercion_free_primitive(ctx, index, arr_id, counter_id) + } + Expr::Binary { left, right, .. } | Expr::Compare { left, right, .. } => { + expr_is_coercion_free_primitive(ctx, left, arr_id, counter_id) + && expr_is_coercion_free_primitive(ctx, right, arr_id, counter_id) + } + Expr::Unary { operand, .. } | Expr::NumberCoerce(operand) => { + expr_is_coercion_free_primitive(ctx, operand, arr_id, counter_id) + } + _ => false, + } +} + fn expr_is_packed_f64_loop_safe( ctx: &FnCtx<'_>, expr: &perry_hir::Expr, @@ -8203,6 +8312,28 @@ pub(crate) fn expr_preserves_array_length( && walk(object) && args.iter().all(&walk) } + // The Error-family construction nodes allocate an error object and + // store their own message; none of them can reach an unrelated local + // array, so they preserve `arr.length`. Together with the `Stmt::Throw` + // admission above this is what keeps `if (bad) throw new Error(…)` — + // the #9151 shape — on the packed fast path. + // + // Identity is not in question at this layer: HIR emits these nodes only + // for the INTRINSIC constructor (`lower_new` gates them on + // `!shadowed_by_user_binding`). A user `class Error { … }`, or any other + // shadowing binding, lowers to `Expr::New`/`NewDynamic` instead and is + // still rejected by the fallback below — checking the NAME here would be + // a wrong-answer bug rather than a missed optimisation. The message and + // options operands are arbitrary expressions and are still walked. + Expr::ErrorNew(message) => message.as_ref().is_none_or(|m| walk(m)), + Expr::TypeErrorNew(message) + | Expr::RangeErrorNew(message) + | Expr::ReferenceErrorNew(message) + | Expr::SyntaxErrorNew(message) => walk(message), + Expr::ErrorNewWithCause { message, cause } => walk(message) && walk(cause), + Expr::ErrorNewWithOptions { + message, options, .. + } => walk(message) && walk(options), Expr::NativeMethodCall { .. } | Expr::CallSpread { .. } => false, Expr::Closure { .. } => false, Expr::Binary { left, right, .. } diff --git a/crates/perry-runtime/src/value/dynamic_arith.rs b/crates/perry-runtime/src/value/dynamic_arith.rs index cd85ad80ac..33e83afedd 100644 --- a/crates/perry-runtime/src/value/dynamic_arith.rs +++ b/crates/perry-runtime/src/value/dynamic_arith.rs @@ -714,6 +714,13 @@ pub unsafe extern "C" fn js_dynamic_mod(a: f64, b: f64) -> f64 { /// Dynamic negate: -BigInt if operand is BigInt, else -f64. #[no_mangle] pub unsafe extern "C" fn js_dynamic_neg(a: f64) -> f64 { + // Unary minus performs ToNumeric on its operand, exactly as `js_dynamic_pos` + // below does. Negating the raw NaN-boxed bits skipped that entirely, so an + // object operand never had `valueOf` called: `-{ valueOf() { throw … } }` + // answered NaN instead of propagating, while `+x`, `~x` and `x * 2` on the + // same object all threw. ToNumeric is a no-op for a number and returns the + // BigInt unchanged for a BigInt, so the two arms below are unaffected. + let a = to_numeric(a); let a_val = JSValue::from_bits(a.to_bits()); if a_val.is_bigint() { let scope = crate::gc::RuntimeHandleScope::new(); diff --git a/crates/perry/tests/packed_loop_error_throw.rs b/crates/perry/tests/packed_loop_error_throw.rs new file mode 100644 index 0000000000..0bd09d54a2 --- /dev/null +++ b/crates/perry/tests/packed_loop_error_throw.rs @@ -0,0 +1,360 @@ +//! `throw new Error(…)` and other constructed operands inside a packed counted +//! loop (#9151, #9232). +//! +//! Admitting these depends on #9210's writeback, which is emitted at the throw +//! site — AFTER the operand is lowered. So an operand that can unwind ON ITS +//! OWN skips the flush and leaves the loop-carried accumulators stale, which is +//! #9185's defect one level deeper. Two things can unwind that way and neither +//! looks like a call at the syntax level: +//! +//! * `new Error(msg)` stringifies `msg`, dispatching to a user `toString`. +//! * `a + b` coerces, dispatching to a user `valueOf`/`toString`. +//! +//! Both are admitted only when the operand is provably coercion-free. The +//! rejection tests below are the load-bearing half of this file: each one +//! returned `c=0 s=0` against node's `c=40 s=780` while the predicate was +//! merely "is this loop-safe", and each is a silent wrong answer rather than a +//! missed optimisation. +//! +//! The `Error`-shadowing tests guard the other half. Accepting the Error nodes +//! is sound only because HIR emits them for the INTRINSIC constructor alone +//! (`lower_new` gates on `!shadowed_by_user_binding`); a user `class Error` +//! whose constructor shrinks the array would otherwise run against a hoisted +//! length. Each shadow case mutates the array MID-LOOP and reports the +//! iteration count, so a wrongly hoisted length reads `iters=10`, not `iters=3`. + +use std::path::PathBuf; +use std::process::{Command, Output}; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run(source: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + let output = dir.path().join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + + let run: Output = Command::new(&output) + .current_dir(dir.path()) + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).trim().to_owned() +} + +const PRELUDE: &str = r#" +const arr: number[] = []; +for (let i = 0; i < 64; i++) arr.push(i); +"#; + +const SHADOW_BODY: &str = r#" +function f(): string { + let n = 0; let s = 0; + for (let i = 0; i < arr.length; i++) { + if (i === 1) { const e = new Error("shrink"); if (n < 0) throw e; } + n++; const v = arr[i]; s += (v === undefined ? 0 : v); + } + return "iters=" + n + " sum=" + s + " len=" + arr.length; +} +console.log(f()); +"#; + +// --- the operand must not be able to unwind before the flush ----------------- + +#[test] +fn an_error_message_whose_tostring_throws_keeps_the_accumulators() { + // Gave `c=0 s=0`: the toString unwound during Error construction, before + // the writeback at the throw site could run. + let out = compile_and_run(&format!( + "{PRELUDE} + const evil: any = {{ toString(): string {{ throw new Error(\"from toString\"); }} }}; + function f(): string {{ + let s = 0; let c = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw new Error(evil); + c++; s += arr[i]; + }} + }} catch (e: any) {{ return \"c=\" + c + \" s=\" + s + \" msg=\" + e.message; }} + return \"none\"; + }} + console.log(f()); + " + )); + assert_eq!(out, "c=40 s=780 msg=from toString"); +} + +#[test] +fn a_binary_operand_whose_valueof_throws_keeps_the_accumulators() { + // No Error construction at all — plain `+` dispatching to a user valueOf. + // Gave `c=0 s=0`. + let out = compile_and_run(&format!( + "{PRELUDE} + const evil: any = {{ valueOf(): number {{ throw new Error(\"from valueOf\"); }} }}; + const two: any = 2; + function f(): string {{ + let s = 0; let c = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw (evil + two); + c++; s += arr[i]; + }} + }} catch (e: any) {{ return \"c=\" + c + \" s=\" + s + \" msg=\" + (e.message || e); }} + return \"none\"; + }} + console.log(f()); + " + )); + assert_eq!(out, "c=40 s=780 msg=from valueOf"); +} + +#[test] +fn a_unary_operand_whose_valueof_throws_keeps_the_accumulators() { + let out = compile_and_run(&format!( + "{PRELUDE} + const evil: any = {{ valueOf(): number {{ throw new Error(\"unary valueOf\"); }} }}; + function f(): string {{ + let s = 0; let c = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw (-evil); + c++; s += arr[i]; + }} + }} catch (e: any) {{ return \"c=\" + c + \" s=\" + s + \" msg=\" + (e.message || e); }} + return \"none\"; + }} + console.log(f()); + " + )); + assert_eq!(out, "c=40 s=780 msg=unary valueOf"); +} + +#[test] +fn a_getter_that_throws_in_the_message_keeps_the_accumulators() { + let out = compile_and_run(&format!( + "{PRELUDE} + const holder: any = {{ get boom(): string {{ throw new Error(\"getter\"); }} }}; + function f(): string {{ + let s = 0; let c = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw new Error(holder.boom); + c++; s += arr[i]; + }} + }} catch (e: any) {{ return \"c=\" + c + \" s=\" + s + \" msg=\" + e.message; }} + return \"none\"; + }} + console.log(f()); + " + )); + assert_eq!(out, "c=40 s=780 msg=getter"); +} + +#[test] +fn throwing_an_object_as_is_never_coerces_it() { + // The counterpart to the rejections above: thrown AS-IS, nothing converts + // it, so this stays on the fast path and the toString must never run. + let out = compile_and_run(&format!( + "{PRELUDE} + const evil: any = {{ toString(): string {{ throw new Error(\"should not run\"); }} }}; + function f(): string {{ + let s = 0; let c = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw evil; + c++; s += arr[i]; + }} + }} catch (e: any) {{ return \"c=\" + c + \" s=\" + s + \" isObj=\" + (typeof e); }} + return \"none\"; + }} + console.log(f()); + " + )); + assert_eq!(out, "c=40 s=780 isObj=object"); +} + +// --- the shapes that must stay FAST ----------------------------------------- + +#[test] +fn an_intrinsic_error_throw_keeps_the_packed_clone() { + let out = compile_and_run(&format!( + r#"{PRELUDE} +function f(): number {{ + let s = 0; + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 999) throw new Error("unreachable"); + s += arr[i]; + }} + return s; +}} +console.log(f()); +"# + )); + assert_eq!(out, "2016"); +} + +#[test] +fn an_intrinsic_error_throw_is_taken_at_the_right_index() { + let out = compile_and_run(&format!( + r#"{PRELUDE} +function f(): string {{ + let s = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw new Error("hit@" + i); + s += arr[i]; + }} + }} catch (e: any) {{ return "caught:" + e.message + " partial=" + s; }} + return "none:" + s; +}} +console.log(f()); +"# + )); + assert_eq!(out, "caught:hit@40 partial=780"); +} + +#[test] +fn an_array_element_in_the_message_is_coercion_free() { + // `arr[i]` is a genuine double by the clone's own guard, so stringifying it + // is builtin. + let out = compile_and_run(&format!( + r#"{PRELUDE} +function f(): string {{ + let s = 0; + try {{ + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 40) throw new Error("v=" + arr[i]); + s += arr[i]; + }} + }} catch (e: any) {{ return "s=" + s + " msg=" + e.message; }} + return "none"; +}} +console.log(f()); +"# + )); + assert_eq!(out, "s=780 msg=v=40"); +} + +#[test] +fn native_error_subclasses_are_accepted_too() { + let out = compile_and_run(&format!( + r#"{PRELUDE} +function f(kind: number): number {{ + let s = 0; + for (let i = 0; i < arr.length; i++) {{ + if (arr[i] === 999) {{ + if (kind === 0) throw new TypeError("t"); + if (kind === 1) throw new RangeError("r"); + if (kind === 2) throw new SyntaxError("s"); + throw new ReferenceError("f"); + }} + s += arr[i]; + }} + return s; +}} +console.log(f(0) + "," + f(1) + "," + f(2) + "," + f(3)); +"# + )); + assert_eq!(out, "2016,2016,2016,2016"); +} + +#[test] +fn a_message_expression_that_shrinks_the_array_is_still_rejected() { + let out = compile_and_run( + r#" +const arr: number[] = [1,2,3,4,5,6,7,8,9,10]; +function shrink(): string { arr.length = 3; return "boom"; } +function f(): string { + let n = 0; let s = 0; + for (let i = 0; i < arr.length; i++) { + if (i === 1) { const e = new Error(shrink()); if (n < 0) throw e; } + n++; const v = arr[i]; s += (v === undefined ? 0 : v); + } + return "iters=" + n + " sum=" + s + " len=" + arr.length; +} +console.log(f()); +"#, + ); + assert_eq!(out, "iters=3 sum=6 len=3"); +} + +// --- `Error` must be the intrinsic, not a look-alike ------------------------ + +#[test] +fn a_user_class_named_error_is_not_the_intrinsic() { + let out = compile_and_run(&format!( + r#" +const arr: number[] = [1,2,3,4,5,6,7,8,9,10]; +class Error {{ constructor(_m: string) {{ arr.length = 3; }} }} +{SHADOW_BODY}"# + )); + assert_eq!(out, "iters=3 sum=6 len=3"); +} + +#[test] +fn a_const_function_named_error_is_not_the_intrinsic() { + let out = compile_and_run(&format!( + r#" +const arr: number[] = [1,2,3,4,5,6,7,8,9,10]; +const Error: any = function (this: any, _m: string) {{ arr.length = 3; }}; +{SHADOW_BODY}"# + )); + assert_eq!(out, "iters=3 sum=6 len=3"); +} + +#[test] +fn a_function_valued_error_binding_is_not_the_intrinsic() { + // The shadow's identity is not visible in the initializer at all. + let out = compile_and_run(&format!( + r#" +const arr: number[] = [1,2,3,4,5,6,7,8,9,10]; +function makeErr(): any {{ return function (this: any, _m: string) {{ arr.length = 3; }}; }} +const Error: any = makeErr(); +{SHADOW_BODY}"# + )); + assert_eq!(out, "iters=3 sum=6 len=3"); +} + +#[test] +fn a_block_scoped_class_named_error_is_not_the_intrinsic() { + let out = compile_and_run( + r#" +const arr: number[] = [1,2,3,4,5,6,7,8,9,10]; +function f(): string { + class Error { constructor(_m: string) { arr.length = 3; } } + let n = 0; let s = 0; + for (let i = 0; i < arr.length; i++) { + if (i === 1) { const e = new Error("shrink"); if (n < 0) throw e; } + n++; const v = arr[i]; s += (v === undefined ? 0 : v); + } + return "iters=" + n + " sum=" + s + " len=" + arr.length; +} +console.log(f()); +"#, + ); + assert_eq!(out, "iters=3 sum=6 len=3"); +}