[PHP 8.0] Fix ChangeSwitchToMatchRector for next new variable re-use (#…

…2798) Co-authored-by: Sergey Burkun <sburkun@eyeconweb.com>
rectorphp · Aug 19, 2022 · d92936b · d92936b
1 parent 66d273e
commit d92936b
Show file tree

Hide file tree

Showing 6 changed files with 114 additions and 22 deletions.
diff --git a/phpstan.neon b/phpstan.neon
@@ -702,3 +702,12 @@ parameters:
                 - packages/BetterPhpDocParser/ValueObject/PhpDoc/DoctrineAnnotation/AbstractValuesAwareNode.php
                 - packages/BetterPhpDocParser/PhpDoc/ArrayItemNode.php
 
+
+        # @todo refactor to StmtsAware in next PR
+        -
+            message: '#Cognitive complexity for "Rector\\Php80\\Rector\\Switch_\\ChangeSwitchToMatchRector\:\:refactor\(\)" is 11, keep it under 10#'
+            path: rules/Php80/Rector/Switch_/ChangeSwitchToMatchRector.php
+
+        -
+            message: '#Class cognitive complexity is 36, keep it under 35#'
+            path: rules/Php80/Rector/Switch_/ChangeSwitchToMatchRector.php
diff --git a/...hp80/Rector/Switch_/ChangeSwitchToMatchRector/Fixture/assign_and_use_as_parameter.php.inc b/...hp80/Rector/Switch_/ChangeSwitchToMatchRector/Fixture/assign_and_use_as_parameter.php.inc
@@ -0,0 +1,52 @@
+<?php
+
+namespace Rector\Tests\Php80\Rector\Switch_\ChangeSwitchToMatchRector\Fixture;
+
+use Rector\Tests\Php80\Rector\Switch_\ChangeSwitchToMatchRector\Source\SomeResponse;
+
+final class AssignAndUseAsParameter
+{
+    public function getErrorResponse(int $errorCode): SomeResponse
+    {
+        switch ($errorCode) {
+            case 1:
+                $content = 'BAD REQUEST';
+                break;
+            case 2:
+                $content = 'CHANGE STATUS FAILED';
+                break;
+            case 3:
+                $content = 'NOT FOUND';
+                break;
+            default:
+                $content = 'FAILED';
+        }
+
+        return new SomeResponse($content, 400);
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\Php80\Rector\Switch_\ChangeSwitchToMatchRector\Fixture;
+
+use Rector\Tests\Php80\Rector\Switch_\ChangeSwitchToMatchRector\Source\SomeResponse;
+
+final class AssignAndUseAsParameter
+{
+    public function getErrorResponse(int $errorCode): SomeResponse
+    {
+        $content = match ($errorCode) {
+            1 => 'BAD REQUEST',
+            2 => 'CHANGE STATUS FAILED',
+            3 => 'NOT FOUND',
+            default => 'FAILED',
+        };
+
+        return new SomeResponse($content, 400);
+    }
+}
+
+?>
diff --git a/...itchToMatchRector/Fixture/skip_assign_on_its_var_not_directly_used_in_next_return.php.inc b/...itchToMatchRector/Fixture/skip_assign_on_its_var_not_directly_used_in_next_return.php.inc
@@ -6,23 +6,16 @@ namespace Rector\Tests\Php80\Rector\Switch_\ChangeSwitchToMatchRector\Fixture;
 
 final class SkipAssignOnItsVarNotDirectlyUsedInNextReturn
 {
-    public const STATUS_A = 'a';
     public const STATUS_B = 'b';
     public const STATUS_C = 'c';
-    public const STATUS_D = 'd';
 
     public function run(): \Closure
     {
         return static function (self $item) {
             $class = [];
             switch ($item->status) {
-                    case self::STATUS_A:
-                            $class[] = 'green';
-                            break;
-
                     case self::STATUS_B:
                     case self::STATUS_C:
-                    case self::STATUS_D:
                             $class[] = 'red';
                             break;
             }

diff --git a/rules-tests/Php80/Rector/Switch_/ChangeSwitchToMatchRector/Source/SomeResponse.php b/rules-tests/Php80/Rector/Switch_/ChangeSwitchToMatchRector/Source/SomeResponse.php
@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Php80\Rector\Switch_\ChangeSwitchToMatchRector\Source;
+
+final class SomeResponse
+{
+    public function __construct($content, $statusCode)
+    {
+    }
+}
diff --git a/rules/Php80/NodeAnalyzer/MatchSwitchAnalyzer.php b/rules/Php80/NodeAnalyzer/MatchSwitchAnalyzer.php
@@ -79,8 +79,8 @@ public function haveCondAndExprsMatchPotential(array $condAndExprs): bool
             }
 
             if ($expr->var instanceof ArrayDimFetch) {
-                $arrayDimFethName = $this->nodeNameResolver->getName($expr->var->var);
-                $assignVariableNames[] = $expr->var::class . $arrayDimFethName . '[]';
+                $arrayDimFecthName = $this->nodeNameResolver->getName($expr->var->var);
+                $assignVariableNames[] = $expr->var::class . $arrayDimFecthName . '[]';
             } else {
                 $assignVariableNames[] = $expr->var::class . $this->nodeNameResolver->getName($expr->var);
             }

diff --git a/rules/Php80/Rector/Switch_/ChangeSwitchToMatchRector.php b/rules/Php80/Rector/Switch_/ChangeSwitchToMatchRector.php
@@ -6,10 +6,12 @@
 
 use PhpParser\Node;
 use PhpParser\Node\Expr;
+use PhpParser\Node\Expr\ArrayDimFetch;
 use PhpParser\Node\Expr\Assign;
 use PhpParser\Node\Expr\Match_;
 use PhpParser\Node\Expr\Throw_;
 use PhpParser\Node\MatchArm;
+use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\Expression;
 use PhpParser\Node\Stmt\Return_;
 use PhpParser\Node\Stmt\Switch_;
@@ -74,6 +76,7 @@ public function getRuleDefinition(): RuleDefinition
      */
     public function getNodeTypes(): array
     {
+        // @todo refactor to stmts aware interface to move away from NEXT_NODE
         return [Switch_::class];
     }
 
@@ -113,6 +116,9 @@ public function refactor(Node $node): ?Node
 
         // implicit return default after switch
         $match = $this->processImplicitReturnAfterSwitch($node, $match, $condAndExprs);
+        if (! $match instanceof Match_) {
+            return null;
+        }
 
         $match = $this->processImplicitThrowsAfterSwitch($node, $match, $condAndExprs);
 
@@ -139,12 +145,11 @@ public function provideMinPhpVersion(): int
 
     private function changeToAssign(Switch_ $switch, Match_ $match, Expr $expr, bool $hasDefaultValue): ?Assign
     {
-        $nextReturn = $switch->getAttribute(AttributeKey::NEXT_NODE);
+        /** @var Stmt|null $nextStmt */
+        $nextStmt = $switch->getAttribute(AttributeKey::NEXT_NODE);
 
-        if ($nextReturn instanceof Return_ && $nextReturn->expr instanceof Expr && ! $this->nodeComparator->areNodesEqual(
-            $expr,
-            $nextReturn->expr
-        )) {
+        // containts next this expr?
+        if (! $hasDefaultValue && $this->isFollowedByReturnWithExprUsage($nextStmt, $expr)) {
             return null;
         }
 
@@ -180,9 +185,7 @@ private function changeToAssign(Switch_ $switch, Match_ $match, Expr $expr, bool
 
     private function resolveCurrentAssign(bool $hasDefaultValue, Assign $assign): ?Assign
     {
-        return $hasDefaultValue
-            ? $assign
-            : null;
+        return $hasDefaultValue ? $assign : null;
     }
 
     /**
@@ -205,14 +208,14 @@ private function resolveAssignVar(array $condAndExprs): ?Expr
     /**
      * @param CondAndExpr[] $condAndExprs
      */
-    private function processImplicitReturnAfterSwitch(Switch_ $switch, Match_ $match, array $condAndExprs): Match_
+    private function processImplicitReturnAfterSwitch(Switch_ $switch, Match_ $match, array $condAndExprs): ?Match_
     {
-        $nextNode = $switch->getAttribute(AttributeKey::NEXT_NODE);
-        if (! $nextNode instanceof Return_) {
+        $nextStmt = $switch->getAttribute(AttributeKey::NEXT_NODE);
+        if (! $nextStmt instanceof Return_) {
             return $match;
         }
 
-        $returnedExpr = $nextNode->expr;
+        $returnedExpr = $nextStmt->expr;
         if (! $returnedExpr instanceof Expr) {
             return $match;
         }
@@ -222,9 +225,12 @@ private function processImplicitReturnAfterSwitch(Switch_ $switch, Match_ $match
         }
 
         $assignVar = $this->resolveAssignVar($condAndExprs);
+        if ($assignVar instanceof ArrayDimFetch) {
+            return null;
+        }
 
         if (! $assignVar instanceof Expr) {
-            $this->removeNode($nextNode);
+            $this->removeNode($nextStmt);
         }
 
         $condAndExprs[] = new CondAndExpr([], $returnedExpr, MatchKind::RETURN);
@@ -252,4 +258,24 @@ private function processImplicitThrowsAfterSwitch(Switch_ $switch, Match_ $match
         $condAndExprs[] = new CondAndExpr([], $throw, MatchKind::RETURN);
         return $this->matchFactory->createFromCondAndExprs($switch->cond, $condAndExprs);
     }
+
+    private function isFollowedByReturnWithExprUsage(Stmt|null $nextStmt, Expr $expr): bool
+    {
+        if (! $nextStmt instanceof Return_) {
+            return false;
+        }
+
+        if (! $nextStmt->expr instanceof Expr) {
+            return false;
+        }
+
+        $returnExprs = $this->betterNodeFinder->findInstanceOf($nextStmt, Expr::class);
+        foreach ($returnExprs as $returnExpr) {
+            if ($this->nodeComparator->areNodesEqual($expr, $returnExpr)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }