Fix param castable to string rule #3141
Conversation
| if ($flags === null || $flags->equals(new ConstantIntegerType(SORT_REGULAR))) { | ||
| return []; | ||
| } | ||
|
|
||
| foreach (TypeUtils::getConstantIntegers($flags) as $flag) { | ||
| if ($flag->getValue() !== SORT_NUMERIC) { |
There was a problem hiding this comment.
I'm not sure what's the best way to handle flags like this. My thinking is as follows:
SORT_REGULARdoes not have any constraints, so I only check for equality. If there is a union (or int), then the stricter check should be maintained.SORT_NUMERICseems the strictest (though not exactly, because it does allow arrays, unlikeSORT_STRING), so if it's a unionSORT_NUMERICand something else, I go withSORT_NUMERIC.- Otherwise, I assume
SORT_STRING(orSORT_LOCALE_STRINGwhich is the same for our purposes).
The only edge-case that I can think of right now is e.g. array_unique([[1], [2]], rand() ? SORT_NUMERIC : SORT_STRING). IDK if it's worth it to complicate it further with such edge cases. 😅
There was a problem hiding this comment.
Upon further consideration I changed it. My thinking is that it should enforce the strongest constraint which it cannot prove to be unnecessary. So sort($arr, $int) and sort($arr, rand() ? SORT_STRING : SORT_NUMERIC) will require castability to both string and float. So the previously mentioned edge-case is now prohibited as well.
I found one more edge-case: statically known invalid flag (e.g. sort($arr, 128)). Based on my testing it seems to fall back to SORT_REGULAR in that case (I included array_unique because it has a different default value - I was thinking that it may fall back to the default value). I allowed this case (it should be prohibited, just not by this rule).
|
An idea: instead of these huge if-else, let's split this rule in multiple ones focused on each family of functions. |
|
I'll try that on Wednesday, I need a bit of a break. 😅 |
schlndh
force-pushed
the
fix-paramCastableToStringRule
branch
from
ad439fd
to
3ec4699
Compare
| !in_array($functionName, $checkAllArgsFunctions, true) | ||
| && !in_array($functionName, $checkFirstArgFunctions, true) |
There was a problem hiding this comment.
I kept these two groups together, since the only difference is which parameters are checked.
| $flags = $functionParameters[1]->getDefaultValue(); | ||
| } | ||
|
|
||
| if ($flags === null || $flags->equals(new ConstantIntegerType(SORT_REGULAR))) { |
There was a problem hiding this comment.
should this check support union-types?
There was a problem hiding this comment.
All other valid flags are stricter, so IMO it's not necessary to support union types in this condition.
Because the issue is not yet tagged as bug / feature request. |
|
|
||
| $errors = []; | ||
|
|
||
| foreach ($argsToCheck as $argIdx => $arg) { |
There was a problem hiding this comment.
The duplicated part of the rules should be moved to a common "Something...Check" class.
tests/PHPStan/Rules/Functions/ImplodeParameterCastableToStringRuleTest.php
Outdated
Show resolved
Hide resolved
|
Perfect, thank you! |
Fixes phpstan/phpstan#11167, but for some reason the issue is not reported as changed by the Issue Bot action. 🤔