Fix phpstan/phpstan#13566: False positive staticMethod.impossibleType#5121
Fix phpstan/phpstan#13566: False positive staticMethod.impossibleType#5121phpstan-bot wants to merge 23 commits intophpstan:2.1.xfrom
Conversation
… for conditional return type with void/never - Fixed ImpossibleCheckTypeHelper to skip impossible check when rootExpr is a call argument in null (void) context - The conditional return type ($exit is true ? never : void) describes behavioral control flow, not a type check - Added regression test in tests/PHPStan/Rules/Comparison/data/bug-13566.php - The root cause was that getConditionalSpecifiedTypes set rootExpr to the call argument via inner specifyTypesInCondition, causing findSpecifiedType to short-circuit with a false "always evaluates to false" result
ad8821e to
0eb267b
Compare
src/Analyser/TypeSpecifier.php
Outdated
| $parameterExpr instanceof Node\Scalar | ||
| || ($parameterExpr instanceof ConstFetch && in_array(strtolower($parameterExpr->name->toString()), ['true', 'false', 'null'], true)) |
There was a problem hiding this comment.
I had to hardcode this literal-value ast-expressions to differentiate real literal-values from infered types. otherwise existing tests started to fail, because we e.g. still want
$foo = null
assertNotNull($foo);to error with "is always false".
|
This pull request has been marked as ready for review. |
VincentLanglet
left a comment
There was a problem hiding this comment.
It ends up really differently form the initial fix
db849fa
^^'
|
|
||
| class ReturnViaInt | ||
| { | ||
| /** @return ($exit is 1 ? never : void) */ |
There was a problem hiding this comment.
We need a string scalar also no ?
/** @return ($exit is '1' ? never : void) */
public static function notFound(string $exit = '1'): void
There was a problem hiding this comment.
And what about non scalar like
/**
* @return ($calledFromShutdownHandler is int ? void : never)
*/
public static function notFound(int|string $exit = 1): void
There was a problem hiding this comment.
I hope I have interpreted your comments correctly, that you want me to add more tests for the mentioned cases (which I just did)?
There was a problem hiding this comment.
And what about non scalar like
I don't see a way to support this case without also regressing other existing rule-errors
There was a problem hiding this comment.
And what about non scalar like
I don't see a way to support this case without also regressing other existing rule-errors
That's what I thought seeing the fix. I'm worried this would mean we're not fixing the bug at the right place
There was a problem hiding this comment.
ok - please see the latest commit which fixes union types
I think it is acceptable to remove the expected errors for the following code
phpstan-src/tests/PHPStan/Rules/Comparison/data/check-type-function-call.php
Lines 102 to 107 in f829abe
because the examples feel pretty synthetic and useless. why should one pass a raw literal-string into a assertion-function?
I think more real world would be
$x = 123;
if (is_numeric($x)) {
}which works with this PR
There was a problem hiding this comment.
I will need some time to understand how union type and is_numeric('123') is related and more important, how is_numeric('123') and a conditional return type ($calledFromShutdownHandler is int ? void : never) is related.
I assume the conditional return type is rewritten to another expression which is analysed. Maybe here we could keep a trace of the fact it's a ternary with a never branch (if it's the issue ?)
ohh yeah.. I had multiple tries and different patches until I finally hopefully understood the problem good enough to come up with a proper fix |
This reverts commit ee23b03.
| return null; | ||
| } | ||
|
|
||
| $argsMap = []; |
There was a problem hiding this comment.
removed this $argsMap because we built the array here and passed it into getConditionalSpecifiedTypes just to extract a single offset out of it, which we already knew beforehand.
so instead I am just extracting the argumentExpr we are interessted in and pass it over.
(just refactoring)
f829abe to
8406cc6
Compare
8406cc6 to
3d44fe9
Compare
Fixes phpstan/phpstan#13566
Closes phpstan/phpstan#10337
Prevents that function/method calls with conditional-defined return types like
/** @return ($exit is true ? never : void) */- which compare against constant-values - do not lead to a "always false/true" error.