Preserve constant array shape when spreading a union of constant arrays in array literals - #5774
Conversation
…ys in array literals
- In `InitializerExprTypeResolver::getArrayType()`, change the
`count($constantArrays) === 1` check to `count($constantArrays) > 0`
to handle unions of constant arrays (e.g. `array{key: T}|array{}`)
- For string-key arrays: merge keys across all constant arrays, marking
keys not present in all branches as optional, with value types unioned
- For integer-key arrays: merge by position across all constant arrays,
with positions not present in all branches marked optional
- Update `$hasOffsetValueTypes` tracking to correctly handle merged keys
that overlap with previously-set keys
510302a to
857effa
Compare
| break; | ||
| foreach ($constantArrays as $constantArrayType) { | ||
| foreach ($constantArrayType->getKeyTypes() as $keyType) { | ||
| if ($keyType->isString()->yes()) { |
There was a problem hiding this comment.
uncovered mutation
if ($this->phpVersion->supportsArrayUnpackingWithStringKeys()) {
foreach ($constantArrays as $constantArrayType) {
foreach ($constantArrayType->getKeyTypes() as $keyType) {
- if ($keyType->isString()->yes()) {
+ if (!$keyType->isString()->no()) {
$hasStringKey = true;
break 2;
}There was a problem hiding this comment.
Done and pushed. Here's what I found and changed.
The suggested mutant is equivalent — but it pointed at a real bug
!$keyType->isString()->no() cannot be distinguished from $keyType->isString()->yes() at that spot: ConstantArrayType::getKeyTypes() is typed and asserted as list<ConstantIntegerType|ConstantStringType>, and isString() on those is hard-coded createYes() (src/Type/StringType.php:250) / createNo() (src/Type/JustNullableTypeTrait.php:119) — never maybe. So no analysed code can produce a key where the two forms differ, and no test can kill that mutant as written.
What writing the test did surface is that the $hasStringKey scan was wrong in a different way — and it was wrong before this PR too:
[9, ...['a' => 1, 5]]
// before: array{0: 5, a: 1} ← the 5 overwrote the leading 9, one element lost
// PHP: [0 => 9, 'a' => 1, 1 => 5]Because any string key anywhere flipped the whole unpacked array to "set by original key", integer keys were kept instead of renumbered. My union handling inherited it and made it worse: [9, ...($flag ? ['a' => 1] : [5])] inferred array{0: 5|9, a?: 1}.
Fix
Decide per key rather than per array: string keys merge by name, integer keys merge by position among the integer keys of each unpacked array and are appended with a null key so the builder renumbers them. This also collapses the two separate merging paths (string-key path and integer-key path) into one loop — the diff is now net-neutral in size (+83/-83).
[9, ...['a' => 1, 5]] → array{0: 9, a: 1, 1: 5}
[9, ...($flag ? ['a' => 1] : [5])] → array{0: 9, a?: 1, 1?: 5}
[...($flag ? ['a' => 1, 7] : [5, 'a' => 2])] → array{a: 1|2, 0: 5|7}
All verified against real PHP output.
Tests
nsrt/array-unpacking-string-keys.php: mixed int/string keys in one spread, and[9, ...[3 => 'x']](renumbering).nsrt/bug-14708.php: string-key branch vs integer-key branch, and mixed keys in both branches.
Both fail before the source change (array{0: 6, a: 1, b: 2} and array{0: 5|9, a?: 1}) and pass after — confirmed by stashing the fix. So the isString() check is now genuinely covered: it decides preserve-vs-renumber per key, and flipping or dropping it changes the inferred shape. The one mutation that remains unkillable is the exact !...->no() rewrite, for the type-contract reason above.
make tests (21251 tests, 97 skipped — the usual version-gated ones), make phpstan (no errors) and make cs all pass. Pushed as 760b96584.
Unpacking merges string keys by name, but integer keys are always
renumbered by PHP. The previous code decided per unpacked array: as soon
as any string key was present, *all* keys including integer ones were
set by their original key, so `[9, ...['a' => 1, 5]]` was inferred as
`array{0: 5, a: 1}` instead of `array{0: 9, a: 1, 1: 5}` - the 5
overwrote the leading 9 and one element was lost.
Decide per key instead: string keys are merged by name, integer keys by
their position among the integer keys of each unpacked array, and are
appended so the builder renumbers them. This makes the shape correct for
mixed-key arrays, both for a single unpacked array and for a union of
them, and collapses the previously separate string-key and integer-key
merging paths into one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`getKeyTypes()` only ever yields ConstantIntegerType|ConstantStringType, so `isString()` there can never return `maybe` and `$keyType->isString()->yes()` was interchangeable with `!$keyType->isString()->no()` - an equivalent mutant that no test can kill. Branch on `is_string($keyType->getValue())` instead, matching how ConstantArrayType itself tells constant keys apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed as What I changed — $keyValue = $keyType->getValue();
if ($keepStringKeys && is_string($keyValue)) {
$slotKey = 's' . $keyValue;Why not a test. Note the branch had moved while I was working — Verification: |
This reverts commit eae0576.
Summary
When using the spread operator in array literals with a value that is a union of constant arrays (e.g.
...($flag ? ['key' => true] : [])), PHPStan was degrading the result to a general array type likenon-empty-array<'key'|'other', bool>instead of preserving the precise array shapearray{other: bool, key?: true}.Changes
InitializerExprTypeResolver::getArrayType()insrc/Reflection/InitializerExprTypeResolver.phpto handle unions of multiple constant arrays when processing spread itemscount($constantArrays) === 1tocount($constantArrays) > 0$hasOffsetValueTypestracking when merged spread keys overlap with previously-set keysRoot cause
In
InitializerExprTypeResolver::getArrayType(), when a spread item's value was a union type likearray{spread: true}|array{},getConstantArrays()returned 2 constant arrays. The conditioncount($constantArrays) === 1failed, causing the code to fall through to the general fallback that called$arrayBuilder->degradeToGeneralArray(), losing the array shape information entirely.Analogous cases probed
OversizedArrayBuilder: Uses$valueType instanceof ConstantArrayType(single type only) — affects only arrays with >256 items, a rare edge case. Not fixed here.FuncCallHandlerarg unpacking ($callArg->unpack): Similarcount($constantArrays) === 1pattern for function call argument unpacking (used byarray_pushetc.). Different context with different semantics — not fixed here.count($constantArrays) === 1sites (NodeScopeResolver foreach, ConstantArrayType list-ness, ArrayType truncation): Inspected and confirmed to be unrelated to array literal spreading.Test
Added
tests/PHPStan/Analyser/nsrt/bug-14708.phpwith 9 test functions covering:Fixes phpstan/phpstan#14708