Fix pure annotations - First pass#605
Merged
ramsey merged 2 commits intoramsey:fix-pure-annotationsfrom Jun 25, 2025
Merged
Conversation
Owner
|
Quick note before I dive into reviewing this...
I know what's going on here, but it's strange to me that PHPStan only started complaining about these when I added the |
ramsey
approved these changes
Jun 25, 2025
Owner
ramsey
left a comment
There was a problem hiding this comment.
Thank you so much! This looks great!
| /** @phpstan-ignore possiblyImpure.methodCall */ | ||
| $bytes = $this->getBytes($encodedUuid); | ||
|
|
||
| /** @phpstan-ignore possiblyImpure.methodCall, possiblyImpure.methodCall */ |
Owner
There was a problem hiding this comment.
It's so wild you have to list the error ID twice, since there are two occurrences of it here. It makes sense, but I wouldn't have guessed it.
ramsey
pushed a commit
that referenced
this pull request
Jun 25, 2025
Coming from #603, this is an attempt to fix the errors raised by the current phpstan settings. I went through each of the errors raised by phpstan with the following approach. - If a method is part of an `@immutable` class, we can consider it pure, assuming it only affects internal variables. - If a potentially pure method is calling a class's method that is only swapped during testing (and not during normal usage), then we can consider the calling method pure. - If a class is marked deprecated, don't bother with attempting to mark it pure or immutable.
ramsey
pushed a commit
that referenced
this pull request
Jun 25, 2025
Coming from #603, this is an attempt to fix the errors raised by the current phpstan settings. I went through each of the errors raised by phpstan with the following approach. - If a method is part of an `@immutable` class, we can consider it pure, assuming it only affects internal variables. - If a potentially pure method is calling a class's method that is only swapped during testing (and not during normal usage), then we can consider the calling method pure. - If a class is marked deprecated, don't bother with attempting to mark it pure or immutable.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Coming from Re-add the @pure annotations PR, this is an attempt to fix the errors raised by the current phpstan settings.
Description
I went through each of the errors raised by phpstan with the following approach.
@immutableclass, we can consider it pure, assuming it only affects internal variables.There's a lot of ignores that have been placed for this PR, so a second set of eyes is welcome and appreciated.
@ramsey, I have this PR set to merge into
uuid/fix-pure-annotations, but happy to switch touuid/mainif that makes more sense.One thing to note that bugs me about the initial state of this PR is
phpstanflags about 40 differentmethod.resultUnusedissues in the tests folder. Generally, these tests are either for testing exceptions, or as part of a benchmarking suite. In both cases, the code works as intended. However, having to explicitly flagphpstanto ignore these feels like a downgrade to the developer experience here, and might be an additional minor hindrance to external contrib. My view is likely skewed by placing all the exceptions at the same time, but I would be open to a different approach that automatically filters those warnings in the test folder.How has this been tested?
This has been tested by running
composer run-script testlocally. No actual code changes were made, this should all bephpstancomments.Types of changes
PR checklist