Repository navigation
Strip the final-by-keyword override from the declaring class before comparing a method's return type with $this
#6941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
VincentLanglet
merged 2 commits into
phpstan:2.3.x
from
phpstan-bot:create-pull-request/patch-f3agoxc
Nov 4, 2026
+224
−7
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| <?php // lint >= 8.0 | ||
|
|
||
| declare(strict_types = 1); | ||
|
|
||
| namespace Bug15320; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| class Foo | ||
| { | ||
| public function testOutClassVoid(): void | ||
| { | ||
| $m = new Model(); | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setVoid('name', 10); | ||
| assertType('int', $m->get('name')); | ||
| } | ||
|
|
||
| public function testOutClassThis(): void | ||
| { | ||
| $m = new Model(); | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setThis('name', 10); | ||
| assertType('int', $m->get('name')); | ||
| } | ||
|
|
||
| public function testOutClassStatic(): void | ||
| { | ||
| $m = new Model(); | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setStatic('name', 10); | ||
| assertType('5', $m->get('name')); | ||
| } | ||
|
|
||
| public function testOutClassThisParameter(Model $m): void | ||
| { | ||
| assert($m->get('name') === 5); | ||
| $m->setThis('name', 10); | ||
| assertType('int', $m->get('name')); | ||
| } | ||
|
|
||
| public function testOutClassThisArgument(): void | ||
| { | ||
| $m = new Model(); | ||
| $other = new Model(); | ||
|
|
||
| assert($other->get('name') === 5); | ||
| $m->setThisFromObject($other); | ||
| assertType('5', $other->get('name')); | ||
| } | ||
|
|
||
| public function testOutClassAnnotationMethod(): void | ||
| { | ||
| $m = new MagicModel(); | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setMagic('name', 10); | ||
| assertType('int', $m->get('name')); | ||
| } | ||
| } | ||
|
|
||
| class Model | ||
| { | ||
| public function setVoid(string $field, int $value): void | ||
| { | ||
| } | ||
|
|
||
| /** | ||
| * @return $this | ||
| */ | ||
| public function setThis(string $field, int $value): object | ||
| { | ||
| return $this; | ||
| } | ||
|
|
||
| public function setStatic(string $field, int $value): static | ||
| { | ||
| return $this; | ||
| } | ||
|
|
||
| /** | ||
| * @return $this | ||
| */ | ||
| public function setThisFromObject(object $other): object | ||
| { | ||
| return $this; | ||
| } | ||
|
|
||
| public function get(string $field): int | ||
| { | ||
| return 1; | ||
| } | ||
|
|
||
| public function testInClassVoid(): void | ||
| { | ||
| $m = $this; | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setVoid('name', 10); | ||
| assertType('int', $m->get('name')); | ||
| } | ||
|
|
||
| public function testInClassThis(): void | ||
| { | ||
| $m = $this; | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setThis('name', 10); | ||
| assertType('int', $m->get('name')); | ||
| } | ||
|
|
||
| public function testInClassStatic(): void | ||
| { | ||
| $m = $this; | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setStatic('name', 10); | ||
| assertType('5', $m->get('name')); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @method $this setMagic(string $field, int $value) | ||
| */ | ||
| class MagicModel | ||
| { | ||
| public function get(string $field): int | ||
| { | ||
| return 1; | ||
| } | ||
|
|
||
| public function __call(string $name, array $args): mixed | ||
| { | ||
| return $this; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| <?php declare(strict_types = 1); // lint >= 8.0 | ||
|
|
||
| namespace Bug15320DeadCode; | ||
|
|
||
| class Foo | ||
| { | ||
| public function testOutClassVoid(): void | ||
| { | ||
| $m = new Model(); | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setVoid('name', 10); | ||
| } | ||
|
|
||
| public function testOutClassThis(): void | ||
| { | ||
| $m = new Model(); | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setThis('name', 10); | ||
| } | ||
|
|
||
| public function testOutClassStatic(): void | ||
| { | ||
| $m = new Model(); | ||
|
|
||
| assert($m->get('name') === 5); | ||
| $m->setStatic('name', 10); | ||
| } | ||
| } | ||
|
|
||
| class Model | ||
| { | ||
| public function setVoid(string $field, int $value): void | ||
| { | ||
| } | ||
|
|
||
| /** | ||
| * @return $this | ||
| */ | ||
| public function setThis(string $field, int $value): object | ||
| { | ||
| return $this; | ||
| } | ||
|
|
||
| public function setStatic(string $field, int $value): static | ||
| { | ||
| return $this; | ||
| } | ||
|
|
||
| public function get(string $field): int | ||
| { | ||
| return 1; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is explained by
By default
$m->setStatic('name', 10);is considered as pure since it returns something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I answered the review question and left the code unchanged. I also added a comment in the test explaining why that error is expected, and pushed it as commit 65d34e9.
Why
setStatic()is still reported:staticreturn type doesn't mean the method has side effects.voidand$thisboth mean the call only matters for what it changes, so PHPStan treats them as having side effects. Astaticmethod, though, can return a modified copy and leave the original object alone (a "wither"). So inPhpMethodReflection::hasSideEffects(),ThisType(Model)->isSuperTypeOf(static)is Maybe, not Yes, and the method stays "possibly pure".setStatic()is justreturn $this;, so the rule can confirm the method is pure.new Model(), soPossiblyPureMethodCallCollectorknows no subclass can overridesetStatic().Put together, discarding the result of
$m->setStatic('name', 10);really does nothing, so the error is correct. The issue asks for it too ("error on line 30 is expected"). That matches the nsrt test, wheretestOutClassStatickeeps the remembered5after the call.make testsandmake phpstanboth pass.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't push anything: the change this review calls for is already on the branch.
VincentLanglet's comment wasn't asking for a code change. It answered staabm's question ("why this error?"): a method returning
staticdoesn't imply side effects, because it might return a new instance, like an immutable "wither". So$m->setStatic('name', 10)on a separate line, called onnew Model(), really has no effect. The issue (phpstan/phpstan#15320) expects that error too.I wrote a commit that added a comment explaining this to the test data file, but the push was rejected. Another phpstan-bot commit had already reached the branch,
65d34e96f, "Explain why the static-returning method call is still reported in the bug-15320 test". It puts the same explanation, in more detail, inCallToMethodStatementWithoutImpurePointsRuleTest::testBug15320. Mine would only have duplicated it, so I dropped it and reset my local branch to match the remote.make testsandmake phpstanboth passed with my version, which differed only by a comment in the test data. After resetting, I re-ranCallToMethodStatementWithoutImpurePointsRuleTeston the branch as it now stands, and it passes. I didn't re-run the full suite ormake phpstanafter resetting.