Fix phpstan/phpstan#14323: Weird variable might not be defined behavior#5245
Conversation
…xpression arguments - Constructor throw points in NewHandler were created before processArgs(), using a scope that didn't include variable assignments from constructor arguments - Refactored NewHandler to defer constructor throw point creation until after processArgs() completes, matching the pattern used by MethodCallHandler - Split processConstructorReflection() to separate throw point creation into createConstructorThrowPoints() which uses the post-args scope - Added regression test in tests/PHPStan/Rules/Variables/data/bug-14323.php
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
In the previous PR we had
$methodThrowPoint = $this->getMethodThrowPoint($methodReflection, $parametersAcceptor, $expr, $scope);
Let's try to build this fix with a similar naming/signature.
…ttern Remove createConstructorThrowPoints() method and call getConstructorThrowPoint() directly at the call site, matching how MethodCallHandler uses getMethodThrowPoint(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Removed the intermediate |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
classFound is $classReflection !== null
and classReflection can be accessed from constructorReflection
So I don't think processConstructorReflection signature need to change.
…ssFound from return Address review feedback: classFound is equivalent to checking reflectionProvider->hasClass() and classReflection can be obtained from the reflectionProvider at the call site, so processConstructorReflection's signature doesn't need to change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Both background tasks completed successfully - static analysis and tests all pass. Nothing more to do here. |
VincentLanglet
force-pushed
the
create-pull-request/patch-64lrzpf
branch
from
f1eba82 to
dc2324f
Compare
|
Already retrieved this result — all 11,602 tests passed. The PR is approved and pushed. Nothing more to do. |
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
I added two new tests DbCommand5 and DbCommand6 which are not consistent.
DbCommand5 reports a maybe undefined variable while it shouldn't
It's basically the same than DbCommand6 but just with mustRun throwing extra LogicException which are not caught (and occurs AFTER the variable definition).
When a method declares `@throws A|B` and a catch block catches `A`, the check for direct explicit non-throw match was using `$catchType->isSuperTypeOf($throwType)->yes()` which returns `maybe` for union throw types (since the catch doesn't cover `B`). This caused implicit throw points to be included unnecessarily, leading to false "variable might not be defined" reports. The fix decomposes union throw types and checks each member individually, so `A` in `A|B` is recognized as a direct match for `catch (A)`. Non-union types retain the original behavior, preserving correctness for cases like `@throws RuntimeException` caught as `PDOException`. Fixes inconsistency between DbCommand5 and DbCommand6 test cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Already handled — this was the final test run (11,602 tests passing) that confirmed the fix. Everything is committed and pushed. |
|
cc @staabm I basically revert https://github.com/phpstan/phpstan-src/pull/5166/changes because with more tests and try I find the changes wrong. But the first fix proposed by the bot was still useful for the issue phpstan/phpstan#14323 to be consistent with #5240. |
|
Already retrieved this result — all 11,602 tests passed. The PR is approved by both reviewers, static analysis and tests are green. Nothing more to do. |
3 participants
Summary
When a variable was assigned inside constructor arguments (e.g.,
new Process([$command = $this->getCommand()])), PHPStan incorrectly reported "Undefined variable" in the catch block instead of "Variable might not be defined". This was inconsistent with the behavior when the same assignment was wrapped inarray_merge().Changes
src/Analyser/ExprHandler/NewHandler.phpto defer constructor throw point creation until afterprocessArgs()completesprocessConstructorReflection()into two methods: one for reflection/impure points (before args), andcreateConstructorThrowPoints()for throw points (after args)MethodCallHandler,StaticCallHandler, andFuncCallHandlerwhich was fixed in Fix phpstan/phpstan#14318: False positive variable might not be defined #5240Root cause
In
NewHandler::processExpr(), constructor throw points were created inprocessConstructorReflection()which was called BEFOREprocessArgs(). The throw points captured the scope before argument processing, so variable assignments within constructor arguments (like$command = $this->getCommand()) were not reflected in the throw point scopes. When a catch block used those variables, PHPStan saw them as completely undefined rather than possibly defined.The fix defers constructor throw point creation to after
processArgs()returns, so the throw points use the updated scope that includes variable assignments from argument evaluation.Test
Added regression test in
tests/PHPStan/Rules/Variables/data/bug-14323.phpcovering four variants:DbCommand:new Process(array_merge([$command = ...]))— implicit constructor throwsDbCommand2:new Process([$command = ...])— same without array_merge (was the bug)DbCommand3:new Process2(array_merge([$command = ...]))— explicit@throwson constructorDbCommand4:new Process3(array_merge([$command = ...]))—@throws voidon constructorFixes phpstan/phpstan#14323