-
Notifications
You must be signed in to change notification settings - Fork 538
Introduce all-methods-are-(im)pure #4422
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
base: 2.1.x
Are you sure you want to change the base?
Conversation
c5a99df
to
9b80946
Compare
I dunno which strategy you will prefer @ondrejmirtes between
The second one seems simpler to me because |
src/PhpDoc/ResolvedPhpDocBlock.php
Outdated
|
||
private function getClassReflection(): ?ClassReflection | ||
{ | ||
$className = $this->nameScope?->getClassName(); |
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.
The phpstan downgrade process does not support null-safe calls. You need regular if-null checks for php7 compat
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 think that ResolvedPhpDocBlock should simply add isAllMethodsPure
and isAllMethodsImpure
.
I imagine the new tags should be handled similarly to isImmutable
/ isReadOnly
.
Above class there can be @phpstan-immutable
tag which marks all properties as readonly. And above properties you can have @phpstan-readonly
.
Having @phpstan-immutable
above class does not change what ResolvedPhpDocBlock says about isReadOnly.
Instead, PhpClassReflectionExtension handles this when creating properties by asking about both tags:
$isReadOnlyByPhpDoc = $isReadOnlyByPhpDoc || $resolvedPhpDoc->isReadOnly(); |
So handling these new tags should be equivalent.
Please do not forget to test that PureMethodRule still correctly reports errors with all possible combinations. This means that these new tags also have to be read in NodeScopeResolver::getPhpDocs()
so that they're available in PhpMethodFromParserNodeReflection (which is the object used when analysing method body itself).
527151a
to
908d9ff
Compare
This reverts commit 9b80946.
908d9ff
to
6723fac
Compare
Rework of #4409
and #4415
I added a non regression of phpstan/phpstan#10215 as a proof it works.
I don't like the naming of my methods, suggestion are opened.
Also I'm not fully sure
getDefaultMethodPurity
shouldn't be called somewhere else too...