diff --git a/docs/running_psalm/issues/InvalidOverride.md b/docs/running_psalm/issues/InvalidOverride.md index b9595437706..1f603d5789d 100644 --- a/docs/running_psalm/issues/InvalidOverride.md +++ b/docs/running_psalm/issues/InvalidOverride.md @@ -19,6 +19,21 @@ class B extends A { } ``` +On PHP 8.5 and above, the same applies to a property carrying the attribute that does not override a parent (or implemented interface) property. + +```php +suppressed_issues + $this->getSuppressedIssues(), ); + $has_override_attribute = false; + foreach ($property_storage->attributes as $attribute_storage) { + if ($attribute_storage->fq_class_name === 'Override') { + $has_override_attribute = true; + break; + } + } + + PropertyOverrideAnalyzer::analyze( + $source, + $codebase, + $class_storage, + $property_name, + $property_storage, + $has_override_attribute, + // A multi-property statement (`public int $a, $b;`) shares one attribute group, so adding + // `#[\Override]` would apply it to every property and could break the ones that do not + // override. The diagnostic still fires; only the auto-fix is withheld. + $source === $this && count($stmt->props) === 1, + ); + if ($class_property_type && ($property_storage->type_location || !$codebase->alter_code)) { return; } diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php index b113496adba..ba061f957b8 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -2055,6 +2055,42 @@ private function getFunctionInformation( } } + // PHP 8.5 allows `#[\Override]` on constructor-promoted properties, so mirror the method + // checks above for each promoted parameter. Skipped on the initialization and mutation + // passes so the diagnostics and the fixer run exactly once. + if ($storage->cased_name === '__construct' + && !$context->collect_initializations + && !$context->collect_mutations + && ($storage->defining_fqcln === null + || !$codebase->classlike_storage_provider->get($storage->defining_fqcln)->is_trait) + ) { + foreach ($storage->params as $param) { + if (!$param->promoted_property + || !isset($appearing_class_storage->properties[$param->name]) + ) { + continue; + } + + $has_promoted_override_attribute = false; + foreach ($param->attributes as $attribute_storage) { + if ($attribute_storage->fq_class_name === 'Override') { + $has_promoted_override_attribute = true; + break; + } + } + + PropertyOverrideAnalyzer::analyze( + $this, + $codebase, + $appearing_class_storage, + $param->name, + $appearing_class_storage->properties[$param->name], + $has_promoted_override_attribute, + true, + ); + } + } + if ($overridden_method_ids && !$context->collect_initializations && !$context->collect_mutations diff --git a/src/Psalm/Internal/Analyzer/PropertyOverrideAnalyzer.php b/src/Psalm/Internal/Analyzer/PropertyOverrideAnalyzer.php new file mode 100644 index 00000000000..f1a4d245d17 --- /dev/null +++ b/src/Psalm/Internal/Analyzer/PropertyOverrideAnalyzer.php @@ -0,0 +1,141 @@ +analysis_php_version_id < 8_05_00) { + return; + } + + // A trait property is intentionally not validated here: the attribute physically lives on the + // shared trait, so there is no single using-class context to check it against (and no safe + // place to apply the fix). This matches how Psalm already leaves trait methods unchecked. + if ($class_storage->is_trait) { + return; + } + + $location = $property_storage->location; + + if ($location === null) { + return; + } + + $property_id = $class_storage->name . '::$' . $property_name; + + $overrides_parent = self::overridesParentProperty($codebase, $class_storage, $property_name); + + $suppressed_issues = $source->getSuppressedIssues() + $property_storage->suppressed_issues; + + if ($has_override_attribute) { + if (!$overrides_parent) { + IssueBuffer::maybeAdd( + new InvalidOverride( + 'Property ' . $property_id . ' does not match any parent property', + $location, + ), + $suppressed_issues, + ); + } + + return; + } + + if (!$codebase->config->ensure_override_attribute || !$overrides_parent) { + return; + } + + IssueBuffer::maybeAdd( + new MissingOverrideAttribute( + 'Property ' . $property_id . ' should have the "Override" attribute', + $location, + ), + $suppressed_issues, + true, + ); + + if (!$can_apply_fix + || !$codebase->alter_code + || $property_storage->stmt_location === null + || !isset($source->getProjectAnalyzer()->getIssuesToFix()['MissingOverrideAttribute']) + ) { + return; + } + + $idx = $property_storage->stmt_location->getSelectionBounds()[0]; + + // A promoted property is part of a parameter list, so the attribute goes inline before it + // (a trailing space, no indentation handling). A regular property gets the attribute on its + // own line above, with the existing indentation reapplied afterwards. + $is_promoted = $property_storage->is_promoted; + $insertion_text = $is_promoted ? '#[\\Override] ' : "#[\\Override]\n"; + + FileManipulationBuffer::add($property_storage->stmt_location->file_path, [ + new FileManipulation($idx, $idx, $insertion_text, !$is_promoted), + ]); + } + + private static function overridesParentProperty( + Codebase $codebase, + ClassLikeStorage $class_storage, + string $property_name, + ): bool { + // Parent classes are tracked here by the Populator (private parents are excluded, matching + // the engine, which does not treat a private parent property as overridable). + if (isset($class_storage->overridden_property_ids[$property_name])) { + return true; + } + + // Interface properties (abstract property hooks, PHP 8.4+) are not copied into the + // implementing class, so check the implemented interfaces directly. This keeps property + // handling consistent with methods, whose getOverriddenMethodIds() already spans interfaces. + foreach ($class_storage->class_implements as $interface_fqcln_lc => $_) { + if (!$codebase->classlike_storage_provider->has($interface_fqcln_lc)) { + continue; + } + + $interface_storage = $codebase->classlike_storage_provider->get($interface_fqcln_lc); + + if (isset($interface_storage->properties[$property_name]) + && $interface_storage->properties[$property_name]->visibility + !== ClassLikeAnalyzer::VISIBILITY_PRIVATE + ) { + return true; + } + } + + return false; + } +} diff --git a/tests/FileManipulation/FileManipulationTestCase.php b/tests/FileManipulation/FileManipulationTestCase.php index 64b97748a21..e7008693011 100644 --- a/tests/FileManipulation/FileManipulationTestCase.php +++ b/tests/FileManipulation/FileManipulationTestCase.php @@ -20,6 +20,12 @@ abstract class FileManipulationTestCase extends TestCase { protected ProjectAnalyzer $project_analyzer; + /** + * Subclasses that exercise the `#[\Override]` fixer opt in by setting this to true, since the + * shared {@see TestConfig} disables the requirement by default. + */ + protected bool $ensure_override_attribute = false; + #[Override] public function setUp(): void { @@ -46,6 +52,7 @@ public function testValidCode( } $config = new TestConfig(); + $config->ensure_override_attribute = $this->ensure_override_attribute; $this->project_analyzer = new ProjectAnalyzer( $config, diff --git a/tests/FileManipulation/MissingOverrideAttributeManipulationTest.php b/tests/FileManipulation/MissingOverrideAttributeManipulationTest.php new file mode 100644 index 00000000000..94416fab3c7 --- /dev/null +++ b/tests/FileManipulation/MissingOverrideAttributeManipulationTest.php @@ -0,0 +1,242 @@ + [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'addToOverridingStaticProperty' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'addToOverridingPromotedPropertyInline' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'addToOverridingPromotedPropertyMultiline' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'addToInterfacePropertyImplementation' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'addAlongsideExistingAttribute' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'doesNotTouchPropertyThatAlreadyHasAttribute' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'doesNotTouchNonOverridingProperty' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'doesNotAddBeforePhp85' => [ + 'input' => ' ' '8.4', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'addToPromotedPropertyAlongsideExistingAttribute' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + 'doesNotFixMultiPropertyStatement' => [ + 'input' => ' ' '8.5', + 'issues_to_fix' => ['MissingOverrideAttribute'], + 'safe_types' => true, + ], + ]; + } +} diff --git a/tests/OverrideTest.php b/tests/OverrideTest.php index 9e682500b52..41a8b918ffa 100644 --- a/tests/OverrideTest.php +++ b/tests/OverrideTest.php @@ -118,6 +118,141 @@ public function __toString(): string { } ', ], + 'overridePropertyClass' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'overrideStaticPropertyClass' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'overridePromotedProperty' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'overrideInterfaceProperty' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'overrideTransitiveInterfaceProperty' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'redeclaredPrivateParentPropertyNeedsNoAttribute' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'overrideOnTraitPropertyIsIgnored' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'traitPropertyUsedInOverridingClassNotFlagged' => [ + // The attribute lives on the shared trait, so Psalm leaves it unchecked, the same + // way it leaves trait methods unchecked. + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.5', + ], + 'missingPropertyAttributeIgnoredBelow85' => [ + 'code' => ' [], + 'ignored_issues' => [], + 'php_version' => '8.4', + ], ]; } @@ -237,6 +372,97 @@ public function __toString(): string { 'error_levels' => [], 'php_version' => '8.3', ], + 'propertyMissingAttribute' => [ + 'code' => ' 'MissingOverrideAttribute', + 'error_levels' => [], + 'php_version' => '8.5', + ], + 'staticPropertyMissingAttribute' => [ + 'code' => ' 'MissingOverrideAttribute', + 'error_levels' => [], + 'php_version' => '8.5', + ], + 'promotedPropertyMissingAttribute' => [ + 'code' => ' 'MissingOverrideAttribute', + 'error_levels' => [], + 'php_version' => '8.5', + ], + 'interfacePropertyMissingAttribute' => [ + 'code' => ' 'MissingOverrideAttribute', + 'error_levels' => [], + 'php_version' => '8.5', + ], + 'multiPropertyStatementStillReports' => [ + 'code' => ' 'MissingOverrideAttribute', + 'error_levels' => [], + 'php_version' => '8.5', + ], + 'propertyWithAttributeButNoParent' => [ + 'code' => ' 'InvalidOverride', + 'error_levels' => [], + 'php_version' => '8.5', + ], + 'promotedPropertyWithAttributeButNoParent' => [ + 'code' => ' 'InvalidOverride', + 'error_levels' => [], + 'php_version' => '8.5', + ], ]; } }