Simplify and improve ImpossibleInstanceOfRule

phpstan · Sep 9, 2017 · 0c4e802 · 0c4e802
1 parent 480e9c8
commit 0c4e802
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 42 deletions.
diff --git a/conf/config.level4.neon b/conf/config.level4.neon
@@ -9,6 +9,8 @@ services:
 
 	-
 		class: PHPStan\Rules\Classes\ImpossibleInstanceOfRule
+		arguments:
+			checkAlwaysTrueInstanceof: %checkAlwaysTrueInstanceof%
 		tags:
 			- phpstan.rules.rule
 

diff --git a/conf/config.neon b/conf/config.neon
@@ -5,6 +5,7 @@ parameters:
 	autoload_files: []
 	fileExtensions:
 		- php
+	checkAlwaysTrueInstanceof: false
 	checkFunctionArgumentTypes: false
 	checkArgumentsPassedByReference: false
 	checkMaybeUndefinedVariables: false

diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php
@@ -145,6 +145,7 @@ public function processNodes(
 		Scope $closureBindScope = null
 	)
 	{
+		/** @var \PhpParser\Node|string $node */
 		foreach ($nodes as $i => $node) {
 			if (!($node instanceof \PhpParser\Node)) {
 				continue;
@@ -189,8 +190,7 @@ public function processNodes(
 			} elseif ($node instanceof Node\Stmt\Declare_) {
 				foreach ($node->declares as $declare) {
 					if (
-						$declare instanceof Node\Stmt\DeclareDeclare
-						&& $declare->key === 'strict_types'
+						$declare->key === 'strict_types'
 						&& $declare->value instanceof Node\Scalar\LNumber
 						&& $declare->value->value === 1
 					) {

diff --git a/src/Analyser/Scope.php b/src/Analyser/Scope.php
@@ -548,7 +548,7 @@ function (Expr\ArrayItem $item): Type {
 				if ($constantClass === 'self') {
 					$constantClass = $this->getClassReflection()->getName();
 				}
-			} elseif ($node->class instanceof Expr) {
+			} else {
 				$constantClassType = $this->getType($node->class);
 				if ($constantClassType->getClass() !== null) {
 					$constantClass = $constantClassType->getClass();
@@ -1090,7 +1090,7 @@ public function enterAnonymousFunction(
 
 	private function isParameterValueNullable(Node\Param $parameter): bool
 	{
-		if ($parameter->default instanceof ConstFetch && $parameter->default->name instanceof Name) {
+		if ($parameter->default instanceof ConstFetch) {
 			return strtolower((string) $parameter->default->name) === 'null';
 		}
 

diff --git a/src/Reflection/Php/PhpParameterFromParserNodeReflection.php b/src/Reflection/Php/PhpParameterFromParserNodeReflection.php
@@ -4,7 +4,6 @@
 
 use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\ConstFetch;
-use PhpParser\Node\Name;
 use PHPStan\Type\Type;
 use PHPStan\Type\TypehintHelper;
 
@@ -71,7 +70,6 @@ public function getType(): Type
 			if ($phpDocType !== null && $this->defaultValue !== null) {
 				if (
 					$this->defaultValue instanceof ConstFetch
-					&& $this->defaultValue->name instanceof Name
 					&& strtolower((string) $this->defaultValue->name) === 'null'
 				) {
 					$phpDocType = \PHPStan\Type\TypeCombinator::addNull($phpDocType);

diff --git a/src/Rules/Classes/ImpossibleInstanceOfRule.php b/src/Rules/Classes/ImpossibleInstanceOfRule.php
@@ -4,20 +4,17 @@
 
 use PhpParser\Node;
 use PHPStan\Analyser\Scope;
-use PHPStan\Broker\Broker;
 use PHPStan\Type\ObjectType;
 
 class ImpossibleInstanceOfRule implements \PHPStan\Rules\Rule
 {
 
-	/**
-	 * @var \PHPStan\Broker\Broker
-	 */
-	private $broker;
+	/** @var bool */
+	private $checkAlwaysTrueInstanceof;
 
-	public function __construct(Broker $broker)
+	public function __construct(bool $checkAlwaysTrueInstanceof)
 	{
-		$this->broker = $broker;
+		$this->checkAlwaysTrueInstanceof = $checkAlwaysTrueInstanceof;
 	}
 
 	public function getNodeType(): string
@@ -39,43 +36,21 @@ public function processNode(Node $node, Scope $scope): array
 			$type = $scope->getType($node->class);
 		}
 
-		if ($type->getClass() === null) {
-			return [];
-		}
-
 		$expressionType = $scope->getType($node->expr);
-		if ($expressionType->getClass() === null) {
-			return [];
-		}
-
-		if (!$this->broker->hasClass($expressionType->getClass())) {
-			return [];
-		}
-
-		$expressionClassReflection = $this->broker->getClass($expressionType->getClass());
-		if (!$this->broker->hasClass($type->getClass())) {
-			return [];
-		}
+		$isSuperset = $type->isSupersetOf($expressionType);
 
-		if ($expressionClassReflection->isSubclassOf($type->getClass())) {
+		if ($isSuperset->no()) {
 			return [
 				sprintf(
-					'Instanceof between %s and %s will always evaluate to true.',
+					'Instanceof between %s and %s will always evaluate to false.',
 					$expressionType->describe(),
 					$type->describe()
 				),
 			];
-		}
-
-		$classReflection = $this->broker->getClass($type->getClass());
-		if ($classReflection->isInterface() || $expressionClassReflection->isInterface()) {
-			return [];
-		}
-
-		if (!$expressionType->accepts($type)) {
+		} elseif ($isSuperset->yes() && $this->checkAlwaysTrueInstanceof) {
 			return [
 				sprintf(
-					'Instanceof between %s and %s will always evaluate to false.',
+					'Instanceof between %s and %s will always evaluate to true.',
 					$expressionType->describe(),
 					$type->describe()
 				),

diff --git a/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php b/tests/PHPStan/Rules/Classes/ImpossibleInstanceOfRuleTest.php
@@ -5,20 +5,32 @@
 class ImpossibleInstanceOfRuleTest extends \PHPStan\Rules\AbstractRuleTest
 {
 
+	/** @var bool */
+	private $checkAlwaysTrueInstanceOf;
+
 	protected function getRule(): \PHPStan\Rules\Rule
 	{
-		return new ImpossibleInstanceOfRule($this->createBroker());
+		return new ImpossibleInstanceOfRule($this->checkAlwaysTrueInstanceOf);
 	}
 
-	public function testInstantiation()
+	public function testInstanceof()
 	{
+		$this->checkAlwaysTrueInstanceOf = true;
 		$this->analyse(
 			[__DIR__ . '/data/impossible-instanceof.php'],
 			[
+				[
+					'Instanceof between ImpossibleInstanceOf\Lorem and ImpossibleInstanceOf\Lorem will always evaluate to true.',
+					59,
+				],
 				[
 					'Instanceof between ImpossibleInstanceOf\Ipsum and ImpossibleInstanceOf\Lorem will always evaluate to true.',
 					65,
 				],
+				[
+					'Instanceof between ImpossibleInstanceOf\Ipsum and ImpossibleInstanceOf\Ipsum will always evaluate to true.',
+					68,
+				],
 				[
 					'Instanceof between ImpossibleInstanceOf\Dolor and ImpossibleInstanceOf\Lorem will always evaluate to false.',
 					71,
@@ -35,4 +47,18 @@ public function testInstantiation()
 		);
 	}
 
+	public function testInstanceofWithoutAlwaysTrue()
+	{
+		$this->checkAlwaysTrueInstanceOf = false;
+		$this->analyse(
+			[__DIR__ . '/data/impossible-instanceof.php'],
+			[
+				[
+					'Instanceof between ImpossibleInstanceOf\Dolor and ImpossibleInstanceOf\Lorem will always evaluate to false.',
+					71,
+				],
+			]
+		);
+	}
+
 }