Merge pull request #2710 from jeroensmit/splitIfs

docs/AllRectorsOverview.md

@@ -7148,7 +7148,7 @@ Finalize every class constant that is used only locally
 
 - class: `Rector\SOLID\Rector\If_\RemoveAlwaysElseRector`
 
-Remove if for last else, if previous values were throw
+Split if statement, when if condition always break execution flow
 
 ```diff
  class SomeClass

packages/SOLID/src/Rector/If_/RemoveAlwaysElseRector.php

@@ -5,30 +5,25 @@
 namespace Rector\SOLID\Rector\If_;
 
 use PhpParser\Node;
+use PhpParser\Node\Expr\Exit_;
+use PhpParser\Node\Stmt\Continue_;
+use PhpParser\Node\Stmt\ElseIf_;
+use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\If_;
-use Rector\PhpParser\Node\Manipulator\IfManipulator;
+use PhpParser\Node\Stmt\Return_;
+use PhpParser\Node\Stmt\Throw_;
 use Rector\Rector\AbstractRector;
 use Rector\RectorDefinition\CodeSample;
 use Rector\RectorDefinition\RectorDefinition;
 
 /**
- * @see \Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElseRector\RemoveAlwaysElseRectorTest
+ * @see \Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\SplitIfsRectorTest
  */
 final class RemoveAlwaysElseRector extends AbstractRector
 {
-    /**
-     * @var IfManipulator
-     */
-    private $ifManipulator;
-
-    public function __construct(IfManipulator $ifManipulator)
-    {
-        $this->ifManipulator = $ifManipulator;
-    }
-
     public function getDefinition(): RectorDefinition
     {
-        return new RectorDefinition('Remove if for last else, if previous values were throw', [
+        return new RectorDefinition('Split if statement, when if condition always break execution flow', [
             new CodeSample(
                 <<<'PHP'
 class SomeClass
@@ -75,16 +70,37 @@ public function getNodeTypes(): array
      */
     public function refactor(Node $node): ?Node
     {
-        if (! $this->ifManipulator->isEarlyElse($node)) {
+        if ($this->lastStatementBreaksFlow($node)) {
             return null;
         }
+        if ($node->elseifs !== []) {
+            $newNode = new If_($node->cond);
+            $newNode->stmts = $node->stmts;
+            $this->addNodeBeforeNode($newNode, $node);
+            /** @var ElseIf_ $firstElseIf */
+            $firstElseIf = array_shift($node->elseifs);
+            $node->cond = $firstElseIf->cond;
+            $node->stmts = $firstElseIf->stmts;
+            return $node;
+        }
 
-        foreach ($node->else->stmts as $elseStmt) {
-            $this->addNodeAfterNode($elseStmt, $node);
+        if ($node->else !== null) {
+            foreach ($node->else->stmts as $stmt) {
+                $this->addNodeAfterNode($stmt, $node);
+            }
+            $node->else = null;
+            return $node;
         }
 
-        $node->else = null;
+        return null;
+    }
 
-        return $node;
+    protected function lastStatementBreaksFlow(Node $node): bool
+    {
+        $lastStmt = end($node->stmts);
+        return ! ($lastStmt instanceof Return_
+            || $lastStmt instanceof Throw_
+            || $lastStmt instanceof Continue_
+            || ($lastStmt instanceof Expression && $lastStmt->expr instanceof Exit_));
     }
 }

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/elseif.php.inc

@@ -0,0 +1,44 @@
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\ElseIf_;
+
+class SomeClass
+{
+    public function run()
+    {
+        if ($cond1) {
+            return 'foo';
+        } elseif ($cond2) {
+            bar();
+        } elseif ($cond3) {
+            baz();
+        } else {
+            foo();
+        }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\ElseIf_;
+
+class SomeClass
+{
+    public function run()
+    {
+        if ($cond1) {
+            return 'foo';
+        }
+        if ($cond2) {
+            bar();
+        } elseif ($cond3) {
+            baz();
+        } else {
+            foo();
+        }
+    }
+}
+
+?>

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/empty_if.php.inc

@@ -0,0 +1,17 @@
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\EmptyIf;
+
+class SomeClass {
+    public function run()
+    {
+        if ($cond1) {
+
+        } else {
+            foo();
+            return 'bar';
+        }
+    }
+}
+
+?>

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/exit.php.inc

@@ -0,0 +1,34 @@
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Exit_;
+
+class SomeClass {
+    public function run()
+    {
+        if ($cond1) {
+            exit('bye');
+        } else {
+            foo();
+            return 'bar';
+        }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Exit_;
+
+class SomeClass {
+    public function run()
+    {
+        if ($cond1) {
+            exit('bye');
+        }
+        foo();
+        return 'bar';
+    }
+}
+
+?>

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/no_break.php.inc

@@ -0,0 +1,16 @@
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\NoBreak;
+
+class SomeClass {
+    public function run()
+    {
+        if ($cond1) {
+            foo();
+        } else {
+            return 'bar';
+        }
+    }
+}
+
+?>

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/process_empty_return_last.php.inc

@@ -4,11 +4,24 @@ namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Fixture;
 
 class ProcessEmptyReturnLast
 {
-    public function runAgain($value)
+    public function firstRun($value)
     {
         if ($value) {
             return 5;
-        } elseif ($value- 1) {
+        } elseif ($value - 1) {
+            $value = 55;
+            return 10;
+        } else {
+            return;
+        }
+    }
+
+    public function secondRun($value)
+    {
+        if ($value) {
+            return 5;
+        }
+        if ($value - 1) {
             $value = 55;
             return 10;
         } else {
@@ -25,11 +38,25 @@ namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Fixture;
 
 class ProcessEmptyReturnLast
 {
-    public function runAgain($value)
+    public function firstRun($value)
     {
         if ($value) {
             return 5;
-        } elseif ($value- 1) {
+        }
+        if ($value - 1) {
+            $value = 55;
+            return 10;
+        } else {
+            return;
+        }
+    }
+
+    public function secondRun($value)
+    {
+        if ($value) {
+            return 5;
+        }
+        if ($value - 1) {
             $value = 55;
             return 10;
         }

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/simple_if.php.inc

@@ -0,0 +1,15 @@
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\SimpleIf;
+
+class SomeClass
+{
+    public function run()
+    {
+        if ($cond1) {
+            return 'foo';
+        }
+    }
+}
+
+?>

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/skip_not_only_else.php.inc

@@ -4,17 +4,6 @@ namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Fixture;
 
 class SkipNotOnlyElse
 {
-    public function run($value)
-    {
-        if ($value) {
-            return 5;
-        } elseif ($value- 1) {
-            $value = 55;
-            return;
-        } else {
-            return 10;
-        }
-    }
 
     public function runAgainAndAgain($value)
     {

packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/throw.php.inc

@@ -0,0 +1,34 @@
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Throw_;
+
+class SomeClass {
+    public function run()
+    {
+        if ($cond1) {
+            throw new \Exception('should not happen');
+        } else {
+            foo();
+            return 'bar';
+        }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Throw_;
+
+class SomeClass {
+    public function run()
+    {
+        if ($cond1) {
+            throw new \Exception('should not happen');
+        }
+        foo();
+        return 'bar';
+    }
+}
+
+?>