Support dynamic name expressions in rules#3886
Conversation
| $methodNames = [$node->name->name]; | ||
| } else { | ||
| $callType = $scope->getType($node->name); | ||
| $methodNames = array_map(static fn ($type): string => $type->getValue(), $callType->getConstantStrings()); |
There was a problem hiding this comment.
This applies to all the rules. We need to pair each name with its own Scope.
Let's say we have this code:
if (rand(0, 1)) {
$foo = 'doFoo';
$bar = 1;
} else {
$foo = 'doBar';
$bar = 'a';
}
$this->$foo($bar);If the first parameter of doFoo accepts int, and the first parameter of doBar accepts string, this code should not report any errors.
You can see the way it should be done here:
phpstan-src/src/Analyser/MutatingScope.php
Line 2149 in ef6cc0a
Also please feel free to do the same thing here:
(I thought it already works but apparently not).416437c to
c5e320a
Compare
| } else { | ||
| $nameType = $scope->getType($node->name); | ||
| $methodNames = array_map(static fn (ConstantStringType $type): string => $type->getValue(), $nameType->getConstantStrings()); | ||
| $methodNameScopes = array_column(array_map( |
There was a problem hiding this comment.
Please write less clever code without multiple array_map and array_column calls. A simple foreach would suffice!
a83bdc4 to
a451859
Compare
a451859 to
fc63761
Compare
| } else { | ||
| $nameType = $scope->getType($node->name); | ||
| $methodNameScopes = []; | ||
| foreach ($nameType->getConstantStrings() as $constantString) { |
There was a problem hiding this comment.
Nice! Now please do it consistently for all the touched rules. Thanks.
|
This pull request has been marked as ready for review. |
|
Perfect, thank you! Please add the new error in a single rule and once we agree how it should work, you can copy the approach to other rules. |
|
I think it solved phpstan/phpstan#2920 Should I write a non regression test or someone already on it ? And seems like it also solved phpstan/phpstan#7990 For the issue phpstan/phpstan#4608, I'm not sure... Seems like there is a regression since are not reported anymore. @ondrejmirtes @zonuexe |
|
phpstan/phpstan#2920 does not need a regression test, there are many examples of tests in this PR with the same value here already. Same for phpstan/phpstan#7990. In phpstan/phpstan#4608 the lines 10 and 11 stopped being reported in september 2022, unrelated to this PR. |
resolve #3885 (review)