EnforceEnumMatchRule (#94)

README.md

@@ -27,6 +27,8 @@ parameters:
             enabled: true
         backedEnumGenerics:
             enabled: true
+        enforceEnumMatch:
+            enabled: true
         enforceListReturn:
             enabled: true
         enforceNativeReturnTypehint:
@@ -136,6 +138,46 @@ enum MyEnum: string { // missing @implements tag
 }
 ```
 
+### enforceEnumMatchRule
+- Enforces usage of `match ($enum)` instead of conditions like `($enum === Enum::Case)`
+- This rule aims to "fix" a bit problematic behaviour of PHPStan (introduced at 1.10). It understands enum cases very well and forces you to adjust following code:
+```php
+enum MyEnum {
+    case Foo;
+    case Bar;
+}
+
+if ($enum === MyEnum::Foo) {
+    // ...
+} elseif ($enum === MyEnum::Bar) { // always true reported by phpstan
+    // ...
+} else {
+    throw new LogicException('Unknown case'); // phpstan knows it cannot happen
+}
+```
+Which someone might fix as:
+```php
+if ($enum === MyEnum::Foo) {
+    // ...
+} elseif ($enum === MyEnum::Bar) {
+    // ...
+}
+```
+Or even worse as:
+```php
+if ($enum === MyEnum::Foo) {
+    // ...
+} else {
+    // ...
+}
+```
+
+We believe that this leads to more error-prone code since adding new enum case may not fail in tests.
+Very good approach within similar cases is to use `match` construct so that (ideally with `forbidMatchDefaultArmForEnums` enabled) phpstan fails once new case is added.
+PHPStan even adds tip about `match` in those cases since `1.10.11`.
+For those reasons, this rule detects any always-true/false enum comparisons and forces you to rewrite it to `match ($enum)`.
+
+
 ### enforceListReturn
 - Enforces usage of `list<T>` when list is always returned from a class method or function
 - When only single return with empty array is present in the method, it is not considered as list

rules.neon

@@ -6,6 +6,8 @@ parameters:
             enabled: true
         backedEnumGenerics:
             enabled: true
+        enforceEnumMatch:
+            enabled: true
         enforceListReturn:
             enabled: true
         enforceNativeReturnTypehint:
@@ -71,6 +73,9 @@ parametersSchema:
         backedEnumGenerics: structure([
             enabled: bool()
         ])
+        enforceEnumMatch: structure([
+            enabled: bool()
+        ])
         enforceListReturn: structure([
             enabled: bool()
         ])
@@ -156,6 +161,8 @@ conditionalTags:
         phpstan.rules.rule: %shipmonkRules.allowNamedArgumentOnlyInAttributes.enabled%
     ShipMonk\PHPStan\Rule\BackedEnumGenericsRule:
         phpstan.rules.rule: %shipmonkRules.backedEnumGenerics.enabled%
+    ShipMonk\PHPStan\Rule\EnforceEnumMatchRule:
+        phpstan.rules.rule: %shipmonkRules.enforceEnumMatch.enabled%
     ShipMonk\PHPStan\Rule\EnforceNativeReturnTypehintRule:
         phpstan.rules.rule: %shipmonkRules.enforceNativeReturnTypehint.enabled%
     ShipMonk\PHPStan\Rule\EnforceListReturnRule:
@@ -223,6 +230,8 @@ services:
         class: ShipMonk\PHPStan\Rule\AllowNamedArgumentOnlyInAttributesRule
     -
         class: ShipMonk\PHPStan\Rule\BackedEnumGenericsRule
+    -
+        class: ShipMonk\PHPStan\Rule\EnforceEnumMatchRule
     -
         class: ShipMonk\PHPStan\Rule\EnforceListReturnRule
     -

src/Rule/EnforceEnumMatchRule.php

@@ -0,0 +1,66 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+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\Rules\Rule;
+use PHPStan\Type\Enum\EnumCaseObjectType;
+use function array_map;
+use function array_merge;
+use function array_unique;
+use function count;
+
+/**
+ * @implements Rule<BinaryOp>
+ */
+class EnforceEnumMatchRule implements Rule
+{
+
+    public function getNodeType(): string
+    {
+        return BinaryOp::class;
+    }
+
+    /**
+     * @param BinaryOp $node
+     * @return list<string>
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        if (!$node instanceof Identical && !$node instanceof NotIdentical) {
+            return [];
+        }
+
+        $conditionType = $scope->getType($node);
+
+        if (!$conditionType->isTrue()->yes() && !$conditionType->isFalse()->yes()) {
+            return [];
+        }
+
+        $leftType = $scope->getType($node->left);
+        $rightType = $scope->getType($node->right);
+
+        if ($leftType->isEnum()->yes() && $rightType->isEnum()->yes()) {
+            $enumCases = array_unique(
+                array_merge(
+                    array_map(static fn (EnumCaseObjectType $type) => "{$type->getClassName()}::{$type->getEnumCaseName()}", $leftType->getEnumCases()),
+                    array_map(static fn (EnumCaseObjectType $type) => "{$type->getClassName()}::{$type->getEnumCaseName()}", $rightType->getEnumCases()),
+                ),
+            );
+
+            if (count($enumCases) !== 1) {
+                return []; // do not report nonsense comparison
+            }
+
+            $trueFalse = $conditionType->isTrue()->yes() ? 'true' : 'false';
+            return ["This condition contains always-$trueFalse enum comparison of $enumCases[0]. Use match expression instead, PHPStan will report unhandled enum cases"];
+        }
+
+        return [];
+    }
+
+}

tests/Rule/EnforceEnumMatchRuleTest.php

@@ -0,0 +1,31 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+use PHPStan\Rules\Rule;
+use ShipMonk\PHPStan\RuleTestCase;
+use const PHP_VERSION_ID;
+
+/**
+ * @extends RuleTestCase<EnforceEnumMatchRule>
+ */
+class EnforceEnumMatchRuleTest extends RuleTestCase
+{
+
+    protected function getRule(): Rule
+    {
+        return new EnforceEnumMatchRule();
+    }
+
+    public function testRule(): void
+    {
+        if (PHP_VERSION_ID < 80_100) {
+            self::markTestSkipped('Requires PHP 8.1');
+        }
+
+        $this->analyseFile(
+            __DIR__ . '/data/EnforceEnumMatchRule/code.php',
+        );
+    }
+
+}

tests/Rule/data/EnforceEnumMatchRule/code.php

@@ -0,0 +1,132 @@
+<?php declare(strict_types = 1);
+
+namespace EnforceEnumMatchRule;
+
+enum SomeEnum: string
+{
+
+    case One = 'one';
+    case Two = 'two';
+    case Three = 'three';
+
+    public function nonEnumConditionIncluded(bool $condition): int
+    {
+        if ($this === self::Three || $this === self::One) {
+            return -1;
+        } elseif ($this === self::Two || $condition) { // error: This condition contains always-true enum comparison of EnforceEnumMatchRule\SomeEnum::Two. Use match expression instead, PHPStan will report unhandled enum cases
+            return 0;
+        }
+
+        return 1;
+    }
+
+    public function exhaustiveWithOrCondition(): int
+    {
+        if ($this === self::Three) {
+            return -1;
+        } elseif ($this === self::Two || $this === self::One) { // error: This condition contains always-true enum comparison of EnforceEnumMatchRule\SomeEnum::One. Use match expression instead, PHPStan will report unhandled enum cases
+            return 0;
+        }
+    }
+
+    public function basicExhaustive(): int
+    {
+        if ($this === self::Three) {
+            return -1;
+        } elseif ($this === self::Two) {
+            return 0;
+        } elseif ($this === self::One) { // error: This condition contains always-true enum comparison of EnforceEnumMatchRule\SomeEnum::One. Use match expression instead, PHPStan will report unhandled enum cases
+            return 1;
+        }
+    }
+
+    public function notExhaustiveWithNegatedConditionInLastElseif(): int
+    {
+        if ($this === self::Three) {
+            return -1;
+        } elseif ($this === self::Two) {
+            return 0;
+        } elseif ($this !== self::One) { // error: This condition contains always-false enum comparison of EnforceEnumMatchRule\SomeEnum::One. Use match expression instead, PHPStan will report unhandled enum cases
+            throw new \LogicException('Not expected case');
+        }
+
+        return 1;
+    }
+
+    public function nonSenseAlwaysFalseCodeNotReported(self $enum): int
+    {
+        if ($enum === self::Three) {
+            return -1;
+        }
+
+        if ($enum === self::Three) { // cannot use $this, see https://github.com/phpstan/phpstan/issues/9142
+            return 0;
+        }
+
+        return 1;
+    }
+
+    public function notExhaustive(): int
+    {
+        if ($this === self::Three) {
+            return -1;
+        } elseif ($this === self::Two) {
+            return 0;
+        }
+
+        return 1;
+    }
+
+    public function exhaustiveButNoElseIf(): int
+    {
+        if ($this === self::Three) {
+            return -1;
+        }
+
+        if ($this === self::Two) {
+            return 0;
+        }
+
+        if ($this === self::One) { // error: This condition contains always-true enum comparison of EnforceEnumMatchRule\SomeEnum::One. Use match expression instead, PHPStan will report unhandled enum cases
+            return 1;
+        }
+    }
+
+    /**
+     * @param self::Two|self::Three $param
+     */
+    public function exhaustiveOfSubset(self $param): int
+    {
+        if ($param === self::Three) {
+            return -1;
+        } elseif ($param === self::Two) { // error: This condition contains always-true enum comparison of EnforceEnumMatchRule\SomeEnum::Two. Use match expression instead, PHPStan will report unhandled enum cases
+            return 0;
+        }
+    }
+
+    public function exhaustiveButNotAllInThatElseIfChain(self $param): int
+    {
+        if ($param === self::One) {
+            throw new \LogicException();
+        }
+
+        if ($param === self::Three) {
+            return -1;
+        } elseif ($param === self::Two) { // error: This condition contains always-true enum comparison of EnforceEnumMatchRule\SomeEnum::Two. Use match expression instead, PHPStan will report unhandled enum cases
+            return 0;
+        }
+    }
+
+    /**
+     * @param true $true
+     */
+    public function alwaysTrueButNoEnumThere(bool $true): int
+    {
+        if ($this === self::Three) {
+            return -1;
+        } elseif ($true === true) {
+            return 0;
+        }
+    }
+
+}