Skip to content

Report impure method overriding pure parent method in MethodSignatureRule#5584

Open
phpstan-bot wants to merge 5 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-zcama98
Open

Report impure method overriding pure parent method in MethodSignatureRule#5584
phpstan-bot wants to merge 5 commits intophpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-zcama98

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

When a parent method is marked @phpstan-pure, child classes could override it with @phpstan-impure without any error being reported. This violated the Liskov Substitution Principle: callers expecting a pure method would get an impure one. This PR adds a purity compatibility check to MethodSignatureRule.

Changes

  • src/Rules/Methods/MethodSignatureRule.php: Added a check at the beginning of the parent method loop — when the child method's isPure() is no and the parent method's isPure() is yes, report an error with identifier method.impureOverridePure
  • tests/PHPStan/Rules/Methods/data/bug-14563.php: New test data file covering all parent method sources and edge cases
  • tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php: Added testBug14563() test method

Root cause

MethodSignatureRule checked return type covariance and parameter type contravariance for overriding methods, but had no check for purity compatibility. An impure child method could silently override a pure parent method, breaking the purity contract that callers rely on.

Test

The regression test covers these cases (all producing errors):

  • Class extends class: child has @phpstan-impure, parent has @phpstan-pure
  • Interface implementation: implementing class marks method @phpstan-impure while interface declares it @phpstan-pure
  • @phpstan-all-methods-pure on parent class: child class marks method @phpstan-impure
  • Abstract trait method: trait declares @phpstan-pure abstract method, using class marks it @phpstan-impure
  • Grandchild inheritance: grandchild overrides with @phpstan-impure a method pure in the parent chain
  • Multiple interfaces: both interfaces declare the method @phpstan-pure, implementing class marks it @phpstan-impure — error reported for each interface

And these non-error cases (verifying no false positives):

  • Child with no annotation overriding pure parent (inherits purity)
  • Child with @phpstan-pure overriding pure parent
  • Child with @phpstan-pure overriding impure parent (safe — more restrictive)
  • Child with @phpstan-impure overriding unannotated parent
  • Child in @phpstan-all-methods-impure class overriding pure parent (PHPDoc inheritance of @phpstan-pure takes precedence over class-level annotation)

Fixes phpstan/phpstan#14563

…eRule`

- Add purity compatibility check in MethodSignatureRule::processNode()
- When a parent method has @phpstan-pure and the child method has @phpstan-impure, report an error with identifier method.impureOverridePure
- The check runs for all parent sources: parent classes, interfaces, and abstract trait methods
- Covers @phpstan-all-methods-pure on parent class (purity is inherited per-method)
- Also covers grandchild inheritance and multiple interface implementation
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Add test for static method too.

@VincentLanglet VincentLanglet self-assigned this May 2, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Added static method test cases covering:

  • Error case: Impure static method overriding a pure static parent method (StaticChildImpureOverridesPure extends StaticFoo)
  • Error case: Impure static method implementing a pure static interface method (StaticImpureImplementation implements StaticPureInterface)
  • No-error cases: Static child with no annotation overriding pure parent, static child with @phpstan-pure overriding pure parent, and static child with @phpstan-impure overriding unannotated parent

All tests pass and make phpstan reports no errors.

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Why tests are failing on PHP 7.4 ?

Trait abstract method detection in collectParentMethods does not work
on PHP 7.4, consistent with other trait-related tests in this file
that use #[RequiresPhp('>= 8.0.0')].

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

The PHP 7.4 test failure is fixed. The issue was that collectParentMethods doesn't detect abstract trait methods on PHP 7.4 — this is a known limitation, consistent with other trait-related tests in MethodSignatureRuleTest that use #[RequiresPhp('>= 8.0.0')] (like testTraits, testBug10166, testBug10184, testOverridenAbstractTraitMethodPhpDoc).

The fix makes the trait abstract method error expectation conditional on PHP_VERSION_ID >= 80000. The other integration test failures (PMMP, doctrine/orm, shopware) are unrelated to this PR — they're caused by other recent changes to main.

$method->getName(),
$parentMethodDeclaringClass->getDisplayName(),
$parentMethod->getName(),
))->identifier('method.impureOverridePure')->build();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change the identifier to method.purity

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Both review comments addressed and pushed:

  1. method.impureOverridePuremethod.purity — changed the error identifier in MethodSignatureRule.php
  2. Split trait test into dedicated file — extracted the trait test case into bug-14563-trait.php with its own testBug14563Trait() method gated by #[RequiresPhp('>= 8.0.0')], removing the PHP_VERSION_ID conditional from the main test

All 11,980 tests pass and make phpstan reports no errors.

$this->analyse([__DIR__ . '/data/bug-14320.php'], []);
}

public function testBug14563(): void
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Split the test in two with a dedicated file for trait ; test for trait will run on PHP8+ only

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Both review comments have been addressed and pushed:

  1. Identifier changed: method.impureOverridePuremethod.purity (commit 3e23d537)
  2. Trait test split: Extracted trait test cases into bug-14563-trait.php with a dedicated testBug14563Trait() method gated by #[RequiresPhp('>= 8.0.0')] (commit 53f49ca4)

All tests pass (11980 tests, 0 failures) and make phpstan reports no errors.

phpstan-bot and others added 2 commits May 2, 2026 21:34
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants