Fix phpstan/phpstan#14324: Foreach on constant array with closures reaching 32 entries causes crash#5246
Conversation
…y has 32+ closures - Disable array degradation in ConstantArrayType::getAllArrays() builder - The builder was degrading to a general array due to CLOSURES_COUNT_LIMIT, causing a ShouldNotHappenException since getAllArrays() expects ConstantArrayType - New regression test in tests/PHPStan/Analyser/nsrt/bug-14324.php
The original problem was an internal error/crash, so the regression test belongs in AnalyserIntegrationTest rather than as an nsrt type inference test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| @@ -241,6 +241,7 @@ public function getAllArrays(): array | |||
| } | |||
|
|
|||
| $builder = ConstantArrayTypeBuilder::createEmpty(); | |||
There was a problem hiding this comment.
do we have similar code in the codebase which asserts the result of a getArray-method call on a ConstantArrayTypeBuilder object ?
There was a problem hiding this comment.
Both tests and static analysis pass.
The reviewer's question was informational — asking whether there are similar patterns elsewhere in the codebase. The answer is no: the getAllArrays() assertion at ConstantArrayType.php:250 is the only place that asserts getArray() returns a ConstantArrayType. No code changes are needed beyond what's already in the PR. There's nothing to commit or push.
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
the fix makes sense to me. ConstantArrayType->getAllArrays expects/assumes a array of ConstantArrayType will be returned.
therefore we cannot/should not degrade arrays in this path.
|
The review from staabm is an approval with no requested code changes. They're confirming the fix is correct — Since this is a pure approval with no action items, there are no changes to make, commit, or push. |
|
The review from VincentLanglet is an approval with no requested code changes. There's nothing to address — the PR is approved by both staabm and VincentLanglet. |
…aching 32 entries causes crash (phpstan#5246)
3 participants
Summary
Fixes a crash (internal error) when analysing a foreach over a constant array that contains 32 or more closure values. The crash was introduced in 2.1.41 and occurs because
ConstantArrayType::getAllArrays()rebuilds the array usingConstantArrayTypeBuilder, which degrades to a general array when the closure count limit (32) is reached.Changes
$builder->disableArrayDegradation()call inConstantArrayType::getAllArrays()(src/Type/Constant/ConstantArrayType.php) to prevent the builder from degrading an already-existing constant array during reconstructiontests/PHPStan/Analyser/nsrt/bug-14324.phpRoot cause
ConstantArrayType::getAllArrays()reconstructs subsets of the array usingConstantArrayTypeBuilder. When the array contains 32+ closures, the builder'sCLOSURES_COUNT_LIMITtriggers degradation to a general array type. The method then fails at the$array instanceof selfassertion since the builder returned a non-constant array type. The fix disables array degradation in the builder sincegetAllArrays()is simply reconstructing subsets of an already-valid constant array.Test
Added
tests/PHPStan/Analyser/nsrt/bug-14324.phpwhich reproduces the original crash scenario: a class with a static map of 29 closures that gets 3 more added in a foreach loop, breaching the 32-closure limit.Fixes phpstan/phpstan#14324