New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TypeDeclaration] Handle infinite loop on array_reverse with index on AddMethodCallBasedStrictParamTypeRector on php8+ feature #3678
Conversation
Fixed with verify min or max of IntegerRangeType is null /cc @internalsystemerror |
d098a5b
to
e6c95c1
Compare
Rebased. |
All checks have passed 🎉 @TomasVotruba I think it is ready. |
if ($type->getMin() === null) { | ||
return null; | ||
} | ||
|
||
if ($type->getMax() === null) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min, max value null seems valid value https://phpstan.org/r/765b0694-8832-4b65-98b7-4944d2914bc3
getMin()
=== null printed as min
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all type applied with string type seems also valid https://phpstan.org/r/474c8c8b-1d17-4ba2-b6bf-c243b7390e19 for this type:
^ PHPStan\Type\UnionType^ {#23962
-sortedTypes: false
-cachedDescriptions: []
-types: array:3 [
0 => PHPStan\Type\IntegerRangeType^ {#22268
-min: null
-max: -1
}
1 => PHPStan\Type\IntegerRangeType^ {#22323
-min: 1
-max: null
}
2 => PHPStan\Type\StringType^ {#19486}
]
-normalized: false
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue seems somehow used index as integer on this type:
// here index must be integer...
$this->someOtherMethod($index, $uri);
}
}
private function someOtherMethod(int $index, string $uri)
{
return sprintf('%d-%s', $index, $uri);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it, it seems due to combination of UnionType
+ MixedType
, then it should be skipped, see 42bcdaa#diff-42a5a059b66868f3ede960ea9a50a3591de610668457d76063ac2f6dc653dcc3
All checks have passed 🎉 @TomasVotruba I think it is ready. |
@TomasVotruba I am merging it ;) |
ty @samsonasik i was still figuring out the problem and you've got the fix already! |
if ($totalTypes > 1 && $argumentStaticType instanceof UnionType) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@internalsystemerror I just re-check that this verify is not valid, because multiple args no longer working now when one of them is UnionType, eg:
final class NarrowUnion
{
public function run(MethodCall $methodCall, StaticCall $staticCall, String_ $string)
{
$this->someExpr($methodCall, $staticCall);
$this->someExpr($staticCall, $staticCall);
$this->someExpr($string, $staticCall);
}
private function someExpr($expr, $staticCall)
{
}
}
above is now completely skipped, which actually should be:
- private function someExpr($expr, $staticCall)
+ private function someExpr(\PhpParser\Node\Expr\MethodCall|\PhpParser\Node\Expr\StaticCall|\PhpParser\Node\Scalar\String_ $expr, \PhpParser\Node\Expr\StaticCall| $staticCall)
{
}
I will add separate PR for that :)
… AddMethodCallBasedStrictParamTypeRector on php8+ feature (#3678) * [TypeDeclaration] Handle infinite loop on array_reverse with index * [ci-review] Rector Rectify * add fixture for php7.4 as well * fix namespace * [ci-review] Rector Rectify * Fix * [ci-review] Rector Rectify --------- Co-authored-by: GitHub Action <actions@github.com>
Closes rectorphp/rector#7896
Ref https://getrector.com/demo/fe336fa4-194f-4bd3-8922-2780d5141d13