forbidIdenticalClassComparison (#77)

* ForbidImmutableClassIdenticalComparisonRule

* typo & minor test impr

* rename

* DateTimeInterface by default

* readme DateTimeInterface

README.md

@@ -42,6 +42,9 @@ parameters:
             enabled: true
         forbidFetchOnMixed:
             enabled: true
+        forbidIdenticalClassComparison:
+            enabled: true
+            blacklist: ['DateTimeInterface']
         forbidMatchDefaultArmForEnums:
             enabled: true
         forbidMethodCallOnMixed:
@@ -257,6 +260,29 @@ function example($unknown) {
 }
 ```
 
+### forbidIdenticalClassComparison
+- Denies comparing configured classes by `===` or `!==`
+- Default configuration contains only `DateTimeInterface`
+- You may want to add more classes from your codebase or vendor
+
+```php
+function isEqual(DateTimeImmutable $a, DateTimeImmutable $b): bool {
+    return $a === $b;  // comparing denied classes
+}
+```
+```neon
+parameters:
+    shipmonkRules:
+        forbidIdenticalClassComparison:
+            blacklist:
+                - DateTimeInterface
+                - Brick\Money
+                - Brick\Math\BigNumber
+                - Brick\Math\BigInteger
+                - Brick\Math\BigDecimal
+                - Brick\Math\BigRational
+```
+
 ### forbidMatchDefaultArmForEnums
 - Denies using default arm in `match()` construct when native enum is passed as subject
 - This rules makes sense only as a complement of [native phpstan rule](https://github.com/phpstan/phpstan-src/blob/1.7.x/src/Rules/Comparison/MatchExpressionRule.php#L94) that guards that all enum cases are handled in match arms

rules.neon

@@ -24,6 +24,9 @@ parameters:
             enabled: true
         forbidFetchOnMixed:
             enabled: true
+        forbidIdenticalClassComparison:
+            enabled: true
+            blacklist: ['DateTimeInterface']
         forbidMatchDefaultArmForEnums:
             enabled: true
         forbidMethodCallOnMixed:
@@ -93,6 +96,10 @@ parametersSchema:
         forbidFetchOnMixed: structure([
             enabled: bool()
         ])
+        forbidIdenticalClassComparison: structure([
+            enabled: bool()
+            blacklist: arrayOf(string())
+        ])
         forbidMatchDefaultArmForEnums: structure([
             enabled: bool()
         ])
@@ -163,6 +170,8 @@ conditionalTags:
         phpstan.rules.rule: %shipmonkRules.forbidEnumInFunctionArguments.enabled%
     ShipMonk\PHPStan\Rule\ForbidFetchOnMixedRule:
         phpstan.rules.rule: %shipmonkRules.forbidFetchOnMixed.enabled%
+    ShipMonk\PHPStan\Rule\ForbidIdenticalClassComparisonRule:
+        phpstan.rules.rule: %shipmonkRules.forbidIdenticalClassComparison.enabled%
     ShipMonk\PHPStan\Rule\ForbidMatchDefaultArmForEnumsRule:
         phpstan.rules.rule: %shipmonkRules.forbidMatchDefaultArmForEnums.enabled%
     ShipMonk\PHPStan\Rule\ForbidMethodCallOnMixedRule:
@@ -234,6 +243,10 @@ services:
         class: ShipMonk\PHPStan\Rule\ForbidFetchOnMixedRule
         arguments:
             checkExplicitMixed: %checkExplicitMixed%
+    -
+        class: ShipMonk\PHPStan\Rule\ForbidIdenticalClassComparisonRule
+        arguments:
+            blacklist: %shipmonkRules.forbidIdenticalClassComparison.blacklist%
     -
         class: ShipMonk\PHPStan\Rule\ForbidMethodCallOnMixedRule
         arguments:

src/Rule/ForbidIdenticalClassComparisonRule.php

@@ -0,0 +1,103 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+use DateTimeInterface;
+use LogicException;
+use PhpParser\Node;
+use PhpParser\Node\Expr\BinaryOp;
+use PhpParser\Node\Expr\BinaryOp\Identical;
+use PhpParser\Node\Expr\BinaryOp\NotIdentical;
+use PHPStan\Analyser\Scope;
+use PHPStan\Reflection\ReflectionProvider;
+use PHPStan\Rules\Rule;
+use PHPStan\Type\Constant\ConstantBooleanType;
+use PHPStan\Type\MixedType;
+use PHPStan\Type\ObjectType;
+use PHPStan\Type\ObjectWithoutClassType;
+use PHPStan\Type\VerbosityLevel;
+use function count;
+
+/**
+ * @implements Rule<BinaryOp>
+ */
+class ForbidIdenticalClassComparisonRule implements Rule
+{
+
+    private const DEFAULT_BLACKLIST = [DateTimeInterface::class];
+
+    /**
+     * @var array<int, class-string<object>>
+     */
+    private array $blacklist;
+
+    /**
+     * @param array<int, class-string<object>> $blacklist
+     */
+    public function __construct(
+        ReflectionProvider $reflectionProvider,
+        array $blacklist = self::DEFAULT_BLACKLIST
+    )
+    {
+        foreach ($blacklist as $className) {
+            if (!$reflectionProvider->hasClass($className)) {
+                throw new LogicException("Class {$className} does not exist.");
+            }
+        }
+
+        $this->blacklist = $blacklist;
+    }
+
+    public function getNodeType(): string
+    {
+        return BinaryOp::class;
+    }
+
+    /**
+     * @param BinaryOp $node
+     * @return list<string>
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        if (count($this->blacklist) === 0) {
+            return [];
+        }
+
+        if (!$node instanceof Identical && !$node instanceof NotIdentical) {
+            return [];
+        }
+
+        $nodeType = $scope->getType($node);
+        $rightType = $scope->getType($node->right);
+        $leftType = $scope->getType($node->left);
+
+        if ($nodeType instanceof ConstantBooleanType) {
+            return []; // always-true or always-false, already reported by native PHPStan (like $a === $a)
+        }
+
+        if (
+            $leftType instanceof MixedType
+            || $leftType instanceof ObjectWithoutClassType
+            || $rightType instanceof MixedType
+            || $rightType instanceof ObjectWithoutClassType
+        ) {
+            return []; // those may contain forbidden class, but that is too strict
+        }
+
+        $errors = [];
+
+        foreach ($this->blacklist as $className) {
+            $forbiddenObjectType = new ObjectType($className);
+
+            if (
+                !$forbiddenObjectType->accepts($leftType, $scope->isDeclareStrictTypes())->no()
+                && !$forbiddenObjectType->accepts($rightType, $scope->isDeclareStrictTypes())->no()
+            ) {
+                $errors[] = "Using {$node->getOperatorSigil()} with {$forbiddenObjectType->describe(VerbosityLevel::typeOnly())} is denied";
+            }
+        }
+
+        return $errors;
+    }
+
+}

tests/Rule/ForbidIdenticalClassComparisonRuleTest.php

@@ -0,0 +1,27 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+use PHPStan\Reflection\ReflectionProvider;
+use PHPStan\Rules\Rule;
+use ShipMonk\PHPStan\RuleTestCase;
+
+/**
+ * @extends RuleTestCase<ForbidIdenticalClassComparisonRule>
+ */
+class ForbidIdenticalClassComparisonRuleTest extends RuleTestCase
+{
+
+    protected function getRule(): Rule
+    {
+        return new ForbidIdenticalClassComparisonRule(
+            self::getContainer()->getByType(ReflectionProvider::class),
+        );
+    }
+
+    public function testClass(): void
+    {
+        $this->analyseFile(__DIR__ . '/data/ForbidIdenticalClassComparisonRule/code.php');
+    }
+
+}

tests/Rule/data/ForbidIdenticalClassComparisonRule/code.php

@@ -0,0 +1,71 @@
+<?php declare(strict_types = 1);
+
+namespace ForbidIdenticalClassComparisonRule;
+
+use DateTimeImmutable;
+
+class DateTimeImmutableChild extends DateTimeImmutable {}
+
+class Dummy {}
+
+interface DummyInterface {}
+
+class A
+{
+
+    public function testNonObject(?DateTimeImmutable $a, string $b): void
+    {
+        $a === $a;
+        $a === $b;
+
+        if ($a !== null) {
+            $a->modify($b) === false;
+        }
+    }
+
+    public function testMixed(DateTimeImmutable $a, mixed $b): void
+    {
+        $a === $b;
+    }
+
+    public function testAnyObject(DateTimeImmutable $a, object $b): void
+    {
+        $a === $b;
+    }
+
+    public function testRegular(DateTimeImmutable $a, DateTimeImmutable $b): void
+    {
+        $a === $b; // error: Using === with DateTimeInterface is denied
+        $a !== $b; // error: Using !== with DateTimeInterface is denied
+    }
+
+    public function testNullable(?DateTimeImmutable $a, DateTimeImmutable $b): void
+    {
+        $a === $b; // error: Using === with DateTimeInterface is denied
+    }
+
+    /**
+     * @param DateTimeImmutable|Dummy $a
+     * @param DateTimeImmutable|Dummy $b
+     */
+    public function testUnion(object $a, object $b, Dummy $c, DateTimeImmutable $d): void
+    {
+        $a === $b; // error: Using === with DateTimeInterface is denied
+        $a === $d; // error: Using === with DateTimeInterface is denied
+        $a === $c;
+    }
+
+    public function testChild(DateTimeImmutableChild $a, ?DateTimeImmutable $b): void
+    {
+        $a === $b; // error: Using === with DateTimeInterface is denied
+    }
+
+    /**
+     * @param DateTimeImmutable&DummyInterface $a
+     */
+    public function testIntersection(object $a, DateTimeImmutable $b): void
+    {
+        $a === $b; // error: Using === with DateTimeInterface is denied
+    }
+
+}