Fix phpstan/phpstan#8926: Incorrect inference of instance variable modified from callback#5347
Conversation
…dified from callback - Added InvalidateExprNode emission for PropertyAssignNode inside closures in NodeScopeResolver::processClosureNode() and ClosureTypeResolver - Changed processImmediatelyCalledCallable to use requireMoreCharacters=false for non-Variable expressions (PropertyFetch), so exact property matches are properly invalidated - Extended existing regression test in bug-8926.php with array_filter case
|
All 11690 tests pass (78932 assertions), and |
|
new error in PMMP integration tests looks valid to me. |
|
|
||
| class Wrap { | ||
| /** | ||
| * @param-immediately-invoked-callable $cb |
There was a problem hiding this comment.
Add the same test with later-invoked-callable ; $this->conn should not be invalidate for later-invoked-callable.
There was a problem hiding this comment.
make tests completed successfully as well — all tests pass. The commit has already been pushed.
Adds a test with @param-later-invoked-callable to verify that property invalidation behavior is distinct from @param-immediately-invoked-callable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| $this->conn = "conn"; | ||
| }); | ||
|
|
||
| if (is_null($this->conn)) { // should be always true - later-invoked callable doesn't invalidate |
There was a problem hiding this comment.
Rather than adding a comment, find why it's not the case.
There was a problem hiding this comment.
Done. Here's what I did to address VincentLanglet's review:
Root cause: deferredInvalidateExpressions was populated unconditionally for all closures (line 3373 and 3421 in NodeScopeResolver.php), regardless of whether the callable was @param-immediately-invoked-callable or @param-later-invoked-callable. This meant property types were incorrectly invalidated even for later-invoked callables.
Fix: Gated deferredInvalidateExpressions[] behind $this->callCallbackImmediately() in both the closure path and the generic callable path, matching how throw/impure points are already gated.
Test update: The bug-11417.php test now properly asserts that is_null($this->conn) reports "If condition is always true" for the later-invoked case (line 66), instead of just having a comment about it.
…ot later-invoked ones The deferredInvalidateExpressions were being collected unconditionally for all closures, regardless of whether the callable was marked with @param-immediately-invoked-callable or @param-later-invoked-callable. This caused property types to be incorrectly invalidated even for later-invoked callables where the callback hasn't executed yet. Gate deferredInvalidateExpressions behind callCallbackImmediately() and update the bug-11417 test to assert that the later-invoked-callable case correctly reports "If condition is always true" instead of just a comment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Is it still ok for you @staabm ? If it's ok for you, you can merge it. |
|
thank you |
…dified from callback (phpstan#5347) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
When a closure passed to
array_filter(or similar functions that invoke their callback immediately) modifies$this->property, PHPStan incorrectly inferred the property retained its pre-call value. This caused false positive "If condition is always false" errors.Changes
src/Analyser/NodeScopeResolver.php: InprocessClosureNode'sclosureStmtsCallback, emitInvalidateExprNodeforPropertyAssignNodeso that property assignments inside closures are tracked for invalidation. InprocessImmediatelyCalledCallable, userequireMoreCharacters=falsefor non-Variable expressions so that exact PropertyFetch matches are properly invalidated.src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php: SameInvalidateExprNodeemission for property assignments in both arrow function and closure type resolution callbacks, for consistency.tests/PHPStan/Rules/Comparison/data/bug-8926.php: Extended existing regression test witharray_filtercallback case and local variableuse(&$test)case.Root cause
Two issues combined:
$this->test = true) inside closures only emittedPropertyAssignNode(for impure point tracking), but notInvalidateExprNode. WithoutInvalidateExprNode,processImmediatelyCalledCallablehad nothing to invalidate after the function call returned.processImmediatelyCalledCallablealways passedrequireMoreCharacters=truetoinvalidateExpression(). This flag means "only invalidate expressions that contain the target as a sub-expression". For Variable expressions (the original use case), this correctly invalidates$x->foowithout invalidating$xitself. But for PropertyFetch expressions like$this->test, it prevented the exact match from being invalidated.Test
Extended the existing
tests/PHPStan/Rules/Comparison/data/bug-8926.phpwith two new methods:errorArrayFilter(the reported bug - closure modifying$this->testviaarray_filtercallback) andsuccessLocal(local variable modified viause(&$test)inarray_filtercallback, which already worked). Both expect no errors.Fixes phpstan/phpstan#8926
Fixes phpstan/phpstan#11417
Fixes phpstan/phpstan#10903