Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/running_psalm/issues/InvalidOverride.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
<?php

class A {
public int $value = 0;
}

class B extends A {
#[Override]
public int $other = 1;
}
```

## Why this is bad

A fatal error will be thrown.
20 changes: 17 additions & 3 deletions docs/running_psalm/issues/MissingOverrideAttribute.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,28 @@ class B extends A {
}
```

On PHP 8.5 and above, the same applies to a property that overrides a parent (or implemented interface) property, including constructor promoted and static properties.

```php
<?php

class A {
public int $value = 0;
}

class B extends A {
public int $value = 1;
}
```

## Why this is bad

Having an `Override` attribute on overridden methods makes intentions clear. Read the [PHP RFC](https://wiki.php.net/rfc/marking_overriden_methods) for more details.
Having an `Override` attribute on overridden methods and properties makes intentions clear. Read the [PHP RFC](https://wiki.php.net/rfc/marking_overriden_methods) for more details.

## How to fix

Declare the `#[\Override]` attribute on all indicated methods, or run `vendor/bin/psalter --issues=MissingOverrideAttribute` to let Psalm do it for you.
Declare the `#[\Override]` attribute on all indicated methods and properties, or run `vendor/bin/psalter --issues=MissingOverrideAttribute` to let Psalm do it for you.

Note that the `#[\Override]` attribute is compatible with **all PHP versions**, even PHP 4.
Note that the `#[\Override]` attribute on a method is compatible with **all PHP versions**, even PHP 4. On a property it requires PHP 8.5, so Psalm reports (and fixes) missing attributes on properties only when analyzing PHP 8.5 or above.

On PHP 8.0-8.2, require [symfony/polyfill-php83](https://packagist.org/packages/symfony/polyfill-php83) to polyfill the missing Override attribute.
21 changes: 21 additions & 0 deletions src/Psalm/Internal/Analyzer/ClassAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,27 @@ private function analyzeProperty(
$property_storage->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;
}
Expand Down
36 changes: 36 additions & 0 deletions src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
142 changes: 142 additions & 0 deletions src/Psalm/Internal/Analyzer/PropertyOverrideAnalyzer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
<?php

declare(strict_types=1);

namespace Psalm\Internal\Analyzer;

use Psalm\Codebase;
use Psalm\FileManipulation;
use Psalm\Internal\FileManipulation\FileManipulationBuffer;
use Psalm\Issue\InvalidOverride;
use Psalm\Issue\MissingOverrideAttribute;
use Psalm\IssueBuffer;
use Psalm\StatementsSource;
use Psalm\Storage\ClassLikeStorage;
use Psalm\Storage\PropertyStorage;

/**
* Mirrors the method `#[\Override]` checks in {@see FunctionLikeAnalyzer} for properties.
*
* PHP 8.5 extended `#[\Override]` to target properties, so the same two diagnostics apply:
* - {@see MissingOverrideAttribute} when a property overrides a parent one but lacks the attribute
* - {@see InvalidOverride} when a property carries the attribute but overrides nothing
*
* Unlike the method check, this is gated to PHP 8.5+: property `#[\Override]` does not exist before
* then, so reporting it on an older target would only push users towards code their engine rejects.
*
* @internal
*/
final class PropertyOverrideAnalyzer
{
public static function analyze(
StatementsSource $source,
Codebase $codebase,
ClassLikeStorage $class_storage,
string $property_name,
PropertyStorage $property_storage,
bool $has_override_attribute,
bool $can_apply_fix,
): void {
if ($codebase->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(ProjectAnalyzer::getInstance()->getIssuesToFix()['MissingOverrideAttribute'])
) {
Comment on lines +89 to +93

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of using the global static ProjectAnalyzer::getInstance(), it is cleaner and more idiomatic to retrieve the ProjectAnalyzer instance directly from the $source parameter via $source->getProjectAnalyzer(). This avoids global state access and makes the analyzer more robust and testable.

        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) {
if (!$codebase->classlike_storage_provider->has($interface_fqcln)) {
continue;
}

$interface_storage = $codebase->classlike_storage_provider->get($interface_fqcln);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The $class_storage->class_implements array is associative, where keys are lowercase fully qualified interface names and values are the original-cased names. Since ClassLikeStorageProvider::has() and get() expect lowercase class/interface names, iterating over the lowercase keys ($interface_fqcln_lc) is safer, more efficient, and avoids potential case-sensitivity lookup issues.

        foreach ($class_storage->class_implements as $interface_fqcln_lc => $interface_fqcln) {
            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;
}
}
7 changes: 7 additions & 0 deletions tests/FileManipulation/FileManipulationTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -46,6 +52,7 @@ public function testValidCode(
}

$config = new TestConfig();
$config->ensure_override_attribute = $this->ensure_override_attribute;

$this->project_analyzer = new ProjectAnalyzer(
$config,
Expand Down
Loading
Loading