Fix phpstan/phpstan#14318: False positive variable might not be defined#5240
Merged
VincentLanglet merged 8 commits intophpstan:2.1.xfrom Mar 18, 2026
Merged
Conversation
…ed in catch block - Moved method call throw point creation to after processArgs in MethodCallHandler - Applied the same fix to StaticCallHandler for consistency - The throw point scope now includes variables assigned in function arguments - New regression test in tests/PHPStan/Rules/Variables/data/bug-14318.php
VincentLanglet
approved these changes
Mar 17, 2026
Collaborator
Author
|
All tests pass (11598 tests, 0 failures) and static analysis reports no errors. The PR is approved and ready to merge — no additional changes needed. |
staabm
reviewed
Mar 18, 2026
Contributor
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
this fix also seem to address more issues.
please add regression tests for
Both issues involve variables in try/catch blocks not being properly tracked - #11284 for by-reference parameters and #7806 for preg_match matches. These are also fixed by the throw point scope fix in phpstan#5240. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
Already handled — all 11600 tests passed and the commit has been pushed. |
staabm
reviewed
Mar 18, 2026
Cover the StaticCallHandler code path with regression tests using static method calls (self::maybeThrows()) alongside the existing instance method call tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
staabm
reviewed
Mar 18, 2026
Adds regression tests for regular function calls (FuncCallHandler) with variable assignment in arguments, proving this handler already had the correct throw point order. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
staabm
approved these changes
Mar 18, 2026
Collaborator
Author
|
Already retrieved — all 11,600 tests passed. Both tests and static analysis are green. The PR is approved with no outstanding changes needed. |
This was referenced Mar 18, 2026
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
3 participants
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
Fixes a false positive "Variable $sql might not be defined" in catch blocks when the variable is assigned as part of a function argument expression (e.g.,
$this->maybeThrows($sql = "SELECT * FROM foo")). This was a regression from #5166.Changes
src/Analyser/ExprHandler/MethodCallHandler.php: Moved the method throw point creation from beforeprocessArgsto after it, so the throw point's scope includes variables assigned in function arguments.src/Analyser/ExprHandler/StaticCallHandler.php: Applied the same fix for static method calls.tests/PHPStan/Rules/Variables/data/bug-14318.php: New test data file reproducing both the direct and nested assignment cases.tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php: AddedtestBug14318test method.Root cause
In
MethodCallHandler, the throw point for a method call was created beforeprocessArgs()was called. This meant the throw point's scope didn't include variables assigned in argument expressions (like$sql = "..."in$this->maybeThrows($sql = "SELECT * FROM foo")). When catch blocks merged throw point scopes, they used a scope where$sqlwasn't yet defined, leading to the false positive "might not be defined" error.FuncCallHandleralready had the correct order (throw point created afterprocessArgs), so onlyMethodCallHandlerandStaticCallHandlerneeded fixing.Test
Added a regression test with two cases:
$this->maybeThrows5($sql = "SELECT * FROM foo")$this->maybeThrows6(strlen($sql = "SELECT * FROM foo"))Both should report no errors when accessing
$sqlin the catch block.Fixes phpstan/phpstan#14318