From 42ba24f3da05d5ff42fc3bb2bd84d9f1331018f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 2 Sep 2026 10:09:07 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(codegen):=20a=20rejected=20sloppy=20`o.?= =?UTF-8?q?x=20+=3D=201`=20/=20`for=20(o.x=20of=20=E2=80=A6)`=20/=20`[o.x]?= =?UTF-8?q?=20=3D=20arr`=20no=20longer=20throws=20(#9459)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit // sloppy (.cts, no "use strict") const o = {x:1}; Object.freeze(o); o.x += 1; // node: silent Perry: TypeError for (o.x of [7]) {} // node: silent Perry: TypeError [o.x] = [7]; // node: silent Perry: TypeError (expression position) o[k] += 1; // node: silent Perry: TypeError o.x++; // node: silent Perry: silent (correct, Expr::PropertyUpdate) o.x = 9; // node: silent Perry: silent (correct, Expr::PutValueSet) ES2024 6.2.5.7 (PutValue) performs Set(O, P, V, Throw) with Throw = IsStrictReference(ref), and 10.1.9 (OrdinarySet) reports `false` -- not a throw -- for a non-writable own or inherited data property, an accessor with no setter, and a new property on a non-extensible object. The reference's own strictness is what turns that `false` into a TypeError. The ordinary-object mirror of #9394 (arrays, fixed by #9426) and the opposite direction from #9422 (an under-throw in strict code). A CommonJS bundle is sloppy top to bottom, so this was a hard failure: a spurious TypeError stopped a program node runs to completion. Root cause: `Expr::PropertySet` carries no strictness field at all, and its codegen tail reaches `js_typed_feedback_object_set_field_by_name_fast` -> `js_object_set_field_by_name`, which has no `strict` parameter and rejects by throwing. `o.x++` was right because it lowers to `Expr::PropertyUpdate` (carries `ctx.current_strict`); `o.x = 9` was right because it lowers to `Expr::PutValueSet` (carries `strict`). Only the spellings that lower to `Expr::PropertySet` -- compound and logical assignment, for-of heads, expression-position destructuring targets -- had no answer to give. The same hole existed on `Expr::IndexSet`'s OBJECT-by-name arms, which #9426 left behind when it carried the flag to that node's array element lanes. The flag comes from the CONTEXT, exactly as #9426 did for `Expr::IndexSet`: `ctx.is_strict_fn` at the ordinary dispatch, `PutValueSet::strict` at the two sites that synthesize a `PropertySet` from a `PutValue`. Deliberately not a new HIR field: `Expr::PropertySet` has 181 mentions across the workspace (119 constructions, 54 in production code), and a large minority live in collectors and transform passes that REBUILD an existing node with no strictness context to copy -- exactly where a wrong default hides. `FnCtx::is_strict_fn` is already the audited answer for the enclosing code (`Function::is_strict`, `Expr::Closure::is_strict`, `Module::init_is_strict` from #9458, and a hard `true` for class methods). Sloppy stores route to `js_put_value_set(target, key, value, receiver, 0)` -- the receiver-aware [[Set]] sloppy `o.x = v` has always used -- so the spellings agree instead of diverging by lane. The class-field fast arm is preserved through `try_lower_sloppy_class_field_store` (#7288/#5094), whose #5093 inline precheck declines every receiver whose store could be rejected, so that arm is mode-independent and only its miss needed a sloppy tail. Strict lowering is byte-identical to before. Two IR tests moved, both because their fixture builders hard-code `is_strict: false` while their subject (the typed-feedback PropertySet site, the property-id store ABI) lives on the strict lane -- the same expectation move #9458 made when `Module::init_is_strict` landed. Each is now asserted on the strict lane AND given a sloppy twin, so neither invariant is pinned on only one of two tails. Verified byte-identical to `node --experimental-strip-types` on test-files/test_gap_9459_property_set_strictness.cts (19 lines differed on unfixed origin/main); perry-codegen --lib 1383 passed / 0 failed; targeted IR suites (typed_feedback, native_proof_regressions, scalar_replaced_slot_roots, class_field_store_pointer_test, shadow_slot_hygiene) 334 passed / 0 failed. Not changed, both pre-existing on main and documented in the fixture: `caller`/`arguments` keep their `js_object_set_field_by_name` route in both modes (that entry's poisoned-accessor handling is not a Throw-flag decision), and strict `+=` against an INHERITED rejecting receiver still skips the prototype walk -- a missing walk rather than a missing Throw flag, filed as #9495. --- .../9459-sloppy-property-set-strictness.md | 95 ++ crates/perry-codegen/src/expr/dispatch.rs | 13 +- crates/perry-codegen/src/expr/index_set.rs | 68 ++ crates/perry-codegen/src/expr/property_set.rs | 154 ++- .../perry-codegen/src/expr/proxy_reflect.rs | 8 + .../tests/native_proof_regressions.rs | 128 ++- crates/perry-codegen/tests/typed_feedback.rs | 87 +- .../test_gap_9459_property_set_strictness.cts | 965 ++++++++++++++++++ 8 files changed, 1510 insertions(+), 8 deletions(-) create mode 100644 changelog.d/9459-sloppy-property-set-strictness.md create mode 100644 test-files/test_gap_9459_property_set_strictness.cts diff --git a/changelog.d/9459-sloppy-property-set-strictness.md b/changelog.d/9459-sloppy-property-set-strictness.md new file mode 100644 index 0000000000..099046e4e1 --- /dev/null +++ b/changelog.d/9459-sloppy-property-set-strictness.md @@ -0,0 +1,95 @@ +### Fixed + +- **A rejected `o.x += 1` / `for (o.x of …)` / `[o.x] = arr` no longer throws in + sloppy code.** + + ```js + // sloppy (.cts, no "use strict") + const o = {x:1}; Object.freeze(o); + o.x += 1; // node: silent Perry: TypeError + for (o.x of [7]) {} // node: silent Perry: TypeError + [o.x] = [7]; // node: silent Perry: TypeError (expression position) + o.x++; // node: silent Perry: silent (correct — Expr::PropertyUpdate) + o.x = 9; // node: silent Perry: silent (correct — Expr::PutValueSet) + ``` + + ES2024 §6.2.5.7 (`PutValue`) performs `Set(O, P, V, Throw)` with + `Throw = IsStrictReference(ref)`, and §10.1.9 (`OrdinarySet`) reports `false` + — not a throw — for a non-writable own or inherited data property, an + accessor with no setter, and a new property on a non-extensible object. The + reference's own strictness is what turns that `false` into a `TypeError`. + + This is the ordinary-object mirror of #9394 (arrays, fixed by #9426) and the + opposite direction from #9422 (an *under*-throw in strict code). A CommonJS + bundle is sloppy from top to bottom, so this was a hard failure — a program + node runs to completion stopped with a spurious `TypeError`. + + Root cause: `Expr::PropertySet` carries **no strictness field at all**, and + its codegen tail reaches `js_typed_feedback_object_set_field_by_name_fast` → + `js_object_set_field_by_name`, which has no `strict` parameter and rejects by + throwing unconditionally. `o.x++` was already right because it lowers to + `Expr::PropertyUpdate`, which carries `ctx.current_strict`; `o.x = 9` was + already right because it lowers to `Expr::PutValueSet`, which carries + `strict`. Only the spellings that lower to `Expr::PropertySet` — compound and + logical assignment, `for`-of heads, and expression-position destructuring + targets — had no answer to give. + + The flag comes from the **context**, exactly as #9426 did for + `Expr::IndexSet`: `ctx.is_strict_fn` at the ordinary dispatch, and + `PutValueSet::strict` at the two sites that synthesize a `PropertySet` from a + `PutValue`. That is deliberately not a new HIR field — `Expr::PropertySet` has + 181 mentions across the workspace (119 constructions, 54 of them in production + code), and a large minority of those live in collectors and transform passes + that *rebuild* an existing node with no strictness context to copy from. A + field would have needed a default at each of those, which is precisely where a + wrong answer hides. `FnCtx::is_strict_fn` is already the audited answer for + the enclosing code: `Function::is_strict`, `Expr::Closure::is_strict`, + `Module::init_is_strict` (#9458), and a hard `true` for class methods. + + Sloppy stores route to `js_put_value_set(target, key, value, receiver, 0)` — + the receiver-aware `[[Set]]` that sloppy `o.x = v` has always used — so the + three spellings now agree instead of diverging by lane. The class-field fast + arm is preserved through `try_lower_sloppy_class_field_store` (#7288/#5094), + whose #5093 inline precheck declines every receiver whose store could be + rejected, so the fast path is mode-independent and only its miss needed a + sloppy-correct tail. Strict lowering is byte-identical to before. + + - `crates/perry-codegen/src/expr/dispatch.rs` — `Expr::PropertySet` passes + `ctx.is_strict_fn`, the twin of the `Expr::IndexSet` line above it. + - `crates/perry-codegen/src/expr/proxy_reflect.rs` — the two `PutValueSet` + routes into `property_set::lower` pass the reference's own `strict`. + - `crates/perry-codegen/src/expr/property_set.rs` — `lower` takes + `assignment_strict`; the `arr.length` arm (`js_array_set_length_strict`) and + the class-field arms (`js_class_field_set_ic` / + `js_class_field_set_fallback`) are strict-only; a new + `lower_sloppy_property_set_by_name` emits the `js_put_value_set(…, 0)` tail + with the same #7154 receiver-rooting window and the same nullish-receiver + guard the strict tail uses (`undefined.x = 1` is a `TypeError` in both + modes — `GetValue` on the base runs before `PutValue` consults `Throw`). + - `test-files/test_gap_9459_property_set_strictness.cts` — `+=`, `-=`, `*=`, + `&&=`, `||=`, `??=`, `for`-of heads (named and computed), `[o.x] = arr` in + statement *and* expression position, `[o[k]] = arr`, `({a: o.x} = obj)`, + against frozen / sealed / non-writable own / inherited non-writable / + getter-only own and inherited / inherited-setter / `preventExtensions` / + frozen class-field / frozen `arr.length` receivers — **both modes**, with + accepted-store and short-circuit controls so a fix that simply stopped + storing would fail. + + Two things this deliberately does not change, both pre-existing on `main` and + both visible in the fixture's own comments: + + - `caller` / `arguments` keep their `js_object_set_field_by_name` route in + both modes. `PutValueSet` sends those two names here specifically to reach + that entry's poisoned-accessor handling, which is not a `Throw`-flag + decision. + - `+=` against a receiver whose rejection lives on the **prototype** (an + inherited non-writable data property, an inherited getter-only accessor, an + inherited setter) is still wrong in **strict** code: the strict tail does an + own-property store and never runs `OrdinarySetWithOwnDescriptor`'s + prototype walk, so the setter does not fire and an own property is created. + That is a missing prototype walk rather than a missing `Throw` flag, it is + wrong on unfixed `main` in both modes, and fixing it means retargeting the + typed-feedback store site that #7480/#5093 gate with their own IR tests. + Filed as #9495. The sloppy half becomes correct here as a side effect of + routing to `js_put_value_set`, and the fixture pins the strict `=` twins so + that change has a baseline. diff --git a/crates/perry-codegen/src/expr/dispatch.rs b/crates/perry-codegen/src/expr/dispatch.rs index e04d1edcba..89ada2b9ed 100644 --- a/crates/perry-codegen/src/expr/dispatch.rs +++ b/crates/perry-codegen/src/expr/dispatch.rs @@ -64,7 +64,18 @@ pub(crate) fn lower_expr(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { let strict = ctx.is_strict_fn; super::index_set::lower(ctx, expr, value_discarded, strict) } - Expr::PropertySet { .. } => super::property_set::lower(ctx, expr), + Expr::PropertySet { .. } => { + // #9459, twin of the `IndexSet` line above (#9426): `Expr::PropertySet` + // carries no strictness of its own, so the assignment's `Throw` flag + // comes from the CONTEXT it is lowered in. Every spelling that reaches + // this arm -- `o.x += 1`, `for (o.x of it)`, `[o.x] = arr` -- is a + // `PutValue` on a property reference whose strictness is the enclosing + // code's (ES2024 SS6.2.5.7). A `PutValueSet` that routes here instead + // carries the reference's own flag and passes it explicitly + // (`expr/proxy_reflect.rs`). + let strict = ctx.is_strict_fn; + super::property_set::lower(ctx, expr, strict) + } Expr::PropertyGet { .. } => super::property_get::lower(ctx, expr), Expr::Conditional { .. } => super::conditional::lower(ctx, expr), Expr::ArrayPush { .. } | Expr::ArrayPushSpread { .. } => { diff --git a/crates/perry-codegen/src/expr/index_set.rs b/crates/perry-codegen/src/expr/index_set.rs index 2366d4d775..48bdd808f0 100644 --- a/crates/perry-codegen/src/expr/index_set.rs +++ b/crates/perry-codegen/src/expr/index_set.rs @@ -355,6 +355,65 @@ fn lower_array_index_set_via_runtime_key( ) } +/// #9459: the SLOPPY object-by-name tail for `Expr::IndexSet`. +/// +/// The two string-key arms below reach `js_typed_feedback_object_set_field_by_name` +/// → `js_object_set_field_by_name`, which has no `strict` parameter and rejects a +/// non-writable slot by throwing. #9426 carried `assignment_strict` to the ARRAY +/// element lanes; the object-by-name lanes on the same node still threw, so +/// `for (o[k] of …)` and `o[k] += 1` on a frozen object stopped a sloppy program +/// node runs to completion. +/// +/// `Set(O, ToPropertyKey(k), V, false)` is `js_put_value_set(target, key, value, +/// receiver, 0)` — the same entry sloppy `o[k] = v` already reaches, because +/// `put_value_index_fast_path` keeps statically-known string keys off this file +/// entirely (`expr/proxy_reflect.rs`). Routing here makes the two spellings agree. +/// +/// Rooting is the #7639/#7201 window the strict arms open: receiver AND key are +/// live across `value`'s lowering, which is arbitrary user code and can drive an +/// evacuating minor. `target` and `receiver` are one evaluation of the base, so +/// the single lowered box fills both operand slots. +fn lower_sloppy_object_index_set( + ctx: &mut FnCtx<'_>, + object: &Expr, + index: &Expr, + value: &Expr, +) -> Result { + rooting::with_operands_rooted_across( + ctx, + &[object, index], + &[value], + |ctx| { + lower_value_for_dynamic_index_set( + ctx, + value, + "index_set.sloppy_object_value_bits", + "sloppy_object_index_set_helper_edge", + ) + }, + |ctx, vals, (val_double, _val_bits)| { + let (obj_box, key_box) = (vals[0].clone(), vals[1].clone()); + let obj_bits = ctx.block().bitcast_double_to_i64(&obj_box); + // Same guard the strict arms emit: `undefined[k] = 1` is a TypeError + // in BOTH modes -- `GetValue` on the base runs before `PutValue` + // ever consults `Throw`. + super::property_set::emit_nullish_write_guard(ctx, &obj_bits, "index", "iset.sloppy"); + let _ = ctx.block().call( + DOUBLE, + "js_put_value_set", + &[ + (DOUBLE, &obj_box), + (DOUBLE, &key_box), + (DOUBLE, &val_double), + (DOUBLE, &obj_box), + (I32, "0"), + ], + ); + Ok(val_double) + }, + ) +} + pub(crate) fn lower( ctx: &mut FnCtx<'_>, expr: &Expr, @@ -1399,6 +1458,11 @@ pub(crate) fn lower( // `obj_box`. Root it across the evaluation and re-read below. // The key is a literal, so it is not an operand here at all — // it is interned into the string pool below. + // + // #9459: strict only -- see `lower_sloppy_object_index_set`. + if !assignment_strict { + return lower_sloppy_object_index_set(ctx, object, index, value); + } return rooting::with_operands_rooted_across( ctx, &[object.as_ref()], @@ -1459,6 +1523,10 @@ pub(crate) fn lower( ); } if is_string_expr(ctx, index) { + // #9459: strict only -- see `lower_sloppy_object_index_set`. + if !assignment_strict { + return lower_sloppy_object_index_set(ctx, object, index, value); + } // #7154: see the literal-key arm above, plus the KEY, which // sits in the same window. A non-literal string key is an // ordinary heap string with no registered root of its own, so diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index 6f32c5f57b..6102a6c420 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -557,7 +557,15 @@ fn lower_runtime_property_set_by_name( object: &Expr, property: &str, value: &Expr, + // #9459: `js_object_set_field_by_property_id` resolves the dispatch id and + // hands the key to `js_object_set_field_by_name`, which has no `strict` + // parameter and rejects by throwing. Correct for a strict `PutValue`, wrong + // for sloppy. + assignment_strict: bool, ) -> Result { + if !assignment_strict { + return lower_sloppy_property_set_by_name(ctx, object, property, value); + } // #7154: root the receiver across the value's evaluation, which allocates. // The group re-reads it as part of emitting the store, so no register of // the receiver exists across the window. @@ -576,6 +584,67 @@ fn lower_runtime_property_set_by_name( }) } +/// #9459: the SLOPPY terminal store for `Expr::PropertySet`. +/// +/// `Set(O, P, V, false)` -- ordinary `[[Set]]` with the receiver, and a +/// rejection (frozen / sealed / non-writable own or inherited data property / +/// getter-only accessor / non-extensible new key) reported as `false` and +/// discarded rather than thrown. That is exactly `js_put_value_set(target, key, +/// value, receiver, 0)`, the entry sloppy `o.x = v` has always used through +/// `Expr::PutValueSet`; routing here makes `o.x += 1`, `for (o.x of it)` and +/// `[o.x] = arr` agree with it instead of throwing where node is silent. +/// +/// `target` and `receiver` are the same expression, evaluated ONCE -- the +/// property reference's base is one evaluation, and `with_operands_rooted` +/// hands the single lowered box to both operand slots. +/// +/// Rooting is the #7154 window the strict tail also opens: the receiver is live +/// across `value`'s lowering, which is arbitrary user code and can drive an +/// evacuating minor. +fn lower_sloppy_property_set_by_name( + ctx: &mut FnCtx<'_>, + object: &Expr, + property: &str, + value: &Expr, +) -> Result { + rooting::with_operands_rooted_across( + ctx, + &[object], + &[value], + |ctx| { + lower_value_for_dynamic_property_set( + ctx, + value, + "property_set.sloppy_dynamic_value_bits", + "sloppy_property_set_helper_edge", + ) + }, + |ctx, vals, (val_double, _val_bits)| { + let obj_box = vals[0].clone(); + let key_idx = ctx.strings.intern(property); + let key_handle_global = format!("@{}", ctx.strings.entry(key_idx).handle_global); + let obj_bits = ctx.block().bitcast_double_to_i64(&obj_box); + // The nullish receiver check is the same one the strict tail emits: + // `undefined.x = 1` is a TypeError in BOTH modes (GetValue on the + // base runs before PutValue's Throw flag is ever consulted). + emit_nullish_write_guard(ctx, &obj_bits, property, "pset_sloppy"); + let key_box = ctx.block().load(DOUBLE, &key_handle_global); + let _ = ctx.block().call( + DOUBLE, + "js_put_value_set", + &[ + (DOUBLE, &obj_box), + (DOUBLE, &key_box), + (DOUBLE, &val_double), + (DOUBLE, &obj_box), + (I32, "0"), + ], + ); + Ok(val_double) + }, + ) +} + fn lower_value_for_dynamic_property_set( ctx: &mut FnCtx<'_>, value: &Expr, @@ -638,7 +707,19 @@ pub(crate) fn emit_nullish_write_guard( ctx.current_block = ok_idx; } -pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { +/// Lower an `Expr::PropertySet`. +/// +/// `assignment_strict` is the assignment's own `Throw` flag (ES2024 SS6.2.5.7 +/// `PutValue` calls `Set(O, P, V, Throw)` with `Throw = IsStrictReference`). +/// #9459: the HIR node carries no strictness, so it comes from the caller -- +/// `ctx.is_strict_fn` for the ordinary dispatch (`expr/dispatch.rs`, the same +/// source `Expr::IndexSet` uses since #9426), and `PutValueSet::strict` for the +/// two routes that synthesize a `PropertySet` from a `PutValue` +/// (`expr/proxy_reflect.rs`). A rejected SLOPPY `[[Set]]` is a silent no-op, so +/// every arm below whose runtime entry rejects by THROWING is strict-only; the +/// sloppy twin of each is the strictness-aware `js_put_value_set(..., 0)` that +/// the surrounding `PutValueSet` lowering already uses for `o.x = v`. +pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr, assignment_strict: bool) -> Result { match expr { Expr::PropertySet { object, @@ -672,7 +753,20 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // (route to js_array_set_length when the target is registered as // an array). Deliberately out of scope here; the static-typed // case covers the issue's repro. - if property == "length" && crate::type_analysis::is_array_expr(ctx, object) { + // + // #9459: strict only. `js_array_set_length_strict` is named for the + // `Throw` flag it hard-codes -- `Set(O, "length", n, true)`. A SLOPPY + // `arr.length` write that `OrdinarySet` rejects (frozen array, or an + // explicit `writable: false` on `length`) must be a silent no-op, so + // sloppy falls through to the generic `js_put_value_set(..., 0)` tail + // below. That is already where sloppy `arr.length = 0` goes today -- + // `put_value_static_property_fast_path` refuses this arm for sloppy + // references (`expr/proxy_reflect.rs`), and #9422's fixture pins the + // result -- so the two spellings agree rather than diverging by lane. + if assignment_strict + && property == "length" + && crate::type_analysis::is_array_expr(ctx, object) + { // #7637: this arm had NO store-operand guard, while every other // `PropertySet` arm in this file has had one since #7154. It is // the same window: `arr.length = f()` lowers the receiver first @@ -906,7 +1000,13 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { if class_has_computed_runtime_members(ctx, &class_name) && !ctx.is_static_class_this(object) { - return lower_runtime_property_set_by_name(ctx, object, property, value); + return lower_runtime_property_set_by_name( + ctx, + object, + property, + value, + assignment_strict, + ); } let setter_key = (class_name.clone(), format!("__set_{}", property)); // STATIC accessors compile under the static (no-`this`) @@ -920,7 +1020,11 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { if let Some(fn_name) = ctx.methods.get(&setter_key).cloned() { if proven_class_name.is_none() { return lower_runtime_property_set_by_name( - ctx, object, property, value, + ctx, + object, + property, + value, + assignment_strict, ); } return with_class_store_operands( @@ -938,6 +1042,35 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { ); } } + // #9459: SLOPPY code stops here. Every class-field arm below + // terminates in `js_class_field_set_ic` / + // `js_class_field_set_fallback`, whose miss path is + // `js_object_set_field_by_name` -- no `strict` parameter, rejects + // by throwing. That is the same reason + // `put_value_static_property_fast_path` bars sloppy references + // from this route for `PutValueSet` (#6542), and the recovery is + // the same one #7288/#5094 built for that lowering: the #5093 + // inline precheck declines every receiver whose store could be + // REJECTED (frozen, descriptor-bearing, wrong class or keys token, + // accessor in the chain), so its fast arm is mode-independent and + // only its miss needed a sloppy-correct tail. A decline lands on + // the same `js_put_value_set(..., 0)` the generic sloppy tail uses, + // so the two are one behaviour with two speeds. + // + // The setter-dispatch arm above is deliberately AHEAD of this: a + // compiled `__set_` accessor runs in both modes, and a + // setter that throws does so because of its own body, not because + // of the assignment's `Throw` flag. + if !assignment_strict { + if matches!(object.as_ref(), Expr::LocalGet(_) | Expr::This) { + if let Some(result) = + try_lower_sloppy_class_field_store(ctx, object, property, value)? + { + return Ok(result); + } + } + return lower_sloppy_property_set_by_name(ctx, object, property, value); + } // Fast path: known class instance + plain instance field. // The runtime guard checks the receiver's class/shape and // descriptor state before this block touches the raw slot. @@ -1659,6 +1792,19 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } } } + // #9459: the generic SLOPPY tail. The strict tail below ends in + // `js_typed_feedback_object_set_field_by_name_fast`, whose underlying + // `js_object_set_field_by_name` has no `strict` parameter and rejects + // by throwing; sloppy `PutValue` must discard the rejection instead. + // + // `caller`/`arguments` are excluded in BOTH modes: those two names are + // routed here from `PutValueSet` specifically to reach + // `js_object_set_field_by_name`'s poisoned-accessor handling, which is + // not a `Throw`-flag decision, and diverting them would change + // behaviour this issue is not about. + if !assignment_strict && !matches!(property.as_str(), "caller" | "arguments") { + return lower_sloppy_property_set_by_name(ctx, object, property, value); + } // #7154: the value expression can collect, and an evacuating minor // inside it relocates the receiver out from under `obj_box` -- // `obj.k = f()` then writes `k` into abandoned from-space memory diff --git a/crates/perry-codegen/src/expr/proxy_reflect.rs b/crates/perry-codegen/src/expr/proxy_reflect.rs index 8bb1a87cc4..23ea00e5a4 100644 --- a/crates/perry-codegen/src/expr/proxy_reflect.rs +++ b/crates/perry-codegen/src/expr/proxy_reflect.rs @@ -1905,6 +1905,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { property: property.clone(), value: value.clone(), }, + // #9459: the reference's own `Throw` flag, not the + // enclosing function's -- module init is a synthetic + // function whose strictness is `Module::init_is_strict` + // (#9458), and `PutValueSet` already carries the right + // answer for every reference it desugars. + *strict, ); } } @@ -1918,6 +1924,8 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { property, value: value.clone(), }, + // #9459: see the `caller`/`arguments` route above. + *strict, ); } // #7288: sloppy code is barred from the class-field route above diff --git a/crates/perry-codegen/tests/native_proof_regressions.rs b/crates/perry-codegen/tests/native_proof_regressions.rs index 7d8e0c5330..2fc630df19 100644 --- a/crates/perry-codegen/tests/native_proof_regressions.rs +++ b/crates/perry-codegen/tests/native_proof_regressions.rs @@ -150,6 +150,29 @@ fn module_with_classes_and_params( } } +/// #9459: the same module, with `probe` (and module init) STRICT. +/// +/// `module_with_classes_and_params` hard-codes `is_strict: false`, which used to +/// be invisible: `Expr::PropertySet` carried no strictness and every store took +/// one lowering. It now takes the assignment's own `Throw` flag from +/// `FnCtx::is_strict_fn`, so a test whose subject is the STRICT store lane has +/// to say so — the same expectation move #9458 made across the codegen test +/// files when `Module::init_is_strict` was introduced. +fn strict_module_with_classes_and_params( + name: &str, + classes: Vec, + params: Vec, + return_type: Type, + body: Vec, +) -> Module { + let mut module = module_with_classes_and_params(name, classes, params, return_type, body); + module.init_is_strict = true; + for function in &mut module.functions { + function.is_strict = true; + } + module +} + fn compile_ir(name: &str, body: Vec) -> String { compile_ir_with_opts(name, body, empty_opts()) } @@ -6913,7 +6936,13 @@ fn artifact_records_array_push_value_bits_before_slot_store() { #[test] fn artifact_records_dynamic_property_set_value_bits_before_helper() { - let module = module_with_classes_and_params( + // #9459: STRICT. The subject is a representation invariant -- the RHS stays + // in `js_value_bits` until the runtime helper edge boxes it -- and it is + // asserted here on the strict tail. The sloppy tail is a different runtime + // entry (`js_put_value_set(..., 0)`, so a rejected sloppy `[[Set]]` is a + // silent no-op) and gets its own twin below, because a representation + // invariant that holds on only one of two tails is not an invariant. + let module = strict_module_with_classes_and_params( "artifact_property_set_slot_js_value_bits.ts", Vec::new(), vec![param(1, "obj", Type::Any), param(2, "value", Type::Any)], @@ -6944,6 +6973,45 @@ fn artifact_records_dynamic_property_set_value_bits_before_helper() { ); } +/// #9459: the sloppy twin of the test above. +/// +/// Same module, `is_strict: false`, so the store takes the sloppy tail. The +/// representation contract is identical -- the RHS is lowered by +/// `lower_value_for_dynamic_property_set` and stays `js_value_bits` until the +/// helper edge -- only the consumer/boxed-at labels and the runtime entry differ. +#[test] +fn artifact_records_sloppy_dynamic_property_set_value_bits_before_helper() { + let module = module_with_classes_and_params( + "artifact_property_set_slot_js_value_bits_sloppy.ts", + Vec::new(), + vec![param(1, "obj", Type::Any), param(2, "value", Type::Any)], + Type::Number, + vec![ + Stmt::Expr(Expr::PropertySet { + object: Box::new(local(1)), + property: "field".to_string(), + value: Box::new(local(2)), + }), + Stmt::Return(Some(int(0))), + ], + ); + + let artifact = compile_artifact_json_for_module(module); + let records = artifact["records"].as_array().unwrap(); + assert!( + records.iter().any(|record| { + record["expr_kind"] == "PropertySet" + && record["consumer"] == "property_set.sloppy_dynamic_value_bits" + && record["native_rep_name"] == "js_value_bits" + && record["llvm_ty"] == "i64" + && record["native_value_state"] == "region_local" + && record["access_mode"].is_null() + && record_has_note(record, "boxed_at=sloppy_property_set_helper_edge") + }), + "expected sloppy dynamic property-set RHS to stay as js_value_bits before the helper edge:\n{artifact:#}" + ); +} + #[test] fn artifact_records_dynamic_index_set_value_bits_before_helper() { let module = module_with_classes_and_params( @@ -14423,8 +14491,13 @@ fn scalar_method_int32_bitwise_rejects_unproven_or_unsigned_shapes() { #[test] fn static_property_access_on_computed_class_uses_property_id_wrappers() { + // #9459: STRICT. The property-id store ABI + // (`js_object_set_field_by_property_id` -> `js_object_set_field_by_name`) + // rejects a non-writable slot by throwing, which is correct only for a + // strict `PutValue`; the sloppy tail is asserted by the twin below. The + // READ side is strictness-independent and is asserted in both. let dynamic = class_with_computed_member(141, "DynamicShape", vec![]); - let module = module_with_classes_and_params( + let module = strict_module_with_classes_and_params( "property_id_static_access.ts", vec![dynamic], vec![ @@ -14457,6 +14530,57 @@ fn static_property_access_on_computed_class_uses_property_id_wrappers() { ); } +/// #9459: the sloppy twin of the test above. +/// +/// A sloppy store must NOT take the property-id ABI: that route ends in +/// `js_object_set_field_by_name`, which has no `strict` parameter and throws on +/// a rejected `[[Set]]`. It goes to `js_put_value_set(..., 0)` instead. The READ +/// keeps the property-id ABI in both modes -- strictness is a property of +/// `PutValue`, not of `GetValue` -- and asserting it here is what keeps this +/// test from passing on an empty program. +#[test] +fn sloppy_static_property_store_on_computed_class_avoids_property_id_setter() { + let dynamic = class_with_computed_member(141, "DynamicShape", vec![]); + let module = module_with_classes_and_params( + "property_id_static_access_sloppy.ts", + vec![dynamic], + vec![ + param(1, "obj", Type::Named("DynamicShape".to_string())), + param(2, "value", Type::Number), + ], + Type::Number, + vec![ + Stmt::Expr(Expr::PropertySet { + object: Box::new(local(1)), + property: "score".to_string(), + value: Box::new(local(2)), + }), + Stmt::Return(Some(Expr::PropertyGet { + byte_offset: 0, + object: Box::new(local(1)), + property: "score".to_string(), + })), + ], + ); + + let ir = compile_ir_for_module_with_opts(module, empty_opts()).unwrap(); + assert!( + !ir.contains("call void @js_object_set_field_by_property_id"), + "a sloppy store must not reach the throwing property-id setter:\n{ir}" + ); + // The CALL, not the symbol -- `js_put_value_set` is `declare`d in every + // module, so matching the bare name would pass on a program with no store. + assert!( + ir.contains("call double @js_put_value_set("), + "a sloppy store should reach the strictness-aware [[Set]]:\n{ir}" + ); + assert!( + ir.contains("call double @js_object_get_field_by_property_id_f64"), + "computed-member class static property reads keep the property-id ABI in \ + sloppy code too:\n{ir}" + ); +} + #[test] fn static_name_method_fallback_uses_rodata_method_id_wrapper() { let module = module_with_classes_and_params( diff --git a/crates/perry-codegen/tests/typed_feedback.rs b/crates/perry-codegen/tests/typed_feedback.rs index b1d5bb35b6..c84fd1cd02 100644 --- a/crates/perry-codegen/tests/typed_feedback.rs +++ b/crates/perry-codegen/tests/typed_feedback.rs @@ -199,6 +199,25 @@ fn module(name: &str, params: Vec, return_type: Type, body: Vec) -> module_with_classes(name, Vec::new(), params, return_type, body) } +/// #9459: the same module with `probe` (and module init) STRICT. +/// +/// `module_with_classes` hard-codes `is_strict: false`, which used to be +/// invisible: `Expr::PropertySet` carried no strictness and every store took one +/// lowering. It now takes the assignment's own `Throw` flag from +/// `FnCtx::is_strict_fn`, and only the STRICT store tail carries the +/// typed-feedback `PropertySet` site -- the sloppy tail is +/// `js_put_value_set(..., 0)`, so a rejected sloppy `[[Set]]` stays a silent +/// no-op. A test whose subject is that site has to ask for the lane it lives on, +/// the same expectation move #9458 made when `Module::init_is_strict` landed. +fn strict_module(name: &str, params: Vec, return_type: Type, body: Vec) -> Module { + let mut module = module_with_classes(name, Vec::new(), params, return_type, body); + module.init_is_strict = true; + for function in &mut module.functions { + function.is_strict = true; + } + module +} + fn module_with_classes( name: &str, classes: Vec, @@ -343,7 +362,11 @@ fn typed_feedback_instruments_property_and_method_boundaries() { // later tests in this binary never observe the changed environment. let _lock = env_lock(); let _env = EnvVarGuard::set("PERRY_TYPED_FEEDBACK", Some("1")); - let ir = ir_for(module( + // #9459: STRICT -- the `object_set_by_name_guard` site and the + // `js_typed_feedback_object_set_field_by_name_fast` dispatcher this asserts + // live on the strict store tail. See `strict_module`. The sloppy tail is + // asserted by `sloppy_property_set_uses_strictness_aware_put_value` below. + let ir = ir_for(strict_module( "typed_feedback_property.ts", vec![param(1, "obj", Type::Any)], Type::Any, @@ -386,6 +409,68 @@ fn typed_feedback_instruments_property_and_method_boundaries() { assert!(ir.contains("call void @js_typed_feedback_observe_property_get")); } +/// #9459: the sloppy twin of the boundaries test above. +/// +/// A sloppy `o.x = v` must not reach `js_object_set_field_by_name` -- it has no +/// `strict` parameter and rejects a non-writable slot by throwing, where sloppy +/// `PutValue` discards the rejection. It goes to `js_put_value_set(..., 0)`. +/// +/// The GET boundary is asserted too, and is deliberately unchanged: strictness +/// is a property of `PutValue`, not of `GetValue`. That assertion is also what +/// keeps this test from passing on an empty program -- every other check here is +/// a negative. +#[test] +fn sloppy_property_set_uses_strictness_aware_put_value() { + let _lock = env_lock(); + let _env = EnvVarGuard::set("PERRY_TYPED_FEEDBACK", Some("1")); + let ir = ir_for(module( + "typed_feedback_property_sloppy.ts", + vec![param(1, "obj", Type::Any)], + Type::Any, + vec![ + Stmt::Expr(Expr::PropertySet { + object: Box::new(Expr::LocalGet(1)), + property: "x".to_string(), + value: Box::new(Expr::Number(1.0)), + }), + Stmt::Return(Some(Expr::PropertyGet { + byte_offset: 0, + object: Box::new(Expr::LocalGet(1)), + property: "x".to_string(), + })), + ], + )); + + // Match the CALL, not the symbol: every runtime entry is `declare`d in + // every module, so `contains("js_put_value_set")` would pass on a module + // that never stores and `contains("...set_field_by_name_fast")` would fail + // on one that never does either. + assert!( + ir.contains("call double @js_put_value_set("), + "a sloppy property store should reach the strictness-aware [[Set]]:\n{ir}" + ); + assert!( + !ir.contains("call void @js_typed_feedback_object_set_field_by_name_fast("), + "the strict typed-feedback store dispatcher must not be CALLED on a \ + sloppy store -- its underlying setter throws on a rejected write:\n{ir}" + ); + assert!( + !ir.contains("object_set_by_name_guard"), + "no typed-feedback SET site is registered on the sloppy tail:\n{ir}" + ); + // ANTI-VACUITY: the reads are untouched by strictness and must still be + // instrumented, so the negatives above are about the store lane and not + // about an empty module. + assert!( + ir.contains("object_get_by_name_guard"), + "the property READ boundary is strictness-independent and must remain:\n{ir}" + ); + assert!( + ir.contains("js_object_get_field_by_name_f64"), + "the property read itself must still be lowered:\n{ir}" + ); +} + /// The negative twin of the test above, and the whole of #7480 step 4's second /// half: a DEFAULT build emits none of the pure-recording helpers. /// diff --git a/test-files/test_gap_9459_property_set_strictness.cts b/test-files/test_gap_9459_property_set_strictness.cts new file mode 100644 index 0000000000..3729dfc0fc --- /dev/null +++ b/test-files/test_gap_9459_property_set_strictness.cts @@ -0,0 +1,965 @@ +// #9459: `Expr::PropertySet` carries no strictness field, so every assignment +// form that lowers to it threw on a rejected `[[Set]]` in SLOPPY code, where +// node is silent. +// +// ES2024 SS6.2.5.7 (PutValue) performs `Set(O, P, V, Throw)` with +// `Throw = IsStrictReference(ref)`, and SS10.1.9 (OrdinarySet) reports `false` +// -- not a throw -- for a non-writable own or inherited data property, an +// accessor with no setter, and a new property on a non-extensible object. The +// reference's strictness is what turns that `false` into a TypeError. +// +// Three spellings of the same store disagreed on main: +// +// o.x = 9 -> `Expr::PutValueSet` (carries `strict`) CORRECT +// o.x++ -> `Expr::PropertyUpdate` (carries `strict`) CORRECT +// o.x += 1 -> `Expr::PropertySet` (carries NOTHING) THREW +// for (o.x of) -> `Expr::PropertySet` THREW +// [o.x] = arr -> `Expr::PropertySet` (expression position) THREW +// +// This is the object-path mirror of #9394 (arrays, fixed by #9426) and the +// opposite direction from #9422 (which was an UNDER-throw in strict code). +// +// This file is `.cts`, so it is a CommonJS script in BOTH runtimes: `sloppyArm` +// is sloppy code and `strictArm` opts in with its own directive prologue. +// BOTH ARMS ARE ASSERTED. Asserting only the sloppy no-op is what let #9422 +// through, and asserting only the strict throw is what let this through. +// +// The two arms are textual duplicates on purpose: a function inherits the +// strictness of the code it is DEFINED in, never its caller's, so a shared +// helper would test one mode twice. Only the mode prefix and the directive +// differ. +// +// Companions: test_gap_9422_strict_object_store_strictness.cts (the `=` lane), +// test_gap_9394_array_element_store_strictness.cts (the array element lane), +// test_gap_9423_module_init_strictness.ts (the ESM always-strict half). + +function report(name: string, threw: boolean, ...rest: unknown[]): void { + console.log(name, threw ? "TypeError" : "silent", ...rest); +} + +function hasOwn(value: any, key: PropertyKey): boolean { + return Object.prototype.hasOwnProperty.call(value, key); +} + +function nonWritableProto(): any { + const proto: any = {}; + Object.defineProperty(proto, "x", { + configurable: true, + enumerable: true, + value: 10, + writable: false, + }); + return proto; +} + +function getterOnlyProto(): any { + const proto: any = {}; + Object.defineProperty(proto, "x", { + configurable: true, + get() { + return 20; + }, + }); + return proto; +} + +function setterProto(calls: any[]): any { + const proto: any = {}; + Object.defineProperty(proto, "x", { + configurable: true, + get() { + return 30; + }, + set(value: any) { + calls.push(value); + }, + }); + return proto; +} + +function readOnlyOwn(initial: any): any { + const o: any = {}; + Object.defineProperty(o, "x", { + configurable: true, + enumerable: true, + value: initial, + writable: false, + }); + return o; +} + +function getterOnlyOwn(): any { + const o: any = {}; + Object.defineProperty(o, "x", { + configurable: true, + enumerable: true, + get() { + return 40; + }, + }); + return o; +} + +class Cell { + x: number; + constructor(x: number) { + this.x = x; + } +} + +function frozenCell(): any { + const c = new Cell(1); + Object.freeze(c); + return c; +} + +function sloppyArm(): void { + let threw = false; + + // ---- compound assignment, every operator, on a frozen own data property ---- + const plus: any = { x: 1 }; + Object.freeze(plus); + threw = false; + try { + plus.x += 1; + } catch { + threw = true; + } + report("sloppy frozen +=:", threw, plus.x); + + const minus: any = { x: 1 }; + Object.freeze(minus); + threw = false; + try { + minus.x -= 1; + } catch { + threw = true; + } + report("sloppy frozen -=:", threw, minus.x); + + const times: any = { x: 3 }; + Object.freeze(times); + threw = false; + try { + times.x *= 2; + } catch { + threw = true; + } + report("sloppy frozen *=:", threw, times.x); + + // Logical assignment writes only on the branch that reaches the store, so + // each operator gets a starting value that makes it write. + const andAnd: any = { x: 1 }; + Object.freeze(andAnd); + threw = false; + try { + andAnd.x &&= 5; + } catch { + threw = true; + } + report("sloppy frozen &&=:", threw, andAnd.x); + + const orOr: any = { x: 0 }; + Object.freeze(orOr); + threw = false; + try { + orOr.x ||= 5; + } catch { + threw = true; + } + report("sloppy frozen ||=:", threw, orOr.x); + + const nullish: any = { x: undefined }; + Object.freeze(nullish); + threw = false; + try { + nullish.x ??= 5; + } catch { + threw = true; + } + report("sloppy frozen ??=:", threw, nullish.x); + + // The short-circuit CONTROL: `&&=` on a falsy value never stores, so it is + // silent in both modes even on a frozen receiver. A fix that made every + // logical assignment throw would show up here. + const andShort: any = { x: 0 }; + Object.freeze(andShort); + threw = false; + try { + andShort.x &&= 5; + } catch { + threw = true; + } + report("sloppy frozen &&= short-circuit:", threw, andShort.x); + + // ---- for-of head, named and computed ---- + const forNamed: any = { x: 1 }; + Object.freeze(forNamed); + threw = false; + try { + for (forNamed.x of [7]) { + } + } catch { + threw = true; + } + report("sloppy frozen for-of head:", threw, forNamed.x); + + const forComputed: any = { x: 1 }; + Object.freeze(forComputed); + const forKey = "x"; + threw = false; + try { + for (forComputed[forKey] of [7]) { + } + } catch { + threw = true; + } + report("sloppy frozen for-of head computed:", threw, forComputed.x); + + // The computed-key twin of `+=` above. `o[k] += 1` and `for (o[k] of ...)` + // both lower to `Expr::IndexSet`, whose OBJECT-by-name lanes kept throwing + // after #9426 carried the flag to its array element lanes. + const computedPlus: any = { x: 1 }; + Object.freeze(computedPlus); + const computedKey = "x"; + threw = false; + try { + computedPlus[computedKey] += 1; + } catch { + threw = true; + } + report("sloppy frozen o[k] +=:", threw, computedPlus.x); + + // A LITERAL computed key takes a different `Expr::IndexSet` arm from a + // runtime string key, so both are asserted. + const literalPlus: any = { x: 1 }; + Object.freeze(literalPlus); + threw = false; + try { + literalPlus["x"] += 1; + } catch { + threw = true; + } + report("sloppy frozen o[\"x\"] +=:", threw, literalPlus.x); + + // And an UNTYPED key, which reaches the runtime STRING_TAG dispatch rather + // than either static arm. + const anyKeyed: any = { x: 1 }; + Object.freeze(anyKeyed); + const anyKey: any = "x"; + threw = false; + try { + anyKeyed[anyKey] += 1; + } catch { + threw = true; + } + report("sloppy frozen o[anyKey] +=:", threw, anyKeyed.x); + + // ---- destructuring assignment targets ---- + // Statement position and expression position are DIFFERENT lowerings + // (`destructuring/assignment_stmt.rs` vs `destructuring/assignment_expr.rs`), + // so both are asserted. + const arrDestr: any = { x: 1 }; + Object.freeze(arrDestr); + threw = false; + try { + [arrDestr.x] = [7]; + } catch { + threw = true; + } + report("sloppy frozen [o.x] = arr:", threw, arrDestr.x); + + const arrDestrExpr: any = { x: 1 }; + Object.freeze(arrDestrExpr); + threw = false; + try { + const seen = ([arrDestrExpr.x] = [7]); + void seen; + } catch { + threw = true; + } + report("sloppy frozen ([o.x] = arr) expr:", threw, arrDestrExpr.x); + + const objDestr: any = { x: 1 }; + Object.freeze(objDestr); + threw = false; + try { + ({ a: objDestr.x } = { a: 7 }); + } catch { + threw = true; + } + report("sloppy frozen ({a: o.x}) = obj:", threw, objDestr.x); + + const arrDestrComputed: any = { x: 1 }; + Object.freeze(arrDestrComputed); + const destrKey = "x"; + threw = false; + try { + [arrDestrComputed[destrKey]] = [7]; + } catch { + threw = true; + } + report("sloppy frozen [o[k]] = arr:", threw, arrDestrComputed.x); + + // ---- the same `+=` against every rejecting receiver shape ---- + const sealedOwn: any = { x: 1 }; + Object.seal(sealedOwn); + threw = false; + try { + sealedOwn.x += 1; + } catch { + threw = true; + } + report("sloppy sealed own +=:", threw, sealedOwn.x); + + // `seal` leaves existing properties WRITABLE, so the line above must succeed + // in both modes -- it is the over-throw control. A NEW key on a sealed object + // is the rejecting half. + const sealedNew: any = { x: 1 }; + Object.seal(sealedNew); + threw = false; + try { + sealedNew.y += 1; + } catch { + threw = true; + } + report("sloppy sealed new +=:", threw, hasOwn(sealedNew, "y")); + + const nonWritable = readOnlyOwn(1); + threw = false; + try { + nonWritable.x += 1; + } catch { + threw = true; + } + report("sloppy non-writable own +=:", threw, nonWritable.x); + + // ---- INHERITED rejecting receivers ---- + // + // `+=` against a receiver whose rejection lives on the PROTOTYPE is asserted + // in the sloppy arm only, and the strict twin is spelled `=` instead. That + // asymmetry is deliberate and is NOT what this issue is about: + // + // Perry's `Expr::PropertySet` tail (`js_object_set_field_by_name`) performs + // an OWN-property store. It never runs `OrdinarySetWithOwnDescriptor`'s + // prototype walk, so an inherited non-writable data property, an inherited + // getter-only accessor, and an inherited SETTER are all mishandled the same + // way: the setter never fires and an own property is created instead. That + // is wrong in BOTH modes, it is wrong on unfixed `main` in both modes, and + // it is a missing prototype walk rather than a missing `Throw` flag. + // + // #9459 routes the SLOPPY tail to `js_put_value_set(..., 0)` -- the + // receiver-aware `[[Set]]` that sloppy `o.x = v` has always used -- so the + // sloppy arm below becomes correct as a side effect of getting the + // strictness right. The strict tail keeps its typed-feedback store site + // (`js_typed_feedback_object_set_field_by_name_fast`, a #7480/#5093 gate + // with its own IR tests), so fixing the strict half means retargeting that + // lane, which is a separate change. Filed as #9495; the strict `=` + // twins below pin the shapes so a future fix has a baseline here. + // + // Asserting the sloppy arm alone would be the #9394 mistake, which is why the + // strict side is still exercised -- on the lane that is already correct. + const inheritedNonWritable: any = Object.create(nonWritableProto()); + threw = false; + try { + inheritedNonWritable.x += 1; + } catch { + threw = true; + } + report( + "sloppy non-writable inherited +=:", + threw, + hasOwn(inheritedNonWritable, "x"), + inheritedNonWritable.x, + ); + + const getterOnly = getterOnlyOwn(); + threw = false; + try { + getterOnly.x += 1; + } catch { + threw = true; + } + report("sloppy getter-only own +=:", threw, getterOnly.x); + + const inheritedGetterOnly: any = Object.create(getterOnlyProto()); + threw = false; + try { + inheritedGetterOnly.x += 1; + } catch { + threw = true; + } + report( + "sloppy getter-only inherited +=:", + threw, + hasOwn(inheritedGetterOnly, "x"), + inheritedGetterOnly.x, + ); + + // An inherited SETTER runs in both modes and creates no own property: the + // rejection is about `[[Set]]` returning false, never about reaching the + // accessor. A fix that routed sloppy stores past the prototype walk would + // show up here as a missing call. + const calls: any[] = []; + const withSetter: any = Object.create(setterProto(calls)); + threw = false; + try { + withSetter.x += 1; + } catch { + threw = true; + } + report("sloppy inherited setter +=:", threw, calls.join(","), hasOwn(withSetter, "x")); + + const noExtend: any = { x: 1 }; + Object.preventExtensions(noExtend); + threw = false; + try { + noExtend.y += 1; + } catch { + threw = true; + } + report("sloppy preventExtensions new +=:", threw, hasOwn(noExtend, "y")); + + // preventExtensions leaves existing properties writable -- the second + // over-throw control. + const noExtendOwn: any = { x: 1 }; + Object.preventExtensions(noExtendOwn); + threw = false; + try { + noExtendOwn.x += 1; + } catch { + threw = true; + } + report("sloppy preventExtensions own +=:", threw, noExtendOwn.x); + + // ---- the class-field store lane ---- + const cell = frozenCell(); + threw = false; + try { + cell.x += 1; + } catch { + threw = true; + } + report("sloppy frozen class field +=:", threw, cell.x); + + const liveCell = new Cell(1); + threw = false; + try { + liveCell.x += 1; + } catch { + threw = true; + } + report("sloppy live class field +=:", threw, liveCell.x); + + // ---- the `arr.length` lane ---- + // `a.length += n` is `Set(O, "length", n, Throw)` on the OBJECT lane, which + // reaches `js_array_set_length_strict` -- named for the `Throw` it hard-codes. + const frozenArray: any[] = [1, 2]; + Object.freeze(frozenArray); + threw = false; + try { + frozenArray.length += 1; + } catch { + threw = true; + } + report("sloppy frozen array length +=:", threw, frozenArray.length); + + const nonWritableLength: any[] = [1, 2]; + Object.defineProperty(nonWritableLength, "length", { writable: false }); + threw = false; + try { + nonWritableLength.length += 1; + } catch { + threw = true; + } + report("sloppy non-writable array length +=:", threw, nonWritableLength.length); + + // A live array's `length` accepts the write in both modes: the control that + // proves the lane still WORKS, not just that it stopped throwing. + const liveArray: any[] = [1, 2]; + threw = false; + try { + liveArray.length += 1; + } catch { + threw = true; + } + report("sloppy live array length +=:", threw, liveArray.length); + + // ---- accepted stores, to prove the sloppy tail still STORES ---- + const plain: any = { x: 1 }; + threw = false; + try { + plain.x += 41; + } catch { + threw = true; + } + report("sloppy plain +=:", threw, plain.x); + + const plainFor: any = { x: 1 }; + threw = false; + try { + for (plainFor.x of [7]) { + } + } catch { + threw = true; + } + report("sloppy plain for-of head:", threw, plainFor.x); + + const plainDestr: any = { x: 1 }; + threw = false; + try { + [plainDestr.x] = [7]; + } catch { + threw = true; + } + report("sloppy plain [o.x] = arr:", threw, plainDestr.x); + + const plainNew: any = { x: 1 }; + threw = false; + try { + plainNew.y ??= 9; + } catch { + threw = true; + } + report("sloppy plain new key ??=:", threw, plainNew.y); + + // ---- `++` (Expr::PropertyUpdate) alongside `+=`, so the two spellings of + // one operation are asserted in the same file and mode ---- + const upd: any = { x: 1 }; + Object.freeze(upd); + threw = false; + try { + upd.x++; + } catch { + threw = true; + } + report("sloppy frozen ++:", threw, upd.x); + + // ---- and plain `=`, the lane that was already right (#9422) ---- + const assign: any = { x: 1 }; + Object.freeze(assign); + threw = false; + try { + assign.x = 9; + } catch { + threw = true; + } + report("sloppy frozen =:", threw, assign.x); +} + +function strictArm(): void { + "use strict"; + + let threw = false; + + // ---- compound assignment, every operator, on a frozen own data property ---- + const plus: any = { x: 1 }; + Object.freeze(plus); + threw = false; + try { + plus.x += 1; + } catch { + threw = true; + } + report("strict frozen +=:", threw, plus.x); + + const minus: any = { x: 1 }; + Object.freeze(minus); + threw = false; + try { + minus.x -= 1; + } catch { + threw = true; + } + report("strict frozen -=:", threw, minus.x); + + const times: any = { x: 3 }; + Object.freeze(times); + threw = false; + try { + times.x *= 2; + } catch { + threw = true; + } + report("strict frozen *=:", threw, times.x); + + // Logical assignment writes only on the branch that reaches the store, so + // each operator gets a starting value that makes it write. + const andAnd: any = { x: 1 }; + Object.freeze(andAnd); + threw = false; + try { + andAnd.x &&= 5; + } catch { + threw = true; + } + report("strict frozen &&=:", threw, andAnd.x); + + const orOr: any = { x: 0 }; + Object.freeze(orOr); + threw = false; + try { + orOr.x ||= 5; + } catch { + threw = true; + } + report("strict frozen ||=:", threw, orOr.x); + + const nullish: any = { x: undefined }; + Object.freeze(nullish); + threw = false; + try { + nullish.x ??= 5; + } catch { + threw = true; + } + report("strict frozen ??=:", threw, nullish.x); + + // The short-circuit CONTROL: `&&=` on a falsy value never stores, so it is + // silent in both modes even on a frozen receiver. A fix that made every + // logical assignment throw would show up here. + const andShort: any = { x: 0 }; + Object.freeze(andShort); + threw = false; + try { + andShort.x &&= 5; + } catch { + threw = true; + } + report("strict frozen &&= short-circuit:", threw, andShort.x); + + // ---- for-of head, named and computed ---- + const forNamed: any = { x: 1 }; + Object.freeze(forNamed); + threw = false; + try { + for (forNamed.x of [7]) { + } + } catch { + threw = true; + } + report("strict frozen for-of head:", threw, forNamed.x); + + const forComputed: any = { x: 1 }; + Object.freeze(forComputed); + const forKey = "x"; + threw = false; + try { + for (forComputed[forKey] of [7]) { + } + } catch { + threw = true; + } + report("strict frozen for-of head computed:", threw, forComputed.x); + + // The computed-key twin of `+=` above. `o[k] += 1` and `for (o[k] of ...)` + // both lower to `Expr::IndexSet`, whose OBJECT-by-name lanes kept throwing + // after #9426 carried the flag to its array element lanes. + const computedPlus: any = { x: 1 }; + Object.freeze(computedPlus); + const computedKey = "x"; + threw = false; + try { + computedPlus[computedKey] += 1; + } catch { + threw = true; + } + report("strict frozen o[k] +=:", threw, computedPlus.x); + + // A LITERAL computed key takes a different `Expr::IndexSet` arm from a + // runtime string key, so both are asserted. + const literalPlus: any = { x: 1 }; + Object.freeze(literalPlus); + threw = false; + try { + literalPlus["x"] += 1; + } catch { + threw = true; + } + report("strict frozen o[\"x\"] +=:", threw, literalPlus.x); + + // And an UNTYPED key, which reaches the runtime STRING_TAG dispatch rather + // than either static arm. + const anyKeyed: any = { x: 1 }; + Object.freeze(anyKeyed); + const anyKey: any = "x"; + threw = false; + try { + anyKeyed[anyKey] += 1; + } catch { + threw = true; + } + report("strict frozen o[anyKey] +=:", threw, anyKeyed.x); + + // ---- destructuring assignment targets ---- + // Statement position and expression position are DIFFERENT lowerings + // (`destructuring/assignment_stmt.rs` vs `destructuring/assignment_expr.rs`), + // so both are asserted. + const arrDestr: any = { x: 1 }; + Object.freeze(arrDestr); + threw = false; + try { + [arrDestr.x] = [7]; + } catch { + threw = true; + } + report("strict frozen [o.x] = arr:", threw, arrDestr.x); + + const arrDestrExpr: any = { x: 1 }; + Object.freeze(arrDestrExpr); + threw = false; + try { + const seen = ([arrDestrExpr.x] = [7]); + void seen; + } catch { + threw = true; + } + report("strict frozen ([o.x] = arr) expr:", threw, arrDestrExpr.x); + + const objDestr: any = { x: 1 }; + Object.freeze(objDestr); + threw = false; + try { + ({ a: objDestr.x } = { a: 7 }); + } catch { + threw = true; + } + report("strict frozen ({a: o.x}) = obj:", threw, objDestr.x); + + const arrDestrComputed: any = { x: 1 }; + Object.freeze(arrDestrComputed); + const destrKey = "x"; + threw = false; + try { + [arrDestrComputed[destrKey]] = [7]; + } catch { + threw = true; + } + report("strict frozen [o[k]] = arr:", threw, arrDestrComputed.x); + + // ---- the same `+=` against every rejecting receiver shape ---- + const sealedOwn: any = { x: 1 }; + Object.seal(sealedOwn); + threw = false; + try { + sealedOwn.x += 1; + } catch { + threw = true; + } + report("strict sealed own +=:", threw, sealedOwn.x); + + // `seal` leaves existing properties WRITABLE, so the line above must succeed + // in both modes -- it is the over-throw control. A NEW key on a sealed object + // is the rejecting half. + const sealedNew: any = { x: 1 }; + Object.seal(sealedNew); + threw = false; + try { + sealedNew.y += 1; + } catch { + threw = true; + } + report("strict sealed new +=:", threw, hasOwn(sealedNew, "y")); + + const nonWritable = readOnlyOwn(1); + threw = false; + try { + nonWritable.x += 1; + } catch { + threw = true; + } + report("strict non-writable own +=:", threw, nonWritable.x); + + // See the note in `sloppyArm`: the `+=` spelling on an inherited receiver is + // a separate, mode-independent defect. `=` is the same three receiver shapes + // on the lane that already walks the prototype chain. + const inheritedNonWritable: any = Object.create(nonWritableProto()); + threw = false; + try { + inheritedNonWritable.x = 11; + } catch { + threw = true; + } + report( + "strict non-writable inherited =:", + threw, + hasOwn(inheritedNonWritable, "x"), + inheritedNonWritable.x, + ); + + const getterOnly = getterOnlyOwn(); + threw = false; + try { + getterOnly.x += 1; + } catch { + threw = true; + } + report("strict getter-only own +=:", threw, getterOnly.x); + + const inheritedGetterOnly: any = Object.create(getterOnlyProto()); + threw = false; + try { + inheritedGetterOnly.x = 21; + } catch { + threw = true; + } + report( + "strict getter-only inherited =:", + threw, + hasOwn(inheritedGetterOnly, "x"), + inheritedGetterOnly.x, + ); + + // An inherited SETTER runs in both modes and creates no own property: the + // rejection is about `[[Set]]` returning false, never about reaching the + // accessor. A fix that routed sloppy stores past the prototype walk would + // show up here as a missing call. + const calls: any[] = []; + const withSetter: any = Object.create(setterProto(calls)); + threw = false; + try { + withSetter.x = 31; + } catch { + threw = true; + } + report("strict inherited setter =:", threw, calls.join(","), hasOwn(withSetter, "x")); + + const noExtend: any = { x: 1 }; + Object.preventExtensions(noExtend); + threw = false; + try { + noExtend.y += 1; + } catch { + threw = true; + } + report("strict preventExtensions new +=:", threw, hasOwn(noExtend, "y")); + + // preventExtensions leaves existing properties writable -- the second + // over-throw control. + const noExtendOwn: any = { x: 1 }; + Object.preventExtensions(noExtendOwn); + threw = false; + try { + noExtendOwn.x += 1; + } catch { + threw = true; + } + report("strict preventExtensions own +=:", threw, noExtendOwn.x); + + // ---- the class-field store lane ---- + const cell = frozenCell(); + threw = false; + try { + cell.x += 1; + } catch { + threw = true; + } + report("strict frozen class field +=:", threw, cell.x); + + const liveCell = new Cell(1); + threw = false; + try { + liveCell.x += 1; + } catch { + threw = true; + } + report("strict live class field +=:", threw, liveCell.x); + + // ---- the `arr.length` lane ---- + // `a.length += n` is `Set(O, "length", n, Throw)` on the OBJECT lane, which + // reaches `js_array_set_length_strict` -- named for the `Throw` it hard-codes. + const frozenArray: any[] = [1, 2]; + Object.freeze(frozenArray); + threw = false; + try { + frozenArray.length += 1; + } catch { + threw = true; + } + report("strict frozen array length +=:", threw, frozenArray.length); + + const nonWritableLength: any[] = [1, 2]; + Object.defineProperty(nonWritableLength, "length", { writable: false }); + threw = false; + try { + nonWritableLength.length += 1; + } catch { + threw = true; + } + report("strict non-writable array length +=:", threw, nonWritableLength.length); + + // A live array's `length` accepts the write in both modes: the control that + // proves the lane still WORKS, not just that it stopped throwing. + const liveArray: any[] = [1, 2]; + threw = false; + try { + liveArray.length += 1; + } catch { + threw = true; + } + report("strict live array length +=:", threw, liveArray.length); + + // ---- accepted stores, to prove the sloppy tail still STORES ---- + const plain: any = { x: 1 }; + threw = false; + try { + plain.x += 41; + } catch { + threw = true; + } + report("strict plain +=:", threw, plain.x); + + const plainFor: any = { x: 1 }; + threw = false; + try { + for (plainFor.x of [7]) { + } + } catch { + threw = true; + } + report("strict plain for-of head:", threw, plainFor.x); + + const plainDestr: any = { x: 1 }; + threw = false; + try { + [plainDestr.x] = [7]; + } catch { + threw = true; + } + report("strict plain [o.x] = arr:", threw, plainDestr.x); + + const plainNew: any = { x: 1 }; + threw = false; + try { + plainNew.y ??= 9; + } catch { + threw = true; + } + report("strict plain new key ??=:", threw, plainNew.y); + + // ---- `++` (Expr::PropertyUpdate) alongside `+=`, so the two spellings of + // one operation are asserted in the same file and mode ---- + const upd: any = { x: 1 }; + Object.freeze(upd); + threw = false; + try { + upd.x++; + } catch { + threw = true; + } + report("strict frozen ++:", threw, upd.x); + + // ---- and plain `=`, the lane that was already right (#9422) ---- + const assign: any = { x: 1 }; + Object.freeze(assign); + threw = false; + try { + assign.x = 9; + } catch { + threw = true; + } + report("strict frozen =:", threw, assign.x); +} + +sloppyArm(); +strictArm(); From 28bf6f6c515f57711eb462069d77524e1917ad2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Wed, 2 Sep 2026 10:09:47 +0200 Subject: [PATCH 2/2] fix(codegen): SIGSEGV storing to a scalar-replaced object-literal field that is never read (#9460) "use strict"; const o = { x: 1 }; o.x = 7; // SIGSEGV -- nothing reads o.x const p = { x: 1 }; for (p.x of [7]) {} // SIGSEGV, sloppy or strict const q = { x: 1 }; q.y++; // TypeError "Cannot assign to read only property 'y'" Three lines of ordinary code, in both modes. The fault is `str d0, [x8]` with x8 = 0x10 -- a raw field store through a NULL receiver at null + sizeof(ObjectHeader). `stmt/let_stmt.rs`'s scalar-replacement arm elides the heap allocation for a non-escaping `new` and gives each field a stack alloca. For the synthetic `__AnonShape_*` class an object literal lowers to, it creates slots only for the fields in `non_escaping_new_used_fields` -- which tracked READS only, on the argument that a store nothing ever reads is unobservable and its slot can be elided. That is true of the STORE and false of the SLOT: the same arm registers `ctx.locals[id]` as an uninitialized DUMMY alloca (the binding has stopped being an object), so a store lowering that looks up the field slot and finds none does not stop -- it falls through to the class-field / Ptr lanes, which load that dummy as an `ObjectHeader*`. The read side has had the matching guard since the synthetic-shape work (`expr/property_get.rs`, whose comment names this exact hazard: "the generic runtime helper that crashes on the dummy slot"). The write side never got it, and needed it on THREE lanes: `Expr::PropertySet` (`o.x += 1`, `for (o.x of ...)`), `Expr::PutValueSet` (`o.x = v`, via `try_lower_sloppy_class_field_store` and the write IC), and `Expr::PropertyUpdate` (`o.y++`). So the fix is at the source, in the two collectors that decide which fields get slots, rather than in each lane: - collectors/escape_news.rs: `non_escaping_new_used_fields` counts a WRITE as a use, so a written field always has a slot. #9024's rule one step further -- #9024 escapes a write to an UNDECLARED property because it would have no slot; this gives a slot to a DECLARED property that would otherwise have none. It costs nothing at runtime (a store into an alloca nothing loads is removed by LLVM). The walker also had NO arm at all for `Expr::PutValueSet`, which is what `o.x = v` lowers to, so neither the written field nor the value's own nested uses were being recorded. - collectors/escape_check.rs: the `Expr::PropertyUpdate` arm gains #9024's `class_chain_has_field` check that the `PropertySet` and `PutValueSet` arms already had. - expr/property_set.rs: a backstop mirroring `property_get.rs` -- a store to a scalar-replaced local with no field slot lowers the value for its side effects and discards the store, the same shape the `this` arm below it has always had. With the collector fixes this should no longer be reachable; kept because the failure it prevents is a null-pointer store and the read side carries the identical guard. Two corrections to the report, which said the crash "does not reproduce in isolation -- the preceding throws are required": - It reproduces in THREE LINES with no exception at all. The original isolated attempt printed `o.x` afterwards, and that read is what creates the slot and hides the crash. "Several rejections first" was the shape it was found in, not the condition. - It is NOT specific to sloppy mode, so it survives the #9459 fix rather than being masked by it -- confirmed by running the fixture against a build with #9459 applied and #9460 not: still SIGSEGV, at the strict case. Neither the `perry_sjlj_try` transport (#9323) nor a rooting hole (#9417/#9444/#9445) is involved: PERRY_GC_PROTECT_FROMSPACE changes nothing, because the address was never a heap object. Verified byte-identical to `node --experimental-strip-types` on test-files/test_gap_9460_unread_scalar_field_store.cts (SIGSEGV before, clean after), and the #9422/#9423 investigation's original `r_lanes.cts` repro now matches node exactly. --- .../9460-unread-scalar-field-store-segv.md | 80 +++++++++ .../src/collectors/escape_check.rs | 13 ++ .../src/collectors/escape_news.rs | 75 +++++++- crates/perry-codegen/src/expr/property_set.rs | 60 +++++++ ...est_gap_9460_unread_scalar_field_store.cts | 167 ++++++++++++++++++ 5 files changed, 389 insertions(+), 6 deletions(-) create mode 100644 changelog.d/9460-unread-scalar-field-store-segv.md create mode 100644 test-files/test_gap_9460_unread_scalar_field_store.cts diff --git a/changelog.d/9460-unread-scalar-field-store-segv.md b/changelog.d/9460-unread-scalar-field-store-segv.md new file mode 100644 index 0000000000..96c3b099b0 --- /dev/null +++ b/changelog.d/9460-unread-scalar-field-store-segv.md @@ -0,0 +1,80 @@ +### Fixed + +- **SIGSEGV storing to a scalar-replaced object-literal field that is never + read.** + + ```js + "use strict"; + const o = { x: 1 }; + o.x = 7; // SIGSEGV — nothing reads o.x + ``` + + ```js + const o = { x: 1 }; + for (o.x of [7]) {} // SIGSEGV, sloppy or strict + const p = { x: 1 }; + p.y++; // TypeError "Cannot assign to read only property 'y'" + ``` + + Three lines of ordinary code, in both modes. The fault is `str d0, [x8]` with + `x8 = 0x10` — a raw field store through a **null** receiver at + `null + sizeof(ObjectHeader)`. + + `stmt/let_stmt.rs`'s scalar-replacement arm elides the heap allocation for a + non-escaping `new` and gives each field a stack alloca. For the synthetic + `__AnonShape_*` class an object literal lowers to, it creates slots only for + the fields in `non_escaping_new_used_fields` — which tracked **reads** only, + on the argument that a store nothing ever reads is unobservable and its slot + can be elided. That is true of the *store* and false of the *slot*: the same + arm registers `ctx.locals[id]` as an **uninitialized dummy alloca** (the + binding has stopped being an object), so a store lowering that looks up the + field slot and finds none does not stop — it falls through to the class-field + / `Ptr` lanes, which load that dummy as an `ObjectHeader*`. + + The read side has had the matching guard since the synthetic-shape work + (`expr/property_get.rs`, whose comment names this exact hazard: "the generic + runtime helper that crashes on the dummy slot"). The write side never got it — + and it needed it on **three different lanes**: `Expr::PropertySet` (`o.x += 1`, + `for (o.x of …)`), `Expr::PutValueSet` (`o.x = v`, via + `try_lower_sloppy_class_field_store` and the write IC), and + `Expr::PropertyUpdate` (`o.y++`). So the fix is at the source, in the two + collectors that decide which fields get slots, rather than in each lane: + + - `collectors/escape_news.rs` — `non_escaping_new_used_fields` now counts a + WRITE as a use, so a written field always has a slot. This is #9024's rule + one step further: #9024 escapes a write to an *undeclared* property because + it would have no slot; this gives a slot to a *declared* property that would + otherwise have none. It costs nothing at runtime — a store into an alloca + nothing loads is removed by LLVM. The walker also had **no arm at all** for + `Expr::PutValueSet`, which is what `o.x = v` lowers to, so neither the + written field nor the value's own nested uses were being recorded. + - `collectors/escape_check.rs` — the `Expr::PropertyUpdate` arm gains #9024's + `class_chain_has_field` check, which the `PropertySet` and `PutValueSet` + arms already had. `o.y++` on an undeclared property has no slot either. + - `expr/property_set.rs` — a backstop mirroring `property_get.rs`: a store to + a scalar-replaced local with no field slot lowers the value for its side + effects and discards the store (`ScalarObjectFieldSetElided`), the same + shape the `this` arm below it has always had for an inlined constructor + whose target field has no slot. With the collector fixes above this should + no longer be reachable; it is kept because the failure mode it prevents is a + null-pointer store, and because the read side carries the identical guard. + + Two things worth recording about the report, which said the crash "does not + reproduce in isolation — the preceding throws are required": + + - It reproduces in **three lines with no exception at all**. The original + isolated attempt printed `o.x` afterwards, and that read is what creates the + slot and hides the crash. "Several rejections first" was the shape it was + found in, not the condition. + - It is **not** specific to sloppy mode, so it survives the #9459 fix rather + than being masked by it. Neither the `perry_sjlj_try` transport (#9323) nor + a rooting hole (#9417/#9444/#9445) is involved: `PERRY_GC_PROTECT_FROMSPACE` + changes nothing, because the address was never a heap object. + + - `test-files/test_gap_9460_unread_scalar_field_store.cts` — every write + spelling (`=`, `+=`, `o[k] =`, `o.x++`, `o.y++`, `for (o.x of …)`, + `[o.x] = arr`, a brand-new field) against an unread scalar-replaced field in + both modes, each printing a sentinel INSTEAD of reading the field, because + reading it is what hides the bug. Controls: the RHS side effects must still + happen, the read-after-store versions must still read back what was stored, + and an escaping receiver must keep its real heap object. diff --git a/crates/perry-codegen/src/collectors/escape_check.rs b/crates/perry-codegen/src/collectors/escape_check.rs index 862b8458ac..ef20f86861 100644 --- a/crates/perry-codegen/src/collectors/escape_check.rs +++ b/crates/perry-codegen/src/collectors/escape_check.rs @@ -320,6 +320,19 @@ pub fn check_escapes_in_expr( escaped.insert(*id); return; } + // #9460: #9024's rule, which the `PropertySet` and + // `PutValueSet` arms above already apply and this one was + // missing. `obj.x++` is a WRITE as well as a read, and a + // write to a property the class does not declare has no + // scalar slot — so `expr/member_update.rs` finds none and + // falls through to the by-name / `Ptr` lanes on the + // uninitialized dummy `ctx.locals[id]` alloca. + // `const o: any = {x:1}; o.y++;` reached that. + if !crate::collectors::class_accessors::class_chain_has_field( + classes, class_name, property, + ) { + escaped.insert(*id); + } // Safe — field increment on a non-escaping local return; } diff --git a/crates/perry-codegen/src/collectors/escape_news.rs b/crates/perry-codegen/src/collectors/escape_news.rs index e62e0d26dc..ef3fd1d397 100644 --- a/crates/perry-codegen/src/collectors/escape_news.rs +++ b/crates/perry-codegen/src/collectors/escape_news.rs @@ -90,11 +90,33 @@ pub fn collect_non_escaping_news( candidates } -/// For scalar-replaced `new` locals, collect the fields that are actually read -/// through the local after construction. This intentionally tracks only reads -/// (plus read-modify-write updates): writes still need their RHS evaluated for -/// JS side effects, but the scalar slot/store can be elided when the field is -/// never observed. +/// For scalar-replaced `new` locals, collect the fields that are accessed +/// through the local after construction — reads, read-modify-write updates, and +/// (since #9460) WRITES. +/// +/// The set decides which fields get a stack alloca in `stmt/let_stmt.rs`'s +/// scalar-replacement arm, for the synthetic `__AnonShape_*` classes that object +/// literals lower to. +/// +/// It used to track reads only, on the argument that a store to a field nothing +/// ever reads is unobservable and its slot can be elided. That is true of the +/// STORE and false of the SLOT. `let_stmt.rs` registers `ctx.locals[id]` as an +/// uninitialized DUMMY alloca for a scalar-replaced binding — the binding has +/// stopped being an object — so a store lowering that looks up the field slot +/// and finds none does not stop: it falls through to the class-field / +/// `Ptr` lanes, which load that dummy as an `ObjectHeader*` and store +/// through `null +
`. `const o: any = {x:1}; o.x = 7;` with no +/// later read of `o.x` segfaulted (#9460), in both modes, through several +/// different store lanes. +/// +/// Reserving a slot for a written-but-unread field is the fix at the source +/// rather than in each lane: `Expr::PropertySet`, `Expr::PutValueSet` and the +/// write IC all resolve the slot the same way, so one answer here covers all of +/// them. It costs nothing at runtime — a store into an alloca nothing loads is +/// removed by LLVM — and it is the same shape as #9024's rule one step further: +/// #9024 escapes a write to an UNDECLARED property because it would have no +/// slot; this gives a slot to a DECLARED property that would otherwise have +/// none. pub fn collect_non_escaping_new_used_fields( stmts: &[perry_hir::Stmt], non_escaping_news: &HashMap, @@ -228,10 +250,51 @@ fn collect_used_new_fields_in_expr( } collect_used_new_fields_in_expr(object, non_escaping_news, used); } - Expr::PropertySet { object, value, .. } => { + Expr::PropertySet { + object, + property, + value, + } => { + // #9460: a WRITE reserves the field's slot — see this function's + // doc comment. Without it the store lowering finds no slot and + // dereferences the dummy `ctx.locals[id]` alloca. + if let Expr::LocalGet(id) = object.as_ref() { + if non_escaping_news.contains_key(id) { + used.entry(*id).or_default().insert(property.clone()); + collect_used_new_fields_in_expr(value, non_escaping_news, used); + return; + } + } collect_used_new_fields_in_expr(object, non_escaping_news, used); collect_used_new_fields_in_expr(value, non_escaping_news, used); } + // #9460: `o.x = v` lowers to `PutValueSet`, not `PropertySet` — and this + // walker had NO arm for it at all, so neither the written field nor the + // value's own nested uses were recorded. Same rule as `PropertySet` + // above, on the same shape `escape_check.rs`'s `PutValueSet` arm + // recognises: a static string key with `target` and `receiver` naming + // the one candidate local. + Expr::PutValueSet { + target, + key, + value, + receiver, + .. + } => { + if let (Expr::LocalGet(id), Expr::LocalGet(receiver_id), Expr::String(property)) = + (target.as_ref(), receiver.as_ref(), key.as_ref()) + { + if id == receiver_id && non_escaping_news.contains_key(id) { + used.entry(*id).or_default().insert(property.clone()); + collect_used_new_fields_in_expr(value, non_escaping_news, used); + return; + } + } + collect_used_new_fields_in_expr(target, non_escaping_news, used); + collect_used_new_fields_in_expr(key, non_escaping_news, used); + collect_used_new_fields_in_expr(value, non_escaping_news, used); + collect_used_new_fields_in_expr(receiver, non_escaping_news, used); + } Expr::Binary { left, right, .. } | Expr::Compare { left, right, .. } | Expr::Logical { left, right, .. } diff --git a/crates/perry-codegen/src/expr/property_set.rs b/crates/perry-codegen/src/expr/property_set.rs index 6102a6c420..8a43d9e808 100644 --- a/crates/perry-codegen/src/expr/property_set.rs +++ b/crates/perry-codegen/src/expr/property_set.rs @@ -898,6 +898,66 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr, assignment_strict: bool) - return Ok(val_double); } } + // #9460: the local IS scalar-replaced, but this property has no + // field slot -- so there is nothing to store into, and nothing that + // could ever read it back. + // + // `stmt/let_stmt.rs`'s scalar-replacement arm creates slots for a + // synthetic `__AnonShape_*` class (an object literal) only for the + // fields in `non_escaping_new_used_fields`, which by design tracks + // READS: "writes still need their RHS evaluated for JS side effects, + // but the scalar slot/store can be elided when the field is never + // observed" (`collectors/escape_news.rs`). It then registers + // `ctx.locals[id]` as a DUMMY entry-block alloca that is never + // initialized, because the binding has stopped being an object at + // all, and overwrites `local_types[id]` with the synthetic class. + // + // Without this arm a slotless store fell through to the class-field / + // `Ptr` lowerings below, which load that dummy slot as an + // `ObjectHeader*` and store through `null +
` -- + // SIGSEGV on `const o: any = {x:1}; o.x = 7;` with no later read of + // `o.x`, in BOTH modes. The read side has had the matching guard + // since the synthetic-shape work (`expr/property_get.rs`, whose + // comment names the same hazard: "the generic runtime helper that + // crashes on the dummy slot"); the write side never got it, which is + // why adding a `console.log(o.x)` made the crash disappear -- the + // read is what creates the slot. + // + // Discarding the store is what the contract above promises and is + // unobservable: the receiver is a non-escaping fresh literal, so no + // alias exists, and a read of a slotless field already answers + // `undefined` on the read side. The RHS is still lowered, so its side + // effects happen -- same shape as the `this` arm just below, which + // has always evaluated the value and dropped the store when the + // inlined constructor's target field has no slot. + if let Expr::LocalGet(id) = object.as_ref() { + if ctx.scalar_replaced.contains_key(id) { + let val_double = lower_expr(ctx, value)?; + let lowered = LoweredValue { + semantic: SemanticKind::JsValue, + rep: NativeRep::JsValue, + llvm_ty: DOUBLE, + value: val_double.clone(), + }; + ctx.record_lowered_value_with_access_mode( + "ScalarObjectFieldSetElided", + Some(*id), + "scalar_object_field_store.unobserved", + &lowered, + None, + None, + None, + None, + false, + false, + vec![ + format!("field={}", property), + "reason=field_never_read_no_scalar_slot".to_string(), + ], + ); + return Ok(val_double); + } + } // Handle `this` during scalar-replaced constructor inlining: if let Expr::This = object.as_ref() { if let Some(target_id) = ctx.scalar_ctor_target.last().copied() { diff --git a/test-files/test_gap_9460_unread_scalar_field_store.cts b/test-files/test_gap_9460_unread_scalar_field_store.cts new file mode 100644 index 0000000000..2b7809c4ce --- /dev/null +++ b/test-files/test_gap_9460_unread_scalar_field_store.cts @@ -0,0 +1,167 @@ +// #9460: SIGSEGV on `o. = v` when `o` is a scalar-replaced object +// literal and that field is never READ. +// +// `stmt/let_stmt.rs`'s scalar-replacement arm elides the heap allocation for a +// non-escaping `new` and gives each field its own stack alloca -- but for a +// synthetic `__AnonShape_*` class (what an object literal lowers to) it only +// creates slots for the fields in `non_escaping_new_used_fields`, which +// deliberately tracks READS: +// +// "writes still need their RHS evaluated for JS side effects, but the +// scalar slot/store can be elided when the field is never observed" +// -- collectors/escape_news.rs +// +// It then registers `ctx.locals[id]` = a DUMMY entry-block alloca that is never +// initialized, because the binding is no longer an object at all. +// +// `expr/property_get.rs` implements the read half of that contract: a property +// of a scalar-replaced local with no slot reads `undefined`, with a comment +// naming the hazard -- "the generic runtime helper that crashes on the dummy +// slot". `expr/property_set.rs` never implemented the write half. A store to a +// slotless field fell past the scalar arm into the class-field / `Ptr` +// lowering, which loads the dummy slot as an `ObjectHeader*` and stores through +// `null + 16`. +// +// The crash needs the field to be written and NEVER read, which is why the +// original report's isolated attempt did not reproduce: it printed `o.x` +// afterwards, and that read is what creates the slot. +// +// It is NOT specific to sloppy mode, to `for (o.x of ...)`, or to a preceding +// throw -- `"use strict"; const o: any = {x:1}; o.x = 7;` segfaults too. This +// file is `.cts` so both modes live in one program. +// +// Every case here prints a sentinel INSTEAD of reading the field: reading it +// is what hides the bug. + +function sloppyForOfHead(): void { + const o: any = { x: 1 }; + for (o.x of [7]) { + } + console.log("sloppy for-of head: survived"); +} + +function strictForOfHead(): void { + "use strict"; + const o: any = { x: 1 }; + for (o.x of [7]) { + } + console.log("strict for-of head: survived"); +} + +function sloppyPlainAssign(): void { + const o: any = { x: 1 }; + o.x = 7; + console.log("sloppy plain assign: survived"); +} + +function strictPlainAssign(): void { + "use strict"; + const o: any = { x: 1 }; + o.x = 7; + console.log("strict plain assign: survived"); +} + +function strictCompoundAssign(): void { + "use strict"; + const o: any = { x: 1 }; + o.x += 1; + console.log("strict compound assign: survived"); +} + +function strictComputedAssign(): void { + "use strict"; + const o: any = { x: 1 }; + const k = "x"; + o[k] = 7; + console.log("strict computed assign: survived"); +} + +function strictUpdateNewField(): void { + "use strict"; + const o: any = { x: 1 }; + o.y++; + console.log("strict update new field: survived"); +} + +function strictUpdate(): void { + "use strict"; + const o: any = { x: 1 }; + o.x++; + console.log("strict update: survived"); +} + +function strictDestructure(): void { + "use strict"; + const o: any = { x: 1 }; + [o.x] = [7]; + console.log("strict destructure: survived"); +} + +// A store to a field the literal never declared, still never read. +function strictNewField(): void { + "use strict"; + const o: any = { x: 1 }; + o.y = 7; + console.log("strict new field: survived"); +} + +// The RHS must still be evaluated for its side effects even when the store +// itself is elided -- that is the half of the contract `escape_news.rs` +// promises, and eliding the whole statement would silently drop this call. +let sideEffects = 0; +function bump(): number { + sideEffects += 1; + return 7; +} + +function strictStoreEvaluatesRhs(): void { + "use strict"; + const o: any = { x: 1 }; + o.x = bump(); + o.x = bump(); + console.log("strict rhs side effects:", sideEffects); +} + +// CONTROL: the same shape WITH a read. This is the version that already +// worked, kept so a fix that broke it would show up here rather than in a +// benchmark. The field must still hold what was stored. +function strictReadAfterStore(): void { + "use strict"; + const o: any = { x: 1 }; + o.x = 7; + console.log("strict read after store:", o.x); +} + +function strictReadAfterForOfHead(): void { + "use strict"; + const o: any = { x: 1 }; + for (o.x of [7]) { + } + console.log("strict read after for-of head:", o.x); +} + +// CONTROL: an ESCAPING receiver keeps its heap object, so the store is a real +// one. `Object.freeze` is a call, which is what makes it escape. +function strictEscapingReceiver(): void { + "use strict"; + const o: any = { x: 1 }; + const seen: any[] = []; + seen.push(o); + o.x = 7; + console.log("strict escaping receiver:", seen[0].x); +} + +sloppyForOfHead(); +strictForOfHead(); +sloppyPlainAssign(); +strictPlainAssign(); +strictCompoundAssign(); +strictComputedAssign(); +strictUpdate(); +strictUpdateNewField(); +strictDestructure(); +strictNewField(); +strictStoreEvaluatesRhs(); +strictReadAfterStore(); +strictReadAfterForOfHead(); +strictEscapingReceiver();