Skip to content

push/unshift/splice take spread arguments; push takes more than one item - #388

Merged
ASDAlexander77 merged 1 commit into
mainfrom
array-builtin-spread-args
Sep 28, 2026
Merged

ASDAlexander77 merged 1 commit into
mainfrom
array-builtin-spread-args

Conversation

@ASDAlexander77

Copy link
Copy Markdown
Owner

Two array-builtin bugs, one of which hid the other.

1. push with more than one item failed to verify.

let m: number[] = [];
m.push(1, 2);   // error: 'ts.ArrayPush' op failed to verify that array element type match all items types

ArrayPush checked its items with RangedTypesMatchWith against TypeRange(elementType), a one-type range. Only a single item can match it. The trait is replaced with a verifier (TypeScriptOps.cpp) that checks every item against the element type. unshift and splice never had the trait, which is why splice(1, 1, 10, 20, 30) worked.

2. Spread arguments to push, unshift and splice. A builtin takes one operand per item, and a spread's length is known only at run time. a.push(...xs) handed xs itself over as one item, so it hit the verifier above.

The new mlirGenArrayInsertWithSpread (MLIRGenAccessCall.cpp) runs before operands are generated, and only when a spread argument is present:

  • Every item argument, spread or not, becomes one array literal, which already expands spreads at run time.
  • Its elements are then inserted one at a time through the same builtin without a spread, so the element casts and the -mm=rc retains stay the builtin's.
  • push reuses mlirGenAppendArrayByEachElement.
  • unshift and splice insert at a position. For splice, the delete runs first, and the position is the start as JavaScript normalizes it: a negative start counts from the end, and the result is clamped to the array.
  • The result is the new length, as the builtins already return.
  • A spread in splice's start or delete-count position is a clear error.

Test: 00array_spread_args.ts, registered for compile, jit and the corpus. It covers:

  • push with two items;
  • push, unshift and splice with a spread alone and mixed with plain items;
  • an empty spread, and a parameter spread;
  • negative, past-the-end and before-the-start splice positions;
  • s32[] spread into number[], and strings.

It fails on main, and it runs clean under -mm=rc --verify-ownership.

Results: the suite passes locally, 3017/3017.

Found along the way, not changed here (both also fail on main):

  • assert(cond, what + ": x"): an assert message built at run time fails with "operation's operand is unlinked", or "'ts.Retain' op using value defined outside the region". The test uses constant messages.
  • Under JIT with gc only, a function that builds a string with s += v + "," loses its first append when it is called after certain array-literal spreads. AOT and -mm=rc are correct, and the Sep 11 build is too.

🤖 Generated with Claude Code

`a.push(1, 2)` failed to verify: ArrayPush checked its items with a
RangedTypesMatchWith against a one-type range, which only a single item
matches. It now has a verifier that checks every item against the
element type.

A spread argument (`a.push(...xs)`, `a.unshift(...xs)`,
`a.splice(start, count, ...xs)`) has a length known only at run time,
and the builtins take one operand per item. Every item argument, spread
or not, now goes into one array literal (which spreads at run time), and
its elements go in one at a time through the same builtin without a
spread, so the casts and retains stay the builtin's. splice deletes
first, then inserts at the start JavaScript normalizes to (negative from
the end, clamped to the array). A spread in splice's start or delete
count position is an error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ASDAlexander77
ASDAlexander77 force-pushed the array-builtin-spread-args branch from 7a1591c to 7cc9038 Compare September 28, 2026 15:06
@ASDAlexander77
ASDAlexander77 merged commit c9a8f8b into main Sep 28, 2026
2 checks passed
@ASDAlexander77
ASDAlexander77 deleted the array-builtin-spread-args branch September 28, 2026 15:58
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