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
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 asResult of && is always false.andStrict 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
src/Parser/ClosureUseByRefReassignmentVisitor.php: aRichParsernode visitor (auto-registered via#[AutowiredService]). It maintains a stack of scope frames (one per function-like) and, using document order, marks eachuse (&$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 abool $generalizeByRefType = falseparameter. When set, captured by-reference variables flagged by the visitor are widened withGeneralizePrecision::lessSpecific(). Variables that are not reassigned externally keep their precise type.src/Analyser/NodeScopeResolver.php: the initial closure-body scope setup now callsprocessClosureScope(..., true). The fixed-point iteration call andProcessClosureResult::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.phpasserts the inferred type of by-reference captured variables inside the closure body for:stringinstead of''),.=), increment (++), conditional reassignment in a nested block, and array-element write after the closure,int<0, max>).$tmpdirinferred as''instead ofstring) and passes after.make tests), self-analysis (make phpstan) and coding standards (make cs) are green.Fixes phpstan/phpstan#14848