Decompose union offset types in ConstantArrayType::flipArray() and fillKeysArray() to preserve per-key value precision#5719
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
Shouldn't the fix be directly inside ConstantArrayTypeBuilder::setOffsetValueType ?
|
This touches similar places the |
|
Pushed. Here's a summary of the changes made to address the reviewer's feedback: What changed: Moved the union offset value-merging logic from the callers ( How: Added a
Why a parameter is needed: The builder can't distinguish between "same runtime key, new value" (variable assignment — replace is correct) and "independent source elements mapping to overlapping output keys" (flipArray — union is correct) without caller context. The default Side benefit: The |
VincentLanglet
left a comment
There was a problem hiding this comment.
Given the fact we want to simulate the $optional behavior, what about calling the builder with something like
$builder->setOffsetValueType(
$valueType->toArrayKey(),
$keyType,
$this->isOptionalKey($i)->or(...),
);
instead of introducing a new param
|
Pushed. Here's what changed to address VincentLanglet's review: Removed the Builder changes (2 lines):
Extension change: Since All tests pass (12122 tests, 79747 assertions) and |
VincentLanglet
left a comment
There was a problem hiding this comment.
Remove any changes in the ConstantArrayBuilder
|
Pushed. The change reverts all modifications to |
a906bdb to
c4bd52f
Compare
I simplified the fix in order to minimize the possible conflict ; is it simple enough to be merge on 2.1.x branch now @ondrejmirtes ? |
|
I don't know yet. Please already think about how this change/fix would impact unsealed array shapes, what the fix would be on 2.2.x once merged. |
Looking at the implementation of unsealed array shaped on 2.2.x. I would say the answer is simple ; there is no conflict between this PR and the unsealed changes. While you added extra logic about sealed later |
|
What I mean is that - if you produce more similar NSRT tests with unsealed types involved, does this mean we need to write some more code on 2.2.x in order to fix them? So my question is - is there a similar bug that needs a fix for example when the unsealed key or value is a union of constant scalars... |
…`fillKeysArray()` to preserve per-key value precision - In `flipArray()`, when a value type decomposes into multiple constant scalar types (e.g. `'a'|'b'|'c'`), iterate over each scalar individually with `optional=true` instead of passing the full union to `setOffsetValueType`. This prevents the builder's optional-key-replacement logic from overwriting previous values and avoids partial-match degradation to general array types. - Apply the same decomposition in `fillKeysArray()` to fix the analogous partial-match degradation when overlapping union value types are used as keys. - Add `NonEmptyArrayType` wrapping in `ArrayFillKeysFunctionReturnTypeExtension` when the input array is non-empty, mirroring the existing logic in `ArrayFlipFunctionReturnTypeExtension`.
c4bd52f to
e54e2f6
Compare
Yes, I opened #5722 for the similar bug with unsealed keys. I'd like your comment on the right behavior to you (especially in how the array should be described) |
Summary
array_flip()on a constant array whose values are union types (e.g.array{0: 'a'|'b'|'c', 1: 'a'|'b'|'c', 2: 'a'|'b'|'c'}) produced wrong results — the last key's index overwrote all previous values, yieldingnon-empty-array{a?: 2, b?: 2, c?: 2}instead of the correctnon-empty-array{a?: 0|1|2, b?: 0|1|2, c?: 0|1|2}.The same underlying issue also caused
array_fill_keysto degrade to a general array type when overlapping union key types were involved.Changes
src/Type/Constant/ConstantArrayType.php: InflipArray(), when a value type decomposes into multiple constant scalar types, iterate over each individually withoptional=trueinstead of passing the full union tosetOffsetValueType. Applied the same decomposition infillKeysArray().src/Type/Php/ArrayFillKeysFunctionReturnTypeExtension.php: AddedNonEmptyArrayTypewrapping when the input array is non-empty, mirroring the logic already present inArrayFlipFunctionReturnTypeExtension.tests/PHPStan/Analyser/nsrt/bug-14656.php: Added regression tests for botharray_flipandarray_fill_keyswith union value types, including overlapping unions and mixed union/constant cases.Root cause
ConstantArrayType::flipArray()calledConstantArrayTypeBuilder::setOffsetValueType()with a union type as the offset andoptional=false. The builder's logic for union offsets matching existing optional keys (line 283 ofConstantArrayTypeBuilder) replaces existing values rather than unioning them when$optional=false. Each iteration offlipArray()passed a different key index as the value, so the last iteration's value (2) overwrote all earlier values (0, 1).For overlapping unions (e.g.
'a'|'b'followed by'b'|'c'), the builder's partial-match path ($hasMatch && !$match) degraded to a general array type, losing constant array precision entirely.The fix decomposes union offsets at the caller level —
flipArray()andfillKeysArray()now iterate over each constant scalar type individually withoptional=true, which:Analogous cases probed:
fillKeysArray()— same degradation bug found and fixedintersectKeyArray(),diffKeyArray(),changeKeyCaseArray(),filterArrayRemovingFalsey(),sliceArray(),spliceArray()— these use original key types (always constant, never union) as offsets, so they are not affectedTest
tests/PHPStan/Analyser/nsrt/bug-14656.phpcovers:array_flipwith all-union values (array{0: 'a'|'b'|'c', 1: 'a'|'b'|'c', 2: 'a'|'b'|'c'}) — the reported bugarray_flipwith overlapping unions (array{0: 'a'|'b', 1: 'b'|'c'})array_flipwith mixed union and constant (array{0: 'a'|'b', 1: 'c'})array_fill_keyswith the same three union patternsFixes phpstan/phpstan#14656