Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[TypeDeclaration] Handle infinite loop on array_reverse with index on AddMethodCallBasedStrictParamTypeRector on php8+ feature #3678

Merged
merged 7 commits into from
Apr 24, 2023
Merged

[TypeDeclaration] Handle infinite loop on array_reverse with index on AddMethodCallBasedStrictParamTypeRector on php8+ feature #3678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7fe1824
[TypeDeclaration] Handle infinite loop on array_reverse with index
samsonasik Apr 24, 2023
28c68c7
[ci-review] Rector Rectify
actions-user Apr 24, 2023
d01432b
add fixture for php7.4 as well
samsonasik Apr 24, 2023
c86aba4
fix namespace
samsonasik Apr 24, 2023
e6c95c1
[ci-review] Rector Rectify
actions-user Apr 24, 2023
42bcdaa
Fix
samsonasik Apr 24, 2023
c2618ae
[ci-review] Rector Rectify
actions-user Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions ...thod/AddMethodCallBasedStrictParamTypeRector/Fixture/skip_no_return_array_reverse.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

declare(strict_types=1);

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\AddMethodCallBasedStrictParamTypeRector\Fixture;

final class SkipNoReturnArrayReverse
{
public function run()
{
$parts = array_reverse(explode('/', '/some/test/url'));
foreach ($parts as $index => $uri) {
if ($index === 0) {
continue;
}

$this->someOtherMethod($index, $uri);
}
}

private function someOtherMethod(int $index, string $uri)
{
return sprintf('%d-%s', $index, $uri);
}
}
23 changes: 23 additions & 0 deletions ...AddMethodCallBasedStrictParamTypeRector/FixtureUnion/skip_no_return_array_reverse.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\AddMethodCallBasedStrictParamTypeRector\FixtureUnion;

final class SkipNoReturnArrayReverse
{
public function run()
{
$parts = array_reverse(explode('/', '/some/test/url'));
foreach ($parts as $index => $uri) {
if ($index === 0) {
continue;
}

$this->someOtherMethod($index, $uri);
}
}

private function someOtherMethod(int $index, string $uri)
{
return sprintf('%d-%s', $index, $uri);
}
}
5 changes: 5 additions & 0 deletions rules/TypeDeclaration/NodeAnalyzer/ClassMethodParamTypeCompleter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,13 @@ public function __construct(
public function complete(ClassMethod $classMethod, array $classParameterTypes, int $maxUnionTypes): ?ClassMethod
{
$hasChanged = false;
$totalTypes = count($classParameterTypes);

foreach ($classParameterTypes as $position => $argumentStaticType) {
if ($totalTypes > 1 && $argumentStaticType instanceof UnionType) {
return null;
}
Comment on lines +44 to +46
Copy link
Member Author

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 :)


/** @var Type $argumentStaticType */
if ($this->shouldSkipArgumentStaticType($classMethod, $argumentStaticType, $position, $maxUnionTypes)) {
continue;
Expand Down