From 6c45692d2034fedb05ab0c4affa38dd03cd730a5 Mon Sep 17 00:00:00 2001 From: Brad <28307684+mad-briller@users.noreply.github.com> Date: Sat, 30 Jul 2022 13:16:45 +0100 Subject: [PATCH 1/3] Added error message to impossible instanceof rule to warn when comparing a trait. --- .../Classes/ImpossibleInstanceOfRule.php | 17 +++++++++++++++ src/Type/ObjectType.php | 4 ++++ .../Classes/ImpossibleInstanceOfRuleTest.php | 18 +++++++++++++++- tests/PHPStan/Rules/Classes/data/bug-7720.php | 21 +++++++++++++++++++ tests/PHPStan/Type/ObjectTypeTest.php | 11 ++++++++++ 5 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 tests/PHPStan/Rules/Classes/data/bug-7720.php diff --git a/src/Rules/Classes/ImpossibleInstanceOfRule.php b/src/Rules/Classes/ImpossibleInstanceOfRule.php index d047d4b2f5..a82baa57e4 100644 --- a/src/Rules/Classes/ImpossibleInstanceOfRule.php +++ b/src/Rules/Classes/ImpossibleInstanceOfRule.php @@ -4,6 +4,7 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; +use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Constant\ConstantBooleanType; @@ -23,6 +24,7 @@ class ImpossibleInstanceOfRule implements Rule public function __construct( private bool $checkAlwaysTrueInstanceof, private bool $treatPhpDocTypesAsCertain, + private ReflectionProvider $reflectionProvider, ) { } @@ -36,6 +38,7 @@ public function processNode(Node $node, Scope $scope): array { $instanceofType = $scope->getType($node); $expressionType = $scope->getType($node->expr); + $className = null; if ($node->class instanceof Node\Name) { $className = $scope->resolveName($node->class); @@ -57,6 +60,20 @@ public function processNode(Node $node, Scope $scope): array } } + if ($className !== null) { + $classReflection = $this->reflectionProvider->getClass($className); + + if ($classReflection->isTrait()) { + return [ + RuleErrorBuilder::message(sprintf( + 'Instanceof between %s and trait %s will always evaluate to false.', + $expressionType->describe(VerbosityLevel::typeOnly()), + $classType->describe(VerbosityLevel::typeOnly()), + ))->build(), + ]; + } + } + if (!$instanceofType instanceof ConstantBooleanType) { return []; } diff --git a/src/Type/ObjectType.php b/src/Type/ObjectType.php index 9febf101f3..d1b7b692a5 100644 --- a/src/Type/ObjectType.php +++ b/src/Type/ObjectType.php @@ -309,6 +309,10 @@ public function isSuperTypeOf(Type $type): TrinaryLogic $thisClassReflection = $this->getClassReflection(); $thatClassReflection = $reflectionProvider->getClass($thatClassName); + if ($thisClassReflection->isTrait() || $thatClassReflection->isTrait()) { + return TrinaryLogic::createNo(); + } + if ($thisClassReflection->getName() === $thatClassReflection->getName()) { return self::$superTypes[$thisDescription][$description] = $transformResult(TrinaryLogic::createYes()); } diff --git a/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php b/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php index 9a778aab3c..fe893c384a 100644 --- a/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php +++ b/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php @@ -17,7 +17,11 @@ class ImpossibleInstanceOfRuleTest extends RuleTestCase protected function getRule(): Rule { - return new ImpossibleInstanceOfRule($this->checkAlwaysTrueInstanceOf, $this->treatPhpDocTypesAsCertain); + return new ImpossibleInstanceOfRule( + $this->checkAlwaysTrueInstanceOf, + $this->treatPhpDocTypesAsCertain, + $this->createReflectionProvider(), + ); } protected function shouldTreatPhpDocTypesAsCertain(): bool @@ -346,4 +350,16 @@ public function testBug6213(): void $this->analyse([__DIR__ . '/data/bug-6213.php'], []); } + public function testBug7720(): void + { + $this->checkAlwaysTrueInstanceOf = true; + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/bug-7720.php'], [ + [ + 'Instanceof between mixed and trait Bug7720\FooBar will always evaluate to false.', + 17, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Classes/data/bug-7720.php b/tests/PHPStan/Rules/Classes/data/bug-7720.php new file mode 100644 index 0000000000..ff20bd0e61 --- /dev/null +++ b/tests/PHPStan/Rules/Classes/data/bug-7720.php @@ -0,0 +1,21 @@ +foo(); + } + } +} diff --git a/tests/PHPStan/Type/ObjectTypeTest.php b/tests/PHPStan/Type/ObjectTypeTest.php index 204ac35e6a..3cb3212860 100644 --- a/tests/PHPStan/Type/ObjectTypeTest.php +++ b/tests/PHPStan/Type/ObjectTypeTest.php @@ -26,6 +26,7 @@ use PHPStan\Type\Generic\TemplateTypeFactory; use PHPStan\Type\Generic\TemplateTypeScope; use PHPStan\Type\Generic\TemplateTypeVariance; +use PHPStan\Type\Traits\ConstantNumericComparisonTypeTrait; use SimpleXMLElement; use stdClass; use Throwable; @@ -418,6 +419,16 @@ public function dataIsSuperTypeOf(): array new ObjectType(ExtendsThrowable::class), TrinaryLogic::createMaybe(), ], + 59 => [ + new ObjectType(DateTime::class), + new ObjectType(ConstantNumericComparisonTypeTrait::class), + TrinaryLogic::createNo(), + ], + 60 => [ + new ObjectType(ConstantNumericComparisonTypeTrait::class), + new ObjectType(DateTime::class), + TrinaryLogic::createNo(), + ], ]; } From 1477644099343245d7769804493b0b39bdbe08f8 Mon Sep 17 00:00:00 2001 From: Brad <28307684+mad-briller@users.noreply.github.com> Date: Sat, 30 Jul 2022 14:31:37 +0100 Subject: [PATCH 2/3] Fixed not calling hasClass before getClass in impossible instanceof rule. --- src/Rules/Classes/ImpossibleInstanceOfRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Rules/Classes/ImpossibleInstanceOfRule.php b/src/Rules/Classes/ImpossibleInstanceOfRule.php index a82baa57e4..7205f74530 100644 --- a/src/Rules/Classes/ImpossibleInstanceOfRule.php +++ b/src/Rules/Classes/ImpossibleInstanceOfRule.php @@ -60,7 +60,7 @@ public function processNode(Node $node, Scope $scope): array } } - if ($className !== null) { + if ($className !== null && $this->reflectionProvider->hasClass($className)) { $classReflection = $this->reflectionProvider->getClass($className); if ($classReflection->isTrait()) { From b27cf7ede9aa533a3ac4ea88ff6f583c15abec86 Mon Sep 17 00:00:00 2001 From: Brad <28307684+mad-briller@users.noreply.github.com> Date: Sat, 30 Jul 2022 16:31:18 +0100 Subject: [PATCH 3/3] Moved instanceof trait check to the ExistingClassInInstanceOfRule. --- .../Classes/ExistingClassInInstanceOfRule.php | 22 +++++++++++++++++-- .../Classes/ImpossibleInstanceOfRule.php | 17 -------------- .../ExistingClassInInstanceOfRuleTest.php | 10 +++++++++ .../Classes/ImpossibleInstanceOfRuleTest.php | 18 +-------------- 4 files changed, 31 insertions(+), 36 deletions(-) diff --git a/src/Rules/Classes/ExistingClassInInstanceOfRule.php b/src/Rules/Classes/ExistingClassInInstanceOfRule.php index 775eadf450..c04a3d4b11 100644 --- a/src/Rules/Classes/ExistingClassInInstanceOfRule.php +++ b/src/Rules/Classes/ExistingClassInInstanceOfRule.php @@ -10,6 +10,8 @@ use PHPStan\Rules\ClassNameNodePair; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; +use PHPStan\Type\VerbosityLevel; +use function array_merge; use function in_array; use function sprintf; use function strtolower; @@ -57,6 +59,8 @@ public function processNode(Node $node, Scope $scope): array return []; } + $errors = []; + if (!$this->reflectionProvider->hasClass($name)) { if ($scope->isInClassExists($name)) { return []; @@ -66,10 +70,24 @@ public function processNode(Node $node, Scope $scope): array RuleErrorBuilder::message(sprintf('Class %s not found.', $name))->line($class->getLine())->discoveringSymbolsTip()->build(), ]; } elseif ($this->checkClassCaseSensitivity) { - return $this->classCaseSensitivityCheck->checkClassNames([new ClassNameNodePair($name, $class)]); + $errors = array_merge( + $errors, + $this->classCaseSensitivityCheck->checkClassNames([new ClassNameNodePair($name, $class)]), + ); + } + + $classReflection = $this->reflectionProvider->getClass($name); + $expressionType = $scope->getType($node->expr); + + if ($classReflection->isTrait()) { + $errors[] = RuleErrorBuilder::message(sprintf( + 'Instanceof between %s and trait %s will always evaluate to false.', + $expressionType->describe(VerbosityLevel::typeOnly()), + $name, + ))->build(); } - return []; + return $errors; } } diff --git a/src/Rules/Classes/ImpossibleInstanceOfRule.php b/src/Rules/Classes/ImpossibleInstanceOfRule.php index 7205f74530..d047d4b2f5 100644 --- a/src/Rules/Classes/ImpossibleInstanceOfRule.php +++ b/src/Rules/Classes/ImpossibleInstanceOfRule.php @@ -4,7 +4,6 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; -use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Constant\ConstantBooleanType; @@ -24,7 +23,6 @@ class ImpossibleInstanceOfRule implements Rule public function __construct( private bool $checkAlwaysTrueInstanceof, private bool $treatPhpDocTypesAsCertain, - private ReflectionProvider $reflectionProvider, ) { } @@ -38,7 +36,6 @@ public function processNode(Node $node, Scope $scope): array { $instanceofType = $scope->getType($node); $expressionType = $scope->getType($node->expr); - $className = null; if ($node->class instanceof Node\Name) { $className = $scope->resolveName($node->class); @@ -60,20 +57,6 @@ public function processNode(Node $node, Scope $scope): array } } - if ($className !== null && $this->reflectionProvider->hasClass($className)) { - $classReflection = $this->reflectionProvider->getClass($className); - - if ($classReflection->isTrait()) { - return [ - RuleErrorBuilder::message(sprintf( - 'Instanceof between %s and trait %s will always evaluate to false.', - $expressionType->describe(VerbosityLevel::typeOnly()), - $classType->describe(VerbosityLevel::typeOnly()), - ))->build(), - ]; - } - } - if (!$instanceofType instanceof ConstantBooleanType) { return []; } diff --git a/tests/PHPStan/Rules/Classes/ExistingClassInInstanceOfRuleTest.php b/tests/PHPStan/Rules/Classes/ExistingClassInInstanceOfRuleTest.php index e346e8cb1a..5533f01879 100644 --- a/tests/PHPStan/Rules/Classes/ExistingClassInInstanceOfRuleTest.php +++ b/tests/PHPStan/Rules/Classes/ExistingClassInInstanceOfRuleTest.php @@ -60,4 +60,14 @@ public function testClassExists(): void $this->analyse([__DIR__ . '/data/instanceof-class-exists.php'], []); } + public function testBug7720(): void + { + $this->analyse([__DIR__ . '/data/bug-7720.php'], [ + [ + 'Instanceof between mixed and trait Bug7720\FooBar will always evaluate to false.', + 17, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php b/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php index fe893c384a..9a778aab3c 100644 --- a/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php +++ b/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php @@ -17,11 +17,7 @@ class ImpossibleInstanceOfRuleTest extends RuleTestCase protected function getRule(): Rule { - return new ImpossibleInstanceOfRule( - $this->checkAlwaysTrueInstanceOf, - $this->treatPhpDocTypesAsCertain, - $this->createReflectionProvider(), - ); + return new ImpossibleInstanceOfRule($this->checkAlwaysTrueInstanceOf, $this->treatPhpDocTypesAsCertain); } protected function shouldTreatPhpDocTypesAsCertain(): bool @@ -350,16 +346,4 @@ public function testBug6213(): void $this->analyse([__DIR__ . '/data/bug-6213.php'], []); } - public function testBug7720(): void - { - $this->checkAlwaysTrueInstanceOf = true; - $this->treatPhpDocTypesAsCertain = true; - $this->analyse([__DIR__ . '/data/bug-7720.php'], [ - [ - 'Instanceof between mixed and trait Bug7720\FooBar will always evaluate to false.', - 17, - ], - ]); - } - }