Merge pull request #10651 from edsrzf/missing-override

vimeo · Feb 5, 2024 · 578046d · 578046d
2 parents c40f232 + 6d572a6
commit 578046d
Show file tree

Hide file tree

Showing 11 changed files with 260 additions and 96 deletions.
diff --git a/config.xsd b/config.xsd
@@ -42,6 +42,7 @@
         <xs:attribute name="checkForThrowsInGlobalScope" type="xs:boolean" default="false" />
         <xs:attribute name="ensureArrayIntOffsetsExist" type="xs:boolean" default="false" />
         <xs:attribute name="ensureArrayStringOffsetsExist" type="xs:boolean" default="false" />
+        <xs:attribute name="ensureOverrideAttribute" type="xs:boolean" default="false" />
         <xs:attribute name="findUnusedCode" type="xs:boolean" default="false" />
         <xs:attribute name="findUnusedVariablesAndParams" type="xs:boolean" default="false" />
         <xs:attribute name="findUnusedPsalmSuppress" type="xs:boolean" default="false" />
@@ -319,6 +320,7 @@
             <xs:element name="MissingDocblockType" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="MissingFile" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="MissingImmutableAnnotation" type="IssueHandlerType" minOccurs="0" />
+            <xs:element name="MissingOverrideAttribute" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="MissingParamType" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="MissingPropertyType" type="PropertyIssueHandlerType" minOccurs="0" />
             <xs:element name="MissingReturnType" type="IssueHandlerType" minOccurs="0" />

diff --git a/docs/running_psalm/configuration.md b/docs/running_psalm/configuration.md
@@ -273,6 +273,14 @@ When `true`, Psalm will complain when referencing an explicit string offset on a
 ```
 When `true`, Psalm will complain when referencing an explicit integer offset on an array e.g. `$arr[7]` without a user first asserting that it exists (either via an `isset` check or via an object-like array). Defaults to `false`.
 
+#### ensureOverrideAttribute
+```xml
+<psalm
+  ensureOverrideAttribute="[bool]"
+>
+```
+When `true`, Psalm will report class and interface methods that override a method on a parent, but do not have an `Override` attribute. Defaults to `false`.
+
 #### phpVersion
 ```xml
 <psalm

diff --git a/docs/running_psalm/error_levels.md b/docs/running_psalm/error_levels.md
@@ -278,6 +278,7 @@ Level 5 and above allows a more non-verifiable code, and higher levels are even
 
 ## Feature-specific errors
 
+ - [MissingOverrideAttribute](issues/MissingOverrideAttribute.md)
  - [PossiblyUndefinedIntArrayOffset](issues/PossiblyUndefinedIntArrayOffset.md)
  - [PossiblyUndefinedStringArrayOffset](issues/PossiblyUndefinedStringArrayOffset.md)
  - [PossiblyUnusedMethod](issues/PossiblyUnusedMethod.md)

diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md
@@ -120,6 +120,7 @@
  - [MissingDocblockType](issues/MissingDocblockType.md)
  - [MissingFile](issues/MissingFile.md)
  - [MissingImmutableAnnotation](issues/MissingImmutableAnnotation.md)
+ - [MissingOverrideAttribute](issues/MissingOverrideAttribute.md)
  - [MissingParamType](issues/MissingParamType.md)
  - [MissingPropertyType](issues/MissingPropertyType.md)
  - [MissingReturnType](issues/MissingReturnType.md)

diff --git a/docs/running_psalm/issues/MissingOverrideAttribute.md b/docs/running_psalm/issues/MissingOverrideAttribute.md
@@ -0,0 +1,23 @@
+# MissingOverrideAttribute
+
+Emitted when the config flag `ensureOverrideAttribute` is set to `true` and a method on a child class or interface overrides a method on a parent, but no `Override` attribute is present.
+
+```php
+<?php
+
+class A {
+    function receive(): void
+    {
+    }
+}
+
+class B extends A {
+    function receive(): void
+    {
+    }
+}
+```
+
+## Why this is bad
+
+Having an `Override` attribute on overridden methods makes intentions clear. Read the [PHP RFC](https://wiki.php.net/rfc/marking_overriden_methods) for more details.
diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php
@@ -445,6 +445,11 @@ class Config
      */
     public $ensure_array_int_offsets_exist = false;
 
+    /**
+     * @var bool
+     */
+    public $ensure_override_attribute = false;
+
     /**
      * @var array<lowercase-string, bool>
      */
@@ -1081,6 +1086,7 @@ private static function fromXmlAndPaths(
             'includePhpVersionsInErrorBaseline' => 'include_php_versions_in_error_baseline',
             'ensureArrayStringOffsetsExist' => 'ensure_array_string_offsets_exist',
             'ensureArrayIntOffsetsExist' => 'ensure_array_int_offsets_exist',
+            'ensureOverrideAttribute' => 'ensure_override_attribute',
             'reportMixedIssues' => 'show_mixed_issues',
             'skipChecksOnUnresolvableIncludes' => 'skip_checks_on_unresolvable_includes',
             'sealAllMethods' => 'seal_all_methods',

diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
@@ -37,6 +37,7 @@
 use Psalm\Issue\MethodSignatureMismatch;
 use Psalm\Issue\MismatchingDocblockParamType;
 use Psalm\Issue\MissingClosureParamType;
+use Psalm\Issue\MissingOverrideAttribute;
 use Psalm\Issue\MissingParamType;
 use Psalm\Issue\MissingThrowsDocblock;
 use Psalm\Issue\ReferenceConstraintViolation;
@@ -1973,20 +1974,37 @@ private function getFunctionInformation(
                 true,
             );
 
-            if ($codebase->analysis_php_version_id >= 8_03_00
-                && (!$overridden_method_ids || $storage->cased_name === '__construct')
-                && array_filter(
+            if ($codebase->analysis_php_version_id >= 8_03_00) {
+                $has_override_attribute = array_filter(
                     $storage->attributes,
                     static fn(AttributeStorage $s): bool => $s->fq_class_name === 'Override',
-                )
-            ) {
-                IssueBuffer::maybeAdd(
-                    new InvalidOverride(
-                        'Method ' . $storage->cased_name . ' does not match any parent method',
-                        $codeLocation,
-                    ),
-                    $this->getSuppressedIssues(),
                 );
+
+                if ($has_override_attribute
+                    && (!$overridden_method_ids || $storage->cased_name === '__construct')
+                ) {
+                    IssueBuffer::maybeAdd(
+                        new InvalidOverride(
+                            'Method ' . $storage->cased_name . ' does not match any parent method',
+                            $codeLocation,
+                        ),
+                        $this->getSuppressedIssues(),
+                    );
+                }
+
+                if (!$has_override_attribute
+                    && $codebase->config->ensure_override_attribute
+                    && $overridden_method_ids
+                    && $storage->cased_name !== '__construct'
+                ) {
+                    IssueBuffer::maybeAdd(
+                        new MissingOverrideAttribute(
+                            'Method ' . $storage->cased_name . ' should have the "Override" attribute',
+                            $codeLocation,
+                        ),
+                        $this->getSuppressedIssues(),
+                    );
+                }
             }
 
             if ($overridden_method_ids

diff --git a/src/Psalm/Issue/MissingOverrideAttribute.php b/src/Psalm/Issue/MissingOverrideAttribute.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace Psalm\Issue;
+
+final class MissingOverrideAttribute extends CodeIssue
+{
+    public const ERROR_LEVEL = -2;
+    public const SHORTCODE = 358;
+}
diff --git a/tests/AttributeTest.php b/tests/AttributeTest.php
@@ -293,36 +293,6 @@ class Foo
                 'ignored_issues' => [],
                 'php_version' => '8.2',
             ],
-            'override' => [
-                'code' => '<?php
-                    class C {
-                        public function f(): void {}
-                    }
-
-                    class C2 extends C {
-                        #[Override]
-                        public function f(): void {}
-                    }
-                ',
-                'assertions' => [],
-                'ignored_issues' => [],
-                'php_version' => '8.3',
-            ],
-            'overrideInterface' => [
-                'code' => '<?php
-                    interface I {
-                        public function f(): void;
-                    }
-
-                    interface I2 extends I {
-                        #[Override]
-                        public function f(): void;
-                    }
-                ',
-                'assertions' => [],
-                'ignored_issues' => [],
-                'php_version' => '8.3',
-            ],
             'sensitiveParameter' => [
                 'code' => '<?php
 
@@ -541,61 +511,6 @@ function foo() : void {}',
                     function foo(#[Pure] string $str) : void {}',
                 'error_message' => 'UndefinedAttributeClass - src' . DIRECTORY_SEPARATOR . 'somefile.php:4:36',
             ],
-            'overrideWithNoParent' => [
-                'code' => '<?php
-                    class C {
-                        #[Override]
-                        public function f(): void {}
-                    }
-                ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:25',
-                'error_levels' => [],
-                'php_version' => '8.3',
-            ],
-            'overrideConstructor' => [
-                'code' => '<?php
-                    /**
-                     * @psalm-consistent-constructor
-                     */
-                    class C {
-                        public function __construct() {}
-                    }
-
-                    class C2 extends C {
-                        #[Override]
-                        public function __construct() {}
-                    }
-                ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:10:25',
-                'error_levels' => [],
-                'php_version' => '8.3',
-            ],
-            'overridePrivate' => [
-                'code' => '<?php
-                    class C {
-                        private function f(): void {}
-                    }
-
-                    class C2 extends C {
-                        #[Override]
-                        private function f(): void {}
-                    }
-                ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:7:25',
-                'error_levels' => [],
-                'php_version' => '8.3',
-            ],
-            'overrideInterfaceWithNoParent' => [
-                'code' => '<?php
-                    interface I {
-                        #[Override]
-                        public function f(): void;
-                    }
-                ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:25',
-                'error_levels' => [],
-                'php_version' => '8.3',
-            ],
             'tooFewArgumentsToAttributeConstructor' => [
                 'code' => '<?php
                     namespace Foo;

diff --git a/tests/DocumentationTest.php b/tests/DocumentationTest.php
@@ -221,6 +221,8 @@ public function testInvalidCode(string $code, string $error_message, array $igno
         $this->project_analyzer->getConfig()->ensure_array_string_offsets_exist = $is_array_offset_test;
         $this->project_analyzer->getConfig()->ensure_array_int_offsets_exist = $is_array_offset_test;
 
+        $this->project_analyzer->getConfig()->ensure_override_attribute = $error_message === 'MissingOverrideAttribute';
+
         foreach ($ignored_issues as $error_level) {
             $this->project_analyzer->getCodebase()->config->setCustomErrorLevel($error_level, Config::REPORT_SUPPRESS);
         }
@@ -313,6 +315,7 @@ public function providerInvalidCodeParse(): array
                     break;
 
                 case 'InvalidOverride':
+                case 'MissingOverrideAttribute':
                     $php_version = '8.3';
                     break;
             }