Skip to content

Propagate variable flow and end statements through declare() {} blocks in DeclareHandler - #6804

Open
phpstan-bot wants to merge 2 commits into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-re6f00g
Open

phpstan-bot wants to merge 2 commits into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-re6f00g

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

Variables used inside a declare(...) { ... } block were not considered used by the outer code: $b = 1; declare(ticks=1) { echo $b; } reported Variable $b is never read. The fix forwards the variable flow of the declare block's body in DeclareHandler.

Changes

  • src/Analyser/StmtHandler/DeclareHandler.php: pass the body's variableFlow and endStatements into the returned InternalStatementResult.
  • turbo-ext/src/DeclareHandler.cpp: same change in the native mirror.
  • Probed the other statement handlers that don't forward variable flow (NamespaceHandler, UseHandler, ConstHandler, ClassLikeHandler, LabelHandler, InlineHtmlHandler, ...). None of them can contain variable accesses, so they are not affected.

Root cause

DeclareHandler built a new InternalStatementResult and only copied the scope, yield, termination, exit, throw and impure points from the body's result. The variableFlow was dropped, so VariableLivenessResolver never saw the reads and writes inside the block. This affected UnusedVariableRule, 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 like BlockHandler does, 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.
  • Both tests fail without the fix and pass with it, in PHP mode and with the turbo extension loaded.

Fixes phpstan/phpstan#15394

…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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing thru $endStatements into InternalStatementResult is not covered by the tests of this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 $endStatements passthrough. Passing through variableFlow, which is the actual fix for the issue, stays.
  • turbo-ext/src/DeclareHandler.cpp: same change in the native mirror; it passes NULL for end statements again.

Verification:

  • make tests: OK, 22599 tests, 106 skipped.
  • make phpstan: no errors.
  • make build-turbo succeeded. With the extension loaded, UnusedVariableRuleTest, UnusedFunctionParametersRuleTest and MissingReturnRuleTest pass.

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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Variable in declare() {} block seems not to be considered used

2 participants