Skip to content

Decompose union offset types in ConstantArrayType::flipArray() and fillKeysArray() to preserve per-key value precision#5719

Open
phpstan-bot wants to merge 1 commit into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-ee85bdm
Open

Decompose union offset types in ConstantArrayType::flipArray() and fillKeysArray() to preserve per-key value precision#5719
phpstan-bot wants to merge 1 commit into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-ee85bdm

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

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, yielding non-empty-array{a?: 2, b?: 2, c?: 2} instead of the correct non-empty-array{a?: 0|1|2, b?: 0|1|2, c?: 0|1|2}.

The same underlying issue also caused array_fill_keys to degrade to a general array type when overlapping union key types were involved.

Changes

  • src/Type/Constant/ConstantArrayType.php: In flipArray(), when a value type decomposes into multiple constant scalar types, iterate over each individually with optional=true instead of passing the full union to setOffsetValueType. Applied the same decomposition in fillKeysArray().
  • src/Type/Php/ArrayFillKeysFunctionReturnTypeExtension.php: Added NonEmptyArrayType wrapping when the input array is non-empty, mirroring the logic already present in ArrayFlipFunctionReturnTypeExtension.
  • tests/PHPStan/Analyser/nsrt/bug-14656.php: Added regression tests for both array_flip and array_fill_keys with union value types, including overlapping unions and mixed union/constant cases.

Root cause

ConstantArrayType::flipArray() called ConstantArrayTypeBuilder::setOffsetValueType() with a union type as the offset and optional=false. The builder's logic for union offsets matching existing optional keys (line 283 of ConstantArrayTypeBuilder) replaces existing values rather than unioning them when $optional=false. Each iteration of flipArray() 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() and fillKeysArray() now iterate over each constant scalar type individually with optional=true, which:

  1. Correctly unions values across iterations (the builder's else branch at line 286)
  2. Avoids partial-match degradation (each call has a single scalar offset)

Analogous cases probed:

  • fillKeysArray() — same degradation bug found and fixed
  • intersectKeyArray(), diffKeyArray(), changeKeyCaseArray(), filterArrayRemovingFalsey(), sliceArray(), spliceArray() — these use original key types (always constant, never union) as offsets, so they are not affected

Test

  • tests/PHPStan/Analyser/nsrt/bug-14656.php covers:
    • array_flip with all-union values (array{0: 'a'|'b'|'c', 1: 'a'|'b'|'c', 2: 'a'|'b'|'c'}) — the reported bug
    • array_flip with overlapping unions (array{0: 'a'|'b', 1: 'b'|'c'})
    • array_flip with mixed union and constant (array{0: 'a'|'b', 1: 'c'})
    • array_fill_keys with the same three union patterns

Fixes phpstan/phpstan#14656

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the fix be directly inside ConstantArrayTypeBuilder::setOffsetValueType ?

@ondrejmirtes
Copy link
Copy Markdown
Member

This touches similar places the unsealed branch touches. Please fix this only on 2.2.x after the unsealed PR is merged (soon).

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

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 (flipArray() and fillKeysArray()) into ConstantArrayTypeBuilder::setOffsetValueType() itself, as the reviewer suggested.

How: Added a bool $mergeUnionValues = false parameter to setOffsetValueType() that controls two behaviors in the union-offset code path:

  1. "All match" case (line 283): When $mergeUnionValues is true, values are always unioned with existing values instead of being replaced. Without it, !$optional writes to optional keys replace values (correct for $result[$k] = 2 semantics).

  2. "Partial match" case: When $mergeUnionValues is true, matched keys get their values unioned and unmatched keys are added as new optional entries — instead of degrading to a general array type.

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 false preserves existing behavior for all current callers.

Side benefit: The ArrayFillKeysFunctionReturnTypeExtension no longer needs its own NonEmptyArrayType wrapping, since the builder's isNonEmpty is now correctly set via the $optional=false parameter that callers pass through.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Here's what changed to address VincentLanglet's review:

Removed the $mergeUnionValues parameter from ConstantArrayTypeBuilder::setOffsetValueType() entirely. Instead, the callers (flipArray() and fillKeysArray()) now compute $optional correctly: when the offset type has multiple constant scalar types (i.e., it's a union), each individual key is inherently optional, so $optional = true.

Builder changes (2 lines):

  • Line 284: reverted to the original if (!$optional && ...) — since union offsets now pass $optional=true, values are unioned instead of replaced
  • Line 306: changed if (!$hasMatch || $mergeUnionValues) to if (!$hasMatch || $optional) — partial matches with optional offsets keep matched values and add unmatched entries instead of degrading

Extension change: Since $optional=true prevents the builder from setting isNonEmpty, ArrayFillKeysFunctionReturnTypeExtension now wraps with NonEmptyArrayType when the input is non-empty (matching what ArrayFlipFunctionReturnTypeExtension already does), with a guard for non-array results (ErrorType).

All tests pass (12122 tests, 79747 assertions) and make phpstan reports no errors.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove any changes in the ConstantArrayBuilder

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. The change reverts all modifications to ConstantArrayTypeBuilder.php and instead decomposes union offset types in the callers (flipArray() and fillKeysArray()) — each individual scalar type is passed to the builder separately with optional=true, which correctly unions values and avoids partial-match degradation without touching the builder. All 12122 tests pass and make phpstan reports no errors.

@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-ee85bdm branch from a906bdb to c4bd52f Compare May 20, 2026 12:04
@VincentLanglet
Copy link
Copy Markdown
Contributor

This touches similar places the unsealed branch touches. Please fix this only on 2.2.x after the unsealed PR is merged (soon).

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 ?

@VincentLanglet VincentLanglet self-assigned this May 20, 2026
@ondrejmirtes
Copy link
Copy Markdown
Member

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.

@VincentLanglet
Copy link
Copy Markdown
Contributor

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.
I change this line

$this->isOptionalKey($i),

While you added extra logic about sealed later

if ($this->isUnsealed()->yes() && $this->unsealed !== null) {

@ondrejmirtes
Copy link
Copy Markdown
Member

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`.
@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-ee85bdm branch from c4bd52f to e54e2f6 Compare May 20, 2026 13:47
@VincentLanglet
Copy link
Copy Markdown
Contributor

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...

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)

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.

3 participants