From b530f23acc779d175195be31078577f604aabd74 Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Thu, 30 Dec 2021 11:23:04 +0100 Subject: [PATCH 1/4] PHP 8.2: seal all properties by default, add configuration key for lower versions --- config.xsd | 1 + docs/running_psalm/configuration.md | 10 ++++++++ src/Psalm/Codebase.php | 10 ++++++++ src/Psalm/Config.php | 6 +++++ .../ExistingAtomicMethodCallAnalyzer.php | 4 +-- .../Fetch/AtomicPropertyFetchAnalyzer.php | 2 +- tests/MagicPropertyTest.php | 25 +++++++++++++++++++ 7 files changed, 55 insertions(+), 3 deletions(-) diff --git a/config.xsd b/config.xsd index 9f8676ce823..c119abdda45 100644 --- a/config.xsd +++ b/config.xsd @@ -95,6 +95,7 @@ + diff --git a/docs/running_psalm/configuration.md b/docs/running_psalm/configuration.md index 6a84f20742e..21a253e0be0 100644 --- a/docs/running_psalm/configuration.md +++ b/docs/running_psalm/configuration.md @@ -305,6 +305,16 @@ This defaults to `false`. When `true`, Psalm will treat all classes as if they had sealed methods, meaning that if you implement the magic method `__call`, you also have to add `@method` for each magic method. Defaults to false. +#### sealAllProperties + +```xml + +``` + +When `true`, Psalm will treat all classes as if they had sealed properties, meaning that Psalm will disallow getting and setting any properties not contained in a list of `@property` (or `@property-read`/`@property-write`) annotations and not explicitly defined as a `property`. Defaults to false. + #### runTaintAnalysis ```xml diff --git a/src/Psalm/Codebase.php b/src/Psalm/Codebase.php index cb72efa8b76..d0c1c38acf8 100644 --- a/src/Psalm/Codebase.php +++ b/src/Psalm/Codebase.php @@ -1896,6 +1896,16 @@ public function isTypeContainedByType( return UnionTypeComparator::isContainedBy($this, $input_type, $container_type); } + /** + * Returns whether all properties of a class should be sealed. + */ + public function shouldSealAllProperties(ClassLikeStorage $storage): bool + { + return $storage->sealed_properties + || $this->config->seal_all_properties + || ($this->php_major_version >= 8 && $this->php_minor_version >= 2); + } + /** * Checks if type has any part that is a subtype of other * diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index db8f8f06df1..a8310c21120 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -346,6 +346,11 @@ class Config */ public $seal_all_methods = false; + /** + * @var bool + */ + public $seal_all_properties = false; + /** * @var bool */ @@ -907,6 +912,7 @@ private static function fromXmlAndPaths( 'reportMixedIssues' => 'show_mixed_issues', 'skipChecksOnUnresolvableIncludes' => 'skip_checks_on_unresolvable_includes', 'sealAllMethods' => 'seal_all_methods', + 'sealAllProperties' => 'seal_all_properties', 'runTaintAnalysis' => 'run_taint_analysis', 'usePhpStormMetaPath' => 'use_phpstorm_meta_path', 'allowInternalNamedArgumentsCalls' => 'allow_internal_named_arg_calls', 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 1fda720d0c2..4c67d452d83 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php @@ -548,7 +548,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 + if ($codebase->shouldSealAllProperties($class_storage) && !isset($class_storage->pseudo_property_set_types['$' . $prop_name]) && IssueBuffer::accepts( new UndefinedThisPropertyAssignment( @@ -647,7 +647,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 + if ($codebase->shouldSealAllProperties($class_storage) && !isset($class_storage->pseudo_property_get_types['$' . $prop_name]) && IssueBuffer::accepts( new UndefinedThisPropertyFetch( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index ee9f642c186..81b80f6e295 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -676,7 +676,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 && !$override_property_visibility) { + if (!$codebase->shouldSealAllProperties($class_storage) && !$override_property_visibility) { return false; } diff --git a/tests/MagicPropertyTest.php b/tests/MagicPropertyTest.php index 62d0dd9773b..f142bb1bd65 100644 --- a/tests/MagicPropertyTest.php +++ b/tests/MagicPropertyTest.php @@ -4,6 +4,7 @@ use Psalm\Config; use Psalm\Context; +use Psalm\Exception\CodeException; use Psalm\Tests\Traits\InvalidCodeAnalysisTestTrait; use Psalm\Tests\Traits\ValidCodeAnalysisTestTrait; @@ -1159,4 +1160,28 @@ class A { ], ]; } + + public function testSealAllMethodsWithoutFoo(): void + { + Config::getInstance()->seal_all_properties = true; + + $this->addFile( + 'somefile.php', + 'foo; + ' + ); + + $error_message = 'UndefinedMagicPropertyFetch'; + $this->expectException(CodeException::class); + $this->expectExceptionMessage($error_message); + $this->analyzeFile('somefile.php', new Context()); + } } From 4445612ebb9d9b81abfa1ef4cfa2bc2b4878d6e0 Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Thu, 30 Dec 2021 11:26:31 +0100 Subject: [PATCH 2/4] Improve docs --- docs/running_psalm/configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/running_psalm/configuration.md b/docs/running_psalm/configuration.md index 21a253e0be0..dee384a0e3e 100644 --- a/docs/running_psalm/configuration.md +++ b/docs/running_psalm/configuration.md @@ -313,7 +313,7 @@ When `true`, Psalm will treat all classes as if they had sealed methods, meaning > ``` -When `true`, Psalm will treat all classes as if they had sealed properties, meaning that Psalm will disallow getting and setting any properties not contained in a list of `@property` (or `@property-read`/`@property-write`) annotations and not explicitly defined as a `property`. Defaults to false. +When `true`, Psalm will treat all classes as if they had sealed properties, meaning that Psalm will disallow getting and setting any properties not contained in a list of `@property` (or `@property-read`/`@property-write`) annotations and not explicitly defined as a `property`. Defaults to true if the [phpVersion][#phpVersion] is >= 8.2, false otherwise. #### runTaintAnalysis From a35ccfb0cbfdd947b58a310d43ddeab55a421e05 Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Mon, 10 Jan 2022 10:19:12 +0100 Subject: [PATCH 3/4] Revert misguided changes --- docs/running_psalm/configuration.md | 2 +- src/Psalm/Codebase.php | 10 ---------- .../Call/Method/ExistingAtomicMethodCallAnalyzer.php | 4 ++-- .../Expression/Fetch/AtomicPropertyFetchAnalyzer.php | 2 +- 4 files changed, 4 insertions(+), 14 deletions(-) diff --git a/docs/running_psalm/configuration.md b/docs/running_psalm/configuration.md index dee384a0e3e..21a253e0be0 100644 --- a/docs/running_psalm/configuration.md +++ b/docs/running_psalm/configuration.md @@ -313,7 +313,7 @@ When `true`, Psalm will treat all classes as if they had sealed methods, meaning > ``` -When `true`, Psalm will treat all classes as if they had sealed properties, meaning that Psalm will disallow getting and setting any properties not contained in a list of `@property` (or `@property-read`/`@property-write`) annotations and not explicitly defined as a `property`. Defaults to true if the [phpVersion][#phpVersion] is >= 8.2, false otherwise. +When `true`, Psalm will treat all classes as if they had sealed properties, meaning that Psalm will disallow getting and setting any properties not contained in a list of `@property` (or `@property-read`/`@property-write`) annotations and not explicitly defined as a `property`. Defaults to false. #### runTaintAnalysis diff --git a/src/Psalm/Codebase.php b/src/Psalm/Codebase.php index d0c1c38acf8..cb72efa8b76 100644 --- a/src/Psalm/Codebase.php +++ b/src/Psalm/Codebase.php @@ -1896,16 +1896,6 @@ public function isTypeContainedByType( return UnionTypeComparator::isContainedBy($this, $input_type, $container_type); } - /** - * Returns whether all properties of a class should be sealed. - */ - public function shouldSealAllProperties(ClassLikeStorage $storage): bool - { - return $storage->sealed_properties - || $this->config->seal_all_properties - || ($this->php_major_version >= 8 && $this->php_minor_version >= 2); - } - /** * Checks if type has any part that is a subtype of other * 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 4c67d452d83..26faf4e593e 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php @@ -548,7 +548,7 @@ private static function getMagicGetterOrSetterProperty( case '__set': // If `@psalm-seal-properties` is set, the property must be defined with // a `@property` annotation - if ($codebase->shouldSealAllProperties($class_storage) + if (($class_storage->sealed_properties || $codebase->config->seal_all_properties) && !isset($class_storage->pseudo_property_set_types['$' . $prop_name]) && IssueBuffer::accepts( new UndefinedThisPropertyAssignment( @@ -647,7 +647,7 @@ private static function getMagicGetterOrSetterProperty( case '__get': // If `@psalm-seal-properties` is set, the property must be defined with // a `@property` annotation - if ($codebase->shouldSealAllProperties($class_storage) + if (($class_storage->sealed_properties || $codebase->config->seal_all_properties) && !isset($class_storage->pseudo_property_get_types['$' . $prop_name]) && IssueBuffer::accepts( new UndefinedThisPropertyFetch( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index 81b80f6e295..453d8f556f0 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -676,7 +676,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 (!$codebase->shouldSealAllProperties($class_storage) && !$override_property_visibility) { + if (!($class_storage->sealed_properties || $codebase->config->seal_all_properties) && !$override_property_visibility) { return false; } From b9e3979c3f285ed1c25b72b5490dea53f7107c30 Mon Sep 17 00:00:00 2001 From: Daniil Gentili Date: Mon, 10 Jan 2022 10:22:51 +0100 Subject: [PATCH 4/4] Cs-fix --- .../Expression/Fetch/AtomicPropertyFetchAnalyzer.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index 453d8f556f0..b0961eec718 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -676,7 +676,9 @@ 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) && !$override_property_visibility) { + if (!($class_storage->sealed_properties || $codebase->config->seal_all_properties) + && !$override_property_visibility + ) { return false; }