fix: expand spread args to compiled Array.prototype.concat (#952)#962
Merged
Conversation
Compiled `[0].concat(...[[1, 2]])` returned `[0,[1,2]]` instead of `[0,1,2]`. The array-method emitters built concat's argument array by emitting each AST arg via EmitExpression, but a spread `...arr` is an Expr.Spread whose emit yields the inner array object — so the spread element reached the runtime ArrayConcat as a single nested item, and concat's per-argument one-level flatten left it nested. Route concat's args through the existing EmitArgsArrayWithSpread helper (added for Math.max spread, #951) at every compiled dispatch path that emits Array.prototype.concat, so the spread is flattened before concat applies its per-argument flatten. The helper is identical to the old inline loop when no spread is present, so non-spread concat IL is unchanged (no regression): - ArrayEmitter (statically-typed `arr.concat(...)`, the repro path): stash the receiver list, build the spread-expanded args array await-safely (the helper evaluates into temps with a clear stack, so the spread may contain await), reload list under items. Excluded from the #850 plain-arg pre-spill to avoid double-evaluating the args. - The two runtime-typed (string-or-list / any) dispatch paths in ILEmitter.Calls.AmbiguousDispatch and ExpressionEmitterBase.CallHelpers. - The shared argsArray case in ILEmitter.Calls (Array.prototype.X.call), which also closes the same spread gap for reduce/slice/toSpliced/with. The interpreter already expanded spreads correctly (EvaluateCall via GetIterableElements), so this was compiled-only. Tests: ArrayConcatSpreadTests covers the repro plus flat/multi-array/ mixed/boxed-string/variable/empty/async spreads across interpreter and compiled modes. Full suite 14383 green.
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.
Closes #952.
Problem
In compiled mode, a spread argument to
Array.prototype.concatwas not flattened:The array-method emitters built
concat's argument array by emitting each AST arg viaEmitExpression, but a spread...arris anExpr.Spreadwhose emit yields the inner array object — so the spread element reached the runtimeArrayConcat(list, object?[] items)as a single nested item, and concat's per-argument one-level flatten left it nested.[0].concat(b)(no spread) was always correct; it failed for boxed (non-numeric) arrays too.The interpreter already expanded spreads correctly (
EvaluateCall→GetIterableElements), so this was compiled-only, exactly as the issue stated.Same family as the
Math.max(...arr)→NaNgap (#951, PR #957), which introduced the helper this fix reuses.Changes
Route
concat's args through the existingEmitArgsArrayWithSpreadhelper (onIEmitterContext, added in #957) at every compiled dispatch path that emitsArray.prototype.concat, so the spread is flattened before concat applies its per-argument flatten. The helper is identical to the old inline loop when no spread is present, so non-spread concat IL is unchanged → no regression.Compilation/Emitters/ArrayEmitter.cs— primaryarr.concat(...)path (statically-typed array). Stash the receiver list, build the spread-expanded args array await-safely (the helper evaluates each arg into a temp with a clear stack, so the spread may containawait), then reloadlistunderitems. Excluded from the Compiled async-arrow write-capture emits invalid IL (PathStackDepth), even without shadowing #850 plain-arg pre-spill (!concatSpread) to avoid double-evaluating the args.Compilation/ILEmitter.Calls.AmbiguousDispatch.csandCompilation/ExpressionEmitterBase.CallHelpers.cs— the two runtime-typed (string-or-list /any) dispatch paths forx.concat(...).Compilation/ILEmitter.Calls.cs— the sharedargsArraycase (Array.prototype.X.call), which also closes the same spread gap forreduce/slice/toSpliced/withvia.call.Tests
SharpTS.Tests/SharedTests/ArrayConcatSpreadTests.cs(new) — the issue repro plus flat-array / multi-array / mixed spread+plain / boxed-string / variable-source / empty / async spreads, run in both interpreter (the Node-matching oracle) and compiled modes.Verification