From fcb2dc6f33eb6a69e72cb3bf42de24ccbb7b0639 Mon Sep 17 00:00:00 2001
From: Abdul Malik Ikhsan <samsonasik@gmail.com>
Date: Sat, 14 May 2022 14:32:02 +0700
Subject: [PATCH] [CodeQuality] Do not duplicate Expr on
 SimplifyIfElseToTernaryRector+SimplifyUselessVariableRector+CompleteDynamicPropertiesRector
 (#2308)

* [CodeQuality] Do not duplicate Expr on SimplifyIfElseToTernaryRector+SimplifyUselessVariableRector+CompleteDynamicPropertiesRector

* Fixed :tada:

* clen up
---
 .../SimplifyUselessVariableRector.php         | 69 ++++++++++---------
 .../Fixture/do_not_duplicated_expr.php.inc    | 37 ++++++++++
 .../SimplifyVariableIfElseTernaryTest.php     | 33 +++++++++
 .../config/configured_rule.php                | 16 +++++
 4 files changed, 122 insertions(+), 33 deletions(-)
 create mode 100644 tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc
 create mode 100644 tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.php
 create mode 100644 tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php

diff --git a/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php b/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php
index 9efbd7989fc..c8ca09fbc21 100644
--- a/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php
+++ b/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php
@@ -73,53 +73,56 @@ CODE_SAMPLE
             return null;
         }
 
-        $previousStmt = null;
-        $hasChanged = false;
-
-        foreach ($stmts as $stmt) {
-            if ($previousStmt === null || ! $stmt instanceof Return_) {
-                $previousStmt = $stmt;
+        foreach ($stmts as $key => $stmt) {
+            if (! isset($stmts[$key - 1])) {
                 continue;
             }
 
+            if (! $stmt instanceof Return_) {
+                continue;
+            }
+
+            $previousStmt = $stmts[$key - 1];
             if ($this->shouldSkipStmt($stmt, $previousStmt)) {
-                $previousStmt = $stmt;
                 continue;
             }
 
-            /** @var Expression $previousStmt */
-            /** @var Assign|AssignOp $previousNode */
-            $previousNode = $previousStmt->expr;
-
-            /** @var Return_ $stmt */
-            if ($previousNode instanceof Assign) {
-                if ($this->isReturnWithVarAnnotation($stmt)) {
-                    continue;
-                }
-
-                $stmt->expr = $previousNode->expr;
-            } else {
-                $binaryClass = $this->assignAndBinaryMap->getAlternative($previousNode);
-                if ($binaryClass === null) {
-                    continue;
-                }
-
-                $stmt->expr = new $binaryClass($previousNode->var, $previousNode->expr);
+            if ($this->isReturnWithVarAnnotation($stmt)) {
+                continue;
             }
 
-            $this->removeNode($previousStmt);
-            $hasChanged = true;
-
-            $previousStmt = $stmt;
-        }
-
-        if ($hasChanged) {
-            return $node;
+            /**
+             * @var Expression $previousStmt
+             * @var Assign|AssignOp $assign
+             */
+            $assign = $previousStmt->expr;
+            return $this->processSimplifyUselessVariable($node, $stmt, $assign, $key);
         }
 
         return null;
     }
 
+    private function processSimplifyUselessVariable(
+        FunctionLike $functionLike,
+        Return_ $return,
+        Assign|AssignOp $assign,
+        int $key
+    ): ?FunctionLike {
+        if (! $assign instanceof Assign) {
+            $binaryClass = $this->assignAndBinaryMap->getAlternative($assign);
+            if ($binaryClass === null) {
+                return null;
+            }
+
+            $return->expr = new $binaryClass($assign->var, $assign->expr);
+        } else {
+            $return->expr = $assign->expr;
+        }
+
+        unset($functionLike->stmts[$key - 1]);
+        return $functionLike;
+    }
+
     private function shouldSkipStmt(Return_ $return, Stmt $previousStmt): bool
     {
         if ($this->hasSomeComment($previousStmt)) {
diff --git a/tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc b/tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc
new file mode 100644
index 00000000000..01be9c7b6df
--- /dev/null
+++ b/tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc
@@ -0,0 +1,37 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\SimplifyVariableIfElseTernary\Fixture;
+
+final class DoNotDuplicateExpr
+{
+    function test($foo, $bar)
+    {
+        if ($foo > 0) {
+            $baz = 'a';
+        } else {
+            $baz = 'b';
+        }
+
+        return $baz;
+    }
+}
+
+?>
+-----
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\SimplifyVariableIfElseTernary\Fixture;
+
+final class DoNotDuplicateExpr
+{
+    function test($foo, $bar)
+    {
+        return $foo > 0 ? 'a' : 'b';
+    }
+}
+
+?>
diff --git a/tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.php b/tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.php
new file mode 100644
index 00000000000..d7af27ce6ed
--- /dev/null
+++ b/tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.php
@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\SimplifyVariableIfElseTernary;
+
+use Iterator;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+use Symplify\SmartFileSystem\SmartFileInfo;
+
+final class SimplifyVariableIfElseTernaryTest extends AbstractRectorTestCase
+{
+    /**
+     * @dataProvider provideData()
+     */
+    public function test(SmartFileInfo $fileInfo): void
+    {
+        $this->doTestFileInfo($fileInfo);
+    }
+
+    /**
+     * @return Iterator<SmartFileInfo>
+     */
+    public function provideData(): Iterator
+    {
+        return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/configured_rule.php';
+    }
+}
diff --git a/tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php b/tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php
new file mode 100644
index 00000000000..f1389f44239
--- /dev/null
+++ b/tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php
@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector;
+use Rector\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector;
+use Rector\CodeQuality\Rector\If_\SimplifyIfElseToTernaryRector;
+use Rector\Config\RectorConfig;
+
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->rules([
+        SimplifyIfElseToTernaryRector::class,
+        SimplifyUselessVariableRector::class,
+        CompleteDynamicPropertiesRector::class,
+    ]);
+};