Repository navigation
Do not generalize a union of constant arrays that is no bigger than its largest member in TypeCombinator::optimizeConstantArrays() - #6829
Open
phpstan-bot wants to merge 1 commit into
Conversation
…ts largest member in `TypeCombinator::optimizeConstantArrays()` - `optimizeConstantArrays()` generalized any union whose constant array value count exceeded `ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT`, even when the union reduced to a single shape no bigger than one of its own members. A declared shape with more than 256 values (nested ones included) is kept as-is on its own, but after `$r['opt'] ?? null` (or `isset`, `array_key_exists`, `empty`, `!== null`) the scope merge unions it with its narrowed variant, and the merged result was degraded to `non-empty-array<..., ...>&oversized-array`. - The threshold is now raised to the largest per-member count of the unioned types (new `getConstantArrayValueTypesLimit()`), computed only once the total is already over the plain limit. Both callers in `processArrayTypes()` pass the types being unioned. - Ported the same change to the native mirror in `turbo-ext/src/TypeCombinator.cpp`. - Probed: big list shapes (fixed by the same change, test added); declared unions of a large shape with another shape (already generalized when they are declared, which is long-standing behavior and left unchanged); shapes with more than 256 *keys* (degraded by `ConstantArrayTypeBuilder::createFromConstantArray()`, an older design decision, left unchanged).
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.
Summary
Since 2.3.0, reading an optional key of a large declared array shape with
??could return(array{...}&oversized-array)|string|null, because the shape had been degraded to an oversized general array. That happened when the shape held more than 256 constant array values, nested ones included. This PR keeps the shape precise when a union of constant arrays is no bigger than its largest member.Changes
src/Type/TypeCombinator.phpoptimizeConstantArrays()now also receives the types being unioned. When the total value count goes overConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT, it raises the threshold to the largest count held by a single member, using the newgetConstantArrayValueTypesLimit(). It generalizes only when the result is still over that threshold.processArrayTypes()(the reduced constant-array path and the general-array value-type path) pass their input types.turbo-ext/src/TypeCombinator.cpp: the same change in the native mirror.Probed analogous cases:
isset,array_key_exists,!empty,(... ?? null) !== nullandisset(...) ? ... : nullall went through the same scope merge and were broken. They are fixed by this change and covered by the test.ConstantArrayTypeBuilder::createFromConstantArray()whenever an offset is set on it, including theHasOffsetValueTypeintersection from??. This is older behavior (2023), not this regression, and is left unchanged.Root cause
$r['o0'] ?? nullnarrows$rin one branch, and the scopes are then merged byTypeCombinator::union().reduceArrays()correctly merges the two variants back into the single original shape.optimizeConstantArrays()then counted that shape's constant array values (197 + 20 × 3 = 257), compared the count against the fixed limit of 256, and generalized it. That limit exists to stop unions of many constant arrays from growing. A union that is no bigger than one of its members does not grow anything, so generalizing it only lost precision. A single large shape that never goes through a union was already left alone, so the type depended on whether a scope merge had happened.Test
tests/PHPStan/Analyser/nsrt/bug-15459.phpis generated from the issue's script and covers:stringkeys + 20 optional nested keys, read twice with??)isset,array_key_exists,!empty,!== nulland ternary-isset narrowingstring+ 60 optional nested entries)The test fails without the fix, passes with it, and also passes with the PHP fix stashed and the native extension loaded.
Fixes phpstan/phpstan#15459