From a769a1c90c7d9b051e42225104dbc3c83768a553 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Sat, 1 Jul 2023 12:42:03 +0200 Subject: [PATCH] TypeSpecifier - handle AlwaysRememberedExpr in handling Identical --- src/Analyser/TypeSpecifier.php | 119 +++++++++++------- .../Analyser/NodeScopeResolverTest.php | 1 + tests/PHPStan/Analyser/TypeSpecifierTest.php | 32 +++++ .../Comparison/MatchExpressionRuleTest.php | 9 ++ .../Rules/Comparison/data/bug-9499.php | 52 ++++++++ 5 files changed, 169 insertions(+), 44 deletions(-) create mode 100644 tests/PHPStan/Rules/Comparison/data/bug-9499.php diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 131d450537..7b75de30f5 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -1645,6 +1645,10 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty if ($leftExpr instanceof AlwaysRememberedExpr) { $unwrappedLeftExpr = $leftExpr->getExpr(); } + $unwrappedRightExpr = $rightExpr; + if ($rightExpr instanceof AlwaysRememberedExpr) { + $unwrappedRightExpr = $rightExpr->getExpr(); + } $rightType = $scope->getType($rightExpr); if ( $context->true() @@ -1695,57 +1699,54 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty $specifiedType = $this->specifyTypesForConstantBinaryExpression($exprNode, $constantType, $context, $scope, $rootExpr); if ($specifiedType !== null) { + if ($exprNode instanceof AlwaysRememberedExpr) { + $specifiedType->unionWith( + $this->create($exprNode->getExpr(), $constantType, $context, false, $scope, $rootExpr), + ); + } return $specifiedType; } } - if ($rightExpr instanceof AlwaysRememberedExpr) { - $rightExpr = $rightExpr->getExpr(); - } - - if ($leftExpr instanceof AlwaysRememberedExpr) { - $leftExpr = $leftExpr->getExpr(); - } - if ( $context->true() && - $leftExpr instanceof ClassConstFetch && - $leftExpr->class instanceof Expr && - $leftExpr->name instanceof Node\Identifier && - $rightExpr instanceof ClassConstFetch && + $unwrappedLeftExpr instanceof ClassConstFetch && + $unwrappedLeftExpr->class instanceof Expr && + $unwrappedLeftExpr->name instanceof Node\Identifier && + $unwrappedRightExpr instanceof ClassConstFetch && $rightType instanceof ConstantStringType && - strtolower($leftExpr->name->toString()) === 'class' + strtolower($unwrappedLeftExpr->name->toString()) === 'class' ) { return $this->specifyTypesInCondition( $scope, new Instanceof_( - $leftExpr->class, + $unwrappedLeftExpr->class, new Name($rightType->getValue()), ), $context, $rootExpr, - )->unionWith($this->create($expr->left, $rightType, $context, false, $scope, $rootExpr)); + )->unionWith($this->create($leftExpr, $rightType, $context, false, $scope, $rootExpr)); } $leftType = $scope->getType($leftExpr); if ( $context->true() && - $rightExpr instanceof ClassConstFetch && - $rightExpr->class instanceof Expr && - $rightExpr->name instanceof Node\Identifier && - $leftExpr instanceof ClassConstFetch && + $unwrappedRightExpr instanceof ClassConstFetch && + $unwrappedRightExpr->class instanceof Expr && + $unwrappedRightExpr->name instanceof Node\Identifier && + $unwrappedLeftExpr instanceof ClassConstFetch && $leftType instanceof ConstantStringType && - strtolower($rightExpr->name->toString()) === 'class' + strtolower($unwrappedRightExpr->name->toString()) === 'class' ) { return $this->specifyTypesInCondition( $scope, new Instanceof_( - $rightExpr->class, + $unwrappedRightExpr->class, new Name($leftType->getValue()), ), $context, $rootExpr, - )->unionWith($this->create($expr->right, $leftType, $context, false, $scope, $rootExpr)); + )->unionWith($this->create($rightExpr, $leftType, $context, false, $scope, $rootExpr)); } if ($context->false()) { @@ -1753,44 +1754,64 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty if ($identicalType instanceof ConstantBooleanType) { $never = new NeverType(); $contextForTypes = $identicalType->getValue() ? $context->negate() : $context; - $leftTypes = $this->create($expr->left, $never, $contextForTypes, false, $scope, $rootExpr); - $rightTypes = $this->create($expr->right, $never, $contextForTypes, false, $scope, $rootExpr); + $leftTypes = $this->create($leftExpr, $never, $contextForTypes, false, $scope, $rootExpr) + ->unionWith($this->create($unwrappedLeftExpr, $never, $contextForTypes, false, $scope, $rootExpr)); + $rightTypes = $this->create($rightExpr, $never, $contextForTypes, false, $scope, $rootExpr) + ->unionWith($this->create($unwrappedRightExpr, $never, $contextForTypes, false, $scope, $rootExpr)); return $leftTypes->unionWith($rightTypes); } } $types = null; - $exprLeftType = $scope->getType($expr->left); - $exprRightType = $scope->getType($expr->right); if ( - count($exprLeftType->getFiniteTypes()) === 1 - || ($exprLeftType->isConstantValue()->yes() && !$exprRightType->equals($exprLeftType) && $exprRightType->isSuperTypeOf($exprLeftType)->yes()) + count($leftType->getFiniteTypes()) === 1 + || ($leftType->isConstantValue()->yes() && !$rightType->equals($leftType) && $rightType->isSuperTypeOf($leftType)->yes()) ) { $types = $this->create( - $expr->right, - $exprLeftType, + $rightExpr, + $leftType, $context, false, $scope, $rootExpr, ); + if ($rightExpr instanceof AlwaysRememberedExpr) { + $types = $types->unionWith($this->create( + $unwrappedRightExpr, + $leftType, + $context, + false, + $scope, + $rootExpr, + )); + } } if ( - count($exprRightType->getFiniteTypes()) === 1 - || ($exprRightType->isConstantValue()->yes() && !$exprLeftType->equals($exprRightType) && $exprLeftType->isSuperTypeOf($exprRightType)->yes()) + count($rightType->getFiniteTypes()) === 1 + || ($rightType->isConstantValue()->yes() && !$leftType->equals($rightType) && $leftType->isSuperTypeOf($rightType)->yes()) ) { - $leftType = $this->create( - $expr->left, - $exprRightType, + $leftTypes = $this->create( + $leftExpr, + $rightType, $context, false, $scope, $rootExpr, ); + if ($leftExpr instanceof AlwaysRememberedExpr) { + $leftTypes = $leftTypes->unionWith($this->create( + $unwrappedLeftExpr, + $rightType, + $context, + false, + $scope, + $rootExpr, + )); + } if ($types !== null) { - $types = $types->unionWith($leftType); + $types = $types->unionWith($leftTypes); } else { - $types = $leftType; + $types = $leftTypes; } } @@ -1798,21 +1819,31 @@ public function resolveIdentical(Expr\BinaryOp\Identical $expr, Scope $scope, Ty return $types; } - $leftExprString = $this->exprPrinter->printExpr($expr->left); - $rightExprString = $this->exprPrinter->printExpr($expr->right); + $leftExprString = $this->exprPrinter->printExpr($unwrappedLeftExpr); + $rightExprString = $this->exprPrinter->printExpr($unwrappedRightExpr); if ($leftExprString === $rightExprString) { - if (!$expr->left instanceof Expr\Variable || !$expr->right instanceof Expr\Variable) { + if (!$unwrappedLeftExpr instanceof Expr\Variable || !$unwrappedRightExpr instanceof Expr\Variable) { return new SpecifiedTypes([], [], false, [], $rootExpr); } } if ($context->true()) { - $leftTypes = $this->create($expr->left, $exprRightType, $context, false, $scope, $rootExpr); - $rightTypes = $this->create($expr->right, $exprLeftType, $context, false, $scope, $rootExpr); + $leftTypes = $this->create($leftExpr, $rightType, $context, false, $scope, $rootExpr); + $rightTypes = $this->create($rightExpr, $leftType, $context, false, $scope, $rootExpr); + if ($leftExpr instanceof AlwaysRememberedExpr) { + $leftTypes = $leftTypes->unionWith( + $this->create($unwrappedLeftExpr, $rightType, $context, false, $scope, $rootExpr), + ); + } + if ($rightExpr instanceof AlwaysRememberedExpr) { + $rightTypes = $rightTypes->unionWith( + $this->create($unwrappedRightExpr, $leftType, $context, false, $scope, $rootExpr), + ); + } return $leftTypes->unionWith($rightTypes); } elseif ($context->false()) { - return $this->create($expr->left, $exprLeftType, $context, false, $scope, $rootExpr)->normalize($scope) - ->intersectWith($this->create($expr->right, $exprRightType, $context, false, $scope, $rootExpr)->normalize($scope)); + return $this->create($leftExpr, $leftType, $context, false, $scope, $rootExpr)->normalize($scope) + ->intersectWith($this->create($rightExpr, $rightType, $context, false, $scope, $rootExpr)->normalize($scope)); } return new SpecifiedTypes([], [], false, [], $rootExpr); diff --git a/tests/PHPStan/Analyser/NodeScopeResolverTest.php b/tests/PHPStan/Analyser/NodeScopeResolverTest.php index 564695cb89..afe25e5951 100644 --- a/tests/PHPStan/Analyser/NodeScopeResolverTest.php +++ b/tests/PHPStan/Analyser/NodeScopeResolverTest.php @@ -1260,6 +1260,7 @@ public function dataFileAsserts(): iterable yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-8827.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-4907.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-8924.php'); + yield from $this->gatherAssertTypes(__DIR__ . '/../Rules/Comparison/data/bug-9499.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-5998.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/trait-type-alias.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-8609.php'); diff --git a/tests/PHPStan/Analyser/TypeSpecifierTest.php b/tests/PHPStan/Analyser/TypeSpecifierTest.php index 9e13b40e29..44c164f5b5 100644 --- a/tests/PHPStan/Analyser/TypeSpecifierTest.php +++ b/tests/PHPStan/Analyser/TypeSpecifierTest.php @@ -2,6 +2,7 @@ namespace PHPStan\Analyser; +use Bug9499\FooEnum; use PhpParser\Node\Arg; use PhpParser\Node\Expr; use PhpParser\Node\Expr\BinaryOp\Equal; @@ -18,6 +19,7 @@ use PhpParser\Node\Scalar\String_; use PhpParser\Node\VarLikeIdentifier; use PhpParser\PrettyPrinter\Standard; +use PHPStan\Node\Expr\AlwaysRememberedExpr; use PHPStan\Node\Printer\Printer; use PHPStan\Testing\PHPStanTestCase; use PHPStan\Type\ArrayType; @@ -1211,6 +1213,36 @@ public function dataCondition(): array ], [], ], + [ + new Identical( + new PropertyFetch(new Variable('foo'), 'bar'), + new Expr\ClassConstFetch(new Name(FooEnum::class), 'A'), + ), + [ + '$foo->bar' => 'Bug9499\FooEnum::A', + ], + [ + '$foo->bar' => '~Bug9499\FooEnum::A', + ], + ], + [ + new Identical( + new AlwaysRememberedExpr( + new PropertyFetch(new Variable('foo'), 'bar'), + new ObjectType(FooEnum::class), + new ObjectType(FooEnum::class), + ), + new Expr\ClassConstFetch(new Name(FooEnum::class), 'A'), + ), + [ + '__phpstanRembered($foo->bar)' => 'Bug9499\FooEnum::A', + '$foo->bar' => 'Bug9499\FooEnum::A', + ], + [ + '__phpstanRembered($foo->bar)' => '~Bug9499\FooEnum::A', + '$foo->bar' => '~Bug9499\FooEnum::A', + ], + ], ]; } diff --git a/tests/PHPStan/Rules/Comparison/MatchExpressionRuleTest.php b/tests/PHPStan/Rules/Comparison/MatchExpressionRuleTest.php index 3bd79bdd02..8547b2f603 100644 --- a/tests/PHPStan/Rules/Comparison/MatchExpressionRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/MatchExpressionRuleTest.php @@ -516,4 +516,13 @@ public function testBug8536(): void $this->analyse([__DIR__ . '/data/bug-8536.php'], []); } + public function testBug9499(): void + { + if (PHP_VERSION_ID < 80100) { + $this->markTestSkipped('Test requires PHP 8.1.'); + } + + $this->analyse([__DIR__ . '/data/bug-9499.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Comparison/data/bug-9499.php b/tests/PHPStan/Rules/Comparison/data/bug-9499.php new file mode 100644 index 0000000000..0a833ad59d --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-9499.php @@ -0,0 +1,52 @@ += 8.1 + +namespace Bug9499; + +use function PHPStan\Testing\assertType; + +enum FooEnum +{ + case A; + case B; + case C; + case D; +} + +class Foo +{ + public function __construct(public readonly FooEnum $f) + { + } +} + +function test(FooEnum $f, Foo $foo): void +{ + $arr = ['f' => $f]; + match ($arr['f']) { + FooEnum::A, FooEnum::B => match ($arr['f']) { + FooEnum::A => 'a', + FooEnum::B => 'b', + }, + default => '', + }; + match ($foo->f) { + FooEnum::A, FooEnum::B => match ($foo->f) { + FooEnum::A => 'a', + FooEnum::B => 'b', + }, + default => '', + }; +} + +function test2(FooEnum $f, Foo $foo): void +{ + $arr = ['f' => $f]; + match ($arr['f']) { + FooEnum::A, FooEnum::B => assertType(FooEnum::class . '::A|' . FooEnum::class . '::B', $arr['f']), + default => '', + }; + match ($foo->f) { + FooEnum::A, FooEnum::B => assertType(FooEnum::class . '::A|' . FooEnum::class . '::B', $foo->f), + default => '', + }; +}