fix(codegen): strict o.x += 1 on an inherited non-writable / accessor property walks the prototype chain (#9495)

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.

changelog.d/9495-strict-inherited-property-set-prototype-walk.md

@@ -0,0 +1,96 @@
+### Fixed
+
+- **Strict `o.x += 1` against an INHERITED non-writable property, getter-only
+  accessor or setter now runs the prototype walk.**
+
+  ```js
+  "use strict";
+  const proto = {};
+  Object.defineProperty(proto, "x", { value: 10, writable: false, configurable: true });
+  const a = Object.create(proto);
+  a.x += 1;        // node: TypeError        Perry: silent, created own a.x = 11
+
+  const g = {};
+  Object.defineProperty(g, "x", { get() { return 20; }, configurable: true });
+  const b = Object.create(g);
+  b.x += 1;        // node: TypeError        Perry: silent, created own b.x = 21
+
+  const calls = [];
+  const s = {};
+  Object.defineProperty(s, "x", { get() { return 30; }, set(v) { calls.push(v); }, configurable: true });
+  const d = Object.create(s);
+  d.x += 1;        // node: setter runs with 31, no own property
+                   // Perry: setter NEVER ran, created own d.x = 31
+  ```
+
+  ES2024 §10.1.9.2 (`OrdinarySetWithOwnDescriptor`): when the receiver has no
+  own property, `[[Set]]` walks to the parent and the *parent's* descriptor
+  decides — a non-writable data property rejects, a getter-only accessor
+  rejects, a setter runs with the original receiver, and only a writable data
+  property (or the end of the chain) creates a new own property. `PutValue`
+  then throws on a rejection iff the reference is strict.
+
+  `o.x = v` was already right — it lowers to `Expr::PutValueSet` →
+  `js_put_value_set`, whose `ordinary_set_with_receiver` walks the chain — and
+  #9459 made the *sloppy* half of these spellings right as a side effect of
+  routing the sloppy `Expr::PropertySet` tail to that same entry. Only the
+  **strict** spellings that lower to `Expr::PropertySet` — compound and logical
+  assignment, `for`-of heads, expression-position destructuring targets — and
+  the strict object-by-name arms of `Expr::IndexSet` (`o["x"] += 1`,
+  `o[k] += 1`) were still wrong. This is a missing prototype walk, not a
+  missing `Throw` flag: the opposite direction from #9422 and a different
+  defect from #9459.
+
+  Root cause: the strict tails ended in
+  `js_typed_feedback_object_set_field_by_name_fast` /
+  `js_typed_feedback_object_set_field_by_name` → `js_object_set_field_by_name`,
+  an **own-property** store. The shape-transition fast path inside it already
+  declines any receiver whose prototype is not the ordinary one, so every one of
+  these receivers fell to the slow branch — which appended an own property
+  without ever consulting the chain.
+
+  Fix: the strict tails now reach the same receiver-aware `[[Set]]` the sloppy
+  tails and the `=` lane use, `js_put_value_set(target, key, value, receiver,
+  strict)`, so the two modes are one tail distinguished by the `Throw` flag
+  alone. The typed-feedback `PropertySet` site moves with the store: it is
+  registered in both modes (it describes the store, not its strictness) and
+  observed by the pure-recording `js_typed_feedback_observe_property_set`,
+  compile-gated on `PERRY_TYPED_FEEDBACK` exactly as #7480 step 4 gates every
+  other recording helper — a default build emits the bare `js_put_value_set`
+  call and nothing else. Nothing was left for the old dispatching wrapper to
+  decide (the receiver-aware entry makes the fast-path choice itself), so the
+  #7480 "dispatching wrappers still emitted in a default build" gate is
+  re-pointed at the method-call dispatcher the same fixture emits, and asserted
+  as a *call* rather than a symbol (the symbol match was satisfied by the
+  `declare` line alone).
+
+  - `crates/perry-codegen/src/expr/property_set.rs` —
+    `lower_put_value_property_set_by_name` replaces the sloppy-only helper
+    and is the generic tail for both modes; `caller` / `arguments` keep their
+    `js_object_set_field_by_name` route (poisoned-accessor handling, unrelated
+    to either flag or walk). `emit_typed_feedback_property_set_observation`
+    carries the site.
+  - `crates/perry-codegen/src/expr/index_set.rs` —
+    `lower_object_index_set_put_value` replaces the sloppy-only helper on the
+    literal-string-key and string-typed-key object arms.
+  - `crates/perry-codegen/src/expr/typed_feedback.rs` —
+    `TypedFeedbackContract::put_value_set`.
+  - `test-files/test_gap_9495_strict_inherited_property_set.cts` — the three
+    inherited receivers plus a two-level chain, a class accessor on the chain
+    and a Proxy on the chain (receiver forwarded), across `+=`, `&&=`, `??=`
+    (short-circuit control), `for`-of heads, destructuring in statement and
+    expression position, `o["x"]`, `o[k]`, `o[anyKey]`, `[o[k]] = arr`, with
+    accepted-store controls (inherited writable data, new key beside an
+    inherited accessor, class-ref receiver) and the already-correct `=` /
+    `++` lanes — **both modes**, so "silent because sloppy" and "silent because
+    the walk never ran" are told apart by the setter-call log and `hasOwn`.
+  - `test-files/test_gap_9459_property_set_strictness.cts` — the strict
+    inherited twins are spelled `+=` now, as that file's comment promised.
+
+  Left as it was: `js_class_field_set_fallback` (the class-field arm's
+  guard-miss path) and `js_object_set_field_by_property_id` (the
+  computed-runtime-members class route) are still own-property stores; neither
+  is reachable for the receivers above without a class-typed variable holding
+  an `Object.create`d value, and each is its own lane. A DECLARED `static`
+  field losing `K.n += 1` in both modes (found while building the fixture) is
+  a static-slot lane defect, not a walk, and is filed as #9526.

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