From 3de3c8574ec1a293922b2f7c03086a518b8a96a7 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Mon, 30 Oct 2023 19:03:47 +0100 Subject: [PATCH] Fix variable certainty for ArrayDimFetch in falsey isset context --- .../EnsuredNonNullabilityResultExpression.php | 7 + src/Analyser/MutatingScope.php | 18 +- src/Analyser/NodeScopeResolver.php | 13 +- src/Analyser/TypeSpecifier.php | 32 ++-- .../Analyser/NodeScopeResolverTest.php | 3 + tests/PHPStan/Analyser/data/bug-7291.php | 25 +++ .../Analyser/data/falsey-empty-certainty.php | 69 +++++++ .../Analyser/data/falsey-isset-certainty.php | 168 ++++++++++++++++++ .../Variables/DefinedVariableRuleTest.php | 22 +++ .../PHPStan/Rules/Variables/IssetRuleTest.php | 17 ++ .../Variables/data/isstring-certainty.php | 22 +++ 11 files changed, 379 insertions(+), 17 deletions(-) create mode 100644 tests/PHPStan/Analyser/data/bug-7291.php create mode 100644 tests/PHPStan/Analyser/data/falsey-empty-certainty.php create mode 100644 tests/PHPStan/Analyser/data/falsey-isset-certainty.php create mode 100644 tests/PHPStan/Rules/Variables/data/isstring-certainty.php diff --git a/src/Analyser/EnsuredNonNullabilityResultExpression.php b/src/Analyser/EnsuredNonNullabilityResultExpression.php index a7ed1572f8..33f94341e6 100644 --- a/src/Analyser/EnsuredNonNullabilityResultExpression.php +++ b/src/Analyser/EnsuredNonNullabilityResultExpression.php @@ -3,6 +3,7 @@ namespace PHPStan\Analyser; use PhpParser\Node\Expr; +use PHPStan\TrinaryLogic; use PHPStan\Type\Type; class EnsuredNonNullabilityResultExpression @@ -12,6 +13,7 @@ public function __construct( private Expr $expression, private Type $originalType, private Type $originalNativeType, + private TrinaryLogic $certainty, ) { } @@ -31,4 +33,9 @@ public function getOriginalNativeType(): Type return $this->originalNativeType; } + public function getCertainty(): TrinaryLogic + { + return $this->certainty; + } + } diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 7bd669c920..653fbe732d 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -2340,6 +2340,10 @@ public function isSpecified(Expr $node): bool /** @api */ public function hasExpressionType(Expr $node): TrinaryLogic { + if ($node instanceof Variable && is_string($node->name)) { + return $this->hasVariableType($node->name); + } + $exprString = $this->getNodeKey($node); if (!isset($this->expressionTypes[$exprString])) { return TrinaryLogic::createNo(); @@ -3432,7 +3436,7 @@ public function unsetExpression(Expr $expr): self return $scope->invalidateExpression($expr); } - public function specifyExpressionType(Expr $expr, Type $type, Type $nativeType): self + public function specifyExpressionType(Expr $expr, Type $type, Type $nativeType, ?TrinaryLogic $certainty = null): self { if ($expr instanceof ConstFetch) { $loweredConstName = strtolower($expr->name->toString()); @@ -3466,6 +3470,7 @@ public function specifyExpressionType(Expr $expr, Type $type, Type $nativeType): if ($dimType instanceof ConstantIntegerType) { $types[] = new StringType(); } + $scope = $scope->specifyExpressionType( $expr->var, TypeCombinator::intersect( @@ -3473,16 +3478,23 @@ public function specifyExpressionType(Expr $expr, Type $type, Type $nativeType): new HasOffsetValueType($dimType, $type), ), $scope->getNativeType($expr->var), + $certainty, ); } } } + if ($certainty === null) { + $certainty = TrinaryLogic::createYes(); + } elseif ($certainty->no()) { + throw new ShouldNotHappenException(); + } + $exprString = $this->getNodeKey($expr); $expressionTypes = $scope->expressionTypes; - $expressionTypes[$exprString] = ExpressionTypeHolder::createYes($expr, $type); + $expressionTypes[$exprString] = new ExpressionTypeHolder($expr, $type, $certainty); $nativeTypes = $scope->nativeExpressionTypes; - $nativeTypes[$exprString] = ExpressionTypeHolder::createYes($expr, $nativeType); + $nativeTypes[$exprString] = new ExpressionTypeHolder($expr, $nativeType, $certainty); return $this->scopeFactory->create( $this->context, diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 1f8fd4d8d1..3c3acb499b 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1753,6 +1753,14 @@ private function ensureShallowNonNullability(MutatingScope $scope, Scope $origin if ($isNull->yes()) { return new EnsuredNonNullabilityResult($scope, []); } + + // keep certainty + $certainty = TrinaryLogic::createYes(); + $hasExpressionType = $originalScope->hasExpressionType($exprToSpecify); + if (!$hasExpressionType->no()) { + $certainty = $hasExpressionType; + } + $exprTypeWithoutNull = TypeCombinator::removeNull($exprType); if ($exprType->equals($exprTypeWithoutNull)) { $originalExprType = $originalScope->getType($exprToSpecify); @@ -1760,7 +1768,7 @@ private function ensureShallowNonNullability(MutatingScope $scope, Scope $origin $originalNativeType = $originalScope->getNativeType($exprToSpecify); return new EnsuredNonNullabilityResult($scope, [ - new EnsuredNonNullabilityResultExpression($exprToSpecify, $originalExprType, $originalNativeType), + new EnsuredNonNullabilityResultExpression($exprToSpecify, $originalExprType, $originalNativeType, $certainty), ]); } return new EnsuredNonNullabilityResult($scope, []); @@ -1776,7 +1784,7 @@ private function ensureShallowNonNullability(MutatingScope $scope, Scope $origin return new EnsuredNonNullabilityResult( $scope, [ - new EnsuredNonNullabilityResultExpression($exprToSpecify, $exprType, $nativeType), + new EnsuredNonNullabilityResultExpression($exprToSpecify, $exprType, $nativeType, $certainty), ], ); } @@ -1806,6 +1814,7 @@ private function revertNonNullability(MutatingScope $scope, array $specifiedExpr $specifiedExpressionResult->getExpression(), $specifiedExpressionResult->getOriginalType(), $specifiedExpressionResult->getOriginalNativeType(), + $specifiedExpressionResult->getCertainty(), ); } diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 532a03e89a..f981395f34 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -717,11 +717,14 @@ public function specifyTypesInCondition( $types = null; foreach ($vars as $var) { + $type = new SpecifiedTypes(); + if ($var instanceof Expr\Variable && is_string($var->name)) { if ($scope->hasVariableType($var->name)->no()) { return new SpecifiedTypes([], [], false, [], $rootExpr); } } + if ( $var instanceof ArrayDimFetch && $var->dim !== null @@ -738,36 +741,32 @@ public function specifyTypesInCondition( $scope, $rootExpr, ); - } else { - $type = new SpecifiedTypes(); } - - $type = $type->unionWith( - $this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope, $rootExpr), - ); - } else { - $type = $this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope, $rootExpr); } if ( $var instanceof PropertyFetch && $var->name instanceof Node\Identifier ) { - $type = $type->unionWith($this->create($var->var, new IntersectionType([ + $type = $this->create($var->var, new IntersectionType([ new ObjectWithoutClassType(), new HasPropertyType($var->name->toString()), - ]), TypeSpecifierContext::createTruthy(), false, $scope, $rootExpr)); + ]), TypeSpecifierContext::createTruthy(), false, $scope, $rootExpr); } elseif ( $var instanceof StaticPropertyFetch && $var->class instanceof Expr && $var->name instanceof Node\VarLikeIdentifier ) { - $type = $type->unionWith($this->create($var->class, new IntersectionType([ + $type = $this->create($var->class, new IntersectionType([ new ObjectWithoutClassType(), new HasPropertyType($var->name->toString()), - ]), TypeSpecifierContext::createTruthy(), false, $scope, $rootExpr)); + ]), TypeSpecifierContext::createTruthy(), false, $scope, $rootExpr); } + $type = $type->unionWith( + $this->create($var, new NullType(), TypeSpecifierContext::createFalse(), false, $scope, $rootExpr), + ); + if ($types === null) { $types = $type; } else { @@ -792,6 +791,15 @@ public function specifyTypesInCondition( } elseif ( $expr instanceof Expr\Empty_ ) { + if (!$scope instanceof MutatingScope) { + throw new ShouldNotHappenException(); + } + + $isset = $scope->issetCheck($expr->expr, static fn () => true); + if ($isset === false) { + return new SpecifiedTypes(); + } + return $this->specifyTypesInCondition($scope, new BooleanOr( new Expr\BooleanNot(new Expr\Isset_([$expr->expr])), new Expr\BooleanNot($expr->expr), diff --git a/tests/PHPStan/Analyser/NodeScopeResolverTest.php b/tests/PHPStan/Analyser/NodeScopeResolverTest.php index 1fcaba3983..95e17a2eb6 100644 --- a/tests/PHPStan/Analyser/NodeScopeResolverTest.php +++ b/tests/PHPStan/Analyser/NodeScopeResolverTest.php @@ -1356,6 +1356,9 @@ public function dataFileAsserts(): iterable yield from $this->gatherAssertTypes(__DIR__ . '/data/impure-connection-fns.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-9963.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-9995.php'); + yield from $this->gatherAssertTypes(__DIR__ . '/data/falsey-isset-certainty.php'); + yield from $this->gatherAssertTypes(__DIR__ . '/data/falsey-empty-certainty.php'); + yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-7291.php'); } /** diff --git a/tests/PHPStan/Analyser/data/bug-7291.php b/tests/PHPStan/Analyser/data/bug-7291.php new file mode 100644 index 0000000000..cae3e945b3 --- /dev/null +++ b/tests/PHPStan/Analyser/data/bug-7291.php @@ -0,0 +1,25 @@ +foo; + + assertType('stdClass|null', $a); + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } +} diff --git a/tests/PHPStan/Analyser/data/falsey-empty-certainty.php b/tests/PHPStan/Analyser/data/falsey-empty-certainty.php new file mode 100644 index 0000000000..cde86585aa --- /dev/null +++ b/tests/PHPStan/Analyser/data/falsey-empty-certainty.php @@ -0,0 +1,69 @@ + null]; + if (rand() % 3) { + $a = ['bar' => 'hello']; + } + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + if (empty($a['bar'])) { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); +} + +function falseyEmptyUncertainPropertyFetch(): void +{ + if (rand() % 2) { + $a = new \stdClass(); + if (rand() % 3) { + $a->x = 'hello'; + } + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + if (empty($a->x)) { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); +} + +function justEmpty(): void +{ + assertVariableCertainty(TrinaryLogic::createNo(), $foo); + if (!empty($foo)) { + assertVariableCertainty(TrinaryLogic::createNo(), $foo); + } else { + assertVariableCertainty(TrinaryLogic::createNo(), $foo); + } + assertVariableCertainty(TrinaryLogic::createNo(), $foo); +} + +function maybeEmpty(): void +{ + if (rand() % 2) { + $foo = 1; + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); + if (!empty($foo)) { + assertVariableCertainty(TrinaryLogic::createYes(), $foo); + } else { + assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); + } +} diff --git a/tests/PHPStan/Analyser/data/falsey-isset-certainty.php b/tests/PHPStan/Analyser/data/falsey-isset-certainty.php new file mode 100644 index 0000000000..f13f702364 --- /dev/null +++ b/tests/PHPStan/Analyser/data/falsey-isset-certainty.php @@ -0,0 +1,168 @@ +bar = null; + if (rand() % 3) { + $a->bar = 'hello'; + } + + assertVariableCertainty(TrinaryLogic::createYes(), $a); + if (isset($a->bar)) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } + + assertVariableCertainty(TrinaryLogic::createYes(), $a); +} + +function falseyIssetUncertainArrayDimFetchOnProperty(): void +{ + if (rand() % 2) { + $a = new \stdClass(); + $a->bar = null; + $a = ['bar' => null]; + if (rand() % 3) { + $a->bar = 'hello'; + } + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + if (isset($a->bar)) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); +} + +function falseyIssetUncertainPropertyFetch(): void +{ + if (rand() % 2) { + $a = new \stdClass(); + if (rand() % 3) { + $a->x = 'hello'; + } + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + if (isset($a->x)) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); +} + +function falseyIssetArrayDimFetch(): void +{ + $a = ['bar' => null]; + if (rand() % 3) { + $a = ['bar' => 'hello']; + } + + assertVariableCertainty(TrinaryLogic::createYes(), $a); + if (isset($a['bar'])) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } + + assertVariableCertainty(TrinaryLogic::createYes(), $a); +} + +function falseyIssetUncertainArrayDimFetch(): void +{ + if (rand() % 2) { + $a = ['bar' => null]; + if (rand() % 3) { + $a = ['bar' => 'hello']; + } + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + if (isset($a['bar'])) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); +} + +function falseyIssetVariable(): void +{ + if (rand() % 2) { + $a = 'bar'; + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + if (isset($a)) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + } else { + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); + } + + assertVariableCertainty(TrinaryLogic::createMaybe(), $a); +} + +function falseyIssetWithAssignment(): void +{ + if (rand() % 2) { + $x = ['x' => 1]; + } + + if (isset($x[$z = getFoo()])) { + assertVariableCertainty(TrinaryLogic::createYes(), $z); + assertVariableCertainty(TrinaryLogic::createYes(), $x); + + } else { + assertVariableCertainty(TrinaryLogic::createYes(), $z); + assertVariableCertainty(TrinaryLogic::createMaybe(), $x); + } + + assertVariableCertainty(TrinaryLogic::createYes(), $z); + assertVariableCertainty(TrinaryLogic::createMaybe(), $x); +} + +function justIsset(): void +{ + if (isset($foo)) { + assertVariableCertainty(TrinaryLogic::createNo(), $foo); + } +} + +function maybeIsset(): void +{ + if (rand() % 2) { + $foo = 1; + } + assertVariableCertainty(TrinaryLogic::createMaybe(), $foo); + if (isset($foo)) { + assertVariableCertainty(TrinaryLogic::createYes(), $foo); + assertType('1', $foo); + } +} + +function isStringNarrowsMaybeCertainty(int $i, string $s): void +{ + if (rand(0, 1)) { + $a = rand(0,1) ? $i : $s; + } + + if (is_string($a)) { + assertVariableCertainty(TrinaryLogic::createYes(), $a); + echo $a; + } +} diff --git a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php index d3c3badffb..2b8eff2d25 100644 --- a/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php +++ b/tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php @@ -102,6 +102,10 @@ public function testDefinedVariables(): void 'Undefined variable: $variableInEmpty', 145, ], + [ + 'Undefined variable: $negatedVariableInEmpty', + 152, + ], [ 'Undefined variable: $variableInEmpty', 155, @@ -993,4 +997,22 @@ public function testBug5326(): void $this->analyse([__DIR__ . '/data/bug-5326.php'], []); } + public function testIsStringNarrowsCertainty(): void + { + $this->cliArgumentsVariablesRegistered = true; + $this->polluteScopeWithLoopInitialAssignments = true; + $this->checkMaybeUndefinedVariables = true; + $this->polluteScopeWithAlwaysIterableForeach = true; + $this->analyse([__DIR__ . '/data/isstring-certainty.php'], [ + [ + 'Variable $a might not be defined.', + 11, + ], + [ + 'Undefined variable: $a', + 19, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/IssetRuleTest.php b/tests/PHPStan/Rules/Variables/IssetRuleTest.php index a9c594bec5..34caffcc7b 100644 --- a/tests/PHPStan/Rules/Variables/IssetRuleTest.php +++ b/tests/PHPStan/Rules/Variables/IssetRuleTest.php @@ -450,4 +450,21 @@ public function testObjectShapes(): void $this->analyse([__DIR__ . '/data/isset-object-shapes.php'], []); } + public function testBug3985(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->strictUnnecessaryNullsafePropertyFetch = true; + + $this->analyse([__DIR__ . '/../../Analyser/data/bug-3985.php'], [ + [ + 'Variable $foo in isset() is never defined.', + 13, + ], + [ + 'Variable $foo in isset() is never defined.', + 21, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/isstring-certainty.php b/tests/PHPStan/Rules/Variables/data/isstring-certainty.php new file mode 100644 index 0000000000..270e978e68 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/isstring-certainty.php @@ -0,0 +1,22 @@ +