Fix phpstan/phpstan#13637: Array might not have offset, if array is deep#5203
Fix phpstan/phpstan#13637: Array might not have offset, if array is deep#5203phpstan-bot wants to merge 7 commits intophpstan:2.1.xfrom
Conversation
- Added recursive handling in ArrayType::setExistingOffsetValueType() for arrays whose item type is itself an array with constant array values (3+ nesting levels) - New regression test in tests/PHPStan/Analyser/nsrt/bug-13637.php - The root cause was that setExistingOffsetValueType() only handled the case where the item type was directly a constant array, falling through to a naive union for deeper nesting which re-introduced intermediate states with optional keys Closes phpstan/phpstan#13637
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
However, for 3+ levels of nesting, $this->itemType is a general ArrayType (not a constant array), so this path was skipped.
Why ?
… depth The previous fix only handled 3-level nesting by checking that inner value types were constant arrays. For 4+ levels, the intermediate value types are general ArrayTypes, so the condition failed. Generalize the condition to recurse through any depth of array nesting: - Check both types are non-constant arrays (constant arrays are handled by the existing block above) - Check both types' value types are arrays (ensuring there's deeper nesting structure to recurse into, excluding generic arrays like non-empty-array whose value type is mixed) The recursion naturally bottoms out when it reaches constant arrays at the deepest level. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
That was the earlier test suite run. Everything is already addressed — the fix has been committed and pushed. |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
Add test for 5 or 6 level deep
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Already retrieved this output earlier — tests completed with only a pre-existing unrelated failure in |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
For all the tests also assertType on $final inside and outside the for loop.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Added |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
An elseif coulbe use to save some computation
The two branches in setExistingOffsetValueType() are mutually exclusive (first checks isConstantArray()->yes(), second checks !isConstantArray()->yes()), so use elseif to make this explicit and save unnecessary condition evaluation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Already retrieved that output — PHPStan passed with no errors. Everything is done. |
|
Tests pass. Everything from the review has been addressed. The PR is approved and ready to merge - no further changes needed. |
Tests that when the item type is a union of array and non-array (isArray() returns maybe), the recursive elseif branch is correctly skipped and the fallback TypeCombinator::union path is used instead. Also tests the definite array case where the recursive path produces correct merged results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace unit tests in ArrayTypeTest with an NSRT test in bug-13637.php that covers the maybe-array item type scenario (union with non-array skips recursive path) alongside the existing deep nesting tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
When assigning to deeply nested arrays (3+ levels of
ArrayDimFetch), e.g.$final[$i][$j][$k]['abc'] = $i, PHPStan incorrectly marked subsequent keys like'def'as optional. This worked correctly for 2-level nesting but not for 3+.Changes
src/Type/ArrayType.phpinsetExistingOffsetValueType()that handles the case where the item type is an array whose value type is a constant arraysetExistingOffsetValueType()instead of falling through to the naiveTypeCombinator::union()fallbacktests/PHPStan/Analyser/nsrt/bug-13637.phpRoot cause
ArrayType::setExistingOffsetValueType()had special handling (lines 377-411) for when$this->itemTypeis a constant array — it would iterate the keys and properly merge them. However, for 3+ levels of nesting,$this->itemTypeis a generalArrayType(not a constant array), so this path was skipped. The fallback at line 413 performed a rawTypeCombinator::union($this->itemType, $valueType)which re-introduced intermediate type states where keys were missing, causing them to appear as optional.The fix adds an intermediate check: if both the current item type and the value type are arrays whose value types are constant arrays, recursively call
setExistingOffsetValueType()on the item type with the value's key/value types.Test
Added
tests/PHPStan/Analyser/nsrt/bug-13637.phpwith two functions:doesNotWork()— 3-level deep nesting that was previously broken (keys marked as optional)thisWorks()— 2-level deep nesting that already worked correctlyBoth use
assertType()to verify all keys are required (not optional).Fixes phpstan/phpstan#13637