Fix phpstan/phpstan#14369: Null coalescing assignment operator combined with throwing#5297
Conversation
…always terminating - Fixed AssignOpHandler to not propagate isAlwaysTerminating from the RHS of ??= - The RHS of ??= only executes when the LHS is null, so the expression cannot always terminate - Added NSRT regression test verifying type narrowing after ??= throw - Added UnreachableStatementRule test verifying no false positive unreachable code
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
Add a test where $test is always null and so the expression is always terminating.
Also add a test where the left expr is always terminating (so the whole left + right expression should be considered as always terminating)
…assignment When the LHS of ??= is always null, the RHS always executes, so the expression should be considered always-terminating if the RHS is. When the LHS evaluation itself always terminates (e.g. calling a never-returning function), the whole expression is always-terminating. Added tests for both cases as requested in review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The background |
| $test = null; | ||
| $test ??= throw new Exception(); |
There was a problem hiding this comment.
I would add another test in which $test is maybe null
like
if (rand(0,1)) {
$test = null;
} else {
$test = 'hallo';
}
There was a problem hiding this comment.
That's already the original issue with $test being ?string (line 12)
There was a problem hiding this comment.
The working tree is clean and all the reviewer feedback has already been addressed in previous commits:
- VincentLanglet's latest comment is just noting that the
?stringcase is already covered at line 12 — no action needed. - staabm's request for a "maybe null" test was already addressed in commit
6435689ae. - Both reviewers have approved the PR.
There are no outstanding changes to make. The branch is already up to date with the remote.
There was a problem hiding this comment.
This task already completed too — all 11658 tests passed. The commit addressing staabm's review feedback has already been pushed. Everything is done.
Addresses review feedback: adds a test where the variable is sometimes null (via rand branch) to cover the maybe-null path for ??= with throw. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ed with throwing (phpstan#5297) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
When using
$test ??= throw new Exception(), PHPStan incorrectly treated the entire expression as always-terminating, causing false positives: "Function always throws an exception" and "Unreachable statement - code above always terminates." The RHS of??=only executes when the LHS is null, so the expression should not be considered always-terminating.Changes
src/Analyser/ExprHandler/AssignOpHandler.php: ChangedisAlwaysTerminatingfrom$exprResult->isAlwaysTerminating()tofalseforExpr\AssignOp\Coalesce, since the RHS only runs in the null branchtests/PHPStan/Analyser/nsrt/bug-14369.php: NSRT test verifying type narrowing tostringafter??= throwtests/PHPStan/Rules/DeadCode/data/bug-14369.phpand test method inUnreachableStatementRuleTest.php: Rule test verifying no false positive unreachable statementRoot cause
In
AssignOpHandler, when processing??=, theisAlwaysTerminatingflag from the RHS expression result was propagated directly. When the RHS isthrow new Exception(), this flag istrue, making PHPStan think the entire??=expression always terminates. However,??=only evaluates the RHS when the LHS is null — the non-null branch continues execution normally. The fix setsisAlwaysTerminatingtofalsefor coalesce assignment expressions.Test
tests/PHPStan/Analyser/nsrt/bug-14369.php— verifies that after$test ??= throw new Exception(),$testis correctly narrowed tostringtests/PHPStan/Rules/DeadCode/data/bug-14369.php— verifies no unreachable statement false positive after??= throwFixes phpstan/phpstan#14369