Merge pull request #9442 from weirdan/forbid-private-final-methods

vimeo · Mar 3, 2023 · 6975acf · 6975acf
2 parents e3078cd + 98d96fb
commit 6975acf
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 1 deletion.
diff --git a/config.xsd b/config.xsd
@@ -407,6 +407,7 @@
             <xs:element name="PossiblyUnusedParam" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="PossiblyUnusedProperty" type="PropertyIssueHandlerType" minOccurs="0" />
             <xs:element name="PossiblyUnusedReturnValue" type="IssueHandlerType" minOccurs="0" />
+            <xs:element name="PrivateFinalMethod" type="MethodIssueHandlerType" minOccurs="0" />
             <xs:element name="PropertyNotSetInConstructor" type="PropertyIssueHandlerType" minOccurs="0" />
             <xs:element name="PropertyTypeCoercion" type="PropertyIssueHandlerType" minOccurs="0" />
             <xs:element name="RawObjectIteration" type="IssueHandlerType" minOccurs="0" />

diff --git a/docs/running_psalm/error_levels.md b/docs/running_psalm/error_levels.md
@@ -238,6 +238,7 @@ Level 5 and above allows a more non-verifiable code, and higher levels are even
 - [MissingPropertyType](issues/MissingPropertyType.md)
 - [MissingReturnType](issues/MissingReturnType.md)
 - [NullOperand](issues/NullOperand.md)
+- [PrivateFinalMethod](issues/PrivateFinalMethod.md)
 - [PropertyNotSetInConstructor](issues/PropertyNotSetInConstructor.md)
 - [RawObjectIteration](issues/RawObjectIteration.md)
 - [RedundantConditionGivenDocblockType](issues/RedundantConditionGivenDocblockType.md)

diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md
@@ -211,6 +211,7 @@
  - [PossiblyUnusedParam](issues/PossiblyUnusedParam.md)
  - [PossiblyUnusedProperty](issues/PossiblyUnusedProperty.md)
  - [PossiblyUnusedReturnValue](issues/PossiblyUnusedReturnValue.md)
+ - [PrivateFinalMethod](issues/PrivateFinalMethod.md)
  - [PropertyNotSetInConstructor](issues/PropertyNotSetInConstructor.md)
  - [PropertyTypeCoercion](issues/PropertyTypeCoercion.md)
  - [RawObjectIteration](issues/RawObjectIteration.md)

diff --git a/docs/running_psalm/issues/PrivateFinalMethod.md b/docs/running_psalm/issues/PrivateFinalMethod.md
@@ -0,0 +1,14 @@
+# PrivateFinalMethod
+
+Emitted when a class defines private final method. PHP 8.0+ emits a warning when it sees private final method (except `__construct` where it's allowed), and allows redefinition in child classes (effectively ignoring `final` modifier). Before PHP 8.0, `final` was respected in this case.
+
+```php
+<?php
+class Foo {
+    final private function baz(): void {}
+}
+```
+
+## Why this is bad
+
+It causes a warning, and behavior differs between versions.
diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/FunctionLikeNodeScanner.php b/src/Psalm/Internal/PhpVisitor/Reflector/FunctionLikeNodeScanner.php
@@ -34,6 +34,7 @@
 use Psalm\Issue\InvalidDocblock;
 use Psalm\Issue\MissingDocblockType;
 use Psalm\Issue\ParseError;
+use Psalm\Issue\PrivateFinalMethod;
 use Psalm\IssueBuffer;
 use Psalm\Storage\ClassLikeStorage;
 use Psalm\Storage\FileStorage;
@@ -1101,7 +1102,24 @@ private function createStorageForFunctionLike(
             $storage->is_static = $stmt->isStatic();
             $storage->abstract = $stmt->isAbstract();
 
-            $storage->final = $classlike_storage->final || $stmt->isFinal();
+            if ($stmt->isPrivate() && $stmt->isFinal() && $method_name_lc !== '__construct') {
+                IssueBuffer::maybeAdd(
+                    new PrivateFinalMethod(
+                        'Private methods cannot be final',
+                        new CodeLocation($this->file_scanner, $stmt, null, true),
+                        (string) $method_id,
+                    ),
+                );
+                if ($this->codebase->analysis_php_version_id >= 8_00_00) {
+                    // ignore `final` on the method as that's what PHP does
+                    $storage->final = $classlike_storage->final;
+                } else {
+                    $storage->final = true;
+                }
+            } else {
+                $storage->final = $classlike_storage->final || $stmt->isFinal();
+            }
+
             $storage->final_from_docblock = $classlike_storage->final_from_docblock;
 
             if ($stmt->isPrivate()) {

diff --git a/src/Psalm/Issue/PrivateFinalMethod.php b/src/Psalm/Issue/PrivateFinalMethod.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace Psalm\Issue;
+
+class PrivateFinalMethod extends MethodIssue
+{
+    public const ERROR_LEVEL = 2;
+    public const SHORTCODE = 320;
+}
diff --git a/tests/ClassTest.php b/tests/ClassTest.php
@@ -839,6 +839,14 @@ function f(): I {
                     throw new $test();
                 ',
             ],
+            'privateFinalConstructorsAreAllowed' => [
+                'code' => <<<'PHP'
+                    <?php
+                    class Foo {
+                        private final function __construct() {}
+                    }
+                    PHP,
+            ],
         ];
     }
 
@@ -1231,6 +1239,15 @@ public function f(): void { $this->__construct(); }
                 ',
                 'error_message' => 'DirectConstructorCall',
             ],
+            'privateFinalMethodsAreForbidden' => [
+                'code' => <<<'PHP'
+                    <?php
+                    class Foo {
+                        final private function baz(): void {}
+                    }
+                    PHP,
+                'error_message' => 'PrivateFinalMethod',
+            ],
         ];
     }
 }