Repository navigation
Propagate variable flow and end statements through declare() {} blocks in DeclareHandler - #6804
phpstan-bot wants to merge 2 commits into
Conversation
…cks in `DeclareHandler`
- DeclareHandler rebuilt the InternalStatementResult of a declare block
without the variableFlow of its body, so variable reads/writes inside
`declare(ticks=1) { ... }` were invisible to VariableLivenessResolver.
This caused false "Variable $x is never read." (variable.unused) and
false "has an unused parameter" errors, and missed overwrites inside
the block. The body's variable flow is now passed through as-is (the
block always executes, unlike BlockHandler's optional choice).
- The body's endStatements are forwarded as well, like BlockHandler does.
- Ported the same change to the turbo-ext mirror (DeclareHandler.cpp).
- Probed the other statement handlers that do not forward variable flow
(namespace, use, const, class-like, label, inline HTML, ...): none of
them can contain variable accesses, so DeclareHandler was the only one
affected.
| $impurePoints = $result->getImpurePoints(); | ||
| $alwaysTerminating = $result->isAlwaysTerminating(); | ||
| $exitPoints = $result->getExitPoints(); | ||
| $endStatements = $result->getEndStatements(); |
There was a problem hiding this comment.
passing thru $endStatements into InternalStatementResult is not covered by the tests of this PR.
There was a problem hiding this comment.
staabm was right that no test covers passing $endStatements through, and no test can: the value is always an empty array. I removed the passthrough instead of adding a test.
Why it's always empty: the declare body runs through processStmtNodesInternal(). That builds its result in StatementListWalkState::toResult() (src/Analyser/StatementListWalkState.php:40), which never sets endStatements. So any statement list, including a declare body, returns []. Only IfHandler creates end statements itself, from the last statement of each branch. I checked a function ending in declare(ticks=1) { if ($b) { return 1; } else { echo 'x'; } } with and without the passthrough. Both times the missing-return error was on line 7, the declare line.
Change (commit e97b1b835, pushed):
src/Analyser/StmtHandler/DeclareHandler.php: removed the$endStatementspassthrough. Passing throughvariableFlow, which is the actual fix for the issue, stays.turbo-ext/src/DeclareHandler.cpp: same change in the native mirror; it passesNULLfor end statements again.
Verification:
make tests: OK, 22599 tests, 106 skipped.make phpstan: no errors.make build-turbosucceeded. With the extension loaded,UnusedVariableRuleTest,UnusedFunctionParametersRuleTestandMissingReturnRuleTestpass.
Making declare blocks report errors at their last inner statement, as if branches do, would mean adding IfHandler-style end-statement logic to DeclareHandler. That would change where missing-return errors are reported, so I left it out of this PR.
The body is processed as a statement list, whose result never carries end statements, so forwarding them was a no-op. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Variables used inside a
declare(...) { ... }block were not considered used by the outer code:$b = 1; declare(ticks=1) { echo $b; }reportedVariable $b is never read.The fix forwards the variable flow of the declare block's body inDeclareHandler.Changes
src/Analyser/StmtHandler/DeclareHandler.php: pass the body'svariableFlowandendStatementsinto the returnedInternalStatementResult.turbo-ext/src/DeclareHandler.cpp: same change in the native mirror.NamespaceHandler,UseHandler,ConstHandler,ClassLikeHandler,LabelHandler,InlineHtmlHandler, ...). None of them can contain variable accesses, so they are not affected.Root cause
DeclareHandlerbuilt a newInternalStatementResultand only copied the scope, yield, termination, exit, throw and impure points from the body's result. ThevariableFlowwas dropped, soVariableLivenessResolvernever saw the reads and writes inside the block. This affectedUnusedVariableRule, where reads went unseen and overwrites inside the block were missed. It also affected the unused-parameter checks, where a parameter read only inside a declare block was reported as unused. The flow is passed through unchanged, not wrapped in an optional choice likeBlockHandlerdoes, because a declare block always executes.Test
tests/PHPStan/Rules/DeadCode/data/bug-15394.php(UnusedVariableRuleTest::testBug15394): the reproducer from the issue, plus a write inside / read after, an overwrite inside the block (now reported), an unused write inside the block (still reported) and nested declare blocks.tests/PHPStan/Rules/Functions/data/unused-function-parameters-declare.php(UnusedFunctionParametersRuleTest::testDeclareBlock): a parameter read only inside a declare block is no longer reported as unused.Fixes phpstan/phpstan#15394