fix(hir): class destructuring (dstr) + default-parameter parity (test262) - #4766
Merged
Conversation
proggeramlug
force-pushed
the
class-dstr-parity
branch
from
June 7, 2026 17:51
1d64f52 to
de5a484
Compare
…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
force-pushed
the
class-dstr-parity
branch
from
June 7, 2026 17:54
de5a484 to
25a121c
Compare
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>
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
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.
Summary
Brings
language/{statements,expressions}/class/dstrfrom 512 → 664 passing (41.6% → 53.9%) with zero regressions, and lifts the widerbuilt-ins+languagesweep by ~6–9% per shard (0 regressions across 4 sampled shards0,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
lower_pattern_bindingloweredlet [a,b] = x(and every method/param destructuring) to raw index reads (x[0],x[1]) — never invokingSymbol.iterator, never closing the iterator, mishandling holes/rest, and unable to destructure non-array iterables. Rewrote thePat::Arrayarm to emitGetIterator→IteratorStep/IteratorValueper element, drain a rest element via a newjs_iterator_rest_to_array, advance the iterator on elisions, andIteratorCloseon both normal completion (when not exhausted) and any abrupt completion. Destructuring defaults now use strict=== undefined(a genuineNaNelement no longer triggers the default).RequireObjectCoercible(newjs_require_object_coercible) —TypeErrorfor anull/undefinedsource even for an empty pattern{}.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)returned0,static m([x]=[])threw.lower_private_methodcomputedparam.defaultbut never calledbuild_default_param_stmts.undefined—call_vtable_methodpadded a bare IEEENaN; reached without call-site padding when a method is invoked as a value (const f = obj.m; f(), or aget m(){ return this.#m }accessor exposing a private method).Validation
mainbaseline.built-ins+language, shards 0/3/6/9 of 12): 0 regressed, +20/+20/+19/+26 fixed.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.