From 4d9d7cebd994aa7aa10ae54445b67dbcf38ddac4 Mon Sep 17 00:00:00 2001 From: RobChett Date: Thu, 20 Apr 2023 07:14:17 +0100 Subject: [PATCH] Add support for @psalm-no-seal-properties and @psalm-no-seal-methods --- docs/annotating_code/supported_annotations.md | 26 ++++++++++++++++++- src/Psalm/DocComment.php | 1 + .../InstancePropertyAssignmentAnalyzer.php | 2 +- .../ExistingAtomicMethodCallAnalyzer.php | 4 +-- .../Call/Method/MissingMethodCallHandler.php | 2 +- .../Fetch/AtomicPropertyFetchAnalyzer.php | 2 +- src/Psalm/Internal/Codebase/Populator.php | 8 +++--- .../Reflector/ClassLikeDocblockParser.php | 16 ++++++++++++ .../Reflector/ClassLikeNodeScanner.php | 6 ----- .../Scanner/ClassLikeDocblockComment.php | 4 +-- src/Psalm/Storage/ClassLikeStorage.php | 19 +++++++++++--- tests/MagicMethodAnnotationTest.php | 25 ++++++++++++++++++ tests/MagicPropertyTest.php | 24 +++++++++++++++++ 13 files changed, 117 insertions(+), 22 deletions(-) diff --git a/docs/annotating_code/supported_annotations.md b/docs/annotating_code/supported_annotations.md index 33113329349..1195fc66532 100644 --- a/docs/annotating_code/supported_annotations.md +++ b/docs/annotating_code/supported_annotations.md @@ -157,9 +157,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 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 +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 index 3c4d09ad586..e720c9a3fc7 100644 --- 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/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php index 9435ea9ed81..127bab853f6 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php +++ b/src/Psalm/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/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php index c3f4717e30e..37dfe41be80 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php @@ -546,7 +546,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( @@ -644,7 +644,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 index 2f16c392ab4..8e9346d803d 100644 --- 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 index 0a6f7e4529e..acb87e6d634 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -702,7 +702,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 index a033401e135..022cc684e46 100644 --- 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 index 38f524b668d..043eaab2090 100644 --- 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 index 943efaa2fb1..aabf613f556 100644 --- 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 index fc0a83c91d5..43cae886202 100644 --- 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 index 45fa635d444..0f184137f9e 100644 --- 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 index 197b9fc62b1..6b7f05e32af 100644 --- 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', + '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 index 34e04f80974..27b5b36a5ea 100644 --- 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', + 'foo; + ', + ); + + $this->analyzeFile('somefile.php', new Context()); + } }