More class const improvements.

Add InvalidClassConstType issue as alternative to LessSpecificClassConstType when type isn't contravariant. Handle final class consts (#6395). Use double quotes for types in class const issues.
vimeo · Feb 13, 2022 · cc2334f · cc2334f
1 parent b796f45
commit cc2334f
Show file tree

Hide file tree

Showing 11 changed files with 195 additions and 27 deletions.
diff --git a/config.xsd b/config.xsd
@@ -249,6 +249,7 @@
             <xs:element name="InvalidCast" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="InvalidCatch" type="ClassIssueHandlerType" minOccurs="0" />
             <xs:element name="InvalidClass" type="ClassIssueHandlerType" minOccurs="0" />
+            <xs:element name="InvalidClassConstantType" type="ClassConstantIssueHandlerType" minOccurs="0" />
             <xs:element name="InvalidClone" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="InvalidConstantAssignmentValue" type="ClassConstantIssueHandlerType" minOccurs="0" />
             <xs:element name="InvalidDocblock" type="IssueHandlerType" minOccurs="0" />
@@ -343,6 +344,7 @@
             <xs:element name="NullPropertyAssignment" type="PropertyIssueHandlerType" minOccurs="0" />
             <xs:element name="NullPropertyFetch" type="PropertyIssueHandlerType" minOccurs="0" />
             <xs:element name="NullReference" type="IssueHandlerType" minOccurs="0" />
+            <xs:element name="OverriddenFinalConstant" type="ClassConstantIssueHandlerType" minOccurs="0" />
             <xs:element name="OverriddenInterfaceConstant" type="ClassConstantIssueHandlerType" minOccurs="0" />
             <xs:element name="OverriddenMethodAccess" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="OverriddenPropertyAccess" type="PropertyIssueHandlerType" minOccurs="0" />

diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md
@@ -61,6 +61,7 @@
  - [InvalidCast](issues/InvalidCast.md)
  - [InvalidCatch](issues/InvalidCatch.md)
  - [InvalidClass](issues/InvalidClass.md)
+ - [InvalidClassConstantType](issues/InvalidClassConstantType.md)
  - [InvalidClone](issues/InvalidClone.md)
  - [InvalidConstantAssignmentValue](issues/InvalidConstantAssignmentValue.md)
  - [InvalidDocblock](issues/InvalidDocblock.md)
@@ -155,6 +156,7 @@
  - [NullPropertyAssignment](issues/NullPropertyAssignment.md)
  - [NullPropertyFetch](issues/NullPropertyFetch.md)
  - [NullReference](issues/NullReference.md)
+ - [OverriddenFinalConstant](issues/OverriddenFinalConstant.md)
  - [OverriddenInterfaceConstant](issues/OverriddenInterfaceConstant.md)
  - [OverriddenMethodAccess](issues/OverriddenMethodAccess.md)
  - [OverriddenPropertyAccess](issues/OverriddenPropertyAccess.md)

diff --git a/docs/running_psalm/issues/InvalidClassConstantType.md b/docs/running_psalm/issues/InvalidClassConstantType.md
@@ -0,0 +1,28 @@
+# InvalidClassConstantType
+
+Emitted when a constant type in a child does not satisfy the type in the parent.
+
+```php
+<?php
+
+class Foo
+{
+    /** @var int<1,max> */
+    public const CONSTANT = 3;
+
+    public static function bar(): array
+    {
+        return str_split("foobar", static::CONSTANT);
+    }
+}
+
+class Bar extends Foo
+{
+    /** @var int<min,-1> */
+    public const CONSTANT = -1;
+}
+
+Bar::bar(); // Error: str_split argument 2 must be greater than 0
+```
+
+This issue will always show up when overriding a constant that doesn't have a docblock type. Psalm will infer the most specific type for the constant that it can, you have to add a type annotation to tell it what type constraint you wish to be applied. Otherwise Psalm has no way of telling if you mean for the constant to be a literal `1`, `int<1, max>`, `int`, `numeric`, etc.
diff --git a/docs/running_psalm/issues/OverriddenFinalConstant.md b/docs/running_psalm/issues/OverriddenFinalConstant.md
@@ -0,0 +1,19 @@
+# OverriddenFinalConstant
+
+Emitted when a constant declared as final is overridden in a child class or interface.
+
+```php
+<?php
+
+class Foo
+{
+    /** @var string */
+    final public const BAR='baz';
+}
+
+class Bar extends Foo
+{
+    /** @var string */
+    public const BAR='foobar';
+}
+```
diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/ClassConstAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/ClassConstAnalyzer.php
@@ -23,11 +23,14 @@
 use Psalm\Issue\DeprecatedConstant;
 use Psalm\Issue\InaccessibleClassConstant;
 use Psalm\Issue\InternalClass;
+use Psalm\Issue\InvalidClassConstantType;
 use Psalm\Issue\InvalidConstantAssignmentValue;
 use Psalm\Issue\LessSpecificClassConstantType;
 use Psalm\Issue\NonStaticSelfCall;
+use Psalm\Issue\OverriddenFinalConstant;
 use Psalm\Issue\OverriddenInterfaceConstant;
 use Psalm\Issue\ParentNotFound;
+use Psalm\Issue\ParseError;
 use Psalm\Issue\UndefinedConstant;
 use Psalm\IssueBuffer;
 use Psalm\Storage\ClassConstantStorage;
@@ -721,38 +724,89 @@ public static function analyze(
         Codebase $codebase
     ): void {
         foreach ($class_storage->constants as $const_name => $const_storage) {
-            // Check covariance
             [$parent_classlike_storage, $parent_const_storage] = self::getOverriddenConstant(
                 $class_storage,
                 $const_storage,
                 $const_name,
                 $codebase
             );
+
+            $type_location = $const_storage->location ?? $const_storage->stmt_location;
+            if ($type_location === null) {
+                continue;
+            }
+
             if ($parent_const_storage !== null) {
                 assert($parent_classlike_storage !== null);
-                $location = $const_storage->type_location ?? $const_storage->stmt_location;
-                if ($location !== null
-                    && $const_storage->type !== null
+
+                // Check covariance
+                if ($const_storage->type !== null
                     && $parent_const_storage->type !== null
                     && !UnionTypeComparator::isContainedBy(
                         $codebase,
                         $const_storage->type,
                         $parent_const_storage->type
                     )
                 ) {
+                    if (UnionTypeComparator::isContainedBy(
+                        $codebase,
+                        $parent_const_storage->type,
+                        $const_storage->type,
+                    )) {
+                        // Contravariant
+                        IssueBuffer::maybeAdd(
+                            new LessSpecificClassConstantType(
+                                "The type \"{$const_storage->type->getId()}\" for {$class_storage->name}::"
+                                    . "{$const_name} is more general than the type "
+                                    . "\"{$parent_const_storage->type->getId()}\" inherited from "
+                                    . "{$parent_classlike_storage->name}::{$const_name}",
+                                $type_location,
+                                "{$class_storage->name}::{$const_name}"
+                            ),
+                            $const_storage->suppressed_issues
+                        );
+                    } else {
+                        // Completely different
+                        IssueBuffer::maybeAdd(
+                            new InvalidClassConstantType(
+                                "The type \"{$const_storage->type->getId()}\" for {$class_storage->name}::"
+                                    . "{$const_name} does not satisfy the type "
+                                    . "\"{$parent_const_storage->type->getId()}\" inherited from "
+                                    . "{$parent_classlike_storage->name}::{$const_name}",
+                                $type_location,
+                                "{$class_storage->name}::{$const_name}"
+                            ),
+                            $const_storage->suppressed_issues
+                        );
+                    }
+                }
+
+                // Check overridden final
+                if ($parent_const_storage->final) {
                     IssueBuffer::maybeAdd(
-                        new LessSpecificClassConstantType(
-                            "The type '{$const_storage->type->getId()}' for {$class_storage->name}::"
-                                . "{$const_name} is more general than the type "
-                                . "'{$parent_const_storage->type->getId()}' inherited from "
-                                . "{$parent_classlike_storage->name}::{$const_name}",
-                            $location,
+                        new OverriddenFinalConstant(
+                            "{$const_name} cannot be overridden because it is marked as final in "
+                                . $parent_classlike_storage->name,
+                            $type_location,
                             "{$class_storage->name}::{$const_name}"
                         ),
                         $const_storage->suppressed_issues
                     );
                 }
             }
+
+            if ($const_storage->stmt_location !== null) {
+                // Check final in PHP < 8.1
+                if ($codebase->analysis_php_version_id < 8_01_00 && $const_storage->final) {
+                    IssueBuffer::maybeAdd(
+                        new ParseError(
+                            "Class constants cannot be marked final before PHP 8.1",
+                            $const_storage->stmt_location,
+                        ),
+                        $const_storage->suppressed_issues
+                    );
+                }
+            }
         }
     }
 

diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php b/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php
@@ -989,7 +989,7 @@ private function extendTemplatedType(
             $extended_type_parameters = [];
 
             $storage->template_type_extends_count[$atomic_type->value] = count($atomic_type->type_params);
-            
+
             foreach ($atomic_type->type_params as $type_param) {
                 $extended_type_parameters[] = $type_param;
             }
@@ -1340,6 +1340,7 @@ private function visitClassConstDeclaration(
             }
 
             $constant_storage->description = $description;
+            $constant_storage->final = $stmt->isFinal();
 
             foreach ($stmt->attrGroups as $attr_group) {
                 foreach ($attr_group->attrs as $attr) {

diff --git a/src/Psalm/Issue/InvalidClassConstantType.php b/src/Psalm/Issue/InvalidClassConstantType.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace Psalm\Issue;
+
+final class InvalidClassConstantType extends ClassConstantIssue
+{
+    public const ERROR_LEVEL = 1;
+    public const SHORTCODE = 309;
+}
diff --git a/src/Psalm/Issue/OverriddenFinalConstant.php b/src/Psalm/Issue/OverriddenFinalConstant.php
@@ -0,0 +1,9 @@
+<?php
+
+namespace Psalm\Issue;
+
+final class OverriddenFinalConstant extends ClassConstantIssue
+{
+    public const ERROR_LEVEL = 6;
+    public const SHORTCODE = 310;
+}
diff --git a/src/Psalm/Storage/ClassConstantStorage.php b/src/Psalm/Storage/ClassConstantStorage.php
@@ -58,6 +58,11 @@ final class ClassConstantStorage
      */
     public $deprecated = false;
 
+    /**
+     * @var bool
+     */
+    public $final = false;
+
     /**
      * @var list<AttributeStorage>
      */

diff --git a/tests/ConstantTest.php b/tests/ConstantTest.php
@@ -1879,10 +1879,58 @@ class Baz implements Foo, Bar
                         public const BAR="";
                     }
                 ',
-                'error_message' => "LessSpecificClassConstantType",
+                'error_message' => "InvalidClassConstantType",
                 'ignored_issues' => [],
                 'php_version' => '8.1',
             ],
+            'overrideFinalClassConstFromExtendedClass' => [
+                'code' => '<?php
+                    class Foo
+                    {
+                        /** @var string */
+                        final public const BAR="baz";
+                    }
+
+                    class Baz extends Foo
+                    {
+                        /** @var string */
+                        public const BAR="foobar";
+                    }
+                ',
+                'error_message' => "OverriddenFinalConstant",
+                'ignored_issues' => [],
+                'php_version' => '8.1',
+            ],
+            'overrideFinalClassConstFromImplementedInterface' => [
+                'code' => '<?php
+                    interface Foo
+                    {
+                        /** @var string */
+                        final public const BAR="baz";
+                    }
+
+                    class Baz implements Foo
+                    {
+                        /** @var string */
+                        public const BAR="foobar";
+                    }
+                ',
+                'error_message' => "OverriddenFinalConstant",
+                'ignored_issues' => [],
+                'php_version' => '8.1',
+            ],
+            'finalConstantIsIllegalBefore8.1' => [
+                'code' => '<?php
+                    class Foo
+                    {
+                        /** @var string */
+                        final public const BAR="baz";
+                    }
+                ',
+                'error_message' => 'ParseError - src' . DIRECTORY_SEPARATOR . 'somefile.php:5:44',
+                'ignored_issues' => [],
+                'php_version' => '8.0',
+            ],
         ];
     }
 }
diff --git a/tests/DocumentationTest.php b/tests/DocumentationTest.php
@@ -268,22 +268,12 @@ public function providerInvalidCodeParse(): array
             $php_version = '8.0';
             $ignored_issues = [];
             switch ($issue_name) {
-                case 'MissingThrowsDocblock':
-                    continue 2;
-
-                case 'UncaughtThrowInGlobalScope':
-                    continue 2;
-
                 case 'InvalidStringClass':
-                    continue 2;
-
+                case 'MissingThrowsDocblock':
                 case 'PluginClass':
-                    continue 2;
-
                 case 'RedundantIdentityWithTrue':
-                    continue 2;
-
                 case 'TraitMethodSignatureMismatch':
+                case 'UncaughtThrowInGlobalScope':
                     continue 2;
 
                 /** @todo reinstate this test when the issue is restored */
@@ -320,12 +310,13 @@ public function providerInvalidCodeParse(): array
                     break;
 
                 case 'AmbiguousConstantInheritance':
-                case 'InvalidEnumBackingType':
-                case 'InvalidEnumCaseValue':
+                case 'DeprecatedConstant':
                 case 'DuplicateEnumCase':
                 case 'DuplicateEnumCaseValue':
+                case 'InvalidEnumBackingType':
+                case 'InvalidEnumCaseValue':
                 case 'NoEnumProperties':
-                case 'DeprecatedConstant':
+                case 'OverriddenFinalConstant':
                     $php_version = '8.1';
                     break;
             }