implement gettype() return type extension #2437
Conversation
|
|
||
| $valueType = $scope->getType($functionCall->getArgs()[0]->value); | ||
|
|
||
| $type = TypeTraverser::map($valueType, static function (Type $valueType, callable $traverse): Type { |
There was a problem hiding this comment.
alternatively we might implement Type->getType() or Type->getTypeString() instead
|
This pull request has been marked as ready for review. |
src/Analyser/TypeSpecifier.php
Outdated
| if ( | ||
| $leftType instanceof ConstantScalarType | ||
| && !$binaryOperation->right instanceof Node\Scalar\String_ |
There was a problem hiding this comment.
Eh, what's so special about string? Why did you add this change?
There was a problem hiding this comment.
its required to make https://github.com/staabm/phpstan-src/blob/1.10.x/tests/PHPStan/Analyser/data/bug-6901.php pass
otherwise - because of switch beeing re-written to comparisons and the new return type extension - $leftType and $rightType will resolve to constant strings and the method - as it was before the PR - would not specify the FuncCall expression but the String_
There was a problem hiding this comment.
That feels like a wrong placement for such fix.
gettype is handled in TypeSpecifier around line 1072. Wouldn't it help to first check for gettype condition and only after that to perform the thing on line 1424?
There was a problem hiding this comment.
I think we will have the same problem for other functions being used in a switch statement/identical comparison and have a return type extension with constant scalars.
we cannot detect it in line 1072, as the problem is not within the FuncCall itself, but the comparison.
see the bug in action in https://phpstan.org/r/d00e29f6-3163-414b-b83d-f591fd1eaf7d
(I have added a separate test-case for it in e60ff34)
There was a problem hiding this comment.
If you prefer we could separate that bugfix in a new PR
There was a problem hiding this comment.
Yeah https://phpstan.org/r/d00e29f6-3163-414b-b83d-f591fd1eaf7d shouldn't be happening, but I'm not sure if it's the same bug...
There was a problem hiding this comment.
ok, turned it into a separate bug: phpstan/phpstan#9404
There was a problem hiding this comment.
I had another look at it.
you were right that phpstan/phpstan#9404 does not related to the problem here.
I think the fix is right though and the description in https://github.com/phpstan/phpstan-src/pull/2437/files#r1220239214 is accurate
There was a problem hiding this comment.
to simplify the test in question:
<?php
use function PHPStan\Testing\assertType;
/**
* @param int $y
*/
function foo($y): void
{
switch (gettype($y)) {
case "integer":
assertType('int', $y);
break;
default:
assertType('*NEVER*', $y);
}
}runs into
------ ------------------------------------
Line tst.php
------ ------------------------------------
15 Expected type *NEVER*, actual: int
------ ------------------------------------
in this PR, when we drop the ... instanceof Node\Scalar\String_ changes
fixed mixed type fixed default return another test-case
|
As I said, I didn't like the changes in TypeSpecifier. I made some changes on 1.10.x, essentially doing this #2437 (comment) And now the tests in your PR pass. I'm gonna merge it if the pipeline is going to be green. Thanks. |
|
Thanks |
refs phpstan/phpstan#8614 (comment)