Fix phpstan/phpstan#14407: !in_array with enum values incorrectly narrows to *NEVER*#5365
Conversation
| if ($guaranteedValueType === null) { | ||
| $guaranteedValueType = $innerValueType; | ||
| } else { | ||
| $guaranteedValueType = TypeCombinator::intersect($guaranteedValueType, $innerValueType); |
There was a problem hiding this comment.
For performance use a single intersect call at the end of the foreach
There was a problem hiding this comment.
Done. The change collects all inner value types into an array and passes them to a single TypeCombinator::intersect(...$innerValueTypes) call instead of intersecting incrementally in the loop, as requested by VincentLanglet. Tests and static analysis both pass.
VincentLanglet
left a comment
There was a problem hiding this comment.
Add test for union of constant array with optional keys.
/**
* @param 'a'|'b'|'c' $x
* @param array{a: 'a', c: 'c'}|array{a?:'a', b: 'b'} $a
*/
function test($x, $a): void
{
assertType("array{a: 'a', c: 'c'}|array{a?: 'a', b: 'b'}", $a);
if (!\in_array($x, $a, true)) {
assertType("'a'|'b'|'c'", $x);
}
}
|
Done. The changes:
|
VincentLanglet
left a comment
There was a problem hiding this comment.
All this computation is only useful if we're in the case
count($needleType->getFiniteTypes()) > 0
&& $arrayType->isIterableAtLeastOnce()->yes()
So refactor to avoid the unnecessary computation
|
Done. The refactoring moves the expensive |
VincentLanglet
left a comment
There was a problem hiding this comment.
Simplify this with a dedicated private method to compute the narrowingValueType in the context false
|
Done. Extracted the narrowing value type computation into a private |
VincentLanglet
left a comment
There was a problem hiding this comment.
With a computeNeedleNarrowingValue method you could write
$narrowingValue = $this->computeNeedleNarrowingValue(...);
if (null !== computeNeedleNarrowingValue) {
...
}
This method would return arrayValueType in context true, and so on
|
Background tests also completed successfully. All clear - the commit is pushed and both static analysis and tests pass. |
VincentLanglet
left a comment
There was a problem hiding this comment.
Remove the computeGuaranteedValueType method and move the implementation directly
|
Done. Inlined |
VincentLanglet
left a comment
There was a problem hiding this comment.
When count($innerValueTypes) === 0 you should return null rather than $arrayValueType no ?
|
Tests already confirmed passing — all 11708 tests green, static analysis clean. The review feedback has been addressed and pushed. |
| $innerTypes = $arrayType instanceof UnionType ? $arrayType->getTypes() : [$arrayType]; | ||
| $innerValueTypes = []; | ||
| foreach ($innerTypes as $innerType) { |
There was a problem hiding this comment.
Rather than iterating on innerTypes then constantArrays you could directly iterate on Type::getArrays
There was a problem hiding this comment.
All done — both background tasks completed successfully (phpstan clean, no errors). The review feedback has been addressed, committed, and pushed.
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression test
/**
* @param 'a'|'b'|'c' $x
* @param non-empty-array<'a'|'b'> $a
*/
function testUnionWithOptionalKeys($x, $a): void
{
assertType("non-empty-array<'a'|'b'>", $a);
if (!\in_array($x, $a, true)) {
assertType("'a'|'b'|'c'", $x);
}
};
Because even non empty, a non-contant-array can only guarantee a value if it's the only one (ie non-empty-array<'a'>).
I don't think it's worth it to handle the general array case, so soolve this by just iterating on constantArrays.
|
Done. Changes pushed:
|
60bf3ff to
7dd643d
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Fix the static analysis build by updating the baseline
|
Done. Added the |
VincentLanglet
left a comment
There was a problem hiding this comment.
Add non regression test for:
|
Tests background task also confirmed complete — 11710 tests passing. Everything is already committed and pushed. |
|
|
||
| $filteredBars = array_filter($bars, fn (Bar $bar) => in_array($bar, $nonEmptyFilterArray)); | ||
|
|
||
| assertType("array{0?: Bug13421\Bar::Bar1, 1?: Bug13421\Bar::Bar2, 2?: Bug13421\Bar::Bar3}", $filteredBars); |
There was a problem hiding this comment.
the origin example contains a error like
Strict comparison using === between 0 and 0 will always evaluate to true
we should have a rule-test, this no longer happens.
use the tests/PHPStan/Analyser/nsrt/bug-13421.php file in the rule-test - don't create another copy of this file.
There was a problem hiding this comment.
Full test suite also passed. All 11710 tests green, static analysis clean. The review feedback from staabm has been addressed and pushed.
|
|
||
| declare(strict_types = 1); | ||
|
|
||
| namespace Bug14407; |
There was a problem hiding this comment.
the origin example contains a error like
Strict comparison using === between NEVER and SomeEnum::A will always evaluate to false.
we should have a rule-test, this no longer happens.
use the tests/PHPStan/Analyser/nsrt/bug-14407.php file in the rule-test - don't create another copy of this file.
…rows to *NEVER*
- In false context (!in_array), compute guaranteed array value type by
intersecting value types across union members instead of using the
full union of all possible values
- For array{A}|array{B}, neither A nor B is guaranteed in every variant,
so no narrowing should occur
- For array{A, B}, both A and B are guaranteed, so narrowing to *NEVER*
remains correct
- New regression test in tests/PHPStan/Analyser/nsrt/bug-14407.php
Instead of incrementally intersecting types in a loop, collect all inner value types and pass them to a single TypeCombinator::intersect() call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When computing guaranteed values for the false context of in_array(),
exclude values from optional keys in constant arrays since they may
not be present at runtime. For array{a: 'a', c: 'c'}|array{a?: 'a', b: 'b'},
the value 'a' from the optional key should not be considered guaranteed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n_array Move the guaranteed value type computation inside the guard condition that checks for finite types and non-empty array, so it's only performed when actually needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract the narrowing value type computation for the !in_array false context into a dedicated private method for better readability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ext logic Consolidates the needle narrowing logic into a single computeNeedleNarrowingType method that returns the narrowing type or null. This eliminates duplicated typeSpecifier->create + AlwaysRememberedExpr handling for both contexts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ngType Remove the separate computeGuaranteedValueType method and move its implementation directly into computeNeedleNarrowingType, as suggested in review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pes is empty Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use $arrayType->getArrays() to iterate over array variants directly, instead of manually decomposing union types and then calling getConstantArrays(). This simplifies the iteration and avoids instanceof UnionType checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ypes For non-constant arrays like `non-empty-array<'a'|'b'>`, we cannot guarantee any specific value is present, so no narrowing should occur in the false context. Only narrow when the value type has exactly one finite type (e.g. `non-empty-array<'a'>`). Added regression test for this case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n#8864 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add rule tests for bug-14407 and bug-13421 to verify that the strict comparison rule no longer reports false positive errors on these files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a96e8e0 to
2b059ba
Compare
…rows to *NEVER* (phpstan#5365) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Vincent Langlet <vincentlanglet@hotmail.fr>
Summary
When
!in_array($x, $arr)is used where$xis an enum case and$arris a non-empty array that may contain different enum cases in different branches (e.g.,array{A}|array{B}), PHPStan incorrectly narrowed$xto*NEVER*. This happened because the type specifier used the full union of all possible array values for narrowing, even though no single value was guaranteed to be present in every array variant.Changes
src/Type/Php/InArrayFunctionTypeSpecifyingExtension.phpto compute a "guaranteed value type" in the false context by intersecting the iterable value types of all union members of the array typearray{A}|array{B}, the intersection of value typesAandBisnever, so no narrowing occursarray{A, B}, behavior is unchanged since there's only one variantRoot cause
InArrayFunctionTypeSpecifyingExtensionused$arrayType->getIterableValueType()for narrowing in the false context. For aUnionTypeof arrays,getIterableValueType()returns the union of all possible values across all variants. But in the false context (!in_array), we can only safely remove values from the needle type that are guaranteed to be in every possible array variant. The fix computes this guaranteed type by intersecting value types across union members.Test
Added
tests/PHPStan/Analyser/nsrt/bug-14407.phpwith three test cases:array{A}|array{B}- needle should NOT be narrowed to*NEVER*array{A, B}- needle should correctly be narrowed to*NEVER*Fixes phpstan/phpstan#14407
Fixes phpstan/phpstan#13421
Fixes phpstan/phpstan#8864