Skip to content

fix(hir): class destructuring (dstr) + default-parameter parity (test262) - #4766

Merged
proggeramlug merged 1 commit into
mainfrom
class-dstr-parity
Jun 7, 2026
Merged

fix(hir): class destructuring (dstr) + default-parameter parity (test262)#4766
proggeramlug merged 1 commit into
mainfrom
class-dstr-parity

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Summary

Brings language/{statements,expressions}/class/dstr from 512 → 664 passing (41.6% → 53.9%) with zero regressions, and lifts the wider built-ins+language sweep by ~6–9% per shard (0 regressions across 4 sampled shards 0,3,6,9 of 12). These are method / constructor / accessor parameter-destructuring tests across plain, generator, async-generator, static, and private class methods.

Root causes fixed

  1. Array binding patterns now use the iterator protocol. lower_pattern_binding lowered let [a,b] = x (and every method/param destructuring) to raw index reads (x[0], x[1]) — never invoking Symbol.iterator, never closing the iterator, mishandling holes/rest, and unable to destructure non-array iterables. Rewrote the Pat::Array arm to emit GetIteratorIteratorStep/IteratorValue per element, drain a rest element via a new js_iterator_rest_to_array, advance the iterator on elisions, and IteratorClose on both normal completion (when not exhausted) and any abrupt completion. Destructuring defaults now use strict === undefined (a genuine NaN element no longer triggers the default).
  2. Object binding patterns enforce RequireObjectCoercible (new js_require_object_coercible) — TypeError for a null/undefined source even for an empty pattern {}.
  3. Static-method calls pad omitted args with undefined. Expr::StaticMethodCall (non-rest path) forwarded only supplied args, so a static method called with fewer args read uninitialized parameter slots — static f(a=1) returned 0, static m([x]=[]) threw.
  4. Private methods emit their default-parameter prologuelower_private_method computed param.default but never called build_default_param_stmts.
  5. Method-as-value calls pad omitted args with undefinedcall_vtable_method padded a bare IEEE NaN; reached without call-site padding when a method is invoked as a value (const f = obj.m; f(), or a get m(){ return this.#m } accessor exposing a private method).

Validation

  • Cluster (class/dstr): 0 regressed, 152 fixed vs main baseline.
  • Wider sweep (built-ins+language, shards 0/3/6/9 of 12): 0 regressed, +20/+20/+19/+26 fixed.
  • Built + tested on a Linux box against the pinned test262 corpus.

Remaining class/dstr failures are concentrated in *-init-fn-name-* (NamedEvaluation of anonymous function/arrow/class defaults) and generator throw-timing (eager param destructuring) — distinct features left for follow-ups.

…262)

Brings language/{statements,expressions}/class/dstr from 512->664 passing
(41.6%->53.9%) with zero regressions, and lifts the wider built-ins+language
sweep ~6-9% per shard (0 regressions across sampled shards).

Root causes:
- Array binding patterns now use the iterator protocol (GetIterator/
  IteratorStep/IteratorValue/IteratorClose) instead of raw index reads, so
  Symbol.iterator is invoked, the iterator is closed, and holes/rest/non-array
  iterables work. Destructuring defaults use strict === undefined.
- Object binding patterns enforce RequireObjectCoercible (TypeError for
  null/undefined source, even for an empty pattern {}).
- Static-method calls pad omitted args with undefined (Expr::StaticMethodCall
  non-rest path) so default params / destructuring see undefined, not an
  uninitialized 0.0 slot.
- Private methods emit their default-parameter prologue (build_default_param_stmts
  was never called from lower_private_method).
- Method-as-value calls pad omitted args with undefined (call_vtable_method
  padded a bare NaN), fixing `const f = obj.m; f()` and getter-exposed private
  methods.

See CHANGELOG.md v0.5.1142 for details.
@proggeramlug
proggeramlug merged commit 8fc8954 into main Jun 7, 2026
13 checks passed
@proggeramlug
proggeramlug deleted the class-dstr-parity branch June 7, 2026 18:13
proggeramlug pushed a commit that referenced this pull request Jun 8, 2026
 regression)

A value pulled from an array binding pattern (const [k, v] = pair, or the
parameter form ([k, v]) => ...) and then copied into another local
(const cb = v) read back as -2147483648 (i32::MIN) instead of the original
object/string; calling a method on it threw
"TypeError: (number).<method> is not a function". This broke every drizzle-orm
schema (Object.fromEntries(Object.entries(cols).map(([name, col]) => { col.setName(name); ... }))),
crashing apps at startup with "(number).setName is not a function".

Regressing commit: #4766 (iterator-protocol array destructuring), which emits a
scaffolding local `let __destruct_N = undefined` per binding element. That
mutable+undefined shape is one of collect_integer_let_ids's seeds (it targets
the clampIdx do-while pattern), so the scaffolding local was optimistically
seeded into integer_locals. The forward closure then propagated integer-ness
down the init-only copy chain (cbBase = __destruct -> cb = cbBase). The
disqualify fixed point only prunes locals via non-int LocalSet writes, so it
removed the scaffolding local (writes are undefined/step.value) but never
re-validated the forward-propagated copies (their Let-init is their sole
definition, no LocalSet). The second hop (const cb = cbBase) ended up both in
integer_locals and strictly_i32_bounded_locals, qualifying for an i32 shadow
slot; storing the NaN-boxed value did fptosi(NaN) = i32::MIN.

Fix: in the disqualify fixed point, also re-validate any candidate Let with no
LocalSet write (collect_non_int_init_only_let_ids) -- every const, plus
never-reassigned lets such as the mutable bindings the parameter-destructuring
path emits -- against its defining init over the current candidate set. When the
source local is disqualified the copy is pruned too, cascading through the
chain. Locals with real int-producing LocalSet writes (clampIdx's xx) stay
governed by those writes, preserving the image_convolution i32 fast paths.

Regression tests in collectors/hir_facts.rs cover the immutable and mutable
(parameter) destructure-copy chains plus an over-pruning guard. Full
perry-codegen test suite green.

Refs #793.
proggeramlug pushed a commit that referenced this pull request Jun 8, 2026
 regression)

A value pulled from an array binding pattern (const [k, v] = pair, or the
parameter form ([k, v]) => ...) and then copied into another local
(const cb = v) read back as -2147483648 (i32::MIN) instead of the original
object/string; calling a method on it threw
"TypeError: (number).<method> is not a function". This broke every drizzle-orm
schema (Object.fromEntries(Object.entries(cols).map(([name, col]) => { col.setName(name); ... }))),
crashing apps at startup with "(number).setName is not a function".

Regressing commit: #4766 (iterator-protocol array destructuring), which emits a
scaffolding local `let __destruct_N = undefined` per binding element. That
mutable+undefined shape is one of collect_integer_let_ids's seeds (it targets
the clampIdx do-while pattern), so the scaffolding local was optimistically
seeded into integer_locals. The forward closure then propagated integer-ness
down the init-only copy chain (cbBase = __destruct -> cb = cbBase). The
disqualify fixed point only prunes locals via non-int LocalSet writes, so it
removed the scaffolding local (writes are undefined/step.value) but never
re-validated the forward-propagated copies (their Let-init is their sole
definition, no LocalSet). The second hop (const cb = cbBase) ended up both in
integer_locals and strictly_i32_bounded_locals, qualifying for an i32 shadow
slot; storing the NaN-boxed value did fptosi(NaN) = i32::MIN.

Fix: in the disqualify fixed point, also re-validate any candidate Let with no
LocalSet write (collect_non_int_init_only_let_ids) -- every const, plus
never-reassigned lets such as the mutable bindings the parameter-destructuring
path emits -- against its defining init over the current candidate set. When the
source local is disqualified the copy is pruned too, cascading through the
chain. Locals with real int-producing LocalSet writes (clampIdx's xx) stay
governed by those writes, preserving the image_convolution i32 fast paths.

Regression tests in collectors/hir_facts.rs cover the immutable and mutable
(parameter) destructure-copy chains plus an over-pruning guard.

Refs #793.
proggeramlug added a commit that referenced this pull request Jun 8, 2026
 regression) (#4785)

A value pulled from an array binding pattern (const [k, v] = pair, or the
parameter form ([k, v]) => ...) and then copied into another local
(const cb = v) read back as -2147483648 (i32::MIN) instead of the original
object/string; calling a method on it threw
"TypeError: (number).<method> is not a function". This broke every drizzle-orm
schema (Object.fromEntries(Object.entries(cols).map(([name, col]) => { col.setName(name); ... }))),
crashing apps at startup with "(number).setName is not a function".

Regressing commit: #4766 (iterator-protocol array destructuring), which emits a
scaffolding local `let __destruct_N = undefined` per binding element. That
mutable+undefined shape is one of collect_integer_let_ids's seeds (it targets
the clampIdx do-while pattern), so the scaffolding local was optimistically
seeded into integer_locals. The forward closure then propagated integer-ness
down the init-only copy chain (cbBase = __destruct -> cb = cbBase). The
disqualify fixed point only prunes locals via non-int LocalSet writes, so it
removed the scaffolding local (writes are undefined/step.value) but never
re-validated the forward-propagated copies (their Let-init is their sole
definition, no LocalSet). The second hop (const cb = cbBase) ended up both in
integer_locals and strictly_i32_bounded_locals, qualifying for an i32 shadow
slot; storing the NaN-boxed value did fptosi(NaN) = i32::MIN.

Fix: in the disqualify fixed point, also re-validate any candidate Let with no
LocalSet write (collect_non_int_init_only_let_ids) -- every const, plus
never-reassigned lets such as the mutable bindings the parameter-destructuring
path emits -- against its defining init over the current candidate set. When the
source local is disqualified the copy is pruned too, cascading through the
chain. Locals with real int-producing LocalSet writes (clampIdx's xx) stay
governed by those writes, preserving the image_convolution i32 fast paths.

Regression tests in collectors/hir_facts.rs cover the immutable and mutable
(parameter) destructure-copy chains plus an over-pruning guard.

Refs #793.

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant