From eb19b7fc00447fa3683c5db87eab982f8017903c Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Wed, 2 Mar 2022 10:58:21 +0100 Subject: [PATCH 1/3] Fix is_subclass_of with same class --- .../IsAFunctionTypeSpecifyingExtension.php | 3 +- .../Php/IsAFunctionTypeSpecifyingHelper.php | 36 ++++++++++- ...classOfFunctionTypeSpecifyingExtension.php | 3 +- .../Analyser/NodeScopeResolverTest.php | 2 + tests/PHPStan/Analyser/data/bug-6305.php | 19 ++++++ tests/PHPStan/Analyser/data/is-a.php | 60 ++++++++++++++++++- .../PHPStan/Analyser/data/is-subclass-of.php | 55 +++++++++++++++++ ...mpossibleCheckTypeFunctionCallRuleTest.php | 16 +++++ .../Rules/Comparison/data/bug-6305.php | 15 +++++ 9 files changed, 201 insertions(+), 8 deletions(-) create mode 100644 tests/PHPStan/Analyser/data/bug-6305.php create mode 100644 tests/PHPStan/Analyser/data/is-subclass-of.php create mode 100644 tests/PHPStan/Rules/Comparison/data/bug-6305.php diff --git a/src/Type/Php/IsAFunctionTypeSpecifyingExtension.php b/src/Type/Php/IsAFunctionTypeSpecifyingExtension.php index 1220eabab0..2e03074b37 100644 --- a/src/Type/Php/IsAFunctionTypeSpecifyingExtension.php +++ b/src/Type/Php/IsAFunctionTypeSpecifyingExtension.php @@ -37,6 +37,7 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n if (count($node->getArgs()) < 2) { return new SpecifiedTypes(); } + $objectOrClassType = $scope->getType($node->getArgs()[0]->value); $classType = $scope->getType($node->getArgs()[1]->value); $allowStringType = isset($node->getArgs()[2]) ? $scope->getType($node->getArgs()[2]->value) : new ConstantBooleanType(false); $allowString = !$allowStringType->equals(new ConstantBooleanType(false)); @@ -47,7 +48,7 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n return $this->typeSpecifier->create( $node->getArgs()[0]->value, - $this->isAFunctionTypeSpecifyingHelper->determineType($classType, $allowString), + $this->isAFunctionTypeSpecifyingHelper->determineType($objectOrClassType, $classType, $allowString, true), $context, false, $scope, diff --git a/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php b/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php index 8a689e6053..ec83066481 100644 --- a/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php +++ b/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php @@ -6,28 +6,54 @@ use PHPStan\Type\Constant\ConstantStringType; use PHPStan\Type\Generic\GenericClassStringType; use PHPStan\Type\IntersectionType; +use PHPStan\Type\NeverType; use PHPStan\Type\ObjectType; use PHPStan\Type\ObjectWithoutClassType; use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; use PHPStan\Type\TypeTraverser; +use PHPStan\Type\TypeWithClassName; use PHPStan\Type\UnionType; final class IsAFunctionTypeSpecifyingHelper { public function determineType( + Type $objectOrClassType, Type $classType, bool $allowString, + bool $allowSameClass, ): Type { + $objectOrClassTypeClassNameType = TypeTraverser::map( + $objectOrClassType, + static function (Type $type, callable $traverse): Type { + if ($type instanceof UnionType || $type instanceof IntersectionType) { + return $traverse($type); + } + if ($type instanceof GenericClassStringType) { + return $traverse($type->getGenericType()); + } + if ($type instanceof TypeWithClassName) { + return $type; + } + return new ObjectType(''); + }, + ); + $objectOrClassTypeClassName = $objectOrClassTypeClassNameType instanceof TypeWithClassName + ? $objectOrClassTypeClassNameType->getClassName() + : null; + return TypeTraverser::map( $classType, - static function (Type $type, callable $traverse) use ($allowString): Type { + static function (Type $type, callable $traverse) use ($objectOrClassTypeClassName, $allowString, $allowSameClass): Type { if ($type instanceof UnionType || $type instanceof IntersectionType) { return $traverse($type); } if ($type instanceof ConstantStringType) { + if (!$allowSameClass && $type->getValue() === $objectOrClassTypeClassName) { + return new NeverType(); + } if ($allowString) { return TypeCombinator::union( new ObjectType($type->getValue()), @@ -38,14 +64,18 @@ static function (Type $type, callable $traverse) use ($allowString): Type { return new ObjectType($type->getValue()); } if ($type instanceof GenericClassStringType) { + $genericType = $type->getGenericType(); + if (!$allowSameClass && $genericType instanceof TypeWithClassName && $genericType->getClassName() === $objectOrClassTypeClassName) { + return new NeverType(); + } if ($allowString) { return TypeCombinator::union( - $type->getGenericType(), + $genericType, $type, ); } - return $type->getGenericType(); + return $genericType; } if ($allowString) { return TypeCombinator::union( diff --git a/src/Type/Php/IsSubclassOfFunctionTypeSpecifyingExtension.php b/src/Type/Php/IsSubclassOfFunctionTypeSpecifyingExtension.php index 15c773784e..2740401e4f 100644 --- a/src/Type/Php/IsSubclassOfFunctionTypeSpecifyingExtension.php +++ b/src/Type/Php/IsSubclassOfFunctionTypeSpecifyingExtension.php @@ -37,6 +37,7 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n if (count($node->getArgs()) < 2) { return new SpecifiedTypes(); } + $objectOrClassType = $scope->getType($node->getArgs()[0]->value); $classType = $scope->getType($node->getArgs()[1]->value); $allowStringType = isset($node->getArgs()[2]) ? $scope->getType($node->getArgs()[2]->value) : new ConstantBooleanType(true); $allowString = !$allowStringType->equals(new ConstantBooleanType(false)); @@ -47,7 +48,7 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n return $this->typeSpecifier->create( $node->getArgs()[0]->value, - $this->isAFunctionTypeSpecifyingHelper->determineType($classType, $allowString), + $this->isAFunctionTypeSpecifyingHelper->determineType($objectOrClassType, $classType, $allowString, false), $context, false, $scope, diff --git a/tests/PHPStan/Analyser/NodeScopeResolverTest.php b/tests/PHPStan/Analyser/NodeScopeResolverTest.php index 7adb46e867..2620bf7552 100644 --- a/tests/PHPStan/Analyser/NodeScopeResolverTest.php +++ b/tests/PHPStan/Analyser/NodeScopeResolverTest.php @@ -80,6 +80,7 @@ public function dataFileAsserts(): iterable } yield from $this->gatherAssertTypes(__DIR__ . '/data/is-numeric.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/is-a.php'); + yield from $this->gatherAssertTypes(__DIR__ . '/data/is-subclass-of.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-3142.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/array-shapes-keys-strings.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-1216.php'); @@ -754,6 +755,7 @@ public function dataFileAsserts(): iterable yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6505.php'); } + yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6305.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6699.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6715.php'); yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-6682.php'); diff --git a/tests/PHPStan/Analyser/data/bug-6305.php b/tests/PHPStan/Analyser/data/bug-6305.php new file mode 100644 index 0000000000..5cf5d353b4 --- /dev/null +++ b/tests/PHPStan/Analyser/data/bug-6305.php @@ -0,0 +1,19 @@ +', $foo); + \PHPStan\Testing\assertType('class-string', $foo); } }; @@ -26,3 +28,55 @@ function (string $foo, string $someString) { \PHPStan\Testing\assertType('class-string', $foo); } }; + +function (Bar $a, Bar $b, Bar $c, Bar $d) { + if (is_a($a, Bar::class)) { + \PHPStan\Testing\assertType('IsA\Bar', $a); + } + + if (is_a($b, Foo::class)) { + \PHPStan\Testing\assertType('IsA\Bar', $b); + } + + /** @var class-string $barClassString */ + $barClassString = 'Bar'; + if (is_a($c, $barClassString)) { + \PHPStan\Testing\assertType('IsA\Bar', $c); + } + + /** @var class-string $fooClassString */ + $fooClassString = 'Foo'; + if (is_a($d, $fooClassString)) { + \PHPStan\Testing\assertType('IsA\Bar', $d); + } +}; + +function (string $a, string $b, string $c, string $d) { + /** @var class-string $a */ + if (is_a($a, Bar::class, true)) { + \PHPStan\Testing\assertType('class-string', $a); + } + + /** @var class-string $b */ + if (is_a($b, Foo::class, true)) { + \PHPStan\Testing\assertType('class-string', $b); + } + + /** @var class-string $c */ + /** @var class-string $barClassString */ + $barClassString = 'Bar'; + if (is_a($c, $barClassString, true)) { + \PHPStan\Testing\assertType('class-string', $c); + } + + /** @var class-string $d */ + /** @var class-string $fooClassString */ + $fooClassString = 'Foo'; + if (is_a($d, $fooClassString, true)) { + \PHPStan\Testing\assertType('class-string', $d); + } +}; + +class Foo {} + +class Bar extends Foo {} diff --git a/tests/PHPStan/Analyser/data/is-subclass-of.php b/tests/PHPStan/Analyser/data/is-subclass-of.php new file mode 100644 index 0000000000..96ee15cd0e --- /dev/null +++ b/tests/PHPStan/Analyser/data/is-subclass-of.php @@ -0,0 +1,55 @@ + $barClassString */ + $barClassString = 'Bar'; + if (is_subclass_of($c, $barClassString)) { + \PHPStan\Testing\assertType('*NEVER*', $c); + } + + /** @var class-string $fooClassString */ + $fooClassString = 'Foo'; + if (is_subclass_of($d, $fooClassString)) { + \PHPStan\Testing\assertType('IsSubclassOf\Bar', $d); + } +}; + +function (string $a, string $b, string $c, string $d) { + /** @var class-string $a */ + if (is_subclass_of($a, Bar::class)) { + \PHPStan\Testing\assertType('*NEVER*', $a); + } + + /** @var class-string $b */ + if (is_subclass_of($b, Foo::class)) { + \PHPStan\Testing\assertType('class-string', $b); + } + + /** @var class-string $c */ + /** @var class-string $barClassString */ + $barClassString = 'Bar'; + if (is_subclass_of($c, $barClassString)) { + \PHPStan\Testing\assertType('*NEVER*', $c); + } + + /** @var class-string $d */ + /** @var class-string $fooClassString */ + $fooClassString = 'Foo'; + if (is_subclass_of($d, $fooClassString)) { + \PHPStan\Testing\assertType('class-string', $d); + } +}; + +class Foo {} + +class Bar extends Foo {} diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php index 02ef452f79..f057e1aa81 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php @@ -448,6 +448,22 @@ public function testBug3766(): void $this->analyse([__DIR__ . '/data/bug-3766.php'], []); } + public function testBug6305(): void + { + $this->checkAlwaysTrueCheckTypeFunctionCall = true; + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/bug-6305.php'], [ + [ + 'Call to function is_subclass_of() with Bug6305\B and \'Bug6305\\\A\' will always evaluate to true.', + 11, + ], + [ + 'Call to function is_subclass_of() with Bug6305\B and \'Bug6305\\\B\' will always evaluate to false.', + 14, + ], + ]); + } + public function testBug6698(): void { $this->checkAlwaysTrueCheckTypeFunctionCall = true; diff --git a/tests/PHPStan/Rules/Comparison/data/bug-6305.php b/tests/PHPStan/Rules/Comparison/data/bug-6305.php new file mode 100644 index 0000000000..c4d142a276 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-6305.php @@ -0,0 +1,15 @@ + Date: Wed, 2 Mar 2022 13:51:33 +0100 Subject: [PATCH 2/3] Remove incorrect generics support --- .../Php/IsAFunctionTypeSpecifyingHelper.php | 40 ++++++++----------- .../PHPStan/Analyser/data/is-subclass-of.php | 6 +-- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php b/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php index ec83066481..e131382caf 100644 --- a/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php +++ b/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php @@ -25,24 +25,7 @@ public function determineType( bool $allowSameClass, ): Type { - $objectOrClassTypeClassNameType = TypeTraverser::map( - $objectOrClassType, - static function (Type $type, callable $traverse): Type { - if ($type instanceof UnionType || $type instanceof IntersectionType) { - return $traverse($type); - } - if ($type instanceof GenericClassStringType) { - return $traverse($type->getGenericType()); - } - if ($type instanceof TypeWithClassName) { - return $type; - } - return new ObjectType(''); - }, - ); - $objectOrClassTypeClassName = $objectOrClassTypeClassNameType instanceof TypeWithClassName - ? $objectOrClassTypeClassNameType->getClassName() - : null; + $objectOrClassTypeClassName = $this->determineClassNameFromObjectOrClassType($objectOrClassType); return TypeTraverser::map( $classType, @@ -64,18 +47,14 @@ static function (Type $type, callable $traverse) use ($objectOrClassTypeClassNam return new ObjectType($type->getValue()); } if ($type instanceof GenericClassStringType) { - $genericType = $type->getGenericType(); - if (!$allowSameClass && $genericType instanceof TypeWithClassName && $genericType->getClassName() === $objectOrClassTypeClassName) { - return new NeverType(); - } if ($allowString) { return TypeCombinator::union( - $genericType, + $type->getGenericType(), $type, ); } - return $genericType; + return $type->getGenericType(); } if ($allowString) { return TypeCombinator::union( @@ -89,4 +68,17 @@ static function (Type $type, callable $traverse) use ($objectOrClassTypeClassNam ); } + private function determineClassNameFromObjectOrClassType(Type $type): ?string + { + if ($type instanceof TypeWithClassName) { + return $type->getClassName(); + } + + if ($type instanceof ConstantStringType) { + return $type->getValue(); + } + + return null; + } + } diff --git a/tests/PHPStan/Analyser/data/is-subclass-of.php b/tests/PHPStan/Analyser/data/is-subclass-of.php index 96ee15cd0e..1469236ea5 100644 --- a/tests/PHPStan/Analyser/data/is-subclass-of.php +++ b/tests/PHPStan/Analyser/data/is-subclass-of.php @@ -14,7 +14,7 @@ function (Bar $a, Bar $b, Bar $c, Bar $d) { /** @var class-string $barClassString */ $barClassString = 'Bar'; if (is_subclass_of($c, $barClassString)) { - \PHPStan\Testing\assertType('*NEVER*', $c); + \PHPStan\Testing\assertType('IsSubclassOf\Bar', $c); } /** @var class-string $fooClassString */ @@ -27,7 +27,7 @@ function (Bar $a, Bar $b, Bar $c, Bar $d) { function (string $a, string $b, string $c, string $d) { /** @var class-string $a */ if (is_subclass_of($a, Bar::class)) { - \PHPStan\Testing\assertType('*NEVER*', $a); + \PHPStan\Testing\assertType('class-string', $a); } /** @var class-string $b */ @@ -39,7 +39,7 @@ function (string $a, string $b, string $c, string $d) { /** @var class-string $barClassString */ $barClassString = 'Bar'; if (is_subclass_of($c, $barClassString)) { - \PHPStan\Testing\assertType('*NEVER*', $c); + \PHPStan\Testing\assertType('class-string', $c); } /** @var class-string $d */ From 70fd1fbd9185045594bb3a00422816ffa4cbcdbb Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Wed, 2 Mar 2022 14:00:44 +0100 Subject: [PATCH 3/3] Consider allowString --- src/Type/Php/IsAFunctionTypeSpecifyingHelper.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php b/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php index e131382caf..c608156f84 100644 --- a/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php +++ b/src/Type/Php/IsAFunctionTypeSpecifyingHelper.php @@ -25,7 +25,7 @@ public function determineType( bool $allowSameClass, ): Type { - $objectOrClassTypeClassName = $this->determineClassNameFromObjectOrClassType($objectOrClassType); + $objectOrClassTypeClassName = $this->determineClassNameFromObjectOrClassType($objectOrClassType, $allowString); return TypeTraverser::map( $classType, @@ -68,13 +68,13 @@ static function (Type $type, callable $traverse) use ($objectOrClassTypeClassNam ); } - private function determineClassNameFromObjectOrClassType(Type $type): ?string + private function determineClassNameFromObjectOrClassType(Type $type, bool $allowString): ?string { if ($type instanceof TypeWithClassName) { return $type->getClassName(); } - if ($type instanceof ConstantStringType) { + if ($allowString && $type instanceof ConstantStringType) { return $type->getValue(); }