Fix phpstan/phpstan#14117: Incorrect 'Variable $value might not be defined.'#5125
Conversation
…ve with same condition - Fixed createConditionalExpressions in MutatingScope to preserve variables in newVariableTypes when their type matches but certainty differs between branches - This ensures conditional expressions are created to track that a variable is definitely defined when the same condition holds as when it was assigned - Removed now-unnecessary @phpstan-ignore variable.undefined comments in OptimizedDirectorySourceLocator.php that were working around this bug - New regression test in tests/PHPStan/Rules/Variables/data/bug-14117.php
| } | ||
|
|
||
| if ($key === 3) { | ||
| echo $value; // this one SHOULD report "might not be defined" because $key === 3 doesn't guarantee either earlier block ran |
There was a problem hiding this comment.
Technically the variable should be reported as always undefined...
VincentLanglet
left a comment
There was a problem hiding this comment.
@staabm I feel like it's an ok fix but I almost never worked with ExpressionTypeHolder...
Are you familiar enough with this topic or should we rely on ondrej review ?
|
I think its good. Will add more regression tests tomorrow |
|
@VincentLanglet added more tests, and I think it looks good. while this PR touches one of the snippets in bug 6830, it does not resolve the original problem, therefore I don't use a "closes 6830" in the PR description. |
| } | ||
| } | ||
|
|
||
| function baz(): void { |
There was a problem hiding this comment.
in this file I added a few more tests which e.g. also check the interaction with unset().
while doing so I found a case which does not yet work: phpstan/phpstan#14227
Summary
Fixes a false positive "Variable $value might not be defined" when a variable is assigned inside an if-block and then used inside a subsequent if-block with the same condition, when there is a preceding if-block that also assigns the same variable under a different condition.
Changes
createConditionalExpressions()insrc/Analyser/MutatingScope.php(line ~4097): Added a check that prevents removing a variable fromnewVariableTypeswhen it has the same type in both branches but different certainty (e.g.,Yesin the body branch vsMaybein the falsey branch). This ensures the conditional expressionwhen $key = 1 → $value is definitely definedis created during scope merging.@phpstan-ignore variable.undefinedcomments insrc/Reflection/BetterReflection/SourceLocator/OptimizedDirectorySourceLocator.php(lines 130 and 174) that were suppressing the same false positive pattern.Root cause
When merging scopes after an if-without-else,
MutatingScope::createConditionalExpressions()builds conditional expressions that track "when variable X has type T, variable Y has type U". It first constructsnewVariableTypesby removing entries where the "their" (other branch) type matches the merged type, since those variables didn't meaningfully change.The bug was that this removal only checked whether types matched (via
equalTypes), ignoring certainty. When a preceding if-block assigned the same variable under a different condition, the falsey branch already had the variable withMaybecertainty (matching the merged result), so the variable was incorrectly removed fromnewVariableTypes. This prevented the creation of the conditional expression that would track "when $key equals 1, $value is definitely defined".The fix adds a targeted check: when the types are the same but the certainties differ (our branch has
Yes, their branch hasMaybe), the variable is kept innewVariableTypesso the appropriate conditional expression is created.Test
Added
tests/PHPStan/Rules/Variables/data/bug-14117.phpwith three test cases:foo(): The exact reproduction from the issue — variable assigned in two if-blocks with different conditions, used in a third if with the same condition as the second. No error expected (was false positive).bar(): Similar but using the first if's condition — error still expected (separate, pre-existing issue with conditional expression preservation across intervening assignments).baz(): Control case with a condition that doesn't match any assignment — error correctly expected.Fixes phpstan/phpstan#14117
Closes phpstan/phpstan#8430
Closes phpstan/phpstan#10657