Fix phpstan/phpstan#9444: return statement is missing#5172
Open
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Open
Fix phpstan/phpstan#9444: return statement is missing#5172phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Conversation
- Exclude always-terminating (NeverType) match arm scopes from post-match scope merge in MatchHandler, so variables are correctly narrowed after a match with `default => throw` - Re-evaluate for loop condition against the real processing's loop scope to detect when body narrowing makes the condition always true - New regression test in tests/PHPStan/Rules/Missing/data/bug-9444.php Closes phpstan/phpstan#9444
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
PHPStan falsely reported "return statement is missing" for a method containing a
forloop withtry { return ... } catch { match(...) { ..., default => throw $e } }. The method always either returns or throws, but PHPStan couldn't prove it.Changes
src/Analyser/ExprHandler/MatchHandler.php: When merging arm body scopes after an exhaustive match expression, exclude arms whose body type isNeverType(e.g.,default => throw $e). This correctly narrows the match subject variable to only the values from non-throwing arms in the post-match scope.src/Analyser/NodeScopeResolver.php: After the real processing of aforloop body, re-evaluate the loop condition against the loop scope (body result + loop expressions). If the condition is always true, upgrade$alwaysIteratesto YES. This allows the narrowing from within the loop body to influence whether the loop is recognized as always iterating.tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php: AddedtestBug9444test method.tests/PHPStan/Rules/Missing/data/bug-9444.php: Regression test reproducing the issue.Root cause
Two issues combined to cause the false positive:
Match scope pollution: The match handler merged ALL arm body scopes (including always-terminating arms like
default => throw $e) into the post-match scope. This prevented the match subject variable ($i) from being properly narrowed. After the fix, only non-throwing arms contribute to the post-match scope, so$iis correctly narrowed to0|1|2|3|4.For loop condition evaluation timing: The for loop's
$alwaysIteratesflag was computed using the warmup scope (which generalizes types across iterations), making itmaybeinstead ofyes. The fix adds a secondary check after the real body processing: if the loop scope (after body + increment) shows the condition is always true,$alwaysIteratesis upgraded toyes. With the match narrowing fix,$iafter the body is{0,1,2,3,4}, and after$i++is{1,2,3,4,5}, making$i <= 5always true — so the loop is recognized as always iterating (and thus always terminating since there's nobreak).Test
Added a regression test in
tests/PHPStan/Rules/Missing/data/bug-9444.phpthat reproduces the exact code from the issue: aforloop withtry { return ... } catch { match with default => throw }. The test expects no errors.Fixes phpstan/phpstan#9444