diff --git a/src/Analyser/ExprHandler/AssignHandler.php b/src/Analyser/ExprHandler/AssignHandler.php index a5df088329d..249199568bb 100644 --- a/src/Analyser/ExprHandler/AssignHandler.php +++ b/src/Analyser/ExprHandler/AssignHandler.php @@ -8,7 +8,12 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\ArrayDimFetch; use PhpParser\Node\Expr\Assign; +use PhpParser\Node\Expr\AssignOp; use PhpParser\Node\Expr\AssignRef; +use PhpParser\Node\Expr\BinaryOp\BooleanAnd; +use PhpParser\Node\Expr\BinaryOp\BooleanOr; +use PhpParser\Node\Expr\BinaryOp\LogicalAnd; +use PhpParser\Node\Expr\BinaryOp\LogicalOr; use PhpParser\Node\Expr\ConstFetch; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\List_; @@ -58,6 +63,7 @@ use PHPStan\Node\VariableAssignNode; use PHPStan\Node\VirtualNode; use PHPStan\Php\PhpVersion; +use PHPStan\Reflection\InitializerExprTypeResolver; use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\ShouldNotHappenException; use PHPStan\TrinaryLogic; @@ -90,9 +96,11 @@ use function array_slice; use function count; use function in_array; +use function is_array; use function is_int; use function is_string; use function spl_object_id; +use function str_contains; /** * @implements ExprHandler @@ -119,6 +127,7 @@ public function __construct( private StaticPropertyFetchHandler $staticPropertyFetchHandler, private MethodThrowPointHelper $methodThrowPointHelper, private PropertyHookThrowPointsResolver $propertyHookThrowPointsResolver, + private InitializerExprTypeResolver $initializerExprTypeResolver, ) { } @@ -1157,6 +1166,29 @@ public function applyWrite( : $this->defaultNarrowingHelper->specifyTypesForNode($scope, $assignedExpr, TypeSpecifierContext::createFalsey()); $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($nodeScopeResolver, $scope, $storage, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $impurePoints, $assignedExpr, $storedAssignedExprResult); $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($nodeScopeResolver, $scope, $storage, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType, $impurePoints, $assignedExpr, $storedAssignedExprResult); + + // An assignment inside a short-circuit operand may or may not have run + // (its variable is only maybe-defined after the RHS walk), but a truthy + // && (or falsey ||) guarantees the right operand was evaluated. The + // operand walk's truthy/falsey scope knows exactly which variables it + // defined - record their certainty as a consequence of the assigned + // boolean, e.g. "$bool = $x && ($var = 'foo'); if ($bool) { … $var … }". + if ( + $storedAssignedExprResult !== null + && ( + $assignedExpr instanceof BooleanAnd + || $assignedExpr instanceof BooleanOr + || $assignedExpr instanceof LogicalAnd + || $assignedExpr instanceof LogicalOr + ) + && self::containsVariableAssignment($assignedExpr) + ) { + if ($assignedExpr instanceof BooleanAnd || $assignedExpr instanceof LogicalAnd) { + $conditionalExpressions = $this->processDefinednessForConditionalExpressionsAfterAssign($conditionalExpressions, $var->name, $truthyType, $storedAssignedExprResult->getTruthyScope(), $scope); + } else { + $conditionalExpressions = $this->processDefinednessForConditionalExpressionsAfterAssign($conditionalExpressions, $var->name, $falseyType, $storedAssignedExprResult->getFalseyScope(), $scope); + } + } } foreach ([null, false, 0, 0.0, '', '0', []] as $falseyScalar) { @@ -1206,7 +1238,27 @@ public function applyWrite( } $nodeScopeResolver->callNodeCallback($nodeCallback, new VariableAssignNode($var, $assignedExpr), $scopeBeforeAssignEval, $storage); + $remappedConditionalExpressions = []; + if ( + // only the concat-assign's own write remaps - an enclosing plain + // assignment of the same variable (`$s = $s .= 'x'`) sees the + // already-remapped holders and would remap them a second time + $isAssignOp + && $assignedExpr instanceof AssignOp\Concat + && $assignedExpr->var instanceof Variable + && $assignedExpr->var->name === $var->name + && $scope->getConditionalExpressions() !== [] + ) { + $remappedConditionalExpressions = $this->remapConditionalExpressionsThroughConcatAssign( + $scope->getConditionalExpressions(), + '$' . $var->name, + $valueResult->getType(), + ); + } $scope = $scope->assignVariable($var->name, $type, $this->readAssignedValueType($nodeScopeResolver, $storedAssignedExprResult, $assignedExpr, $scope->doNotTreatPhpDocTypesAsCertain()), TrinaryLogic::createYes()); + foreach ($remappedConditionalExpressions as $exprString => $holders) { + $scope = $scope->addConditionalExpressions((string) $exprString, $holders); // @phpstan-ignore cast.useless + } foreach ($conditionalExpressions as $exprString => $holders) { $scope = $scope->addConditionalExpressions((string) $exprString, $holders); } @@ -1845,6 +1897,170 @@ private function processSureNotTypesForConditionalExpressionsAfterAssign(NodeSco return $conditionalExpressions; } + /** + * `$var .= ` keeps the conditional-expression + * holders about $var alive by remapping them through the append instead of + * losing them to the write's invalidation. A consequence type about $var is + * concatenated with the appended constant; a condition on $var is remapped + * only when it is itself a single constant string - appending a fixed + * suffix is injective, so the remapped condition selects exactly the states + * the original condition did. Holders mentioning $var inside a composite + * expression, with a non-Yes certainty about $var, or with a non-constant + * condition on $var are left to the regular invalidation. This keeps e.g. a + * format string built across `if (!empty($target))` correlated with + * $target while further pieces are appended. + * + * @param array $conditionalExpressions + * @return array + */ + private function remapConditionalExpressionsThroughConcatAssign( + array $conditionalExpressions, + string $varExprString, + Type $appendedType, + ): array + { + $appendedConstantStrings = $appendedType->getConstantStrings(); + if (count($appendedConstantStrings) !== 1 || !$appendedType->equals($appendedConstantStrings[0])) { + return []; + } + $appendedConstantString = $appendedConstantStrings[0]; + + $remapped = []; + foreach ($conditionalExpressions as $targetExprString => $holders) { + $targetExprString = (string) $targetExprString; // @phpstan-ignore cast.useless + $targetIsVar = $targetExprString === $varExprString; + if (!$targetIsVar && str_contains($targetExprString, $varExprString)) { + // a composite target containing the variable ($var[0], f($var), ...) + continue; + } + + foreach ($holders as $holder) { + $holderTouchesVar = $targetIsVar; + $remappable = true; + $newConditions = []; + foreach ($holder->getConditionExpressionTypeHolders() as $conditionExprString => $conditionHolder) { + $conditionExprString = (string) $conditionExprString; // @phpstan-ignore cast.useless + if ($conditionExprString === $varExprString) { + $holderTouchesVar = true; + $conditionConstantStrings = $conditionHolder->getType()->getConstantStrings(); + if ( + !$conditionHolder->getCertainty()->yes() + || count($conditionConstantStrings) !== 1 + || !$conditionHolder->getType()->equals($conditionConstantStrings[0]) + ) { + $remappable = false; + break; + } + $newConditions[$conditionExprString] = ExpressionTypeHolder::createYes( + $conditionHolder->getExpr(), + $conditionConstantStrings[0]->append($appendedConstantString), + ); + continue; + } + + if (str_contains($conditionExprString, $varExprString)) { + $remappable = false; + break; + } + + $newConditions[$conditionExprString] = $conditionHolder; + } + if (!$remappable || !$holderTouchesVar) { + continue; + } + + $typeHolder = $holder->getTypeHolder(); + if ($targetIsVar) { + if (!$typeHolder->getCertainty()->yes()) { + // the append leaves the variable defined on every path - + // an undefined/maybe consequence cannot be carried over + continue; + } + $concatType = $this->initializerExprTypeResolver->resolveConcatType($typeHolder->getType(), $appendedType); + if ($concatType instanceof ErrorType) { + continue; + } + $typeHolder = ExpressionTypeHolder::createYes($typeHolder->getExpr(), $concatType); + } + + $newHolder = new ConditionalExpressionHolder($newConditions, $typeHolder); + $remapped[$targetExprString][$newHolder->getKey()] = $newHolder; + } + } + + return $remapped; + } + + /** + * Records "if the assigned variable has $variableType, the target variable is + * certainly defined (with its branch-scope type)" holders for variables whose + * certainty is Yes in the given branch continuation scope of the RHS (the + * truthy scope of a `&&`, the falsey scope of a `||` - the scopes where every + * short-circuit operand was guaranteed evaluated) but only Maybe in the + * merged after-RHS scope. + * + * @param array $conditionalExpressions + * @return array + */ + private function processDefinednessForConditionalExpressionsAfterAssign( + array $conditionalExpressions, + string $variableName, + Type $variableType, + MutatingScope $branchScope, + MutatingScope $mergedScope, + ): array + { + foreach ($branchScope->expressionTypes as $exprString => $holder) { + if (!$holder->getCertainty()->yes()) { + continue; + } + $expr = $holder->getExpr(); + if (!$expr instanceof Variable || !is_string($expr->name) || $expr->name === $variableName) { + continue; + } + $mergedHolder = $mergedScope->expressionTypes[$exprString] ?? null; + if ($mergedHolder === null || !$mergedHolder->getCertainty()->maybe()) { + continue; + } + + $conditionalHolder = new ConditionalExpressionHolder([ + '$' . $variableName => ExpressionTypeHolder::createYes(new Variable($variableName), $variableType), + ], $holder); + $conditionalExpressions[(string) $exprString][$conditionalHolder->getKey()] = $conditionalHolder; // @phpstan-ignore cast.useless + } + + return $conditionalExpressions; + } + + /** + * Whether the expression contains an assignment whose execution short-circuit + * evaluation may have skipped - a cheap AST gate so the truthy/falsey scope + * is only derived for boolean RHS expressions that can define a variable. + */ + private static function containsVariableAssignment(Node $node): bool + { + if ($node instanceof Assign || $node instanceof AssignOp || $node instanceof AssignRef) { + return true; + } + + foreach ($node->getSubNodeNames() as $subNodeName) { + $subNode = $node->$subNodeName; + if ($subNode instanceof Node) { + if (self::containsVariableAssignment($subNode)) { + return true; + } + } elseif (is_array($subNode)) { + foreach ($subNode as $subNodeItem) { + if ($subNodeItem instanceof Node && self::containsVariableAssignment($subNodeItem)) { + return true; + } + } + } + } + + return false; + } + /** * Current type of a conditional-holder expression, used to refine the holder's * projected type. Prefers the tracked scope state over the stored result, diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 96f4301f6df..d122d3efa83 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3827,12 +3827,37 @@ public function applySpecifiedTypes(SpecifiedTypes $specifiedTypes): self $expr = $issetExpr->getExpr(); if ($typeSpecification['sure']) { + $innerExprString = $scope->getNodeKey($expr); $scope = $scope->setExpressionCertaintyKeepingType( $expr, TrinaryLogic::createMaybe(), ); + $specifiedExpressions[$innerExprString] = ExpressionTypeHolder::createMaybe( + $expr, + $scope->expressionTypes[$innerExprString]->getType(), + ); } else { + $innerExprString = $scope->getNodeKey($expr); + // Holders conditioned on this expression being undefined (a + // certainty-No condition) wait for exactly the specification + // applied here, but unsetExpression()'s invalidation would drop + // them before the conditional-expressions matcher below could + // fire them - carve them out and re-add them afterwards. + $rescuedHolders = []; + foreach ($scope->conditionalExpressions as $targetExprString => $targetHolders) { + foreach ($targetHolders as $holderKey => $conditionalHolder) { + $conditionHolder = $conditionalHolder->getConditionExpressionTypeHolders()[$innerExprString] ?? null; + if ($conditionHolder === null || !$conditionHolder->getCertainty()->no()) { + continue; + } + $rescuedHolders[$targetExprString][$holderKey] = $conditionalHolder; + } + } $scope = $scope->unsetExpression($expr); + foreach ($rescuedHolders as $targetExprString => $targetHolders) { + $scope = $scope->addConditionalExpressions((string) $targetExprString, $targetHolders); // @phpstan-ignore cast.useless + } + $specifiedExpressions[$innerExprString] = new ExpressionTypeHolder($expr, new ErrorType(), TrinaryLogic::createNo()); } $scopeIsWorkingCopy = false; diff --git a/src/Analyser/StmtHandler/TryCatchHandler.php b/src/Analyser/StmtHandler/TryCatchHandler.php index ac4612abc84..439d6eb4f5d 100644 --- a/src/Analyser/StmtHandler/TryCatchHandler.php +++ b/src/Analyser/StmtHandler/TryCatchHandler.php @@ -4,9 +4,12 @@ use PhpParser\Node; use PhpParser\Node\Expr; +use PhpParser\Node\Expr\Variable; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\TryCatch; +use PHPStan\Analyser\ConditionalExpressionHolder; use PHPStan\Analyser\ExpressionResultStorage; +use PHPStan\Analyser\ExpressionTypeHolder; use PHPStan\Analyser\InternalStatementResult; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; @@ -18,7 +21,10 @@ use PHPStan\Node\FinallyExitPointsNode; use PHPStan\Node\VariableAssignNode; use PHPStan\ShouldNotHappenException; +use PHPStan\TrinaryLogic; +use PHPStan\Type\ErrorType; use PHPStan\Type\NeverType; +use PHPStan\Type\NullType; use PHPStan\Type\ObjectType; use PHPStan\Type\TypeCombinator; use Throwable; @@ -213,7 +219,13 @@ public function processStmt( $catchScopeResult = $nodeScopeResolver->processStmtNodesInternal($catchNode, $catchNode->stmts, $catchScope->enterCatchType($catchType, $variableName), $storage, $nodeCallback, $context); $catchScopeForFinally = $catchScopeResult->getScope(); - $finalScope = $catchScopeResult->isAlwaysTerminating() ? $finalScope : $catchScopeResult->getScope()->mergeWith($finalScope); + if (!$catchScopeResult->isAlwaysTerminating()) { + $mergedScope = $catchScopeResult->getScope()->mergeWith($finalScope); + if ($variableName !== null && $finalScope !== null) { + $mergedScope = $this->addCatchVariableDefinednessConditionals($mergedScope, $finalScope, $catchScopeResult->getScope(), $variableName); + } + $finalScope = $mergedScope; + } $alwaysTerminating = $alwaysTerminating && $catchScopeResult->isAlwaysTerminating(); $hasYield = $hasYield || $catchScopeResult->hasYield(); $catchThrowPoints = $catchScopeResult->getThrowPoints(); @@ -274,4 +286,70 @@ public function processStmt( return new InternalStatementResult($finalScope, hasYield: $hasYield, isAlwaysTerminating: $alwaysTerminating, exitPoints: $exitPoints, throwPoints: array_merge($throwPoints, $throwPointsForLater), impurePoints: $impurePoints); } + /** + * The catch variable's definedness after the try/catch tells the two joined + * paths apart: when it is untracked on the non-catch path and certainly + * defined at the end of the catch body, "the catch variable is undefined" + * later implies the non-catch path ran. Record that as conditional + * expression holders with a certainty-No condition on the catch variable, + * restoring the non-catch certainty (and type) of variables the join + * demoted to maybe-defined - e.g. `isset($e) || $var instanceof \DateTime` + * evaluates `$var` only where `$e` is narrowed away. + */ + private function addCatchVariableDefinednessConditionals( + MutatingScope $mergedScope, + MutatingScope $nonCatchScope, + MutatingScope $catchEndScope, + string $variableName, + ): MutatingScope + { + $variableExprString = '$' . $variableName; + if (isset($nonCatchScope->expressionTypes[$variableExprString])) { + return $mergedScope; + } + + $catchVariableHolder = $catchEndScope->expressionTypes[$variableExprString] ?? null; + if ($catchVariableHolder === null || !$catchVariableHolder->getCertainty()->yes()) { + return $mergedScope; + } + + $conditions = [ + [ + $variableExprString => new ExpressionTypeHolder(new Variable($variableName), new ErrorType(), TrinaryLogic::createNo()), + ], + ]; + if ($catchVariableHolder->getType()->isNull()->no()) { + // In a scope where any variable can exist (e.g. the top level), the + // isset() machinery models "!isset($e)" on a maybe-defined variable as + // "maybe defined, null when defined" instead of unsetting it. That + // state excludes the catch path just the same - it guarantees a + // defined, non-null catch variable. + $conditions[] = [ + $variableExprString => ExpressionTypeHolder::createMaybe(new Variable($variableName), new NullType()), + ]; + } + foreach ($nonCatchScope->expressionTypes as $exprString => $holder) { + if (!$holder->getCertainty()->yes()) { + continue; + } + $expr = $holder->getExpr(); + if (!$expr instanceof Variable || !is_string($expr->name)) { + continue; + } + $mergedHolder = $mergedScope->expressionTypes[$exprString] ?? null; + if ($mergedHolder === null || !$mergedHolder->getCertainty()->maybe()) { + continue; + } + + $conditionalHolders = []; + foreach ($conditions as $condition) { + $conditionalHolder = new ConditionalExpressionHolder($condition, $holder); + $conditionalHolders[$conditionalHolder->getKey()] = $conditionalHolder; + } + $mergedScope = $mergedScope->addConditionalExpressions((string) $exprString, $conditionalHolders); // @phpstan-ignore cast.useless + } + + return $mergedScope; + } + } diff --git a/tests/PHPStan/Analyser/nsrt/bug-11109.php b/tests/PHPStan/Analyser/nsrt/bug-11109.php new file mode 100644 index 00000000000..9693317ec2b --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/bug-11109.php @@ -0,0 +1,40 @@ +%s'; + +if (empty($target)) { + assertType('\'%s\'', $htmlLinkStructure); + return sprintf($htmlLinkStructure, $url, $linkText); +} + +assertType('\'%s\'', $htmlLinkStructure); +return sprintf($htmlLinkStructure, $url, $target, $linkText); diff --git a/tests/PHPStan/Rules/Functions/PrintfParametersRuleTest.php b/tests/PHPStan/Rules/Functions/PrintfParametersRuleTest.php index d31344d8cdb..8f2f17c5f86 100644 --- a/tests/PHPStan/Rules/Functions/PrintfParametersRuleTest.php +++ b/tests/PHPStan/Rules/Functions/PrintfParametersRuleTest.php @@ -160,4 +160,9 @@ public function testBug14567(): void $this->analyse([__DIR__ . '/data/bug-14567.php'], []); } + public function testBug9854(): void + { + $this->analyse([__DIR__ . '/data/bug-9854.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Functions/data/bug-9854.php b/tests/PHPStan/Rules/Functions/data/bug-9854.php new file mode 100644 index 00000000000..1f1e308c505 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/bug-9854.php @@ -0,0 +1,16 @@ +%s'; + +if (empty($target)) { + return sprintf($htmlLinkStructure, $url, $linkText); +} + +return sprintf($htmlLinkStructure, $url, $target, $linkText); diff --git a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php index 397dabed083..b0968ae3b07 100644 --- a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php +++ b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php @@ -1723,4 +1723,22 @@ public function testBug2032(): void ]); } + public function testBug11109(): void + { + $this->cliArgumentsVariablesRegistered = true; + $this->polluteScopeWithLoopInitialAssignments = false; + $this->checkMaybeUndefinedVariables = true; + $this->polluteScopeWithAlwaysIterableForeach = true; + $this->analyse([__DIR__ . '/data/bug-11109.php'], []); + } + + public function testBug6608(): void + { + $this->cliArgumentsVariablesRegistered = true; + $this->polluteScopeWithLoopInitialAssignments = false; + $this->checkMaybeUndefinedVariables = true; + $this->polluteScopeWithAlwaysIterableForeach = true; + $this->analyse([__DIR__ . '/data/bug-6608.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/bug-11109.php b/tests/PHPStan/Rules/Variables/data/bug-11109.php new file mode 100644 index 00000000000..e97dcc67072 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-11109.php @@ -0,0 +1,16 @@ +