Narrow array element type after a keyless foreach whose body guards each element with an early exit#5877
Open
phpstan-bot wants to merge 1 commit into
Open
Conversation
… each element with an early exit - Generalize the post-loop array rewrite in NodeScopeResolver::processStmtNode() so it also fires for `foreach ($a as $v)` (no key variable). Previously the rewrite required `$stmt->keyVar !== null`, so type guards like `if (!is_string($v)) return;` never narrowed the iterated array. - Track iterations through the original value expression (OriginalForeachValueExpr) when no key variable is present, instead of the original key expression, and read the narrowed element type directly from the value variable rather than from an ArrayDimFetch (which requires a key). - Reassigning the value variable inside the loop still invalidates the tracking expression, so narrowing is correctly skipped in that case; loops that `break` out are still excluded. - This also covers the analogous keyless constructs that share the same code path: `instanceof` guards, `throw`, by-reference value variables, list vs non-list arrays, and `continue` (which correctly does not narrow). - Update tests/PHPStan/Analyser/nsrt/bug-7076.php (noKeyVar): the value type is now narrowed even without a key variable. - Remove a now-provably-dead defensive `instanceof ClassConstFetch` re-check in MatchHandler: the pre-validation loop already guarantees it via `break 2`, which the sharper inference now understands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When an array is iterated with a keyless
foreachand the loop body validates eachelement with an early-exit guard, PHPStan failed to understand that, after the loop,
every element satisfies the guard. For example:
PHPStan already supported the keyed form (
foreach ($a as $k => $v)), but thepost-loop array rewrite was gated on a key variable being present. This PR
generalizes that mechanism to the keyless form, so
$idsis now correctly narrowedto
list<string>after the loop.Changes
src/Analyser/NodeScopeResolver.php(processStmtNode(), foreach handling):$stmt->keyVar !== nullgate with($keyVarExpr !== null || $originalValueExpr !== null).narrowing may be projected back onto the array: the original key expression when
a key variable exists, otherwise the original value expression
(
OriginalForeachValueExpr).(narrowed) value variable instead of from
new ArrayDimFetch($expr, $keyVar),which is impossible without a key. Key-type remapping is skipped in that case.
tests/PHPStan/Analyser/nsrt/bug-7076.php: thenoKeyVarcase asserted the old,limited behavior (
array<int|string, mixed>); updated to the now-correctarray<int|string, string>.src/Analyser/ExprHandler/MatchHandler.php: removed a defensiveif (!$cond instanceof Expr\ClassConstFetch) throw …re-check that the sharperinference now flags as always-true. The preceding pre-validation loop already
guarantees every reached condition is an enum-case
ClassConstFetch(it doesbreak 2otherwise), so the check was dead.Root cause
The post-loop array element/key rewrite in
NodeScopeResolverwas written only forthe keyed
foreach, where each iteration is identified byOriginalForeachKeyExprand the element is addressed via
$array[$key]. The keyless form has no keyexpression and no addressable dim fetch, so the whole block was skipped — the array
type never picked up the element narrowing that the body had proven. The fix tracks
the keyless case through
OriginalForeachValueExpr(which is always assigned inMutatingScope::enterForeach(), independent of the key) and rewrites the array'svalue type from the value variable directly.
Analogous cases probed
Because the single code path now handles all keyless guards, the following sibling
constructs were verified to narrow correctly with the same fix (tests added in
bug-5755.phpfor the representative ones):is_string/is_int/… guards withreturn— the reported case.instanceofguards.throwas the early exit (in addition toreturn).foreach ($a as &$v)).array<int|string, …>), in addition tolist<…>.continueguards correctly do not narrow (non-matching elements remain).Test
tests/PHPStan/Analyser/nsrt/bug-5755.phpreproduces every snippet linked inthe issue (the original
Test::Value()/Test::Strings()example and the reducedvalidate()example) plus the analogous cases above, usingassertType(). Theassertions fail without the fix (the array stays un-narrowed) and pass with it.
bug-7076.phpto reflect the corrected behavior.NodeScopeResolverTest, the Analyser suite, the Comparison/DeadCode/Matchrule tests and PHPStan self-analysis all pass.
Fixes phpstan/phpstan#5755