Adds type specifier for settype. #2920
Conversation
ChrisBrenton
force-pushed
the
bug-3250
branch
3 times, most recently
from
d163a46
to
f5f7545
Compare
| /** | ||
| * @param 'string'|'int'|'integer'|'float'|'double'|'bool'|'boolean'|'array'|'object'|'null' $castTo | ||
| */ | ||
| function setTypeSpecifying(string $value, string $castTo): void |
There was a problem hiding this comment.
I think a 2nd test function which works with a non-constant $x would be helpful.
Like
function doFoo(int $i, string $s ...)
And assert similar to what you did in this method
There was a problem hiding this comment.
If I caught the intention of your message correctly, this is just a mistake on my part when writing this test initially. The function was supposed to receive a mixed type for the first argument to match the signature of settype. I've changed this now, but if that wasn't the intention, please correct me.
There was a problem hiding this comment.
I would add a test-functions like
function doBoolean(int $i, string $s) {
settype($i, 'boolean');
assertType('bool', $x);
settype($s, 'boolean');
assertType('bool', $x);
}
since for this non-constant values, different types are returned.
there are lots of combinations and I think it doesn't make sense to test all combinations.. but at least a few most common ones should be tested
There was a problem hiding this comment.
Updated - I've added the functions as you described to the existing test cases.
ChrisBrenton
force-pushed
the
bug-3250
branch
from
f5f7545
to
393a7d9
Compare
ChrisBrenton
changed the title
ChrisBrenton
force-pushed
the
bug-3250
branch
2 times, most recently
from
421162e
to
9685155
Compare
ChrisBrenton
force-pushed
the
bug-3250
branch
from
9685155
to
446cab4
Compare
| $valueType = $scope->getType($value); | ||
| $castType = $scope->getType($node->getArgs()[1]->value); | ||
|
|
||
| $constantStrings = $castType->getConstantStrings(); |
There was a problem hiding this comment.
If the constant string is empty, we should return empty new SpecifiedTypes here.
| } | ||
| } | ||
|
|
||
| return new SpecifiedTypes(['$value' => [$value, TypeCombinator::union(...$types)]], [], true); |
There was a problem hiding this comment.
$value is wrong here, it cannot work. It'd only work if the call looked like this: settype($value, 'int').
You need to call TypeSpecifier::create(), as all the other type-specifying extensions do. That will create the correct SpecifiedTypes object for the right expression.
There was a problem hiding this comment.
I've made the change as requested, but I am not confident that I have implemented it correctly. The tests pass as I'd expect but I am very unsure as to the purpose/behaviour of $context. I would appreciate feedback on this PR if possible, and if you have time and are willing, an explanation of $context - for me this is one of the biggest unknowns I have come across so far.
ChrisBrenton
force-pushed
the
bug-3250
branch
from
6140690
to
281edfa
Compare
| public function isFunctionSupported(FunctionReflection $functionReflection, FuncCall $node, TypeSpecifierContext $context): bool | ||
| { | ||
| return strtolower($functionReflection->getName()) === 'settype' | ||
| && count($node->getArgs()) > 1; |
There was a problem hiding this comment.
This needs to check that $context->null().
| switch ($constantString->getValue()) { | ||
| case 'bool': | ||
| case 'boolean': | ||
| var_dump('converting to bool'); |
There was a problem hiding this comment.
Forgot var_dump calls there
| $value, | ||
| TypeCombinator::union(...$types), | ||
| TypeSpecifierContext::createTruthy(), | ||
| true, |
There was a problem hiding this comment.
No overwriting - this needs to be false.
|
Yeah, and a lot of tests are failing, please fix them :) Also, I've got an idea for a future PR: There could be a rule that checks whether the performed |
|
Oh I'm sorry, |
|
No worries. Fixed. My understanding is this is |
|
Let's say that the original type is
Most type-specifying functions are about type-checking and narrowing, not about casting (changing) the type. That's why overwrite=false is there in 99 % of cases. |
|
Thank you. |
|
FYI I thought you meant to fix phpstan/phpstan#3250 but I don't think it does that. Please try that with a regression test in a new PR. |
I'm fairly new to PHPStan's code base and I've been looking through some of the issues marked "Easy fixes" for a way to get started. I'm not certain if I am going about this the right way or if its completely mad - particularly the test.
I don't expect this to be accepted as is, but it'd be good to get some pointers to how I might otherwise go about it if/when you have time?