Simplify property memoization for Flyweight pattern by replacing it with ??=
#4084
Conversation
|
Would be great to have an auto-fixable rule that would enforce this under https://github.com/phpstan/phpstan-src/tree/2.1.x/build/PHPStan/Build. Do you think it'd be easy for you to implement that? And verify it by actually using it to make these changes. And compare it to your manual changes 😊 (Yeah, RuleErrorBuilder has |
|
I hadn't noticed that they added |
|
Yeah, just detect if === null and assignment to the same thing in the only statement. |
zonuexe
force-pushed
the
replace-memoization
branch
4 times, most recently
from
d51316c to
2154e33
Compare
|
This pull request has been marked as ready for review. |
|
This pull request has been marked as ready for review. |
zonuexe
force-pushed
the
replace-memoization
branch
2 times, most recently
from
23939e1 to
3c4bcf2
Compare
|
@ondrejmirtes The refactoring feature integrated into PHPStan is a game changer. 🎉 |
|
|
||
| [$ifNode, $returnNode] = $stmts; | ||
| if (!$returnNode instanceof Return_ | ||
| || !$returnNode->expr instanceof PropertyFetch |
There was a problem hiding this comment.
I think you can do the same for StaticPropertyFetch
There was a problem hiding this comment.
Thanks for the suggestion, I implemented it.
zonuexe
force-pushed
the
replace-memoization
branch
from
468fab5 to
9d72e31
Compare
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
I described the logic as following:
Yeah, just detect if === null and assignment to the same thing in the only statement.
So even if there's if in the middle of a function body, this rule could detect and change it.
It doesn't matter if there's $this->foo = $foo; or return $this->foo = $foo;. The rule should be able to handle both.
|
|
||
| public function getNodeType(): string | ||
| { | ||
| return InClassMethodNode::class; |
There was a problem hiding this comment.
I think the rule should apply to If_, not to a method body.
There was a problem hiding this comment.
Ok, it has been reflected in the rule.
zonuexe
force-pushed
the
replace-memoization
branch
from
7e86217 to
a6e89c9
Compare
|
This pull request has been marked as ready for review. |
| return []; | ||
| } | ||
|
|
||
| $errorBuilder = RuleErrorBuilder::message('Memoization property can be simplified.') |
There was a problem hiding this comment.
The message could be better. Something like "This initializing if statement can be replaced with null coalescing assignment operator (??=)."
There was a problem hiding this comment.
I changed the message and squashed the commits!
zonuexe
force-pushed
the
replace-memoization
branch
from
a6e89c9 to
e23329b
Compare
MemoizationPropertyRule supports static properties
zonuexe
force-pushed
the
replace-memoization
branch
from
e23329b to
0d7141f
Compare
|
Thank you! |
4 participants
Now that PHP 7.2 has been dropped, this pattern can be replaced.