diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index c81d35e98f..68fea1c842 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3584,7 +3584,7 @@ public function isInFirstLevelStatement(): bool return $this->inFirstLevelStatement; } - public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false): self + public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = false, bool $createConditionalDefinedness = false): self { if ($otherScope === null || $this === $otherScope) { return $this; @@ -3618,6 +3618,20 @@ public function mergeWith(?self $otherScope, bool $preserveVacuousConditionals = $ourExpressionTypes, $mergedExpressionTypes, ); + if ($createConditionalDefinedness) { + $conditionalExpressions = $this->createConditionalDefinednessExpressions( + $conditionalExpressions, + $ourExpressionTypes, + $theirExpressionTypes, + $mergedExpressionTypes, + ); + $conditionalExpressions = $this->createConditionalDefinednessExpressions( + $conditionalExpressions, + $theirExpressionTypes, + $ourExpressionTypes, + $mergedExpressionTypes, + ); + } $filter = static function (ExpressionTypeHolder $expressionTypeHolder) { if ($expressionTypeHolder->getCertainty()->yes()) { @@ -3900,6 +3914,96 @@ private function createConditionalExpressions( return $conditionalExpressions; } + /** + * Records that a variable defined only in our branch is defined again + * whenever the same branch condition holds — so a later `if` reusing that + * condition does not report the variable as possibly undefined. + * + * The branch condition is reconstructed from the expressions that the + * condition narrowed in our branch: + * - "type guards": expressions defined in both branches whose type in our + * branch differs from the merged type (e.g. `$s` narrowed to `'banana'` + * by `$s === 'banana'`, or an array gaining a `hasOffset` accessory), and + * - "existence guards": expressions made certain (Yes) in our branch but not + * in the other one (e.g. `$a['x']` made certain by `isset($a['x'])`). + * + * All guards are combined into a single condition, so re-establishing only + * part of it later (`if ($a && $b)` then `if ($a)`) does not wrongly imply + * the variable is defined. At least one type guard is required, which keeps + * out spurious guards produced when the other branch's scope tracks nothing + * for an expression (e.g. an untracked superglobal access). + * + * @param array $conditionalExpressions + * @param array $ourExpressionTypes + * @param array $theirExpressionTypes + * @param array $mergedExpressionTypes + * @return array + */ + private function createConditionalDefinednessExpressions( + array $conditionalExpressions, + array $ourExpressionTypes, + array $theirExpressionTypes, + array $mergedExpressionTypes, + ): array + { + $guards = []; + $hasTypeGuard = false; + $newlyDefined = []; + foreach ($ourExpressionTypes as $exprString => $holder) { + if ($holder->getExpr() instanceof VirtualNode) { + continue; + } + if (!$holder->getCertainty()->yes()) { + continue; + } + if (!array_key_exists($exprString, $mergedExpressionTypes)) { + continue; + } + + $theirHolder = $theirExpressionTypes[$exprString] ?? null; + if ($theirHolder === null || !$theirHolder->getCertainty()->yes()) { + $guards[$exprString] = $holder; + + $expr = $holder->getExpr(); + if ( + $theirHolder === null + && $expr instanceof Variable + && is_string($expr->name) + && !$this->isGlobalVariable($expr->name) + ) { + $newlyDefined[$exprString] = $holder; + } + + continue; + } + + if ($mergedExpressionTypes[$exprString]->equalTypes($holder)) { + continue; + } + + $guards[$exprString] = $holder; + $hasTypeGuard = true; + } + + if (!$hasTypeGuard || count($newlyDefined) === 0) { + return $conditionalExpressions; + } + + foreach ($newlyDefined as $exprString => $holder) { + $variableGuards = $guards; + unset($variableGuards[$exprString]); + + if (count($variableGuards) === 0) { + continue; + } + + $conditionalExpression = new ConditionalExpressionHolder($variableGuards, $holder); + $conditionalExpressions[$exprString][$conditionalExpression->getKey()] = $conditionalExpression; + } + + return $conditionalExpressions; + } + /** * @param array $ourVariableTypeHolders * @param array $theirVariableTypeHolders diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index dfb9bc6908..17bad5b8ee 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1311,6 +1311,13 @@ public function processStmtNode( $alwaysTerminating = true; $hasYield = $condResult->hasYield(); + // Only a deterministic (side-effect-free) condition guarantees that a + // later `if` with the same condition picks the same branch, so that a + // variable defined here is defined there too. Elseif chains rebind the + // condition and are left to the regular per-branch tracking. + $createConditionalDefinedness = count($condResult->getImpurePoints()) === 0 + && count($stmt->elseifs) === 0; + $branchScopeStatementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $condResult->getTruthyScope(), $storage, $nodeCallback, $context); if (!$conditionType->isTrue()->no()) { @@ -1375,7 +1382,7 @@ public function processStmtNode( if ($stmt->else === null) { if (!$ifAlwaysTrue && !$lastElseIfConditionIsTrue) { - $finalScope = $scope->mergeWith($finalScope, true); + $finalScope = $scope->mergeWith($finalScope, true, $createConditionalDefinedness); $alwaysTerminating = false; } } else { @@ -1387,7 +1394,7 @@ public function processStmtNode( $throwPoints = array_merge($throwPoints, $branchScopeStatementResult->getThrowPoints()); $impurePoints = array_merge($impurePoints, $branchScopeStatementResult->getImpurePoints()); $branchScope = $branchScopeStatementResult->getScope(); - $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true); + $finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope, true, $createConditionalDefinedness); $alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating(); if (count($branchScopeStatementResult->getEndStatements()) > 0) { $endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements()); diff --git a/tests/PHPStan/Analyser/nsrt/bug-7718.php b/tests/PHPStan/Analyser/nsrt/bug-7718.php new file mode 100644 index 0000000000..ee82fc18b1 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-7718.php @@ -0,0 +1,130 @@ + $elements + */ +function isset_twice(array $elements): void +{ + if (isset($elements['a'])) { + $a = 1; + } + + if (isset($elements['a'])) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } +} + +function strict_compare_twice(string $s): void +{ + if ($s === 'banana') { + $bananas = 1; + } + + if ($s === 'banana') { + assertVariableCertainty(TrinaryLogic::createYes(), $bananas); + } +} + +function int_compare_twice(int $x): void +{ + if ($x === 1) { + $a = 1; + } + + if ($x === 1) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } +} + +/** + * @param array $elements + */ +function array_key_exists_twice(array $elements): void +{ + if (array_key_exists('a', $elements)) { + $a = 1; + } + + if (array_key_exists('a', $elements)) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } +} + +function instanceof_twice(mixed $x): void +{ + if ($x instanceof \Throwable) { + $a = 1; + } + + if ($x instanceof \Throwable) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } +} + +function not_null_twice(?int $x): void +{ + if ($x !== null) { + $a = 1; + } + + if ($x !== null) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } +} + +// A different condition must NOT prove the variable defined. +function different_condition(string $s): void +{ + if ($s === 'a') { + $a = 1; + } + + if ($s === 'b') { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } +} + +/** + * @param array $elements + */ +function isset_then_else(array $elements): void +{ + if (isset($elements['a'])) { + $a = 1; + } else { + $a = 2; + } + + if (isset($elements['a'])) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } +} + +// Reusing only part of an `&&` condition must NOT prove the variable defined. +function partial_condition_reuse(int $x, int $y): void +{ + if ($x === 1 && $y === 2) { + $a = 1; + } + + if ($x === 1) { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } +} + +// A non-deterministic condition must NOT prove the variable defined when reused. +function impure_condition(int $x): void +{ + if (rand(0, 1) === 1 && $x === 1) { + $v = 1; + } + + if ($x === 1) { + assertVariableCertainty(TrinaryLogic::createMaybe(), $v); + } +}