Fix oversized-array self-rejection in optimizeConstantArrays#5568
Fix oversized-array self-rejection in optimizeConstantArrays#5568gnutix wants to merge 3 commits intophpstan:2.1.xfrom
optimizeConstantArrays#5568Conversation
The oversized-array generalization in `TypeCombinator` could produce a
type that was not a super-type of the variants it was derived from,
causing `generator.valueType` (and similar) false positives where a
closure's inferred Generator value type rejected the very yields it
was synthesised from.
Two distinct issues both produced contradictory
`*&oversized-array` intersections or asymmetric shapes that
`processArrayTypes` failed to unify cleanly.
1. `processArrayTypes` (`~line 962`) applied non-empty/oversized
accessory types to reduced array results that were already known
empty (`isIterableAtLeastOnce()->no()`), producing `array{}&oversized-array`.
Now filter those accessories out for empty results.
2. `optimizeConstantArrays`'s stage 2 inner traverser
(`~line 1071`), when walking a value position whose type was a
`UnionType` containing a constant array, fell through via the
outer `$traverse` instead of `$innerTraverse`. This re-entered
the outer callback's full generalization branch
(`array<intKey, V>&accessories`) for sealed shapes reached
through a union, while sealed shapes reached directly were only
wrapped (`array{...}&oversized-array`). Mixing those two shapes
left `processArrayTypes` with a union it could not unify
cleanly. Also skip wrapping empty constant arrays at the inner
level — that produces the same contradictory
`array{}&oversized-array`.
The regression was introduced in 2.1.52 by commit `2f66c45222`
("Preserve constant array when assigning a union of scalars"),
which is itself a correct precision improvement; it exposed both
latent bugs in `TypeCombinator` downstream.
Authored by Claude (Opus 4.7) under the user's review.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Satisfies SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly — follow-up to the previous commit which added an `array_filter` call without a matching `use function` statement. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The issue isn't simply the fact you're using numeric-string as key ? See |
|
@VincentLanglet Sorry, misleading playground example. Our project uses textual keys, Claude simplified with numeric keys when making a "generic" example. Here's a new one : https://phpstan.org/r/5b811780-35b4-4ebf-b2a6-cecb90f02b2b I've also adapted the test added in this PR to not use numeric keys, and it fails (locally) without the fix part too. Edit: tested 2.1.54, same issue. |
While I understand it, I'm surprise by the fact we ending trying to add NonEmptyArrayType to an empty array in the first place... I'm not sure a dedicated check for this accessory is the right thing to do cause we could have the same with some Do you mind opening a dedicated issue on https://github.com/phpstan/phpstan ? |
2 participants
Summary
Fixes a regression introduced in 2.1.52 where the oversized-array generalization in
TypeCombinatorproduced a type that was not a super-type of the variants it was derived from, causinggenerator.valueType(and similar) false positives where a closure's inferred Generator value type rejected the very yields it was synthesised from.This work was authored by Claude (Opus 4.7) — bug analysis, root-cause diagnosis, and the patch. It bisected the regression to commit 2f66c45 ("Preserve constant array when assigning a union of scalars"), found that commit was itself a correct precision improvement, and located the two latent
TypeCombinatorbugs it exposed.The fix is two changes in
src/Type/TypeCombinator.php:processArrayTypes(~line 962): when applying common array accessory types to a reduced array result that is already known empty (isIterableAtLeastOnce()->no()), skipOversizedArrayTypeandNonEmptyArrayType. Otherwise the intersection produces a contradictoryarray{}&oversized-arraythat rejects the very value it represents.optimizeConstantArraysstage 2 inner traverser (~line 1071): fall through via$innerTraverse, not the outer$traverse. The outer callback fully generalizes a sealedConstantArrayTypeintoarray<intKey, V>&..., which is correct at the top level but wrong inside a value position. Re-entering it through the outer traverser produced asymmetric treatment — a sealedarray{a: 1}reached viaarray{}|array{a: 1}got fully generalized while one reached directly was only wrapped, leavingprocessArrayTypeswith a mix of shapes it could not unify cleanly. The same callback also now skips wrapping empty constant arrays for the samearray{}&oversized-arrayreason as Fix A.Both parts were verified to be independently necessary: reverting either alone re-fails the targeted regression test. Claude decomposed Fix B into its two sub-changes and tested each in isolation.
A symptomatic playground reproducer is at phpstan.org/r/5b811780-35b4-4ebf-b2a6-cecb90f02b2b (
discarded example - with numeric keys - at https://phpstan.org/r/916330d2-806c-426c-9220-cf99aba58a38).Test plan
testBugYieldOversizedSelfRejectionregression test intests/PHPStan/Rules/Generators/YieldTypeRuleTest.phppasses —tests/PHPStan/Rules/Generators/data/bug-yield-oversized-self-rejection.phpis a domain-agnostic version of the playground reproducer that exhibits the bug at level max withbleedingEdge: true.make tests— 11993 tests, 79545 assertions, 0 new failures (2 pre-existing unrelated failures inAccessPropertiesInAssignRuleTest::testBug14063andCatchWithUnthrownExceptionRuleTest::testBug14479confirmed pre-existing on the unmodified2.1.xHEAD).