Browse Source

feature #3928 Add FinalPublicMethodForAbstractClassFixer (Slamdunk)

This PR was merged into the 2.16-dev branch.

Discussion
----------

Add FinalPublicMethodForAbstractClassFixer

```diff
 abstract class AbstractMachine
 {
-    public function start()
+    final public function start()
     {}
 }
```

#### No exception and no configuration are intentional

If you want to override a method, use the [Template method pattern](https://en.wikipedia.org/wiki/Template_method_pattern).

This fixer enforces API encapsulation in an inheritance environment.

I left out magic methods:

1. Because some of them are not API
1. And the rest have issues with SPL releated array classes etc

Further reading: [When to declare methods final](https://slamdunk.github.io/blog/when-to-declare-methods-final/)

Commits
-------

19f823c6 Add FinalPublicMethodForAbstractClassFixer
Dariusz Ruminski 5 years ago
parent
commit
cb85791708

+ 6 - 0
README.rst

@@ -677,6 +677,12 @@ Choose from the list of available rules:
   - ``consider-absent-docblock-as-internal-class`` (``bool``): should classes
     without any DocBlock be fixed to final?; defaults to ``false``
 
+* **final_public_method_for_abstract_class**
+
+  All public methods of abstract classes should be final.
+
+  *Risky rule: risky when overriding public methods of abstract classes.*
+
 * **final_static_access**
 
   Converts ``static`` access to ``self`` access in final classes.

+ 168 - 0
src/Fixer/ClassNotation/FinalPublicMethodForAbstractClassFixer.php

@@ -0,0 +1,168 @@
+<?php
+
+/*
+ * This file is part of PHP CS Fixer.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *     Dariusz Rumiński <dariusz.ruminski@gmail.com>
+ *
+ * This source file is subject to the MIT license that is bundled
+ * with this source code in the file LICENSE.
+ */
+
+namespace PhpCsFixer\Fixer\ClassNotation;
+
+use PhpCsFixer\AbstractFixer;
+use PhpCsFixer\FixerDefinition\CodeSample;
+use PhpCsFixer\FixerDefinition\FixerDefinition;
+use PhpCsFixer\Tokenizer\Token;
+use PhpCsFixer\Tokenizer\Tokens;
+
+/**
+ * @author Filippo Tessarotto <zoeslam@gmail.com>
+ */
+final class FinalPublicMethodForAbstractClassFixer extends AbstractFixer
+{
+    /**
+     * @var array
+     */
+    private $magicMethods = [
+        '__construct' => true,
+        '__destruct' => true,
+        '__call' => true,
+        '__callstatic' => true,
+        '__get' => true,
+        '__set' => true,
+        '__isset' => true,
+        '__unset' => true,
+        '__sleep' => true,
+        '__wakeup' => true,
+        '__tostring' => true,
+        '__invoke' => true,
+        '__set_state' => true,
+        '__clone' => true,
+        '__debuginfo' => true,
+    ];
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getDefinition()
+    {
+        return new FixerDefinition(
+            'All public methods of abstract classes should be final.',
+            [
+                new CodeSample(
+                    '<?php
+
+abstract class AbstractMachine
+{
+    public function start()
+    {}
+}
+'
+                ),
+            ],
+            'Enforce API encapsulation in an inheritance architecture. '
+            .'If you want to override a method, use the Template method pattern.',
+            'Risky when overriding public methods of abstract classes'
+        );
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isCandidate(Tokens $tokens)
+    {
+        return $tokens->isAllTokenKindsFound([T_CLASS, T_ABSTRACT, T_PUBLIC, T_FUNCTION]);
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isRisky()
+    {
+        return true;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    protected function applyFix(\SplFileInfo $file, Tokens $tokens)
+    {
+        $classes = array_keys($tokens->findGivenKind(T_CLASS));
+
+        while ($classIndex = array_pop($classes)) {
+            $prevToken = $tokens[$tokens->getPrevMeaningfulToken($classIndex)];
+            if (!$prevToken->isGivenKind([T_ABSTRACT])) {
+                continue;
+            }
+
+            $classOpen = $tokens->getNextTokenOfKind($classIndex, ['{']);
+            $classClose = $tokens->findBlockEnd(Tokens::BLOCK_TYPE_CURLY_BRACE, $classOpen);
+
+            $this->fixClass($tokens, $classOpen, $classClose);
+        }
+    }
+
+    /**
+     * @param int $classOpenIndex
+     * @param int $classCloseIndex
+     */
+    private function fixClass(Tokens $tokens, $classOpenIndex, $classCloseIndex)
+    {
+        for ($index = $classCloseIndex - 1; $index > $classOpenIndex; --$index) {
+            // skip method contents
+            if ($tokens[$index]->equals('}')) {
+                $index = $tokens->findBlockStart(Tokens::BLOCK_TYPE_CURLY_BRACE, $index);
+
+                continue;
+            }
+
+            // skip non public methods
+            if (!$tokens[$index]->isGivenKind(T_PUBLIC)) {
+                continue;
+            }
+            $nextIndex = $tokens->getNextMeaningfulToken($index);
+            $nextToken = $tokens[$nextIndex];
+            if ($nextToken->isGivenKind(T_STATIC)) {
+                $nextIndex = $tokens->getNextMeaningfulToken($nextIndex);
+                $nextToken = $tokens[$nextIndex];
+            }
+
+            // skip uses, attributes, constants etc
+            if (!$nextToken->isGivenKind(T_FUNCTION)) {
+                continue;
+            }
+            $nextIndex = $tokens->getNextMeaningfulToken($nextIndex);
+            $nextToken = $tokens[$nextIndex];
+
+            // skip magic methods
+            if (isset($this->magicMethods[strtolower($nextToken->getContent())])) {
+                continue;
+            }
+
+            $prevIndex = $tokens->getPrevMeaningfulToken($index);
+            $prevToken = $tokens[$prevIndex];
+            if ($prevToken->isGivenKind(T_STATIC)) {
+                $index = $prevIndex;
+                $prevIndex = $tokens->getPrevMeaningfulToken($index);
+                $prevToken = $tokens[$prevIndex];
+            }
+            // skip already final methods
+            if ($prevToken->isGivenKind([T_FINAL])) {
+                $index = $prevIndex;
+
+                continue;
+            }
+
+            $tokens->insertAt(
+                $index,
+                [
+                    new Token([T_FINAL, 'final']),
+                    new Token([T_WHITESPACE, ' ']),
+                ]
+            );
+        }
+    }
+}

+ 171 - 0
tests/Fixer/ClassNotation/FinalPublicMethodForAbstractClassFixerTest.php

@@ -0,0 +1,171 @@
+<?php
+
+/*
+ * This file is part of PHP CS Fixer.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *     Dariusz Rumiński <dariusz.ruminski@gmail.com>
+ *
+ * This source file is subject to the MIT license that is bundled
+ * with this source code in the file LICENSE.
+ */
+
+namespace PhpCsFixer\Tests\Fixer\ClassNotation;
+
+use PhpCsFixer\Tests\Test\AbstractFixerTestCase;
+
+/**
+ * @author Filippo Tessarotto <zoeslam@gmail.com>
+ *
+ * @internal
+ *
+ * @covers \PhpCsFixer\Fixer\ClassNotation\FinalPublicMethodForAbstractClassFixer
+ */
+final class FinalPublicMethodForAbstractClassFixerTest extends AbstractFixerTestCase
+{
+    /**
+     * @param string      $expected PHP source code
+     * @param null|string $input    PHP source code
+     *
+     * @dataProvider provideFixCases
+     */
+    public function testFix($expected, $input = null)
+    {
+        $this->doTest($expected, $input);
+    }
+
+    public function provideFixCases()
+    {
+        $original = $fixed = $this->getClassElementStubs();
+        $fixed = str_replace('public function f1', 'final public function f1', $fixed);
+        $fixed = str_replace('public static function f4', 'final public static function f4', $fixed);
+        $fixed = str_replace('static public function f7', 'final static public function f7', $fixed);
+
+        return [
+            'regular-class' => ["<?php class MyClass { {$original} }"],
+            'final-class' => ["<?php final class MyClass { {$original} }"],
+            'trait' => ["<?php trait MyClass { {$original} }"],
+            'interface' => [
+                '<?php interface MyClass {
+                    public function f1();
+                    public static function f4();
+                    static public function f7();
+                }',
+            ],
+            'magic-methods' => [
+                '<?php abstract class MyClass {
+                    public function __construct() {}
+                    public function __destruct() {}
+                    public function __call($a, $b) {}
+                    public static function __callStatic($a, $b) {}
+                    public function __get($a) {}
+                    public function __set($a, $b) {}
+                    public function __isset($a) {}
+                    public function __unset($a) {}
+                    public function __sleep() {}
+                    public function __wakeup() {}
+                    public function __toString() {}
+                    public function __invoke() {}
+                    public function __set_state() {}
+                    public function __clone() {}
+                    public function __debugInfo() {}
+                }',
+            ],
+            'magic-methods-casing' => [
+                '<?php abstract class MyClass {
+                    public function __Construct() {}
+                    public function __SET($a, $b) {}
+                    public function __ToString() {}
+                    public function __DeBuGiNfO() {}
+                }',
+            ],
+            'non magic-methods' => [
+                '<?php abstract class MyClass {
+                    final public function __foo() {}
+                    final public static function __bar($a, $b) {}
+                }',
+                '<?php abstract class MyClass {
+                    public function __foo() {}
+                    public static function __bar($a, $b) {}
+                }',
+            ],
+            'abstract-class' => [
+                "<?php abstract class MyClass { {$fixed} }",
+                "<?php abstract class MyClass { {$original} }",
+            ],
+        ];
+    }
+
+    /**
+     * @param string      $expected PHP source code
+     * @param null|string $input    PHP source code
+     *
+     * @dataProvider provideFix70Cases
+     * @requires PHP 7.0
+     */
+    public function testFix70($expected, $input = null)
+    {
+        $this->doTest($expected, $input);
+    }
+
+    public function provideFix70Cases()
+    {
+        return [
+            'anonymous-class' => [
+                sprintf(
+                    '<?php abstract class MyClass { private function test() { $a = new class { %s }; } }',
+                    $this->getClassElementStubs()
+                ),
+            ],
+        ];
+    }
+
+    /**
+     * @param string      $expected PHP source code
+     * @param null|string $input    PHP source code
+     *
+     * @dataProvider provideFix72Cases
+     * @requires PHP 7.2
+     */
+    public function testFix72($expected, $input = null)
+    {
+        $this->doTest($expected, $input);
+    }
+
+    public function provideFix72Cases()
+    {
+        return [
+            'constant visibility' => [
+                '<?php abstract class MyClass {
+                    public const A = 1;
+                    protected const B = 2;
+                    private const C = 3;
+                }',
+            ],
+        ];
+    }
+
+    /**
+     * @return string
+     */
+    private function getClassElementStubs()
+    {
+        return '
+            public $a1;
+            protected $a2;
+            private $a3;
+            public static $a4;
+            protected static $a5;
+            private static $a6;
+            public function f1(){}
+            protected function f2(){}
+            private function f3(){}
+            public static function f4(){}
+            protected static function f5(){}
+            private static function f6(){}
+            static public function f7(){}
+            static protected function f8(){}
+            static private function f9(){}
+        ';
+    }
+}