Fix phpstan/phpstan#8270: False positive on array item modification#5189
Fix phpstan/phpstan#8270: False positive on array item modification#5189phpstan-bot wants to merge 21 commits intophpstan:2.1.xfrom
Conversation
acd8f5c to
7cb81d7
Compare
7cb81d7 to
390360d
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression tests for the issues
phpstan/phpstan#13623
phpstan/phpstan#13857
|
Already handled — all 11604 tests passed and the commits have been pushed. |
7c8b794 to
a4295bc
Compare
| assertType('non-empty-array<array{itemsCount: mixed, interval: mixed}>', $intervalResults); | ||
| assertType('array{itemsCount: mixed, interval: mixed}', $intervalResults[$key]); | ||
| $intervalResults[$key]['itemsCount'] += $itemsCount; | ||
| assertType('non-empty-array<array{itemsCount: (array|float|int), interval: mixed}>', $intervalResults); |
There was a problem hiding this comment.
I don't see why we previously expected the (array|float|int) here
There was a problem hiding this comment.
All tests pass.
Summary: The reviewer's comment on slevomat-foreach-array-key-exists-bug.php is an observation, not a change request. They're noting that the old expectation (array|float|int) was already wrong — and the new expectation mixed from our PR is correct. When itemsCount is mixed and += $itemsCount is performed, the old code (with unionValues=false) replaced ALL items' type with the arithmetic result, losing the mixed from unmodified elements. The fix correctly unions the types, preserving mixed.
No code changes are needed. I was unable to post a reply comment on the PR due to insufficient token permissions — you may want to reply directly acknowledging that the old (array|float|int) expectation was indeed incorrect and the new mixed correctly reflects that unmodified elements retain their original type.
|
The branch is clean and up to date. The review is an approval with no new changes requested — all previous feedback has been addressed. No action needed. |
|
The review from VincentLanglet is an approval with no changes requested. There's nothing to address — the PR is ready to merge. |
73f2d91 to
d301c50
Compare
|
I cannot re-reproduce the failling phpbench error locally using |
VincentLanglet
left a comment
There was a problem hiding this comment.
There is regression on shopwave code base
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Line src/Core/Profiling/Doctrine/ConnectionProfiler.php
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
156 Parameter #1 $array of function usort contains unresolvable type.
🪪 argument.unresolvableType
✏️ https://github.com/shopware/shopware/blob/061e131a2dffff2207bcc4585b1b3e090205304e/src/Core/Profiling/Doctrine/ConnectionProfiler.php#L156
156 Parameter #2 $callback of function usort contains unresolvable type.
🪪 argument.unresolvableType
✏️ https://github.com/shopware/shopware/blob/061e131a2dffff2207bcc4585b1b3e090205304e/src/Core/Profiling/Doctrine/ConnectionProfiler.php#L156
157 Property Shopware\Core\Profiling\Doctrine\ConnectionProfiler::$groupedQueries (array<string, array<int, array{sql: string, executionMS: float, types: array<int|string, Doctrine\DBAL\ParameterType|int>, params: Symfony\Component\VarDumper\Cloner\Data, runnable: bool, explainable: bool, backtrace?: list<array{function: string, line?: int, file?: string, class?: class-string, type?: '->'|'::', args?: list<mixed>, object?: object}>, count: int, ...}>>|null) does not accept non-empty-array
<string, array<int, non-empty-array<'backtrace'|'count'|'executionMS'|'executionPercent'|'explainable'|'index'|'params'|'runnable'|'sql'|'types', mixed>>>.
🪪 assign.propertyType
✏️ https://github.com/shopware/shopware/blob/061e131a2dffff2207bcc4585b1b3e090205304e/src/Core/Profiling/Doctrine/ConnectionProfiler.php#L157
168 Method Shopware\Core\Profiling\Doctrine\ConnectionProfiler::getGroupedQueries() should return array<string, array<int, array{sql: string, executionMS: float, types: array<int|string, Doctrine\DBAL\ParameterType|int>, params: Symfony\Component\VarDumper\Cloner\Data, runnable: bool, explainable: bool, backtrace?: list<array{function: string, line?: int, file?: string, class?: class-string, type?: '->'|'::', args?: list<mixed>, object?: object}>, count: int, ...}>> but returns array<stri
ng, list<non-empty-array<'backtrace'|'count'|'executionMS'|'executionPercent'|'explainable'|'index'|'params'|'runnable'|'sql'|'types', mixed>>>.
🪪 return.type
✏️ https://github.com/shopware/shopware/blob/061e131a2dffff2207bcc4585b1b3e090205304e/src/Core/Profiling/Doctrine/ConnectionProfiler.php#L168
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
7e78a98 to
d301c50
Compare
- When modifying a sub-key of an array element at a non-constant offset (e.g. $list[$k]['test'] = true), PHPStan incorrectly replaced the item type for ALL elements instead of unioning with the original item type - Added shouldUnionExistingItemType() check in AssignHandler to detect when the composed value changes existing constant-array key values, forcing a union to preserve unmodified elements' types - Updated test expectations in bug-11679 and slevomat-foreach tests to reflect the more correct union behavior - New regression test in tests/PHPStan/Analyser/nsrt/bug-8270.php Closes phpstan/phpstan#8270
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The original bug report was about a false positive "If condition is always true." error, so this adds a non-regression test in IfConstantConditionRuleTest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The original bug report was about a false positive "Offset ... on left side of ??= always exists and is not nullable." error, so this adds a non-regression test in NullCoalesceRuleTest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Point IfConstantConditionRuleTest and NullCoalesceRuleTest to the existing nsrt test files instead of maintaining duplicate copies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 2fb61a3.
d301c50 to
980a943
Compare
The original check only tested one direction ($newValue->isSuperTypeOf($existingValue)), which triggered union for cases like += and ++ operations where the existing type was already wider (e.g., 0|float vs float). This caused type degradation through loop iterations, producing *ERROR* types in the shopware-connection-profiler test. The bidirectional check skips union when either type is a supertype of the other, only triggering for genuinely incompatible types (e.g., false -> true, 'foo' -> 'bar'). This fixes the shopware regression while preserving the core bug-8270 fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The fix has been pushed. Here's a summary of the changes: Problem: The Fix: Added a bidirectional supertype check — union is only triggered when neither type is a supertype of the other (genuinely incompatible types like Test results: All 11613 tests pass, |
Summary
When modifying a sub-key of an array element at a non-constant offset (e.g.,
$list[$k]['test'] = true), PHPStan incorrectly concluded that ALL array items had the modified value. This caused false positives like "If condition is always true" when iterating over the array and checking the modified key.Changes
shouldUnionExistingItemType()method insrc/Analyser/ExprHandler/AssignHandler.phpthat checks whether a composed value changes existing constant-array key valuesproduceArrayDimFetchAssignValueToWrite()to forceunionValues=trueat the outermost dimension when the composed value has conflicting key values with the existing item typetests/PHPStan/Rules/Arrays/data/bug-11679.phpandtests/PHPStan/Rules/Arrays/data/slevomat-foreach-array-key-exists-bug.phpto match the more correct union behaviortests/PHPStan/Analyser/nsrt/bug-8270.phpRoot cause
In
produceArrayDimFetchAssignValueToWrite(), nested array dimension assignments like$list[$k]['test'] = trueare processed bottom-up. The inner dimension correctly computes the modified element value (e.g.,array{test: true, value: int}). However, at the outer dimension,setOffsetValueType()was called withunionValues=false, which replaced the entire item type with the composed value. Since the offset key is non-constant (could match any element), this incorrectly assumed ALL elements had the new value.The fix detects when the composed value changes existing key values (e.g.,
test: false→test: true) by comparing key-by-key. When a conflict is found, it forcesunionValues=trueso the item type becomes the union of old and new (e.g.,test: bool), correctly representing that some elements retain their original values.The check is limited to the outermost dimension to avoid issues with deeply nested array building patterns (like bug-1388).
Test
Added
tests/PHPStan/Analyser/nsrt/bug-8270.phpwith two test functions:@var-annotatednon-empty-listand constant offset$list[0]['test'] = truearray_key_first(), modifying one element, then iteratingBoth verify that after modifying one element's sub-key, the foreach item type correctly shows
bool(nottrue) for the modified key.Fixes phpstan/phpstan#8270
Closes phpstan/phpstan#13623
Closes phpstan/phpstan#13857