[DeadCode] Handle skipped RemoveUnreachableStatementRector on MultiDirnameRector (#2129)

* [DeadCode] Handle skipped RemoveUnreachableStatementRector on MultiDirnameRector

* Fixed 🎉

* Fixed 🎉

* add fixture for has side effect not return early

* final touch: first stmt is never removed

* final touch: back start from - 2 as jump

* really final touch: clean up

* really final touch: comment

* final touch: no - 1 means it already last, so skip early

rector.php

@@ -7,7 +7,6 @@ use Rector\CodingStyle\Rector\ClassMethod\ReturnArrayClassMethodToYieldRector;
 use Rector\CodingStyle\Rector\MethodCall\PreferThisOrSelfMethodCallRector;
 use Rector\CodingStyle\ValueObject\ReturnArrayClassMethodToYield;
 use Rector\Config\RectorConfig;
-use Rector\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector;
 use Rector\Nette\Set\NetteSetList;
 use Rector\Php55\Rector\String_\StringClassNameToClassConstantRector;
 use Rector\Php81\Rector\Class_\MyCLabsClassToEnumRector;
@@ -63,11 +62,6 @@ return static function (RectorConfig $rectorConfig): void {
     $rectorConfig->skip([
         StringClassNameToClassConstantRector::class,
 
-        // phpstan false positive unreachable
-        RemoveUnreachableStatementRector::class => [
-            __DIR__ . '/rules/Php70/Rector/FuncCall/MultiDirnameRector.php',
-        ],
-
         FinalizeClassesWithoutChildrenRector::class => [
             __DIR__ . '/rules/DowngradePhp74/Rector/Array_/DowngradeArraySpreadRector.php',
         ],

rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/side_effect_return_early.php.inc

@@ -0,0 +1,31 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fixture;
+
+class SideEffectReturnEarly
+{
+    public function run()
+    {
+        $this->sideEffect();
+        return 5;
+
+        $removeMe = 10;
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fixture;
+
+class SideEffectReturnEarly
+{
+    public function run()
+    {
+        $this->sideEffect();
+        return 5;
+    }
+}
+
+?>

rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_side_effect_not_return_early.php.inc

@@ -0,0 +1,21 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fixture;
+
+class SkipSideEffectNotReturnEarly
+{
+    public function run()
+    {
+        $this->value = 1;
+
+        if ($this->sideEffect()) {
+
+        }
+
+        if ($this->value === 1) {
+            return 'a';
+        }
+
+        return 'b';
+    }
+}

rules/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector.php

@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace Rector\DeadCode\Rector\Stmt;
 
 use PhpParser\Node;
+use PhpParser\Node\Expr\Exit_;
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\StaticCall;
 use PhpParser\Node\FunctionLike;
@@ -14,7 +15,10 @@ use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\Foreach_;
 use PhpParser\Node\Stmt\If_;
 use PhpParser\Node\Stmt\Nop;
+use PhpParser\Node\Stmt\Return_;
+use PhpParser\Node\Stmt\Throw_;
 use Rector\Core\Rector\AbstractRector;
+use Rector\DeadCode\SideEffect\SideEffectNodeDetector;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
@@ -26,6 +30,10 @@ use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
  */
 final class RemoveUnreachableStatementRector extends AbstractRector
 {
+    public function __construct(private readonly SideEffectNodeDetector $sideEffectNodeDetector)
+    {
+    }
+
     public function getRuleDefinition(): RuleDefinition
     {
         return new RuleDefinition('Remove unreachable statements', [
@@ -71,7 +79,7 @@ CODE_SAMPLE
         $stmts = $node->stmts;
 
         $isPassedTheUnreachable = false;
-        $hasChanged = false;
+        $toBeRemovedKeys = [];
 
         foreach ($stmts as $key => $stmt) {
             if ($this->shouldSkipNode($stmt)) {
@@ -91,15 +99,61 @@ CODE_SAMPLE
                 continue;
             }
 
-            unset($stmts[$key]);
-            $hasChanged = true;
+            $toBeRemovedKeys[] = $key;
         }
 
-        if (! $hasChanged) {
+        if ($toBeRemovedKeys === []) {
             return null;
         }
 
-        $node->stmts = $stmts;
+        $start = reset($toBeRemovedKeys);
+        if (! isset($stmts[$start - 1])) {
+            return null;
+        }
+
+        $previousFirstUnreachable = $stmts[$start - 1];
+        if (in_array($previousFirstUnreachable::class, [Throw_::class, Return_::class, Exit_::class], true)) {
+            return $this->processCleanUpUnreachabelStmts($node, $toBeRemovedKeys);
+        }
+
+        // check previous side effect can check against start jump key - 2
+        // as previously already checked as reachable part
+        if (! $this->hasPreviousSideEffect($start - 2, $stmts)) {
+            return $this->processCleanUpUnreachabelStmts($node, $toBeRemovedKeys);
+        }
+
+        return null;
+    }
+
+    /**
+     * @param Stmt[] $stmts
+     */
+    private function hasPreviousSideEffect(int $start, array $stmts): bool
+    {
+        for ($key = $start; $key > 0; --$key) {
+            $previousStmt = $stmts[$key];
+            $hasSideEffect = (bool) $this->betterNodeFinder->findFirst(
+                $previousStmt,
+                fn (Node $node): bool => $this->sideEffectNodeDetector->detectCallExpr($node)
+            );
+
+            if ($hasSideEffect) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * @param int[] $toBeRemovedKeys
+     */
+    private function processCleanUpUnreachabelStmts(Foreach_|FunctionLike|Else_|If_ $node, array $toBeRemovedKeys): Node
+    {
+        foreach ($toBeRemovedKeys as $toBeRemovedKey) {
+            unset($node->stmts[$toBeRemovedKey]);
+        }
+
         return $node;
     }