Merge pull request #9681 from robchett/no-seal-methods_and_no-seal-pr…

…opeties Add support for @psalm-no-seal-properties and @psalm-no-seal-methods
vimeo · May 2, 2023 · a5effd2 · a5effd2
2 parents cd0bacb + 4d9d7ce
commit a5effd2
Show file tree

Hide file tree

Showing 13 changed files with 117 additions and 22 deletions.
diff --git a/docs/annotating_code/supported_annotations.md b/docs/annotating_code/supported_annotations.md
@@ -202,9 +202,10 @@ takesFoo(getFoo());
 
 This provides the same, but for `false`. Psalm uses this internally for functions like `preg_replace`, which can return false if the given input has encoding errors, but where 99.9% of the time the function operates as expected.
 
-### `@psalm-seal-properties`
+### `@psalm-seal-properties`, `@psalm-no-seal-properties`
 
 If you have a magic property getter/setter, you can use `@psalm-seal-properties` to instruct Psalm to disallow getting and setting any properties not contained in a list of `@property` (or `@property-read`/`@property-write`) annotations.
+This is automatically enabled with the configuration option `sealAllProperties` and can be disabled for a class with `@psalm-no-seal-properties`
 
 ```php
 <?php
@@ -226,6 +227,29 @@ $a = new A();
 $a->bar = 5; // this call fails
 ```
 
+### `@psalm-seal-methods`, `@psalm-no-seal-methods`
+
+If you have a magic method caller, you can use `@psalm-seal-methods` to instruct Psalm to disallow calling any methods not contained in a list of `@method` annotations.
+This is automatically enabled with the configuration option `sealAllMethods` and can be disabled for a class with `@psalm-no-seal-methods`
+
+```php
+<?php
+/**
+ * @method foo(): string
+ * @psalm-seal-methods
+ */
+class A {
+     public function __call(string $name, array $args) {
+          if ($name === "foo") {
+               return "hello";
+          }
+     }
+ }
+
+$a = new A();
+$b = $a->bar(); // this call fails
+```
+
 ### `@psalm-internal`
 
 Used to mark a class, property or function as internal to a given namespace. Psalm treats this slightly differently to

diff --git a/src/Psalm/DocComment.php b/src/Psalm/DocComment.php
@@ -24,6 +24,7 @@ final class DocComment
         'assert', 'assert-if-true', 'assert-if-false', 'suppress',
         'ignore-nullable-return', 'override-property-visibility',
         'override-method-visibility', 'seal-properties', 'seal-methods',
+        'no-seal-properties', 'no-seal-methods',
         'ignore-falsable-return', 'variadic', 'pure',
         'ignore-variable-method', 'ignore-variable-property', 'internal',
         'taint-sink', 'taint-source', 'assert-untainted', 'scope-this',

diff --git a/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php
@@ -1087,7 +1087,7 @@ private static function analyzeAtomicAssignment(
              * If we have an explicit list of all allowed magic properties on the class, and we're
              * not in that list, fall through
              */
-            if (!$var_id || !$class_storage->sealed_properties) {
+            if (!$var_id || !$class_storage->hasSealedProperties($codebase->config)) {
                 if (!$context->collect_initializations && !$context->collect_mutations) {
                     self::taintProperty(
                         $statements_analyzer,

diff --git a/.../Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php b/.../Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php
@@ -570,7 +570,7 @@ private static function getMagicGetterOrSetterProperty(
             case '__set':
                 // If `@psalm-seal-properties` is set, the property must be defined with
                 // a `@property` annotation
-                if (($class_storage->sealed_properties || $codebase->config->seal_all_properties)
+                if (($class_storage->hasSealedProperties($codebase->config))
                     && !isset($class_storage->pseudo_property_set_types['$' . $prop_name])
                 ) {
                     IssueBuffer::maybeAdd(
@@ -668,7 +668,7 @@ private static function getMagicGetterOrSetterProperty(
             case '__get':
                 // If `@psalm-seal-properties` is set, the property must be defined with
                 // a `@property` annotation
-                if (($class_storage->sealed_properties || $codebase->config->seal_all_properties)
+                if (($class_storage->hasSealedProperties($codebase->config))
                     && !isset($class_storage->pseudo_property_get_types['$' . $prop_name])
                 ) {
                     IssueBuffer::maybeAdd(

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MissingMethodCallHandler.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MissingMethodCallHandler.php
@@ -190,7 +190,7 @@ public static function handleMagicMethod(
                 $context,
             );
 
-            if ($class_storage->sealed_methods || $config->seal_all_methods) {
+            if ($class_storage->hasSealedMethods($config)) {
                 $result->non_existent_magic_method_ids[] = $method_id->__toString();
 
                 return null;

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php
@@ -713,7 +713,7 @@ private static function propertyFetchCanBeAnalyzed(
              * If we have an explicit list of all allowed magic properties on the class, and we're
              * not in that list, fall through
              */
-            if (!($class_storage->sealed_properties || $codebase->config->seal_all_properties)
+            if (!($class_storage->hasSealedProperties($codebase->config))
                 && !$override_property_visibility
             ) {
                 return false;

diff --git a/src/Psalm/Internal/Codebase/Populator.php b/src/Psalm/Internal/Codebase/Populator.php
@@ -920,8 +920,8 @@ protected function inheritMethodsFromParent(
         $fq_class_name = $storage->name;
         $fq_class_name_lc = strtolower($fq_class_name);
 
-        if ($parent_storage->sealed_methods) {
-            $storage->sealed_methods = true;
+        if ($parent_storage->sealed_methods !== null) {
+            $storage->sealed_methods = $parent_storage->sealed_methods;
         }
 
         // register where they appear (can never be in a trait)
@@ -1032,8 +1032,8 @@ private function inheritPropertiesFromParent(
         ClassLikeStorage $storage,
         ClassLikeStorage $parent_storage
     ): void {
-        if ($parent_storage->sealed_properties) {
-            $storage->sealed_properties = true;
+        if ($parent_storage->sealed_properties !== null) {
+            $storage->sealed_properties = $parent_storage->sealed_properties;
         }
 
         // register where they appear (can never be in a trait)

diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeDocblockParser.php b/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeDocblockParser.php
@@ -241,10 +241,16 @@ public static function parse(
         if (isset($parsed_docblock->tags['psalm-seal-properties'])) {
             $info->sealed_properties = true;
         }
+        if (isset($parsed_docblock->tags['psalm-no-seal-properties'])) {
+            $info->sealed_properties = false;
+        }
 
         if (isset($parsed_docblock->tags['psalm-seal-methods'])) {
             $info->sealed_methods = true;
         }
+        if (isset($parsed_docblock->tags['psalm-no-seal-methods'])) {
+            $info->sealed_methods = false;
+        }
 
         if (isset($parsed_docblock->tags['psalm-immutable'])
             || isset($parsed_docblock->tags['psalm-mutation-free'])
@@ -296,6 +302,9 @@ public static function parse(
         }
 
         if (isset($parsed_docblock->combined_tags['method'])) {
+            if ($info->sealed_methods === null) {
+                $info->sealed_methods = true;
+            }
             foreach ($parsed_docblock->combined_tags['method'] as $offset => $method_entry) {
                 $method_entry = preg_replace('/[ \t]+/', ' ', trim($method_entry));
 
@@ -481,6 +490,13 @@ public static function parse(
 
         $info->public_api = isset($parsed_docblock->tags['psalm-api']) || isset($parsed_docblock->tags['api']);
 
+        if (isset($parsed_docblock->tags['property'])
+            && $codebase->config->docblock_property_types_seal_properties
+            && $info->sealed_properties === null
+        ) {
+            $info->sealed_properties = true;
+        }
+
         self::addMagicPropertyToInfo($comment, $info, $parsed_docblock->tags, 'property');
         self::addMagicPropertyToInfo($comment, $info, $parsed_docblock->tags, 'psalm-property');
         self::addMagicPropertyToInfo($comment, $info, $parsed_docblock->tags, 'property-read');

diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php b/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php
@@ -577,10 +577,6 @@ public function start(PhpParser\Node\Stmt\ClassLike $node): ?bool
                         );
                     }
                 }
-
-                if ($this->config->docblock_property_types_seal_properties) {
-                    $storage->sealed_properties = true;
-                }
             }
 
             foreach ($docblock_info->methods as $method) {
@@ -607,8 +603,6 @@ public function start(PhpParser\Node\Stmt\ClassLike $node): ?bool
                         $lc_method_name,
                     );
                 }
-
-                $storage->sealed_methods = true;
             }
 
 

diff --git a/src/Psalm/Internal/Scanner/ClassLikeDocblockComment.php b/src/Psalm/Internal/Scanner/ClassLikeDocblockComment.php
@@ -63,9 +63,9 @@ class ClassLikeDocblockComment
      */
     public array $methods = [];
 
-    public bool $sealed_properties = false;
+    public ?bool $sealed_properties = null;
 
-    public bool $sealed_methods = false;
+    public ?bool $sealed_methods = null;
 
     public bool $override_property_visibility = false;
 

diff --git a/src/Psalm/Storage/ClassLikeStorage.php b/src/Psalm/Storage/ClassLikeStorage.php
@@ -4,6 +4,7 @@
 
 use Psalm\Aliases;
 use Psalm\CodeLocation;
+use Psalm\Config;
 use Psalm\Internal\Analyzer\ClassLikeAnalyzer;
 use Psalm\Internal\MethodIdentifier;
 use Psalm\Internal\Type\TypeAlias\ClassTypeAlias;
@@ -66,14 +67,14 @@ final class ClassLikeStorage implements HasAttributesInterface
     public $mixin_declaring_fqcln;
 
     /**
-     * @var bool
+     * @var ?bool
      */
-    public $sealed_properties = false;
+    public $sealed_properties = null;
 
     /**
-     * @var bool
+     * @var ?bool
      */
-    public $sealed_methods = false;
+    public $sealed_methods = null;
 
     /**
      * @var bool
@@ -500,4 +501,14 @@ public function getClassTemplateTypes(): array
 
         return $type_params;
     }
+
+    public function hasSealedProperties(Config $config): bool
+    {
+        return $this->sealed_properties ?? $config->seal_all_properties;
+    }
+
+    public function hasSealedMethods(Config $config): bool
+    {
+        return $this->sealed_methods ?? $config->seal_all_methods;
+    }
 }
diff --git a/tests/MagicMethodAnnotationTest.php b/tests/MagicMethodAnnotationTest.php
@@ -1174,6 +1174,31 @@ class B extends A {}
         $this->analyzeFile('somefile.php', new Context());
     }
 
+    public function testNoSealAllMethods(): void
+    {
+        Config::getInstance()->seal_all_methods = true;
+
+        $this->addFile(
+            'somefile.php',
+            '<?php
+              /** @psalm-no-seal-properties */
+              class A {
+                public function __call(string $method, array $args) {}
+              }
+
+              class B extends A {}
+
+              $b = new B();
+              $b->foo();
+              ',
+        );
+
+        $error_message = 'UndefinedMagicMethod';
+        $this->expectException(CodeException::class);
+        $this->expectExceptionMessage($error_message);
+        $this->analyzeFile('somefile.php', new Context());
+    }
+
     public function testSealAllMethodsWithFoo(): void
     {
         Config::getInstance()->seal_all_methods = true;

diff --git a/tests/MagicPropertyTest.php b/tests/MagicPropertyTest.php
@@ -1213,4 +1213,28 @@ class B extends A {}
         $this->expectExceptionMessage($error_message);
         $this->analyzeFile('somefile.php', new Context());
     }
+
+    public function testNoSealAllProperties(): void
+    {
+        Config::getInstance()->seal_all_properties = true;
+        Config::getInstance()->seal_all_methods = true;
+
+        $this->addFile(
+            'somefile.php',
+            '<?php
+              /** @psalm-no-seal-properties */
+              class A {
+                public function __get(string $name) {}
+              }
+
+              class B extends A {}
+
+              $b = new B();
+              /** @var string $result */
+              $result = $b->foo;
+              ',
+        );
+
+        $this->analyzeFile('somefile.php', new Context());
+    }
 }