- Notifications
You must be signed in to change notification settings - Fork 545
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).
908d9ff to 6723fac Compare | This pull request has been marked as ready for review. |
| I think it's ready for a new review :) |
3301bfb to f320582 Compare 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.
Also I'd like some more tests:
-
Calling
all-methods-pure+ nothing method from impure method (should report that impure method has no side effects) -
Calling
all-methods-pure+ 'impure' method from impure method (no errors) -
Calling
all-methods-pure+ nothing method from pure method (no errors) -
Calling
all-methods-pure+ 'impure' method from pure method (should report a side effect in a pure method) -
Calling
all-methods-impure+ nothing method from pure method (should report a side effect in a pure method) -
Calling
all-methods-impure+ 'pure' method from pure method (no errors) -
Calling
all-methods-impure+ nothing method from impure method (no errors) -
Calling
all-methods-impure+ 'pure' method from impure method (should report that impure method has no side effects)
d2e2fa5 to 82c591a Compare | Lot of test added and void behavior :) |
82c591a to 4393ad3 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
getDefaultMethodPurityshouldn't be called somewhere else too...