[DeadCode] Add early return check to RemoveUnusedNonEmptyArrayBeforeF…

…oreachRector (#3611)

* [DeadCode] Add early return check to RemoveUnusedNonEmptyArrayBeforeForeachRector

* skip edge case

* Add fixture multi if -> foreach

* Add the patch by loop jump to key recursive

* [ci-review] Rector Rectify

* clean up skip config

* Revert clean up skip config

This reverts commit 22c3df8.

* Revert [ci-review] Rector Rectify

This reverts commit 10a8bfa.

* Revert Add the patch by loop jump to key recursive

This reverts commit 9375b38.

* Revert Add fixture multi if -> foreach

This reverts commit ceb6b33.

* add failing fixture throw inside empty

* Add patch ensure if only return with no expr

* add fixture for multi ifs and fixtures

* Fixed 🎉

* rename fixture

* skip has return with value inside foreach

* rename fixture

* optimize, allow multi if check, but last only

* temporary pin phpunit to 10.0.19 due to make error on paratest

* Fix phpstan

* use paratest 7.1.3

---------

Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
Co-authored-by: GitHub Action <actions@github.com>

composer.json

@@ -43,7 +43,7 @@
         "webmozart/assert": "^1.11"
     },
     "require-dev": {
-        "brianium/paratest": "^7.1.2",
+        "brianium/paratest": "^7.1.3",
         "cweagans/composer-patches": "^1.7.2",
         "icanhazstring/composer-unused": "^0.8.5",
         "myclabs/php-enum": "^1.8.4",
@@ -53,7 +53,7 @@
         "phpstan/phpstan-php-parser": "^1.1",
         "phpstan/phpstan-strict-rules": "^1.4.4",
         "phpstan/phpstan-webmozart-assert": "^1.2.2",
-        "phpunit/phpunit": "^10.0.17",
+        "phpunit/phpunit": "^10.1.0",
         "rector/phpstan-rules": "^0.6.5",
         "rector/rector-generator": "dev-main",
         "spatie/enum": "^3.13",

rector.php

@@ -82,6 +82,11 @@
             // false positive on plurals
             __DIR__ . '/packages/Testing/PHPUnit/Behavior/MovingFilesTrait.php',
         ],
+
+        // race condition with stmts aware patch and PHPStan type
+        \Rector\TypeDeclaration\Rector\ClassMethod\AddMethodCallBasedStrictParamTypeRector::class => [
+            __DIR__ . '/rules/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector.php',
+        ],
     ]);
 
     $rectorConfig->phpstanConfig(__DIR__ . '/phpstan-for-rector.neon');

rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_check_with_empty.php.inc

@@ -0,0 +1,35 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+final class IfCheckWithEmpty
+{
+    public function run(array $items)
+    {
+        if (empty($items)) {
+            return;
+        }
+
+        foreach ($items as $item) {
+            echo $item;
+        }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+final class IfCheckWithEmpty
+{
+    public function run(array $items)
+    {
+        foreach ($items as $item) {
+            echo $item;
+        }
+    }
+}
+
+?>

rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_with_return_value_inside_foreach.php.inc

@@ -0,0 +1,35 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+final class IfWithReturnValueInsideForeach
+{
+    public function run(array $items)
+    {
+        if (empty($items)) {
+            return;
+        }
+
+        foreach ($items as $item) {
+            return 1;
+        }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+final class IfWithReturnValueInsideForeach
+{
+    public function run(array $items)
+    {
+        foreach ($items as $item) {
+            return 1;
+        }
+    }
+}
+
+?>

rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/multi_if_check_with_empty.php.inc

@@ -0,0 +1,51 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+final class MultiIfCheckWithEmpty
+{
+    public function run(array $items, array $items2)
+    {
+        if (empty($items)) {
+            return;
+        }
+
+        foreach ($items as $item) {
+            echo $item;
+        }
+
+        if (empty($items2)) {
+            return;
+        }
+
+        foreach ($items2 as $item2) {
+            echo $item2;
+        }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+final class MultiIfCheckWithEmpty
+{
+    public function run(array $items, array $items2)
+    {
+        if (empty($items)) {
+            return;
+        }
+
+        foreach ($items as $item) {
+            echo $item;
+        }
+
+        foreach ($items2 as $item2) {
+            echo $item2;
+        }
+    }
+}
+
+?>

rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/skip_if_check_throw_with_empty.php.inc

@@ -0,0 +1,19 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+use Exception;
+
+final class SkipIfCheckThrowWithEmpty
+{
+    public function run(array $items)
+    {
+        if (empty($items)) {
+            throw new Exception('items must not be empty');
+        }
+
+        foreach ($items as $item) {
+            echo $item;
+        }
+    }
+}

rules/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector.php

@@ -12,6 +12,7 @@
 use PhpParser\Node\Stmt\Foreach_;
 use PhpParser\Node\Stmt\If_;
 use PHPStan\Analyser\Scope;
+use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
 use Rector\Core\NodeAnalyzer\PropertyFetchAnalyzer;
 use Rector\Core\NodeManipulator\IfManipulator;
 use Rector\Core\Php\ReservedKeywordAnalyzer;
@@ -79,28 +80,32 @@ public function run()
      */
     public function getNodeTypes(): array
     {
-        return [If_::class];
+        return [If_::class, StmtsAwareInterface::class];
     }
 
     /**
-     * @param If_ $node
-     * @return Stmt[]|Foreach_|null
+     * @param If_|StmtsAwareInterface $node
+     * @return Stmt[]|Foreach_|StmtsAwareInterface|null
      */
     public function refactorWithScope(Node $node, Scope $scope): array|Node|null
     {
-        if (! $this->isUselessBeforeForeachCheck($node, $scope)) {
-            return null;
-        }
+        if ($node instanceof If_) {
+            if (! $this->isUselessBeforeForeachCheck($node, $scope)) {
+                return null;
+            }
 
-        /** @var Foreach_ $stmt */
-        $stmt = $node->stmts[0];
-        $ifComments = $node->getAttribute(AttributeKey::COMMENTS) ?? [];
-        $stmtComments = $stmt->getAttribute(AttributeKey::COMMENTS) ?? [];
+            /** @var Foreach_ $stmt */
+            $stmt = $node->stmts[0];
+            $ifComments = $node->getAttribute(AttributeKey::COMMENTS) ?? [];
+            $stmtComments = $stmt->getAttribute(AttributeKey::COMMENTS) ?? [];
 
-        $comments = array_merge($ifComments, $stmtComments);
-        $stmt->setAttribute(AttributeKey::COMMENTS, $comments);
+            $comments = array_merge($ifComments, $stmtComments);
+            $stmt->setAttribute(AttributeKey::COMMENTS, $comments);
 
-        return $stmt;
+            return $stmt;
+        }
+
+        return $this->refactorStmtsAware($node);
     }
 
     private function isUselessBeforeForeachCheck(If_ $if, Scope $scope): bool
@@ -156,4 +161,34 @@ private function isUselessBooleanAnd(BooleanAnd $booleanAnd, Expr $foreachExpr):
 
         return $this->countManipulator->isCounterHigherThanOne($booleanAnd->right, $foreachExpr);
     }
+
+    private function refactorStmtsAware(StmtsAwareInterface $stmtsAware): ?StmtsAwareInterface
+    {
+        if ($stmtsAware->stmts === null) {
+            return null;
+        }
+
+        /** @var int $lastKey */
+        $lastKey = array_key_last($stmtsAware->stmts);
+        if (! isset($stmtsAware->stmts[$lastKey], $stmtsAware->stmts[$lastKey - 1])) {
+            return null;
+        }
+
+        $stmt = $stmtsAware->stmts[$lastKey - 1];
+        if (! $stmt instanceof If_) {
+            return null;
+        }
+
+        $nextStmt = $stmtsAware->stmts[$lastKey];
+        if (! $nextStmt instanceof Foreach_) {
+            return null;
+        }
+
+        if (! $this->uselessIfCondBeforeForeachDetector->isMatchingEmptyAndForeachedExpr($stmt, $nextStmt->expr)) {
+            return null;
+        }
+
+        unset($stmtsAware->stmts[$lastKey - 1]);
+        return $stmtsAware;
+    }
 }

rules/DeadCode/UselessIfCondBeforeForeachDetector.php

@@ -14,6 +14,7 @@
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Param;
 use PhpParser\Node\Stmt\If_;
+use PhpParser\Node\Stmt\Return_;
 use PHPStan\Analyser\Scope;
 use Rector\Core\NodeAnalyzer\ParamAnalyzer;
 use Rector\Core\PhpParser\Comparing\NodeComparator;
@@ -30,42 +31,52 @@ public function __construct(
 
     /**
      * Matches:
-     * !empty($values)
+     * empty($values)
      */
-    public function isMatchingNotEmpty(If_ $if, Expr $foreachExpr, Scope $scope): bool
+    public function isMatchingEmptyAndForeachedExpr(If_ $if, Expr $foreachExpr): bool
     {
-        $cond = $if->cond;
-        if (! $cond instanceof BooleanNot) {
-            return false;
-        }
-
-        if (! $cond->expr instanceof Empty_) {
+        if (! $if->cond instanceof Empty_) {
             return false;
         }
 
         /** @var Empty_ $empty */
-        $empty = $cond->expr;
+        $empty = $if->cond;
 
         if (! $this->nodeComparator->areNodesEqual($empty->expr, $foreachExpr)) {
             return false;
         }
 
-        // is array though?
-        $arrayType = $scope->getType($empty->expr);
-        if (! $arrayType->isArray()->yes()) {
+        if ($if->stmts === []) {
+            return true;
+        }
+
+        if (count($if->stmts) !== 1) {
             return false;
         }
 
-        $previousParam = $this->fromPreviousParam($foreachExpr);
-        if (! $previousParam instanceof Param) {
-            return true;
+        $stmt = $if->stmts[0];
+        return $stmt instanceof Return_ && ! $stmt->expr instanceof Expr;
+    }
+
+    /**
+     * Matches:
+     * !empty($values)
+     */
+    public function isMatchingNotEmpty(If_ $if, Expr $foreachExpr, Scope $scope): bool
+    {
+        $cond = $if->cond;
+        if (! $cond instanceof BooleanNot) {
+            return false;
         }
 
-        if ($this->paramAnalyzer->isNullable($previousParam)) {
+        if (! $cond->expr instanceof Empty_) {
             return false;
         }
 
-        return ! $this->paramAnalyzer->hasDefaultNull($previousParam);
+        /** @var Empty_ $empty */
+        $empty = $cond->expr;
+
+        return $this->areCondExprAndForeachExprSame($empty, $foreachExpr, $scope);
     }
 
     /**
@@ -128,4 +139,29 @@ private function isEmptyArray(Expr $expr): bool
 
         return $expr->items === [];
     }
+
+    private function areCondExprAndForeachExprSame(Empty_ $empty, Expr $foreachExpr, Scope $scope): bool
+    {
+        if (! $this->nodeComparator->areNodesEqual($empty->expr, $foreachExpr)) {
+            return false;
+        }
+
+        // is array though?
+        $arrayType = $scope->getType($empty->expr);
+        if (! $arrayType->isArray()
+            ->yes()) {
+            return false;
+        }
+
+        $previousParam = $this->fromPreviousParam($foreachExpr);
+        if (! $previousParam instanceof Param) {
+            return true;
+        }
+
+        if ($this->paramAnalyzer->isNullable($previousParam)) {
+            return false;
+        }
+
+        return ! $this->paramAnalyzer->hasDefaultNull($previousParam);
+    }
 }