fix(runtime): a rejected array element write throws only in strict mode (from #9418) - #9426
Merged
Conversation
added 2 commits
September 1, 2026 19:23
…de (#9394) const a = [1]; Object.freeze(a); a[0] = 9; // node silent, Perry TypeError const a2 = [1]; Object.freeze(a2); a2[5] = 9; // node silent, Perry TypeError Object.defineProperty(a3, 0, {writable:false}); a3[0]=9; // node silent, Perry TypeError Object.preventExtensions(a4); a4[5] = 9; // node silent, Perry TypeError const o = {x:1}; Object.freeze(o); o.x = 9; // node silent, Perry silent (correct) ES2024 6.2.5.7 (PutValue) calls Set(O, P, V, Throw) with Throw = IsStrictReference, so a failed [[Set]] throws ONLY in strict mode — for an Array exactly as for the ordinary object that was already right. A CommonJS bundle is sloppy code from top to bottom, which is where this surfaced. Introduced by #9326 (the merge of #9297, live again via #9370). That change is right about what it set out to fix — an inherited accessor must run, an inherited non-writable index must reject — but it reached the rejection by routing the cold element-store continuation through the STRICT runtime entry unconditionally. The inline store guard declines exactly the receivers whose write can be rejected (frozen, sealed, non-extensible, descriptor-bearing, prototype-sensitive), so every one of those shapes arrived there and threw. The fix carries the assignment's own Throw flag, which codegen already had and already passes to the ordinary-object [[Set]] and to `js_dyn_index_set_strict`. Finding the target is unchanged in both modes — the #9220 inherited-descriptor walk still runs, so a prototype setter still fires on a sloppy assignment; only the rejection differs. - codegen: `assignment_strict` reaches `js_typed_feedback_array_index_set_fallback_boxed` and `js_typed_feedback_array_set_index_or_string` (one new trailing i32 each). - array/indexing.rs: the strict entry's body is strictness-parameterised (`js_array_set_f64_extend_sloppy` is the sloppy twin); `array_spec_set` takes Throw and returns the receiver unchanged instead of throwing when it is false. Array mutators keep Throw = true: their own algorithms specify it regardless of the calling code. - value/dyn_index.rs: `js_dyn_index_set_strict` already carried the flag and its array arm forced true; it now uses it. The realloc arm in expr/index.rs deliberately keeps the strict entry: it runs only for a receiver the guard already accepted, which cannot reject. test-files/test_gap_9394_array_element_store_strictness.cts is a `.cts`, so it is a CommonJS script in BOTH runtimes, with a sloppy arm and a "use strict" arm. BOTH ARMS ARE ASSERTED. Asserting only the throw is precisely what let this through: #9326 shipped with a 64-check differential and a 205-line gap fixture, all green, none of it sloppy code. Byte-compared against node 26.5.1; a compiler built from unfixed origin/main reports TypeError for six sloppy cases where node is silent, and with this change is identical to node. #9326's own fixture (test_gap_9220_9221_array_proto_paths.ts, an ES module and therefore strict) is unchanged and still byte-identical to node. Unit tests assert both arms too: `element_store_rejection_throws_only_in_strict_mode`, and #9326's `typed_feedback_array_set_guards_reject_frozen_arrays`, which now asserts the silent sloppy call alongside the strict throw. Both were confirmed to FAIL with the sloppy entry rewired to the strict one. Three pieces of test infrastructure had to admit a `.cts` fixture at all, each of which would have made it a DARK TEST: the suite's `find … -name '*.ts'` does not match `foo.cts`, so the harness never selected it (`--filter test_gap_9394` selected 0 tests before, and PASSes after); `basename … .ts` named it `…strictness.c`; and `.gitignore` re-included only `.ts`/`.tsx` under test-files/, so it could not be committed. Not addressed here, found while writing the fixture: Perry emits `js_put_value_set(..., strict = 0)` at EVERY property-set site, so a rejected strict ordinary-object write is silent where Node throws — the mirror-image gap on the object path.
#9394's two sloppy-mode no-op returns took indexing.rs from 7 raw-handle sites to 9, over its ceiling. Routing every prototype-handled path through a single exit re-derives the receiver once instead of three times, which is fewer real re-reads rather than a wrapper that only hides them.
This was referenced Sep 1, 2026
proggeramlug
added a commit
that referenced
this pull request
Sep 2, 2026
proggeramlug
added a commit
that referenced
this pull request
Sep 2, 2026
…ite to an unread scalar-replaced field no longer stores through null (#9460) (#9519) * fix(codegen): a rejected sloppy `o.x += 1` / `for (o.x of …)` / `[o.x] = arr` no longer throws (#9459) // 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. * 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<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 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. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands #9418's third commit, which I held back earlier on the raw-handle ratchet. Author's commit preserved; the restructure below is mine.
The block. The two new sloppy-mode no-op returns took
array/indexing.rsfrom 7get_raw_mut_ptrsites to 9, against a ceiling of exactly 7.--no-raise-vsrefuses a bump.What I did not do. All nine reads in
array_spec_setare textually identical, so hoisting them behind a closure would show 1 and turn the gate green. That is gaming — the same read still executes nine times, and each has to stay a re-read because the point is observing the current pointer after a possible move.What I did instead. Every path the inherited property fully handles — setter invoked, getter-only rejection, non-writable rejection — now leaves through one shared exit, so the receiver is re-derived once instead of three times. That is strictly fewer runtime re-reads, not the same count hidden from a scanner, and it makes the "this can run JS, so re-read" reasoning explicit in one place. Debt returns to 963 with the ceiling untouched.
Semantics verified, not assumed: strict mode still throws
TypeErroron both rejection paths (non-writable inherited index, getter-only inherited index), byte-identical to node. The restructure moves thethrowinside thestrictarm rather than relying on fallthrough, so the sloppy no-op and the strict throw stay distinct.Validation:
perry-runtime8 suites green underRUST_TEST_THREADS=1; release build clean; raw-handle (bare and--no-raise-vs), file-size, census, root-holder and fmt gates all pass.