From af425bb2b2891eae9d50726bae760807cc35b23a Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Fri, 4 Mar 2022 16:19:57 +0100 Subject: [PATCH 01/16] Add failing test cases --- .../data/impossible-method-call.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php index c213bfb8d5..1a2a7fbc12 100644 --- a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php +++ b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php @@ -92,6 +92,20 @@ public function doDolor(\stdClass $std1, \stdClass $std2) } } + if ($this->isSame(self::createStdClass('a'), self::createStdClass('a'))) { + + } + if ($this->isNotSame(self::createStdClass('b'), self::createStdClass('b'))) { + + } + $std3 = new \stdClass(); + if ($this->isSame($std3, self::createStdClass('c'))) { + + } + $std4 = new \stdClass(); + if ($this->isNotSame($std4, self::createStdClass('d'))) { + + } } public function nullableInt(): ?int @@ -99,4 +113,9 @@ public function nullableInt(): ?int } + public static function createStdClass(string $foo): \stdClass + { + return new \stdClass(); + } + } From 4656b39edd692757fcd80df3390df07668fa3ed1 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Fri, 4 Mar 2022 16:46:19 +0100 Subject: [PATCH 02/16] Do not further specify identical and equals types if both expression sides are CallLike nodes --- src/Analyser/TypeSpecifier.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index c094bbd0c4..56a58b0553 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -259,6 +259,10 @@ public function specifyTypesInCondition( ); } + if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { + return new SpecifiedTypes([], []); + } + if ($context->true()) { $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); $leftTypes = $this->create($expr->left, $type, $context, false, $scope); @@ -355,6 +359,10 @@ public function specifyTypesInCondition( $leftType = $scope->getType($expr->left); $rightType = $scope->getType($expr->right); + if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { + return new SpecifiedTypes([], []); + } + $leftBooleanType = $leftType->toBoolean(); if ($leftBooleanType instanceof ConstantBooleanType && $rightType instanceof BooleanType) { return $this->specifyTypesInCondition( From 7e425eb877dce86b0e3af73e8f1fce1946c71e2d Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Fri, 4 Mar 2022 17:11:59 +0100 Subject: [PATCH 03/16] Move CallLike conditionals to the top --- src/Analyser/TypeSpecifier.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 56a58b0553..b8795d43b3 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -167,6 +167,10 @@ public function specifyTypesInCondition( return $this->create($exprNode, new ObjectWithoutClassType(), $context, false, $scope); } } elseif ($expr instanceof Node\Expr\BinaryOp\Identical) { + if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { + return new SpecifiedTypes([], []); + } + $expressions = $this->findTypeExpressionsFromBinaryOperation($scope, $expr); if ($expressions !== null) { /** @var Expr $exprNode */ @@ -259,10 +263,6 @@ public function specifyTypesInCondition( ); } - if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { - return new SpecifiedTypes([], []); - } - if ($context->true()) { $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); $leftTypes = $this->create($expr->left, $type, $context, false, $scope); @@ -333,6 +333,10 @@ public function specifyTypesInCondition( $context, ); } elseif ($expr instanceof Node\Expr\BinaryOp\Equal) { + if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { + return new SpecifiedTypes([], []); + } + $expressions = $this->findTypeExpressionsFromBinaryOperation($scope, $expr); if ($expressions !== null) { /** @var Expr $exprNode */ @@ -359,10 +363,6 @@ public function specifyTypesInCondition( $leftType = $scope->getType($expr->left); $rightType = $scope->getType($expr->right); - if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { - return new SpecifiedTypes([], []); - } - $leftBooleanType = $leftType->toBoolean(); if ($leftBooleanType instanceof ConstantBooleanType && $rightType instanceof BooleanType) { return $this->specifyTypesInCondition( From 29b97459278d40d60c3e4a3bda1587f210be6dbe Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Fri, 4 Mar 2022 21:50:22 +0100 Subject: [PATCH 04/16] Use SpecifiedTypes without specifying the default args --- src/Analyser/TypeSpecifier.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index b8795d43b3..42fe574d70 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -168,7 +168,7 @@ public function specifyTypesInCondition( } } elseif ($expr instanceof Node\Expr\BinaryOp\Identical) { if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { - return new SpecifiedTypes([], []); + return new SpecifiedTypes(); } $expressions = $this->findTypeExpressionsFromBinaryOperation($scope, $expr); @@ -334,7 +334,7 @@ public function specifyTypesInCondition( ); } elseif ($expr instanceof Node\Expr\BinaryOp\Equal) { if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { - return new SpecifiedTypes([], []); + return new SpecifiedTypes(); } $expressions = $this->findTypeExpressionsFromBinaryOperation($scope, $expr); From abc79d51ef784e2d77b6a2604d98902df0bfb6f2 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Fri, 4 Mar 2022 22:18:45 +0100 Subject: [PATCH 05/16] Keep specifying types for CallLikes resolving to ConstantTypes --- src/Analyser/TypeSpecifier.php | 20 +++++++++---------- .../ImpossibleCheckTypeMethodCallRuleTest.php | 4 ++++ .../data/impossible-method-call.php | 14 +++++++++---- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 42fe574d70..6afb0d7d7e 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -167,10 +167,6 @@ public function specifyTypesInCondition( return $this->create($exprNode, new ObjectWithoutClassType(), $context, false, $scope); } } elseif ($expr instanceof Node\Expr\BinaryOp\Identical) { - if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { - return new SpecifiedTypes(); - } - $expressions = $this->findTypeExpressionsFromBinaryOperation($scope, $expr); if ($expressions !== null) { /** @var Expr $exprNode */ @@ -263,6 +259,12 @@ public function specifyTypesInCondition( ); } + $exprLeftType = $scope->getType($expr->left); + $exprRightType = $scope->getType($expr->right); + if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike && !$exprLeftType instanceof ConstantType && !$exprRightType instanceof ConstantType) { + return new SpecifiedTypes(); + } + if ($context->true()) { $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); $leftTypes = $this->create($expr->left, $type, $context, false, $scope); @@ -279,9 +281,6 @@ public function specifyTypesInCondition( return $leftTypes->unionWith($rightTypes); } - $exprLeftType = $scope->getType($expr->left); - $exprRightType = $scope->getType($expr->right); - $types = null; if ( @@ -333,10 +332,6 @@ public function specifyTypesInCondition( $context, ); } elseif ($expr instanceof Node\Expr\BinaryOp\Equal) { - if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike) { - return new SpecifiedTypes(); - } - $expressions = $this->findTypeExpressionsFromBinaryOperation($scope, $expr); if ($expressions !== null) { /** @var Expr $exprNode */ @@ -362,6 +357,9 @@ public function specifyTypesInCondition( $leftType = $scope->getType($expr->left); $rightType = $scope->getType($expr->right); + if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike && !$leftType instanceof ConstantType && !$rightType instanceof ConstantType) { + return new SpecifiedTypes(); + } $leftBooleanType = $leftType->toBoolean(); if ($leftBooleanType instanceof ConstantBooleanType && $rightType instanceof BooleanType) { diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php index 31d32db056..67aacce341 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php @@ -80,6 +80,10 @@ public function testRule(): void 'Call to method ImpossibleMethodCall\Foo::isSame() with *NEVER* and stdClass will always evaluate to false.', 84, ], + [ + 'Call to method ImpossibleMethodCall\Foo::isSame() with \'foo\' and \'foo\' will always evaluate to true.', + 101, + ], ]); } diff --git a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php index 1a2a7fbc12..e6b60321c1 100644 --- a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php +++ b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php @@ -98,12 +98,10 @@ public function doDolor(\stdClass $std1, \stdClass $std2) if ($this->isNotSame(self::createStdClass('b'), self::createStdClass('b'))) { } - $std3 = new \stdClass(); - if ($this->isSame($std3, self::createStdClass('c'))) { + if ($this->isSame(self::returnFoo(), self::returnFoo())) { } - $std4 = new \stdClass(); - if ($this->isNotSame($std4, self::createStdClass('d'))) { + if ($this->isNotSame(self::returnFoo(), self::returnFoo())) { } } @@ -118,4 +116,12 @@ public static function createStdClass(string $foo): \stdClass return new \stdClass(); } + /** + * @return 'foo' + */ + public static function returnFoo(): string + { + return 'foo'; + } + } From 86a7c671070ebfe2a6324d5b060a2aecad97a5bd Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sat, 5 Mar 2022 07:53:07 +0100 Subject: [PATCH 06/16] Also make the isNotSame with CallLikes returning constants fail --- .../Comparison/ImpossibleCheckTypeMethodCallRuleTest.php | 4 ++++ .../Rules/Comparison/data/impossible-method-call.php | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php index 67aacce341..6537eb82fc 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php @@ -84,6 +84,10 @@ public function testRule(): void 'Call to method ImpossibleMethodCall\Foo::isSame() with \'foo\' and \'foo\' will always evaluate to true.', 101, ], + [ + 'Call to method ImpossibleMethodCall\Foo::isNotSame() with \'foo\' and \'foo\' will always evaluate to false.', + 104, + ], ]); } diff --git a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php index e6b60321c1..eebd802df7 100644 --- a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php +++ b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php @@ -98,10 +98,10 @@ public function doDolor(\stdClass $std1, \stdClass $std2) if ($this->isNotSame(self::createStdClass('b'), self::createStdClass('b'))) { } - if ($this->isSame(self::returnFoo(), self::returnFoo())) { + if ($this->isSame(self::returnFoo('a'), self::returnFoo('a'))) { } - if ($this->isNotSame(self::returnFoo(), self::returnFoo())) { + if ($this->isNotSame(self::returnFoo('b'), self::returnFoo('b'))) { } } @@ -119,7 +119,7 @@ public static function createStdClass(string $foo): \stdClass /** * @return 'foo' */ - public static function returnFoo(): string + public static function returnFoo(string $foo): string { return 'foo'; } From 83ed44aeee21993dbefeaf9724a435ee90d6a224 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Sat, 5 Mar 2022 11:45:17 +0100 Subject: [PATCH 07/16] Another failing test that suggests we can do this only for Variable now --- .../Rules/Comparison/data/impossible-method-call.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php index eebd802df7..91e5d23018 100644 --- a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php +++ b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php @@ -103,6 +103,12 @@ public function doDolor(\stdClass $std1, \stdClass $std2) } if ($this->isNotSame(self::returnFoo('b'), self::returnFoo('b'))) { + } + if ($this->isSame(self::createStdClass('a')->foo, self::createStdClass('a')->foo)) { + + } + if ($this->isNotSame(self::createStdClass('b')->foo, self::createStdClass('b')->foo)) { + } } From 8d86993737bfb7787acb5428d902e83215e1b7ca Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sat, 5 Mar 2022 14:48:01 +0100 Subject: [PATCH 08/16] Only specify types for Variables This leaves us only the same bugs that were already there. --- src/Analyser/TypeSpecifier.php | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 6afb0d7d7e..e97490ae0c 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -259,12 +259,6 @@ public function specifyTypesInCondition( ); } - $exprLeftType = $scope->getType($expr->left); - $exprRightType = $scope->getType($expr->right); - if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike && !$exprLeftType instanceof ConstantType && !$exprRightType instanceof ConstantType) { - return new SpecifiedTypes(); - } - if ($context->true()) { $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); $leftTypes = $this->create($expr->left, $type, $context, false, $scope); @@ -281,6 +275,9 @@ public function specifyTypesInCondition( return $leftTypes->unionWith($rightTypes); } + $exprLeftType = $scope->getType($expr->left); + $exprRightType = $scope->getType($expr->right); + $types = null; if ( @@ -321,8 +318,10 @@ public function specifyTypesInCondition( return $types; } - return $this->create($expr->left, $exprLeftType, $context, false, $scope)->normalize($scope) - ->intersectWith($this->create($expr->right, $exprRightType, $context, false, $scope)->normalize($scope)); + if ($expr->left instanceof Expr\Variable && $expr->right instanceof Expr\Variable) { + return $this->create($expr->left, $exprLeftType, $context, false, $scope)->normalize($scope) + ->intersectWith($this->create($expr->right, $exprRightType, $context, false, $scope)->normalize($scope)); + } } } elseif ($expr instanceof Node\Expr\BinaryOp\NotIdentical) { @@ -357,9 +356,6 @@ public function specifyTypesInCondition( $leftType = $scope->getType($expr->left); $rightType = $scope->getType($expr->right); - if ($expr->left instanceof Expr\CallLike && $expr->right instanceof Expr\CallLike && !$leftType instanceof ConstantType && !$rightType instanceof ConstantType) { - return new SpecifiedTypes(); - } $leftBooleanType = $leftType->toBoolean(); if ($leftBooleanType instanceof ConstantBooleanType && $rightType instanceof BooleanType) { From 95e510bc88deb0e8ad14fe87c8b2afcd27e262b7 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sat, 5 Mar 2022 15:47:51 +0100 Subject: [PATCH 09/16] Move things around in Identical specification like a madman --- src/Analyser/TypeSpecifier.php | 125 ++++++++++--------- tests/PHPStan/Analyser/TypeSpecifierTest.php | 1 - 2 files changed, 66 insertions(+), 60 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index e97490ae0c..e6e625023d 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -240,7 +240,9 @@ public function specifyTypesInCondition( } } + $leftType = $scope->getType($expr->left); $rightType = $scope->getType($expr->right); + if ( $expr->left instanceof ClassConstFetch && $expr->left->class instanceof Expr && @@ -259,71 +261,71 @@ public function specifyTypesInCondition( ); } - if ($context->true()) { - $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); - $leftTypes = $this->create($expr->left, $type, $context, false, $scope); - $rightTypes = $this->create($expr->right, $type, $context, false, $scope); - return $leftTypes->unionWith($rightTypes); + $types = null; - } elseif ($context->false()) { - $identicalType = $scope->getType($expr); - if ($identicalType instanceof ConstantBooleanType) { - $never = new NeverType(); - $contextForTypes = $identicalType->getValue() ? $context->negate() : $context; - $leftTypes = $this->create($expr->left, $never, $contextForTypes, false, $scope); - $rightTypes = $this->create($expr->right, $never, $contextForTypes, false, $scope); - return $leftTypes->unionWith($rightTypes); + if ( + ( + $leftType instanceof ConstantType + && !$expr->right instanceof Node\Scalar + ) || $leftType instanceof EnumCaseObjectType + ) { + $types = $this->create( + $expr->right, + $leftType, + $context, + false, + $scope, + ); + } + + if ( + ( + $rightType instanceof ConstantType + && !$expr->left instanceof Node\Scalar + ) || $rightType instanceof EnumCaseObjectType + ) { + $leftTypes = $this->create( + $expr->left, + $rightType, + $context, + false, + $scope, + ); + if ($types !== null) { + $types = $types->unionWith($leftTypes); + } else { + $types = $leftTypes; } + } - $exprLeftType = $scope->getType($expr->left); - $exprRightType = $scope->getType($expr->right); + if ($types !== null) { + return $types; + } - $types = null; + $identicalType = $scope->getType($expr); + if ($identicalType instanceof ConstantBooleanType && $context->false()) { + $never = new NeverType(); + $contextForTypes = $identicalType->getValue() ? $context->negate() : $context; + $leftTypes = $this->create($expr->left, $never, $contextForTypes, false, $scope); + $rightTypes = $this->create($expr->right, $never, $contextForTypes, false, $scope); + return $leftTypes->unionWith($rightTypes); + } - if ( - ( - $exprLeftType instanceof ConstantType - && !$expr->right instanceof Node\Scalar - ) || $exprLeftType instanceof EnumCaseObjectType - ) { - $types = $this->create( - $expr->right, - $exprLeftType, - $context, - false, - $scope, - ); - } - if ( - ( - $exprRightType instanceof ConstantType - && !$expr->left instanceof Node\Scalar - ) || $exprRightType instanceof EnumCaseObjectType - ) { - $leftType = $this->create( - $expr->left, - $exprRightType, - $context, - false, - $scope, - ); - if ($types !== null) { - $types = $types->unionWith($leftType); - } else { - $types = $leftType; - } - } + if ( + $expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable + || $expr->left instanceof Node\Scalar || $expr->right instanceof Node\Scalar + ) { + if ($context->true()) { + $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); + $leftTypes = $this->create($expr->left, $type, $context, false, $scope); + $rightTypes = $this->create($expr->right, $type, $context, false, $scope); + return $leftTypes->unionWith($rightTypes); - if ($types !== null) { - return $types; } - if ($expr->left instanceof Expr\Variable && $expr->right instanceof Expr\Variable) { - return $this->create($expr->left, $exprLeftType, $context, false, $scope)->normalize($scope) - ->intersectWith($this->create($expr->right, $exprRightType, $context, false, $scope)->normalize($scope)); - } + return $this->create($expr->left, $leftType, $context, false, $scope)->normalize($scope) + ->intersectWith($this->create($expr->right, $rightType, $context, false, $scope)->normalize($scope)); } - } elseif ($expr instanceof Node\Expr\BinaryOp\NotIdentical) { return $this->specifyTypesInCondition( $scope, @@ -443,9 +445,14 @@ public function specifyTypesInCondition( $leftTypes = $this->create($expr->left, $leftType, $context, false, $scope); $rightTypes = $this->create($expr->right, $rightType, $context, false, $scope); - return $context->true() - ? $leftTypes->unionWith($rightTypes) - : $leftTypes->normalize($scope)->intersectWith($rightTypes->normalize($scope)); + if ( + $expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable + || $expr->left instanceof Node\Scalar || $expr->right instanceof Node\Scalar + ) { + return $context->true() + ? $leftTypes->unionWith($rightTypes) + : $leftTypes->normalize($scope)->intersectWith($rightTypes->normalize($scope)); + } } elseif ($expr instanceof Node\Expr\BinaryOp\NotEqual) { return $this->specifyTypesInCondition( $scope, diff --git a/tests/PHPStan/Analyser/TypeSpecifierTest.php b/tests/PHPStan/Analyser/TypeSpecifierTest.php index 9ea86aedaf..43af6df355 100644 --- a/tests/PHPStan/Analyser/TypeSpecifierTest.php +++ b/tests/PHPStan/Analyser/TypeSpecifierTest.php @@ -554,7 +554,6 @@ public function dataCondition(): array ), [ '$foo' => '123', - 123 => '123', ], ['$foo' => '~123'], ], From c2aae08b81abccaed5271abd16f578c2847e5646 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sat, 5 Mar 2022 22:28:14 +0100 Subject: [PATCH 10/16] Simplify Identical specification, add more tests --- src/Analyser/TypeSpecifier.php | 13 +++------- .../ImpossibleCheckTypeMethodCallRuleTest.php | 24 +++++++++++++++++++ .../data/impossible-method-call.php | 20 ++++++++++++++++ 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index e6e625023d..9f81c6329e 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -303,7 +303,7 @@ public function specifyTypesInCondition( } $identicalType = $scope->getType($expr); - if ($identicalType instanceof ConstantBooleanType && $context->false()) { + if ($identicalType instanceof ConstantBooleanType && !$context->null()) { $never = new NeverType(); $contextForTypes = $identicalType->getValue() ? $context->negate() : $context; $leftTypes = $this->create($expr->left, $never, $contextForTypes, false, $scope); @@ -311,16 +311,12 @@ public function specifyTypesInCondition( return $leftTypes->unionWith($rightTypes); } - if ( - $expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable - || $expr->left instanceof Node\Scalar || $expr->right instanceof Node\Scalar - ) { + if ($expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable) { if ($context->true()) { $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); $leftTypes = $this->create($expr->left, $type, $context, false, $scope); $rightTypes = $this->create($expr->right, $type, $context, false, $scope); return $leftTypes->unionWith($rightTypes); - } return $this->create($expr->left, $leftType, $context, false, $scope)->normalize($scope) @@ -445,10 +441,7 @@ public function specifyTypesInCondition( $leftTypes = $this->create($expr->left, $leftType, $context, false, $scope); $rightTypes = $this->create($expr->right, $rightType, $context, false, $scope); - if ( - $expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable - || $expr->left instanceof Node\Scalar || $expr->right instanceof Node\Scalar - ) { + if ($expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable) { return $context->true() ? $leftTypes->unionWith($rightTypes) : $leftTypes->normalize($scope)->intersectWith($rightTypes->normalize($scope)); diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php index 6537eb82fc..32bcb809d1 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php @@ -88,6 +88,30 @@ public function testRule(): void 'Call to method ImpossibleMethodCall\Foo::isNotSame() with \'foo\' and \'foo\' will always evaluate to false.', 104, ], + [ + 'Call to method ImpossibleMethodCall\Foo::isSame() with array{} and array{} will always evaluate to true.', + 113, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isNotSame() with array{} and array{} will always evaluate to false.', + 116, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isSame() with array{1, 3} and array{1, 3} will always evaluate to true.', + 119, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isNotSame() with array{1, 3} and array{1, 3} will always evaluate to false.', + 122, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isSame() with 1 and stdClass will always evaluate to false.', + 126, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isNotSame() with 1 and stdClass will always evaluate to true.', + 130, + ], ]); } diff --git a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php index 91e5d23018..3e1ba09f58 100644 --- a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php +++ b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php @@ -109,6 +109,26 @@ public function doDolor(\stdClass $std1, \stdClass $std2) } if ($this->isNotSame(self::createStdClass('b')->foo, self::createStdClass('b')->foo)) { + } + if ($this->isSame([], [])) { + + } + if ($this->isNotSame([], [])) { + + } + if ($this->isSame([1, 3], [1, 3])) { + + } + if ($this->isNotSame([1, 3], [1, 3])) { + + } + $std3 = new \stdClass(); + if ($this->isSame(1, $std3)) { + + } + $std4 = new \stdClass(); + if ($this->isNotSame(1, $std4)) { + } } From 94d9b6c2b6a2069d3befbbb9987c4d56f0f1592f Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sat, 5 Mar 2022 22:32:13 +0100 Subject: [PATCH 11/16] Revert unnecessary changes to keep the diff readable --- src/Analyser/TypeSpecifier.php | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 9f81c6329e..7025ecee73 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -240,9 +240,7 @@ public function specifyTypesInCondition( } } - $leftType = $scope->getType($expr->left); $rightType = $scope->getType($expr->right); - if ( $expr->left instanceof ClassConstFetch && $expr->left->class instanceof Expr && @@ -261,40 +259,42 @@ public function specifyTypesInCondition( ); } + $exprLeftType = $scope->getType($expr->left); + $exprRightType = $scope->getType($expr->right); + $types = null; if ( ( - $leftType instanceof ConstantType + $exprLeftType instanceof ConstantType && !$expr->right instanceof Node\Scalar - ) || $leftType instanceof EnumCaseObjectType + ) || $exprLeftType instanceof EnumCaseObjectType ) { $types = $this->create( $expr->right, - $leftType, + $exprLeftType, $context, false, $scope, ); } - if ( ( - $rightType instanceof ConstantType + $exprRightType instanceof ConstantType && !$expr->left instanceof Node\Scalar - ) || $rightType instanceof EnumCaseObjectType + ) || $exprRightType instanceof EnumCaseObjectType ) { - $leftTypes = $this->create( + $leftType = $this->create( $expr->left, - $rightType, + $exprRightType, $context, false, $scope, ); if ($types !== null) { - $types = $types->unionWith($leftTypes); + $types = $types->unionWith($leftType); } else { - $types = $leftTypes; + $types = $leftType; } } @@ -319,9 +319,10 @@ public function specifyTypesInCondition( return $leftTypes->unionWith($rightTypes); } - return $this->create($expr->left, $leftType, $context, false, $scope)->normalize($scope) - ->intersectWith($this->create($expr->right, $rightType, $context, false, $scope)->normalize($scope)); + return $this->create($expr->left, $exprLeftType, $context, false, $scope)->normalize($scope) + ->intersectWith($this->create($expr->right, $exprRightType, $context, false, $scope)->normalize($scope)); } + } elseif ($expr instanceof Node\Expr\BinaryOp\NotIdentical) { return $this->specifyTypesInCondition( $scope, From b712e0bc5e40200cb90e4463d0a6bc7c51c88744 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sat, 5 Mar 2022 23:51:36 +0100 Subject: [PATCH 12/16] Move ConstantBooleanType check up, add more tests --- src/Analyser/TypeSpecifier.php | 18 +++++++++--------- .../ImpossibleCheckTypeMethodCallRuleTest.php | 16 ++++++++++++++++ .../Comparison/data/impossible-method-call.php | 12 ++++++++++++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 7025ecee73..d44894de15 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -262,6 +262,15 @@ public function specifyTypesInCondition( $exprLeftType = $scope->getType($expr->left); $exprRightType = $scope->getType($expr->right); + $identicalType = $scope->getType($expr); + if ($identicalType instanceof ConstantBooleanType && !$context->null()) { + $never = new NeverType(); + $contextForTypes = $identicalType->getValue() ? $context->negate() : $context; + $leftTypes = $this->create($expr->left, $never, $contextForTypes, false, $scope); + $rightTypes = $this->create($expr->right, $never, $contextForTypes, false, $scope); + return $leftTypes->unionWith($rightTypes); + } + $types = null; if ( @@ -302,15 +311,6 @@ public function specifyTypesInCondition( return $types; } - $identicalType = $scope->getType($expr); - if ($identicalType instanceof ConstantBooleanType && !$context->null()) { - $never = new NeverType(); - $contextForTypes = $identicalType->getValue() ? $context->negate() : $context; - $leftTypes = $this->create($expr->left, $never, $contextForTypes, false, $scope); - $rightTypes = $this->create($expr->right, $never, $contextForTypes, false, $scope); - return $leftTypes->unionWith($rightTypes); - } - if ($expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable) { if ($context->true()) { $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php index 32bcb809d1..86128a9bd5 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php @@ -112,6 +112,22 @@ public function testRule(): void 'Call to method ImpossibleMethodCall\Foo::isNotSame() with 1 and stdClass will always evaluate to true.', 130, ], + [ + 'Call to method ImpossibleMethodCall\Foo::isSame() with \'1\' and stdClass will always evaluate to false.', + 133, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isNotSame() with \'1\' and stdClass will always evaluate to true.', + 136, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isSame() with array{\'a\', \'b\'} and array{1, 2} will always evaluate to false.', + 139, + ], + [ + 'Call to method ImpossibleMethodCall\Foo::isNotSame() with array{\'a\', \'b\'} and array{1, 2} will always evaluate to true.', + 142, + ], ]); } diff --git a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php index 3e1ba09f58..14564e2912 100644 --- a/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php +++ b/tests/PHPStan/Rules/Comparison/data/impossible-method-call.php @@ -129,6 +129,18 @@ public function doDolor(\stdClass $std1, \stdClass $std2) $std4 = new \stdClass(); if ($this->isNotSame(1, $std4)) { + } + if ($this->isSame('1', new \stdClass())) { + + } + if ($this->isNotSame('1', new \stdClass())) { + + } + if ($this->isSame(['a', 'b'], [1, 2])) { + + } + if ($this->isNotSame(['a', 'b'], [1, 2])) { + } } From d4800437973fc268a80a0b9e6f06e54197bc1728 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sun, 6 Mar 2022 13:51:42 +0100 Subject: [PATCH 13/16] Add NonexistantOffsetInArrayDimFetchRule test case that was discovered via composer/composer --- ...nexistentOffsetInArrayDimFetchRuleTest.php | 4 ++++ .../Rules/Arrays/data/nonexistent-offset.php | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php b/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php index ccaacd8b11..abb7faec3c 100644 --- a/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php +++ b/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php @@ -163,6 +163,10 @@ public function testRule(): void 'Cannot access offset \'foo\' on array|int.', 443, ], + [ + 'Offset \'feature_pretty…\' does not exist on array{version: string, commit: string|null, pretty_version: string|null, feature_version: non-empty-string, feature_pretty_version?: string|null}.', + 504, + ], ]); } diff --git a/tests/PHPStan/Rules/Arrays/data/nonexistent-offset.php b/tests/PHPStan/Rules/Arrays/data/nonexistent-offset.php index d74ee08af5..065d636722 100644 --- a/tests/PHPStan/Rules/Arrays/data/nonexistent-offset.php +++ b/tests/PHPStan/Rules/Arrays/data/nonexistent-offset.php @@ -485,3 +485,26 @@ function test($array): void { } } + +/** + * @phpstan-type Version array{version: string, commit: string|null, pretty_version: string|null, feature_version?: string|null, feature_pretty_version?: string|null} + */ +class VersionGuesser +{ + /** + * @param array $versionData + * + * @phpstan-param Version $versionData + * + * @return array + * @phpstan-return Version + */ + private function postprocess(array $versionData): array + { + if (!empty($versionData['feature_version']) && $versionData['feature_version'] === $versionData['version'] && $versionData['feature_pretty_version'] === $versionData['pretty_version']) { + unset($versionData['feature_version'], $versionData['feature_pretty_version']); + } + + return $versionData; + } +} From 8c7f3b5e0f8ae208e8f12c9a1738cd46d142df8f Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sun, 6 Mar 2022 14:16:00 +0100 Subject: [PATCH 14/16] Allow further specification for ArrayDimFetch with Variable --- src/Analyser/TypeSpecifier.php | 22 ++++++++++++++----- ...nexistentOffsetInArrayDimFetchRuleTest.php | 2 +- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index d44894de15..2fd395f70f 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -311,7 +311,19 @@ public function specifyTypesInCondition( return $types; } - if ($expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable) { + $furtherSpecificationPossible = static function (Expr $expr): bool { + if ($expr instanceof Expr\Variable) { + return true; + } + + if ($expr instanceof ArrayDimFetch && $expr->var instanceof Expr\Variable) { + return true; + } + + return false; + }; + + if ($furtherSpecificationPossible($expr->left) || $furtherSpecificationPossible($expr->right)) { if ($context->true()) { $type = TypeCombinator::intersect($scope->getType($expr->right), $scope->getType($expr->left)); $leftTypes = $this->create($expr->left, $type, $context, false, $scope); @@ -442,11 +454,9 @@ public function specifyTypesInCondition( $leftTypes = $this->create($expr->left, $leftType, $context, false, $scope); $rightTypes = $this->create($expr->right, $rightType, $context, false, $scope); - if ($expr->left instanceof Expr\Variable || $expr->right instanceof Expr\Variable) { - return $context->true() - ? $leftTypes->unionWith($rightTypes) - : $leftTypes->normalize($scope)->intersectWith($rightTypes->normalize($scope)); - } + return $context->true() + ? $leftTypes->unionWith($rightTypes) + : $leftTypes->normalize($scope)->intersectWith($rightTypes->normalize($scope)); } elseif ($expr instanceof Node\Expr\BinaryOp\NotEqual) { return $this->specifyTypesInCondition( $scope, diff --git a/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php b/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php index abb7faec3c..5621266e40 100644 --- a/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php +++ b/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php @@ -164,7 +164,7 @@ public function testRule(): void 443, ], [ - 'Offset \'feature_pretty…\' does not exist on array{version: string, commit: string|null, pretty_version: string|null, feature_version: non-empty-string, feature_pretty_version?: string|null}.', + 'Offset \'feature_pretty…\' does not exist on array{version: non-empty-string, commit: string|null, pretty_version: string|null, feature_version: non-empty-string, feature_pretty_version?: string|null}.', 504, ], ]); From 89378192809507e2de16070a357c6a3865446c9d Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sun, 6 Mar 2022 16:20:20 +0100 Subject: [PATCH 15/16] Adapt $furtherSpecificationPossible to work recursively --- src/Analyser/TypeSpecifier.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index 2fd395f70f..ab44f6f12d 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -311,13 +311,17 @@ public function specifyTypesInCondition( return $types; } - $furtherSpecificationPossible = static function (Expr $expr): bool { + $furtherSpecificationPossible = static function (Expr $expr) use (&$furtherSpecificationPossible): bool { if ($expr instanceof Expr\Variable) { return true; } - if ($expr instanceof ArrayDimFetch && $expr->var instanceof Expr\Variable) { - return true; + if ($expr instanceof Expr\Cast) { + return $furtherSpecificationPossible($expr->expr); + } + + if ($expr instanceof ArrayDimFetch) { + return $furtherSpecificationPossible($expr->var); } return false; From 993155cb97374446381e7e2528a45324d0d2d59c Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Sun, 6 Mar 2022 20:37:07 +0100 Subject: [PATCH 16/16] Remove unnecessary Cast support, add NodeScopeResolver test for ArrayDimFetch / array offset --- src/Analyser/TypeSpecifier.php | 4 ---- tests/PHPStan/Analyser/data/equal.php | 10 ++++++++++ tests/PHPStan/Analyser/data/identical.php | 10 ++++++++++ 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Analyser/TypeSpecifier.php b/src/Analyser/TypeSpecifier.php index ab44f6f12d..8f786167e6 100644 --- a/src/Analyser/TypeSpecifier.php +++ b/src/Analyser/TypeSpecifier.php @@ -316,10 +316,6 @@ public function specifyTypesInCondition( return true; } - if ($expr instanceof Expr\Cast) { - return $furtherSpecificationPossible($expr->expr); - } - if ($expr instanceof ArrayDimFetch) { return $furtherSpecificationPossible($expr->var); } diff --git a/tests/PHPStan/Analyser/data/equal.php b/tests/PHPStan/Analyser/data/equal.php index 113d2868b2..6fb7929cc1 100644 --- a/tests/PHPStan/Analyser/data/equal.php +++ b/tests/PHPStan/Analyser/data/equal.php @@ -99,4 +99,14 @@ public function stdClass(\stdClass $a, \stdClass $b): void assertType('stdClass', $b); } + /** + * @param array{a: string, b: array{c: string|null}} $a + */ + public function arrayOffset(array $a): void + { + if (strlen($a['a']) > 0 && $a['a'] === $a['b']['c']) { + assertType('array{a: non-empty-string, b: array{c: non-empty-string}}', $a); + } + } + } diff --git a/tests/PHPStan/Analyser/data/identical.php b/tests/PHPStan/Analyser/data/identical.php index 26602e2918..f46c675214 100644 --- a/tests/PHPStan/Analyser/data/identical.php +++ b/tests/PHPStan/Analyser/data/identical.php @@ -41,4 +41,14 @@ public function foo(\stdClass $a, \stdClass $b): void assertType('stdClass', $b); } + /** + * @param array{a: string, b: array{c: string|null}} $a + */ + public function arrayOffset(array $a): void + { + if (strlen($a['a']) > 0 && $a['a'] === $a['b']['c']) { + assertType('array{a: non-empty-string, b: array{c: non-empty-string}}', $a); + } + } + }