[explicit] Add NoProtectedClassStmtRule (#185)

Author: TomasVotruba

README.md

@@ -245,6 +245,17 @@ abstract class AbstractClass
 
 <br>
 
+### NoProtectedClassStmtRule
+
+Avoid protected class stmts as they yield unexpected behavior. Use clear interface contract instead
+
+```yaml
+rules:
+    - Symplify\PHPStanRules\Rules\Explicit\NoProtectedClassStmtRule
+```
+
+<br>
+
 ### ForbiddenArrayMethodCallRule
 
 Array method calls [$this, "method"] are not allowed. Use explicit method instead to help PhpStorm, PHPStan and Rector understand your code

src/Enum/MethodName.php

@@ -11,5 +11,18 @@ final class MethodName
      */
     public const INVOKE = '__invoke';
 
+    /**
+     * @var string
+     */
     public const CONSTRUCTOR = '__construct';
+
+    /**
+     * @var string
+     */
+    public const SET_UP = 'setUp';
+
+    /**
+     * @var string
+     */
+    public const TEAR_DOWN = 'tearDown';
 }

src/Enum/RuleIdentifier.php

@@ -69,4 +69,6 @@ final class RuleIdentifier
     public const STRING_FILE_ABSOLUTE_PATH_EXISTS = 'symplify.stringFileAbsolutePathExists';
 
     public const NO_JUST_PROPERTY_ASSIGN = 'symplify.noJustPropertyAssign';
+
+    public const NO_PROTECTED_CLASS_STMT = 'symplify.noProtectedClassStmt';
 }

src/Rules/Explicit/NoProtectedClassStmtRule.php

@@ -0,0 +1,107 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Symplify\PHPStanRules\Rules\Explicit;
+
+use PhpParser\Node;
+use PhpParser\Node\Stmt\Class_;
+use PhpParser\Node\Stmt\ClassConst;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\Node\Stmt\Property;
+use PHPStan\Analyser\Scope;
+use PHPStan\Node\InClassNode;
+use PHPStan\Reflection\ClassReflection;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleError;
+use PHPStan\Rules\RuleErrorBuilder;
+use Symplify\PHPStanRules\Enum\MethodName;
+use Symplify\PHPStanRules\Enum\RuleIdentifier;
+
+/**
+ * @implements Rule<InClassNode>
+ *
+ * @see \Symplify\PHPStanRules\Tests\Rules\Explicit\NoProtectedClassStmtRule\NoProtectedClassStmtRuleTest
+ */
+final class NoProtectedClassStmtRule implements Rule
+{
+    /**
+     * @var string
+     */
+    public const ERROR_MESSAGE = 'Avoid protected class stmts as they yield unexpected behavior. Use clear interface contract instead';
+
+    public function getNodeType(): string
+    {
+        return InClassNode::class;
+    }
+
+    /**
+     * @param InClassNode $node
+     * @return RuleError[]
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        $classLike = $node->getOriginalNode();
+        if (! $classLike instanceof Class_) {
+            return [];
+        }
+
+        // skip abstract classes for onw
+        if ($classLike->isAbstract()) {
+            return [];
+        }
+
+        $ruleErrors = [];
+
+        foreach ($classLike->stmts as $classStmt) {
+            if (! $classStmt instanceof ClassMethod && ! $classStmt instanceof ClassConst && ! $classStmt instanceof Property) {
+                continue;
+            }
+
+            // skip test one
+            if ($this->shouldSkipClassMethod($classStmt, $scope)) {
+                continue;
+            }
+
+            if (! $classStmt->isProtected()) {
+                continue;
+            }
+
+            $ruleErrors[] = RuleErrorBuilder::message(self::ERROR_MESSAGE)
+                ->line($classStmt->getStartLine())
+                ->identifier(RuleIdentifier::NO_PROTECTED_CLASS_STMT)
+                ->build();
+        }
+
+        return $ruleErrors;
+    }
+
+    private function shouldSkipClassMethod(ClassMethod|ClassConst|Property $classStmt, Scope $scope): bool
+    {
+        if (! $classStmt instanceof ClassMethod) {
+            return false;
+        }
+
+        // PHPUnit test methods
+        if (in_array($classStmt->name->toString(), [MethodName::SET_UP, MethodName::TEAR_DOWN])) {
+            return false;
+        }
+
+        return $this->isParentMethodOverride($classStmt, $scope);
+    }
+
+    private function isParentMethodOverride(ClassMethod $classMethod, Scope $scope): bool
+    {
+        $classReflection = $scope->getClassReflection();
+        if (! $classReflection instanceof ClassReflection) {
+            return false;
+        }
+
+        $parentClassReflection = $classReflection->getParentClass();
+        if (! $parentClassReflection instanceof ClassReflection) {
+            return false;
+        }
+
+        return $parentClassReflection->hasMethod($classMethod->name->toString());
+    }
+}

src/Rules/Symfony/RequireInvokableControllerRule.php

@@ -10,7 +10,6 @@
 use PHPStan\Rules\Rule;
 use PHPStan\Rules\RuleErrorBuilder;
 use Symplify\PHPStanRules\Enum\MethodName;
-use Symplify\PHPStanRules\Enum\SymfonyClass;
 use Symplify\PHPStanRules\Enum\SymfonyRuleIdentifier;
 use Symplify\PHPStanRules\Symfony\NodeAnalyzer\SymfonyControllerAnalyzer;
 

tests/Rules/Explicit/NoProtectedClassStmtRule/Fixture/ClassWithProtected.php

@@ -0,0 +1,8 @@
+<?php
+
+namespace Symplify\PHPStanRules\Tests\Rules\Explicit\NoProtectedClassStmtRule\Fixture;
+
+final class ClassWithProtected
+{
+    protected int $item;
+}

tests/Rules/Explicit/NoProtectedClassStmtRule/Fixture/SkipAbstractWithProtected.php

@@ -0,0 +1,8 @@
+<?php
+
+namespace Symplify\PHPStanRules\Tests\Rules\Explicit\NoProtectedClassStmtRule\Fixture;
+
+abstract class SkipAbstractWithProtected
+{
+    protected int $item;
+}

tests/Rules/Explicit/NoProtectedClassStmtRule/Fixture/SkipParentRequired.php

@@ -0,0 +1,12 @@
+<?php
+
+namespace Symplify\PHPStanRules\Tests\Rules\Explicit\NoProtectedClassStmtRule\Fixture;
+
+use Symplify\PHPStanRules\Tests\Rules\Explicit\NoProtectedClassStmtRule\Source\ParentClassWithMethod;
+
+final class SkipParentRequired extends ParentClassWithMethod
+{
+    protected function some()
+    {
+    }
+}

tests/Rules/Explicit/NoProtectedClassStmtRule/NoProtectedClassStmtRuleTest.php

@@ -0,0 +1,35 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Symplify\PHPStanRules\Tests\Rules\Explicit\NoProtectedClassStmtRule;
+
+use Iterator;
+use PHPStan\Rules\Rule;
+use PHPStan\Testing\RuleTestCase;
+use PHPUnit\Framework\Attributes\DataProvider;
+use Symplify\PHPStanRules\Rules\Explicit\NoProtectedClassStmtRule;
+
+final class NoProtectedClassStmtRuleTest extends RuleTestCase
+{
+    #[DataProvider('provideData')]
+    public function testRule(string $filePath, array $expectedErrorsWithLines): void
+    {
+        $this->analyse([$filePath], $expectedErrorsWithLines);
+    }
+
+    public static function provideData(): Iterator
+    {
+        yield [__DIR__ . '/Fixture/ClassWithProtected.php', [
+            [NoProtectedClassStmtRule::ERROR_MESSAGE, 7],
+        ]];
+
+        yield [__DIR__ . '/Fixture/SkipParentRequired.php', []];
+        yield [__DIR__ . '/Fixture/SkipAbstractWithProtected.php', []];
+    }
+
+    protected function getRule(): Rule
+    {
+        return new NoProtectedClassStmtRule();
+    }
+}

tests/Rules/Explicit/NoProtectedClassStmtRule/Source/ParentClassWithMethod.php

@@ -0,0 +1,10 @@
+<?php
+
+namespace Symplify\PHPStanRules\Tests\Rules\Explicit\NoProtectedClassStmtRule\Source;
+
+class ParentClassWithMethod
+{
+    protected function some()
+    {
+    }
+}