[DeadCode] Add early return check to RemoveUnusedNonEmptyArrayBeforeForeachRector

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/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-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/skip_multi_if_check_with_empty.php.inc

@@ -0,0 +1,25 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector\Fixture;
+
+final class SkipMultiIfCheckWithEmpty
+{
+    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;
+        }
+    }
+}

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,35 @@ private function isUselessBooleanAnd(BooleanAnd $booleanAnd, Expr $foreachExpr):
 
         return $this->countManipulator->isCounterHigherThanOne($booleanAnd->right, $foreachExpr);
     }
+
+    private function refactorStmtsAware(StmtsAwareInterface $stmtsAware): ?StmtsAwareInterface
+    {
+        foreach ((array) $stmtsAware->stmts as $key => $stmt) {
+            if (! $stmt instanceof If_) {
+                continue;
+            }
+
+            $nextStmt = $stmtsAware->stmts[$key + 1] ?? null;
+            if (! $nextStmt instanceof Foreach_) {
+                continue;
+            }
+
+            // the foreach must be the last one
+            if (isset($stmtsAware->stmts[$key + 2])) {
+                return null;
+            }
+
+            if (! $this->uselessIfCondBeforeForeachDetector->isMatchingEmptyAndForeachedExpr(
+                $stmt,
+                $nextStmt->expr
+            )) {
+                continue;
+            }
+
+            unset($stmtsAware->stmts[$key]);
+            return $stmtsAware;
+        }
+
+        return null;
+    }
 }

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);
+    }
 }