Skip to content

Fix phpstan/phpstan#8270: False positive on array item modification#5189

Open
phpstan-bot wants to merge 21 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-w8d4otm
Open

Fix phpstan/phpstan#8270: False positive on array item modification#5189
phpstan-bot wants to merge 21 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-w8d4otm

Conversation

@phpstan-bot
Copy link
Collaborator

@phpstan-bot phpstan-bot commented Mar 10, 2026

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

  • Added shouldUnionExistingItemType() method in src/Analyser/ExprHandler/AssignHandler.php that checks whether a composed value changes existing constant-array key values
  • Modified produceArrayDimFetchAssignValueToWrite() to force unionValues=true at the outermost dimension when the composed value has conflicting key values with the existing item type
  • Updated test expectations in tests/PHPStan/Rules/Arrays/data/bug-11679.php and tests/PHPStan/Rules/Arrays/data/slevomat-foreach-array-key-exists-bug.php to match the more correct union behavior
  • Added regression test in tests/PHPStan/Analyser/nsrt/bug-8270.php

Root cause

In produceArrayDimFetchAssignValueToWrite(), nested array dimension assignments like $list[$k]['test'] = true are 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 with unionValues=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: falsetest: true) by comparing key-by-key. When a conflict is found, it forces unionValues=true so 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.php with two test functions:

  1. A simple case with a @var-annotated non-empty-list and constant offset $list[0]['test'] = true
  2. A realistic case matching the original bug report: building a list in a loop, narrowing with array_key_first(), modifying one element, then iterating

Both verify that after modifying one element's sub-key, the foreach item type correctly shows bool (not true) for the modified key.

Fixes phpstan/phpstan#8270
Closes phpstan/phpstan#13623
Closes phpstan/phpstan#13857

@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-w8d4otm branch 2 times, most recently from acd8f5c to 7cb81d7 Compare March 18, 2026 22:35
@VincentLanglet VincentLanglet self-assigned this Mar 18, 2026
@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-w8d4otm branch from 7cb81d7 to 390360d Compare March 18, 2026 22:42
Copy link
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add non regression tests for the issues
phpstan/phpstan#13623
phpstan/phpstan#13857

@phpstan-bot
Copy link
Collaborator Author

Already handled — all 11604 tests passed and the commits have been pushed.

@staabm staabm force-pushed the create-pull-request/patch-w8d4otm branch from 7c8b794 to a4295bc Compare March 19, 2026 05:49
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we previously expected the (array|float|int) here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@phpstan-bot
Copy link
Collaborator Author

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.

VincentLanglet
VincentLanglet previously approved these changes Mar 19, 2026
@phpstan-bot
Copy link
Collaborator Author

The review from VincentLanglet is an approval with no changes requested. There's nothing to address — the PR is ready to merge.

@staabm staabm force-pushed the create-pull-request/patch-w8d4otm branch 2 times, most recently from 73f2d91 to d301c50 Compare March 19, 2026 11:26
@staabm
Copy link
Contributor

staabm commented Mar 19, 2026

I cannot re-reproduce the failling phpbench error locally using hyperfine, or blackfire.

Copy link
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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                                                                                                                                                                                                                                                                                                                                                             
 ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 

@VincentLanglet VincentLanglet dismissed their stale review March 19, 2026 13:47

Regressions

@VincentLanglet VincentLanglet force-pushed the create-pull-request/patch-w8d4otm branch from 7e78a98 to d301c50 Compare March 19, 2026 13:49
@phpstan phpstan deleted a comment from phpstan-bot Mar 19, 2026
github-actions bot and others added 7 commits March 19, 2026 16:58
- 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>
@staabm staabm force-pushed the create-pull-request/patch-w8d4otm branch from d301c50 to 980a943 Compare March 19, 2026 15:58
Copy link
Contributor

@staabm staabm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix the failling test

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>
@phpstan-bot
Copy link
Collaborator Author

The fix has been pushed. Here's a summary of the changes:

Problem: The shouldUnionExistingItemType method used a unidirectional check ($newValue->isSuperTypeOf($existingValue)) which triggered union too aggressively. Operations like += executionMS (where existing type 0|float and new type float) incorrectly triggered union because float is not a supertype of 0|float. This cascaded through loop iterations, producing degraded *ERROR* types in the shopware-connection-profiler test.

Fix: Added a bidirectional supertype check — union is only triggered when neither type is a supertype of the other (genuinely incompatible types like falsetrue or 'foo''bar'). When one direction holds (e.g., 0|floatfloat), the union is skipped since the modification doesn't create a type conflict.

Test results: All 11613 tests pass, make phpstan reports no errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants