[EarlyReturn] Skip ChangeAndIfToEarlyReturnRector in case of simple s…

…calar return (#2826)

build/target-repository/docs/rector_rules_overview.md

@@ -3883,12 +3883,12 @@ Changes if && to early return
      {
 -        if ($car->hasWheels && $car->hasFuel) {
 -            return true;
-+        if (!$car->hasWheels) {
++        if (! $car->hasWheels) {
 +            return false;
          }
 
 -        return false;
-+        if (!$car->hasFuel) {
++        if (! $car->hasFuel) {
 +            return false;
 +        }
 +

rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/skip_if_stmt_used_in_return.php.inc

@@ -2,18 +2,16 @@
 
 namespace Rector\Tests\EarlyReturn\Rector\If_\ChangeAndIfToEarlyReturnRector\Fixture;
 
-class SkipIfStmtUsedInReturn
+final class SkipIfStmtUsedInReturn
 {
-    public function run()
+    public function run($otherValue)
     {
         $content = '';
 
-        if (1 === 1 && 2 === 2) {
+        if ($otherValue === 1 && $otherValue === 2) {
             $content = execute($content);
         }
 
         return $content . 'execute';
     }
 }
-
-?>

rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/skip_simple_return.php.inc

@@ -0,0 +1,15 @@
+<?php
+
+namespace Rector\Tests\EarlyReturn\Rector\If_\ChangeAndIfToEarlyReturnRector\Fixture;
+
+final class SkipSimpleReturn
+{
+    public function getEerrors($fileName)
+    {
+        if (is_string($fileName) && str_contains($fileName, 'vendor')) {
+            return [];
+        }
+
+        return ['some error'];
+    }
+}

rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/skip_simple_return_of_bool.php.inc

@@ -0,0 +1,18 @@
+<?php
+
+namespace Rector\Tests\EarlyReturn\Rector\If_\ChangeAndIfToEarlyReturnRector\Fixture;
+
+use PhpParser\Node\Expr;
+use PhpParser\Node\Scalar\String_;
+
+final class SkipSimpleReturnOfBool
+{
+    public function resolve(Expr $expr)
+    {
+        if ($expr instanceof String_ && $expr->value === '') {
+            return true;
+        }
+
+        return false;
+    }
+}

rules/EarlyReturn/NodeAnalyzer/IfAndAnalyzer.php

@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\EarlyReturn\NodeAnalyzer;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr;
+use PhpParser\Node\Expr\BinaryOp\BooleanAnd;
+use PhpParser\Node\Expr\Instanceof_;
+use PhpParser\Node\Stmt\If_;
+use PhpParser\Node\Stmt\Return_;
+use Rector\Core\PhpParser\Comparing\NodeComparator;
+use Rector\Core\PhpParser\Node\BetterNodeFinder;
+
+final class IfAndAnalyzer
+{
+    public function __construct(
+        private readonly BetterNodeFinder $betterNodeFinder,
+        private readonly NodeComparator $nodeComparator,
+    ) {
+    }
+
+    public function isIfAndWithInstanceof(BooleanAnd $booleanAnd): bool
+    {
+        if (! $booleanAnd->left instanceof Instanceof_) {
+            return false;
+        }
+
+        // only one instanceof check
+        return ! $booleanAnd->right instanceof Instanceof_;
+    }
+
+    public function isIfStmtExprUsedInNextReturn(If_ $if, Return_ $return): bool
+    {
+        if (! $return->expr instanceof Expr) {
+            return false;
+        }
+
+        $ifExprs = $this->betterNodeFinder->findInstanceOf($if->stmts, Expr::class);
+        foreach ($ifExprs as $ifExpr) {
+            $isExprFoundInReturn = (bool) $this->betterNodeFinder->findFirst(
+                $return->expr,
+                fn (Node $node): bool => $this->nodeComparator->areNodesEqual($node, $ifExpr)
+            );
+            if ($isExprFoundInReturn) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+}

rules/EarlyReturn/NodeAnalyzer/SimpleScalarAnalyzer.php

@@ -0,0 +1,23 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\EarlyReturn\NodeAnalyzer;
+
+use PhpParser\Node\Expr;
+use PhpParser\Node\Expr\Array_;
+use PhpParser\Node\Scalar\String_;
+
+final class SimpleScalarAnalyzer
+{
+    public function isSimpleScalar(Expr $expr): bool
+    {
+        // empty array
+        if ($expr instanceof Array_ && $expr->items === []) {
+            return true;
+        }
+
+        // empty string
+        return $expr instanceof String_ && $expr->value === '';
+    }
+}

rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php

@@ -17,6 +17,8 @@
 use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
 use Rector\Core\NodeManipulator\IfManipulator;
 use Rector\Core\Rector\AbstractRector;
+use Rector\EarlyReturn\NodeAnalyzer\IfAndAnalyzer;
+use Rector\EarlyReturn\NodeAnalyzer\SimpleScalarAnalyzer;
 use Rector\EarlyReturn\NodeFactory\InvertedIfFactory;
 use Rector\NodeCollector\BinaryOpConditionsCollector;
 use Rector\NodeNestingScope\ContextAnalyzer;
@@ -33,7 +35,9 @@ public function __construct(
         private readonly IfManipulator $ifManipulator,
         private readonly InvertedIfFactory $invertedIfFactory,
         private readonly ContextAnalyzer $contextAnalyzer,
-        private readonly BinaryOpConditionsCollector $binaryOpConditionsCollector
+        private readonly BinaryOpConditionsCollector $binaryOpConditionsCollector,
+        private readonly SimpleScalarAnalyzer $simpleScalarAnalyzer,
+        private readonly IfAndAnalyzer $ifAndAnalyzer,
     ) {
     }
 
@@ -61,11 +65,11 @@ class SomeClass
 {
     public function canDrive(Car $car)
     {
-        if (!$car->hasWheels) {
+        if (! $car->hasWheels) {
             return false;
         }
 
-        if (!$car->hasFuel) {
+        if (! $car->hasFuel) {
             return false;
         }
 
@@ -99,6 +103,7 @@ public function refactor(Node $node): ?Node
 
         foreach ($stmts as $key => $stmt) {
             if (! $stmt instanceof If_) {
+                // keep natural original order
                 $newStmts[] = $stmt;
                 continue;
             }
@@ -111,7 +116,7 @@ public function refactor(Node $node): ?Node
             }
 
             if ($nextStmt instanceof Return_) {
-                if ($this->isIfStmtExprUsedInNextReturn($stmt, $nextStmt)) {
+                if ($this->ifAndAnalyzer->isIfStmtExprUsedInNextReturn($stmt, $nextStmt)) {
                     continue;
                 }
 
@@ -225,27 +230,19 @@ private function shouldSkip(StmtsAwareInterface $stmtsAware, If_ $if, ?Stmt $nex
             return true;
         }
 
-        return ! $this->isLastIfOrBeforeLastReturn($if, $nexStmt);
-    }
-
-    private function isIfStmtExprUsedInNextReturn(If_ $if, Return_ $return): bool
-    {
-        if (! $return->expr instanceof Expr) {
-            return false;
+        // is simple return? skip it
+        $onlyStmt = $if->stmts[0];
+        if ($onlyStmt instanceof Return_ && $onlyStmt->expr instanceof Expr && $this->simpleScalarAnalyzer->isSimpleScalar(
+            $onlyStmt->expr
+        )) {
+            return true;
         }
 
-        $ifExprs = $this->betterNodeFinder->findInstanceOf($if->stmts, Expr::class);
-        foreach ($ifExprs as $ifExpr) {
-            $isExprFoundInReturn = (bool) $this->betterNodeFinder->findFirst(
-                $return->expr,
-                fn (Node $node): bool => $this->nodeComparator->areNodesEqual($node, $ifExpr)
-            );
-            if ($isExprFoundInReturn) {
-                return true;
-            }
+        if ($this->ifAndAnalyzer->isIfAndWithInstanceof($if->cond)) {
+            return true;
         }
 
-        return false;
+        return ! $this->isLastIfOrBeforeLastReturn($if, $nexStmt);
     }
 
     private function isParentIfReturnsVoidOrParentIfHasNextNode(StmtsAwareInterface $stmtsAware): bool

src/NodeManipulator/IfManipulator.php

@@ -250,7 +250,7 @@ public function isIfWithoutElseAndElseIfs(If_ $if): bool
             return false;
         }
 
-        return ! (bool) $if->elseifs;
+        return $if->elseifs === [];
     }
 
     public function createIfNegation(Expr $expr, Return_ $return): If_
@@ -259,9 +259,9 @@ public function createIfNegation(Expr $expr, Return_ $return): If_
         return $this->createIfStmt($expr, $return);
     }
 
-    public function createIfStmt(Expr $expr, Stmt $stmt): If_
+    public function createIfStmt(Expr $condExpr, Stmt $stmt): If_
     {
-        return new If_($expr, [
+        return new If_($condExpr, [
             'stmts' => [$stmt],
         ]);
     }