fix(codegen): PropertySet rejections honour sloppy mode (#9459); a write to an unread scalar-replaced field no longer stores through null (#9460)

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.

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<Shape>` 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.

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<Shape>` 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;
                 }

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<Shape>` lanes, which load that dummy as an `ObjectHeader*` and store
+/// through `null + <header size>`. `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<u32, String>,
@@ -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, .. }

crates/perry-codegen/src/expr/dispatch.rs

@@ -64,7 +64,18 @@ pub(crate) fn lower_expr(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result<String> {
             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 { .. } => {