From 8795b5f91415fa067ff865b476bbb991fc469ce2 Mon Sep 17 00:00:00 2001
From: Guy Sartorelli <guy.sartorelli@silverstripe.com>
Date: Thu, 18 Jul 2024 15:54:38 +1200
Subject: [PATCH] NEW Add rule to avoid use of "new" keyword

---
 composer.json                                 |  2 +-
 rules.neon                                    |  1 +
 src/PHPStan/KeywordSelfRule.php               | 75 +++++++++++++++++++
 src/PHPStan/MethodAnnotationsRule.php         |  2 +
 tests/PHPStan/KeywordSelfRuleTest.php         | 69 +++++++++++++++++
 .../KeywordSelfRuleTest/ClassUsesTrait.php    | 11 +++
 .../ClassUsesTraitCorrect.php                 | 11 +++
 .../PHPStan/KeywordSelfRuleTest/TestClass.php | 33 ++++++++
 .../PHPStan/KeywordSelfRuleTest/TestEnum.php  | 32 ++++++++
 .../KeywordSelfRuleTest/TestInterface.php     | 21 ++++++
 .../PHPStan/KeywordSelfRuleTest/TestTrait.php | 32 ++++++++
 .../KeywordSelfRuleTest/TestTraitCorrect.php  | 34 +++++++++
 12 files changed, 322 insertions(+), 1 deletion(-)
 create mode 100644 src/PHPStan/KeywordSelfRule.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest/ClassUsesTrait.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest/ClassUsesTraitCorrect.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest/TestClass.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest/TestEnum.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest/TestInterface.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest/TestTrait.php
 create mode 100644 tests/PHPStan/KeywordSelfRuleTest/TestTraitCorrect.php

diff --git a/composer.json b/composer.json
index 4a335a8..29152e3 100644
--- a/composer.json
+++ b/composer.json
@@ -4,7 +4,7 @@
     "license": "BSD-3-Clause",
     "require": {
         "php": "^8.1",
-        "phpstan/phpstan": "^1.10"
+        "phpstan/phpstan": "^1.11"
     },
     "require-dev": {
         "phpunit/phpunit": "^9.6",
diff --git a/rules.neon b/rules.neon
index 5588bfd..56a6367 100644
--- a/rules.neon
+++ b/rules.neon
@@ -1,5 +1,6 @@
 rules:
   - SilverStripe\Standards\PHPStan\MethodAnnotationsRule
+  - SilverStripe\Standards\PHPStan\KeywordSelfRule
 
 parameters:
   # Setting customRulestUsed to true allows us to avoid using the built-in rules
diff --git a/src/PHPStan/KeywordSelfRule.php b/src/PHPStan/KeywordSelfRule.php
new file mode 100644
index 0000000..24ef7eb
--- /dev/null
+++ b/src/PHPStan/KeywordSelfRule.php
@@ -0,0 +1,75 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SilverStripe\Standards\PHPStan;
+
+use PhpParser\Node;
+use PhpParser\Node\Name;
+use PhpParser\Node\Expr\ClassConstFetch;
+use PhpParser\Node\Expr\New_;
+use PhpParser\Node\Expr\StaticCall;
+use PhpParser\Node\Expr\StaticPropertyFetch;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+
+/**
+ * Validates that the `self` keyword is only used in situations where it's not avoidable.
+ *
+ * See https://phpstan.org/developing-extensions/rules
+ *
+ * @implements Rule<Node>
+ */
+class KeywordSelfRule implements Rule
+{
+    public function getNodeType(): string
+    {
+        return Node::class;
+    }
+
+    public function processNode(Node $node, Scope $scope): array
+    {
+        switch (get_class($node)) {
+            // fetching a constant, e.g. `self::MY_CONST` or `self::class`
+            case ClassConstFetch::class:
+                // Traits can use `self` to get const values - but otherwise follow same
+                // logic as methods or properties.
+                if ($scope->isInTrait()) {
+                    return [];
+                }
+            // static method call, e.g. `self::myMethod()`
+            case StaticCall::class:
+            // fetching a static property, e.g. `self::$my_property`
+            case StaticPropertyFetch::class:
+                if (!is_a($node->class, Name::class) || $node->class->toString() !== 'self') {
+                    return [];
+                }
+                break;
+            // `self` as a type (for a property, argument, or return type)
+            case Name::class:
+                // Trait can use `self` for typehinting
+                if ($scope->isInTrait() || $node->toString() !== 'self') {
+                    return [];
+                }
+                break;
+            // instantiating a new object from `self`, e.g. `new self()`
+            case New_::class:
+                if (!is_a($node->class, Name::class) || $node->class->toString() !== 'self') {
+                    return [];
+                }
+                break;
+            default:
+                return [];
+        }
+
+        $actualClass = $scope->isInTrait()
+            ? 'self::class'
+            : $scope->getClassReflection()->getNativeReflection()->getShortName();
+        return [
+            RuleErrorBuilder::message(
+                "Can't use keyword 'self'. Use '$actualClass' instead."
+            )->build()
+        ];
+    }
+}
diff --git a/src/PHPStan/MethodAnnotationsRule.php b/src/PHPStan/MethodAnnotationsRule.php
index 8619184..d3c2d09 100644
--- a/src/PHPStan/MethodAnnotationsRule.php
+++ b/src/PHPStan/MethodAnnotationsRule.php
@@ -24,6 +24,8 @@
  * Validates that `@method` annotations in DataObject and Extension classes are correct,
  * according to the relations defined within those classes.
  *
+ * See https://phpstan.org/developing-extensions/rules
+ *
  * @implements Rule<Class_>
  */
 class MethodAnnotationsRule implements Rule
diff --git a/tests/PHPStan/KeywordSelfRuleTest.php b/tests/PHPStan/KeywordSelfRuleTest.php
new file mode 100644
index 0000000..6c82fbb
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest.php
@@ -0,0 +1,69 @@
+<?php
+
+declare(strict_types=1);
+
+namespace SilverStripe\Standards\Tests\PHPStan;
+
+use PHPStan\Rules\Rule;
+use PHPStan\Testing\RuleTestCase;
+use SilverStripe\Standards\PHPStan\KeywordSelfRule;
+
+/**
+ * @extends RuleTestCase<KeywordSelfRule>
+ */
+class KeywordSelfRuleTest extends RuleTestCase
+{
+    protected function getRule(): Rule
+    {
+        return new KeywordSelfRule();
+    }
+
+    public function provideRule()
+    {
+        return [
+            'interface' => [
+                'filePaths' => [__DIR__ . '/KeywordSelfRuleTest/TestInterface.php'],
+                'errorMessage' => "Can't use keyword 'self'. Use 'TestInterface' instead.",
+                'errorLines' => [13, 18, 18],
+            ],
+            'class' => [
+                'filePaths' => [__DIR__ . '/KeywordSelfRuleTest/TestClass.php'],
+                'errorMessage' => "Can't use keyword 'self'. Use 'TestClass' instead.",
+                'errorLines' => [9, 11, 16, 16, 18, 20, 21, 21, 25],
+            ],
+            'enum' => [
+                'filePaths' => [__DIR__ . '/KeywordSelfRuleTest/TestEnum.php'],
+                'errorMessage' => "Can't use keyword 'self'. Use 'TestEnum' instead.",
+                'errorLines' => [9, 14, 14, 16, 17, 18, 20, 24],
+            ],
+            'trait' => [
+                'filePaths' => [
+                    __DIR__ . '/KeywordSelfRuleTest/TestTrait.php',
+                    __DIR__ . '/KeywordSelfRuleTest/ClassUsesTrait.php',
+                ],
+                'errorMessage' => "Can't use keyword 'self'. Use 'self::class' instead.",
+                'errorLines' => [17, 19, 20, 24],
+            ],
+            'trait no errors' => [
+                'filePaths' => [
+                    __DIR__ . '/KeywordSelfRuleTest/TestTraitCorrect.php',
+                    __DIR__ . '/KeywordSelfRuleTest/ClassUsesTraitCorrect.php',
+                ],
+                'errorMessage' => '',
+                'errorLines' => [],
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideRule
+     */
+    public function testRule(array $filePaths, string $errorMessage, array $errorLines): void
+    {
+        $errors = [];
+        foreach ($errorLines as $line) {
+            $errors[] = [$errorMessage, $line];
+        }
+        $this->analyse($filePaths, $errors);
+    }
+}
diff --git a/tests/PHPStan/KeywordSelfRuleTest/ClassUsesTrait.php b/tests/PHPStan/KeywordSelfRuleTest/ClassUsesTrait.php
new file mode 100644
index 0000000..8594e10
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest/ClassUsesTrait.php
@@ -0,0 +1,11 @@
+<?php
+
+namespace SilverStripe\Standards\Tests\PHPStan\KeywordSelfRuleTest;
+
+/**
+ * PHPStan doesn't analyse traits unless there's a class that uses it
+ */
+class ClassUsesTrait
+{
+    use TestTrait;
+}
diff --git a/tests/PHPStan/KeywordSelfRuleTest/ClassUsesTraitCorrect.php b/tests/PHPStan/KeywordSelfRuleTest/ClassUsesTraitCorrect.php
new file mode 100644
index 0000000..1768848
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest/ClassUsesTraitCorrect.php
@@ -0,0 +1,11 @@
+<?php
+
+namespace SilverStripe\Standards\Tests\PHPStan\KeywordSelfRuleTest;
+
+/**
+ * PHPStan doesn't analyse traits unless there's a class that uses it
+ */
+class ClassUsesTraitCorrect
+{
+    use TestTraitCorrect;
+}
diff --git a/tests/PHPStan/KeywordSelfRuleTest/TestClass.php b/tests/PHPStan/KeywordSelfRuleTest/TestClass.php
new file mode 100644
index 0000000..87952cb
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest/TestClass.php
@@ -0,0 +1,33 @@
+<?php
+
+namespace SilverStripe\Standards\Tests\PHPStan\KeywordSelfRuleTest;
+
+class TestClass
+{
+    private const MY_CONST = 'some value';
+
+    private static string $myProperty = self::MY_CONST;
+
+    private static self $mySecondProperty;
+
+    /**
+     * self::class is ignored here because this is a comment.
+     */
+    public static function function1(self $someProperty): self
+    {
+        $self = new self();
+        $self::class; // $self:: isn't seen as self::
+        self::self();
+        self::$myProperty = self::class;
+        /* intentionally commented out to prove even multi-line comments are ignored.
+        self::$myProperty = self::class;
+        */
+        return new self();
+    }
+
+    public static function self()
+    {
+        // doesn't need an implementation - the point of this method is just to show that "self" isn't just being
+        // searched for naively.
+    }
+}
diff --git a/tests/PHPStan/KeywordSelfRuleTest/TestEnum.php b/tests/PHPStan/KeywordSelfRuleTest/TestEnum.php
new file mode 100644
index 0000000..e09f78b
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest/TestEnum.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace SilverStripe\Standards\Tests\PHPStan\KeywordSelfRuleTest;
+
+enum TestEnum: string
+{
+    private const MY_CONST = 'some value';
+
+    case self = self::MY_CONST;
+
+    /**
+     * self::class is ignored here because this is a comment.
+     */
+    public static function function1(self $someProperty): self
+    {
+        $selfEnumValue = self::self;
+        $selfName = self::class;
+        $self = new self();
+        $self::class; // $self:: isn't seen as self::
+        self::self();
+        /* intentionally commented out to prove even multi-line comments are ignored.
+        self::$myProperty = self::class;
+        */
+        return new self('self');
+    }
+
+    public static function self()
+    {
+        // doesn't need an implementation - the point of this method is just to show that "self" isn't just being
+        // searched for naively.
+    }
+}
diff --git a/tests/PHPStan/KeywordSelfRuleTest/TestInterface.php b/tests/PHPStan/KeywordSelfRuleTest/TestInterface.php
new file mode 100644
index 0000000..848fe55
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest/TestInterface.php
@@ -0,0 +1,21 @@
+<?php
+
+namespace SilverStripe\Standards\Tests\PHPStan\KeywordSelfRuleTest;
+
+/**
+ * Usage of `self` in an interface refers to the interface itself, unlike with traits.
+ * This means we should avoid using self, since it's not actually needed here.
+ */
+interface TestInterface
+{
+    public const MY_CONST = 'some value';
+
+    public const MY_SECOND_CONST = self::MY_CONST;
+
+    /**
+     * self::class is ignored here because this is a comment.
+     */
+    public static function function1(self $someProperty): self;
+
+    public static function self();
+}
diff --git a/tests/PHPStan/KeywordSelfRuleTest/TestTrait.php b/tests/PHPStan/KeywordSelfRuleTest/TestTrait.php
new file mode 100644
index 0000000..4c5101e
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest/TestTrait.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace SilverStripe\Standards\Tests\PHPStan\KeywordSelfRuleTest;
+
+trait TestTrait
+{
+    // Can't define a const on a trait, but the trait can access consts on the class that uses it
+    private static string $myProperty = self::MY_CONST;
+
+    private static self $mySecondProperty;
+
+    /**
+     * self::class is ignored here because this is a comment.
+     */
+    public static function function1(self $someProperty): self
+    {
+        $self = new self();
+        $self::class; // $self:: isn't seen as self::
+        self::self();
+        self::$myProperty = self::class;
+        /* intentionally commented out to prove even multi-line comments are ignored.
+        self::$myProperty = self::class;
+        */
+        return new self();
+    }
+
+    public static function self()
+    {
+        // doesn't need an implementation - the point of this method is just to show that "self" isn't just being
+        // searched for naively.
+    }
+}
diff --git a/tests/PHPStan/KeywordSelfRuleTest/TestTraitCorrect.php b/tests/PHPStan/KeywordSelfRuleTest/TestTraitCorrect.php
new file mode 100644
index 0000000..002fc24
--- /dev/null
+++ b/tests/PHPStan/KeywordSelfRuleTest/TestTraitCorrect.php
@@ -0,0 +1,34 @@
+<?php
+
+namespace SilverStripe\Standards\Tests\PHPStan\KeywordSelfRuleTest;
+
+trait TestTraitCorrect
+{
+    // Can't define a const on a trait, but the trait can access consts on the class that uses it
+    private static string $myProperty = self::MY_CONST;
+
+    private static self $mySecondProperty;
+
+    /**
+     * self::class is ignored here because this is a comment.
+     */
+    public static function function1(self $someProperty): self
+    {
+        $self = new (self::class)();
+        $self::class; // $self:: isn't seen as self::
+        self::class::self();
+        self::class::$myProperty = self::class;
+        /* intentionally commented out to prove even multi-line comments are ignored.
+        self::$myProperty = self::class;
+        */
+        // Alternative acceptable syntax for instantiating
+        $selfClass = self::class;
+        return new $selfClass();
+    }
+
+    public static function self()
+    {
+        // doesn't need an implementation - the point of this method is just to show that "self" isn't just being
+        // searched for naively.
+    }
+}