Skip to content

Widen by-ref use variables with the types they hold at calls and yields inside the closure body - #6170

Open
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-zzzsp3c
Open

Widen by-ref use variables with the types they hold at calls and yields inside the closure body#6170
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-zzzsp3c

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

A re-entrancy guard built on a by-reference use variable was reported as an always-false condition:

$isRunning = false;

$listener = function () use (&$isRunning, &$listener): void {
    if ($isRunning) { // If condition is always false.
        return;
    }

    $isRunning = true;
    $listener();
    $isRunning = false;
};

PHPStan inferred false for $isRunning at the top of the body. This change makes the closure's by-ref use variables account for the values they hold at the points where control can leave the body, so $isRunning is inferred as bool and the false positive is gone.

Changes

  • src/Analyser/NodeScopeResolver.php, processClosureNode(): the intermediary body walks of the by-ref fixed-point loop no longer use NoopNodeCallback. They run with a GatheringNodeCallback that records, for every by-ref use variable, the type and native type it holds at each re-entry point in the body.
  • src/Analyser/NodeScopeResolver.php, processClosureNode(): before the intermediary scope is fed back through MutatingScope::processClosureScope(), those recorded types are unioned into it via assignVariable(). Immediately invoked closures short-circuit before this, keeping their existing (more precise) behaviour since they cannot be re-entered.
  • src/Analyser/NodeScopeResolver.php, new private isClosureReentryPoint(): a node is a re-entry point when it is a CallLike that is not a first-class callable (FuncCall, MethodCall, NullsafeMethodCall, StaticCall, New_) or a Yield_ / YieldFrom.

Analogous cases fixed by the same change (each confirmed failing before it):

  • re-entry through a plain function call (the shape of the original report's EventDispatcher sample)
  • re-entry through a method call
  • re-entry through a static method call
  • re-entry through a constructor (new)
  • re-entry through a call made by a nested closure that shares the by-ref use
  • assignment and call inside a loop in the closure body
  • the negated direction, if (!$flag) { ... }
  • generator closures suspended on yield
  • generator closures suspended on yield from

Probed and found already correct, so no change was made and no test was kept:

  • global $x inside a function — resolved as mixed
  • static $x inside a function — resolved as mixed
  • properties ($this->running) — never narrowed below the declared type across a method call
  • arrow functions — they capture by value only and cannot have by-ref uses
  • static vs. non-static closures — both go through the same code path and are covered

Root cause

processClosureNode() computes the entry scope for a closure's by-ref use variables by iterating the body to a fixed point. Each iteration only looked at two things: the scope at the end of the body, and the scopes at the body's exit points. The union of those with the outer scope became the next entry scope.

That model silently assumes the closure only ever starts from a state the body ends in. It breaks for re-entrancy: control leaves the closure at every call it makes (and at every yield in a generator closure), and the closure may be invoked again — directly, or indirectly through whatever it called — while the outer invocation is parked there. Any value a by-ref use holds at such a point is therefore observable on entry, even when a later assignment overwrites it before the body ends. In the reported code $isRunning = true is live exactly across $listener() and is then reset to false, so the fixed point converged on false and the guard looked dead.

The fix names those places explicitly (isClosureReentryPoint()) and folds the by-ref use types observed there into the intermediary scope, so the fixed point converges on bool. Non-re-entrant closures are unaffected: a by-ref variable assigned only after the last call in the body, or in a body with no calls at all, keeps its previous, precise type — which is why the change produces no churn in the existing test suite.

Test

  • tests/PHPStan/Analyser/nsrt/bug-15034.php — the reproducer from the issue's second playground link, verbatim, with assertType('bool', $isRunning) at the top of the closure body. Fails with Expected: bool / Actual: false without the fix.
  • tests/PHPStan/Rules/Comparison/data/bug-15034.php + IfConstantConditionRuleTest::testBug15034() — the same reproducer, asserting the reported If condition is always false. error is no longer emitted. Reports the error on line 10 without the fix.
  • tests/PHPStan/Rules/Comparison/data/closure-by-ref-use-reentry.php + IfConstantConditionRuleTest::testClosureByRefUseReentry() — one case per analogous construct listed above, asserting no errors. Without the fix this file reports 8 If condition is always false. errors (lines 27, 39, 51, 63, 86, 99, 114, 126).

make tests (17803 tests), make phpstan and make cs-fix are all green, and self-analysis wall time is unchanged (1m25s with the fix vs. 1m24s without).

Fixes phpstan/phpstan#15034

…elds inside the closure body

* `NodeScopeResolver::processClosureNode()` reached its by-ref `use` fixed point
  from the scope at the end of the closure body plus its exit points only, so a
  value assigned mid-body and overwritten before the body ended was invisible on
  the next entry.
* The intermediary body walks now run with a `GatheringNodeCallback` that records
  each by-ref `use` variable's type (and native type) at every re-entry point, and
  those types are unioned into the intermediary scope before it is fed back through
  `processClosureScope()`.
* Added `NodeScopeResolver::isClosureReentryPoint()`: non-first-class-callable
  `CallLike` nodes (function, method, nullsafe method, static method calls and
  `new`) plus `yield` / `yield from`, i.e. the places where control can leave the
  body and the closure can be entered again before the current invocation finishes.
* Immediately invoked closures keep the old, more precise behaviour - they cannot
  be re-entered.
* Same fix covers the sibling constructs, each verified to be broken before it:
  re-entry through a plain function call, a method call, a static method call, a
  constructor, a call made by a nested closure, a call inside a loop, the negated
  (`if (!$flag)`) direction, and generator suspension on `yield` / `yield from`.
* Probed and found already correct: `global` variables (typed `mixed`), `static`
  variables (typed `mixed`), properties (no narrowing from the declared type), and
  arrow functions (cannot capture by reference).
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.

Incorrect always false detection

1 participant