From 7d07e258a3d15757b446e2cae905e457b12b908b Mon Sep 17 00:00:00 2001
From: Evan Shaw <evan.shaw@lightspeedhq.com>
Date: Sun, 4 Feb 2024 11:17:13 +1300
Subject: [PATCH 1/4] Add MissingOverrideAttribute issue

---
 config.xsd                                    |  2 ++
 docs/running_psalm/configuration.md           |  8 +++++++
 docs/running_psalm/error_levels.md            |  1 +
 docs/running_psalm/issues.md                  |  1 +
 .../issues/MissingOverrideAttribute.md        | 23 +++++++++++++++++++
 src/Psalm/Config.php                          |  6 +++++
 src/Psalm/Issue/MissingOverrideAttribute.php  |  9 ++++++++
 tests/DocumentationTest.php                   |  3 +++
 8 files changed, 53 insertions(+)
 create mode 100644 docs/running_psalm/issues/MissingOverrideAttribute.md
 create mode 100644 src/Psalm/Issue/MissingOverrideAttribute.php

diff --git a/config.xsd b/config.xsd
index 4cdb3298e7d..c15a74e6080 100644
--- 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
index f4976aaad83..d00baae88d3 100644
--- 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
index 923a36eef57..d0edf8c0236 100644
--- 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
index 5f34274acc8..ae683435186 100644
--- 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
new file mode 100644
index 00000000000..e59b35bd11e
--- /dev/null
+++ 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
index e6f05540b55..3fcca335435 100644
--- 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/Issue/MissingOverrideAttribute.php b/src/Psalm/Issue/MissingOverrideAttribute.php
new file mode 100644
index 00000000000..0e146e19971
--- /dev/null
+++ 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/DocumentationTest.php b/tests/DocumentationTest.php
index 6e1506eefc5..74c0cb6eae6 100644
--- 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;
             }

From c71ad2221cae8ebe317a6dc77290b91657c70cc3 Mon Sep 17 00:00:00 2001
From: Evan Shaw <evan.shaw@lightspeedhq.com>
Date: Sun, 4 Feb 2024 15:30:36 +1300
Subject: [PATCH 2/4] Move Override tests to separate file

---
 tests/AttributeTest.php |  85 ------------------------------
 tests/OverrideTest.php  | 111 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 85 deletions(-)
 create mode 100644 tests/OverrideTest.php

diff --git a/tests/AttributeTest.php b/tests/AttributeTest.php
index 4a631c5b3dd..f1051773882 100644
--- 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/OverrideTest.php b/tests/OverrideTest.php
new file mode 100644
index 00000000000..1798bf7804d
--- /dev/null
+++ b/tests/OverrideTest.php
@@ -0,0 +1,111 @@
+<?php
+
+namespace Psalm\Tests;
+
+use Psalm\Tests\Traits\InvalidCodeAnalysisTestTrait;
+use Psalm\Tests\Traits\ValidCodeAnalysisTestTrait;
+
+use const DIRECTORY_SEPARATOR;
+
+class OverrideTest extends TestCase
+{
+    use ValidCodeAnalysisTestTrait;
+    use InvalidCodeAnalysisTestTrait;
+
+    public function providerValidCodeParse(): iterable
+    {
+        return [
+            'overrideClass' => [
+                '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',
+            ],
+        ];
+    }
+
+    public function providerInvalidCodeParse(): iterable
+    {
+        return [
+            'noParent' => [
+                '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',
+            ],
+            'constructor' => [
+                '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',
+            ],
+            'privateMethod' => [
+                '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',
+            ],
+            'interfaceWithNoParent' => [
+                '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',
+            ],
+        ];
+    }
+}

From 8396360d30050ee2c8565c132d5e8c5391896efb Mon Sep 17 00:00:00 2001
From: Evan Shaw <evan.shaw@lightspeedhq.com>
Date: Sun, 4 Feb 2024 11:35:03 +1300
Subject: [PATCH 3/4] Emit MissingOverrideAttribute

---
 .../Analyzer/FunctionLikeAnalyzer.php         | 40 ++++++++---
 tests/OverrideTest.php                        | 69 +++++++++++++++++++
 2 files changed, 98 insertions(+), 11 deletions(-)

diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
index 964b6495148..70786016bf7 100644
--- 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/tests/OverrideTest.php b/tests/OverrideTest.php
index 1798bf7804d..17782bc9eca 100644
--- a/tests/OverrideTest.php
+++ b/tests/OverrideTest.php
@@ -2,6 +2,7 @@
 
 namespace Psalm\Tests;
 
+use Psalm\Config;
 use Psalm\Tests\Traits\InvalidCodeAnalysisTestTrait;
 use Psalm\Tests\Traits\ValidCodeAnalysisTestTrait;
 
@@ -12,9 +13,33 @@ class OverrideTest extends TestCase
     use ValidCodeAnalysisTestTrait;
     use InvalidCodeAnalysisTestTrait;
 
+    protected function makeConfig(): Config
+    {
+        $config = parent::makeConfig();
+        $config->ensure_override_attribute = true;
+        return $config;
+    }
+
     public function providerValidCodeParse(): iterable
     {
         return [
+            'constructor' => [
+                'code' => '<?php
+                    /**
+                     * @psalm-consistent-constructor
+                     */
+                    class C {
+                        public function __construct() {}
+                    }
+
+                    class C2 extends C {
+                        public function __construct() {}
+                    }
+                ',
+                'assertions' => [],
+                'ignored_issues' => [],
+                'php_version' => '8.3',
+            ],
             'overrideClass' => [
                 'code' => '<?php
                     class C {
@@ -62,6 +87,36 @@ public function f(): void {}
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
+            'classMissingAttribute' => [
+                'code' => '<?php
+                    class C {
+                        public function f(): void {}
+                    }
+
+                    class C2 extends C {
+                        public function f(): void {}
+                    }
+                ',
+                'error_message' => 'MissingOverrideAttribute - src' . DIRECTORY_SEPARATOR . 'somefile.php:7:25',
+                'error_levels' => [],
+                'php_version' => '8.3',
+            ],
+            'classUsingTrait' => [
+                'code' => '<?php
+                    trait T {
+                        abstract public function f(): void;
+                    }
+
+                    class C {
+                        use T;
+
+                        public function f(): void {}
+                    }
+                ',
+                'error_message' => 'MissingOverrideAttribute - src' . DIRECTORY_SEPARATOR . 'somefile.php:9:25',
+                'error_levels' => [],
+                'php_version' => '8.3',
+            ],
             'constructor' => [
                 'code' => '<?php
                     /**
@@ -80,6 +135,20 @@ public function __construct() {}
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
+            'interfaceMissingAttribute' => [
+                'code' => '<?php
+                    interface I {
+                        public function f(): void;
+                    }
+
+                    interface I2 extends I {
+                        public function f(): void;
+                    }
+                ',
+                'error_message' => 'MissingOverrideAttribute - src' . DIRECTORY_SEPARATOR . 'somefile.php:7:25',
+                'error_levels' => [],
+                'php_version' => '8.3',
+            ],
             'privateMethod' => [
                 'code' => '<?php
                     class C {

From 6d572a681c390bf26e183907d0389e7ea9b2c94a Mon Sep 17 00:00:00 2001
From: Bruce Weirdan <weirdan@gmail.com>
Date: Sun, 4 Feb 2024 15:41:01 -0400
Subject: [PATCH 4/4] Apply suggestions from code review

---
 tests/OverrideTest.php | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/tests/OverrideTest.php b/tests/OverrideTest.php
index 17782bc9eca..ab808a06df9 100644
--- a/tests/OverrideTest.php
+++ b/tests/OverrideTest.php
@@ -6,8 +6,6 @@
 use Psalm\Tests\Traits\InvalidCodeAnalysisTestTrait;
 use Psalm\Tests\Traits\ValidCodeAnalysisTestTrait;
 
-use const DIRECTORY_SEPARATOR;
-
 class OverrideTest extends TestCase
 {
     use ValidCodeAnalysisTestTrait;
@@ -83,7 +81,7 @@ class C {
                         public function f(): void {}
                     }
                 ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:25',
+                'error_message' => 'InvalidOverride',
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
@@ -97,7 +95,7 @@ class C2 extends C {
                         public function f(): void {}
                     }
                 ',
-                'error_message' => 'MissingOverrideAttribute - src' . DIRECTORY_SEPARATOR . 'somefile.php:7:25',
+                'error_message' => 'MissingOverrideAttribute',
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
@@ -113,7 +111,7 @@ class C {
                         public function f(): void {}
                     }
                 ',
-                'error_message' => 'MissingOverrideAttribute - src' . DIRECTORY_SEPARATOR . 'somefile.php:9:25',
+                'error_message' => 'MissingOverrideAttribute',
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
@@ -131,7 +129,7 @@ class C2 extends C {
                         public function __construct() {}
                     }
                 ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:10:25',
+                'error_message' => 'InvalidOverride',
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
@@ -145,7 +143,7 @@ interface I2 extends I {
                         public function f(): void;
                     }
                 ',
-                'error_message' => 'MissingOverrideAttribute - src' . DIRECTORY_SEPARATOR . 'somefile.php:7:25',
+                'error_message' => 'MissingOverrideAttribute',
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
@@ -160,7 +158,7 @@ class C2 extends C {
                         private function f(): void {}
                     }
                 ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:7:25',
+                'error_message' => 'InvalidOverride',
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],
@@ -171,7 +169,7 @@ interface I {
                         public function f(): void;
                     }
                 ',
-                'error_message' => 'InvalidOverride - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:25',
+                'error_message' => 'InvalidOverride',
                 'error_levels' => [],
                 'php_version' => '8.3',
             ],