Skip to content

Generalize by-reference closure use variables that are reassigned in the enclosing scope after the closure is declared#5902

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

Generalize by-reference closure use variables that are reassigned in the enclosing scope after the closure is declared#5902
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-ulu9qcc

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

When an anonymous function captures a variable by reference (use (&$var)), PHPStan analysed the closure body assuming the variable still held its value at the point the closure was declared. Because the closure can be invoked later — after the captured variable has been reassigned in the enclosing scope — this produced false positives such as Result of && is always false. and Strict comparison using !== between '' and '' will always evaluate to false.

The fix makes the closure body treat such a by-reference captured variable as its generalized type whenever the variable is reassigned in the enclosing scope after the closure was declared.

Changes

  • Added src/Parser/ClosureUseByRefReassignmentVisitor.php: a RichParser node visitor (auto-registered via #[AutowiredService]). It maintains a stack of scope frames (one per function-like) and, using document order, marks each use (&$var) whose variable is assigned in the same enclosing scope after the closure declaration. Detected reassignment forms: =, =&, compound assignment (.=, +=, …), ++/--, and array-element writes ($var[...] = …).
  • src/Analyser/MutatingScope.php: processClosureScope() gains a bool $generalizeByRefType = false parameter. When set, captured by-reference variables flagged by the visitor are widened with GeneralizePrecision::lessSpecific(). Variables that are not reassigned externally keep their precise type.
  • src/Analyser/NodeScopeResolver.php: the initial closure-body scope setup now calls processClosureScope(..., true). The fixed-point iteration call and ProcessClosureResult::applyByRefUseScope() (which feeds the by-ref result back into the outer scope) intentionally remain non-generalizing, so closure-body modifications and outer-scope results stay precise.

Root cause

By-reference closure uses share the variable with the enclosing scope, but the analyser captured the variable's declaration-time type and never accounted for later reassignments at the call site(s). The gate is "is this captured variable reassigned in the enclosing scope after the closure declaration?" — answered statically by a parser visitor, so that the common case (no external reassignment, e.g. counters incremented only inside the closure) keeps its precise type and the fixed-point behaviour relied upon by existing tests is unchanged.

Test

  • tests/PHPStan/Analyser/nsrt/bug-14848.php asserts the inferred type of by-reference captured variables inside the closure body for:
    • the reported case (string reassigned after → string instead of ''),
    • a control case with no external reassignment (kept precise),
    • reassignment occurring only before the closure (kept precise),
    • reassignment to a different type,
    • compound assignment (.=), increment (++), conditional reassignment in a nested block, and array-element write after the closure,
    • a case ensuring closure-body modifications are still tracked (int<0, max>).
  • Verified the new test fails before the fix (e.g. $tmpdir inferred as '' instead of string) and passes after.
  • Full suite (make tests), self-analysis (make phpstan) and coding standards (make cs) are green.

Fixes phpstan/phpstan#14848

…n the enclosing scope after the closure is declared

- Add `ClosureUseByRefReassignmentVisitor` (a RichParser node visitor) that
  marks `use (&$var)` closure uses whose captured variable is reassigned in the
  enclosing scope at a program point after the closure was declared. It tracks
  per-scope frames (one per function-like) and compares document order of
  closure declarations against assignments (`=`, `.=` and friends, `++`/`--`,
  and array-element writes) to the same variable.
- `MutatingScope::processClosureScope()` gains a `$generalizeByRefType` flag;
  when set (initial closure-body scope setup), it generalizes the captured
  variable's type via `GeneralizePrecision::lessSpecific()` but only for uses
  flagged by the visitor. Variables not reassigned externally keep their precise
  declaration-time type, so existing behavior (e.g. body-local fixed-point
  tracking) is preserved.
- This prevents the closure body from assuming a by-reference captured variable
  still holds its declaration-time value, since the closure may be invoked after
  the variable was reassigned.
- Covers analogous reassignment forms: plain `=`, compound assignment, in/decrement,
  array-element writes, conditional reassignment, and reassignment with a
  different type.
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.

By-reference variables in use clauses for anonymous functions are assumed to not change value

1 participant