[Core] Remove RectifiedNode value object for RectifiedAnalyzer (#3079)

* [Core] Remove RectifiedNode value object for RectifiedAnalyzer * clean up * Final touch: apply @var class-string<RectorInterface>[] and @param class-string<RectorInterface> for created by rule attribute * Really Final touch: handle consecutive return null; with phpstan cache printer exists * Really Really Final touch: early create by rule, no need to loop when $node === $originalNode * Really Really Final touch: clean up: re-use if * Really Final touch: different if to easiser to read * Final touch: better method name * Final touch: Move comment to method, more description for future note * Really final touch: clarify comment * Really Really Really final touch: verify comment * Really Final touch: remove unnecessary array_key_first and array_key_last * Final touch: phpstan_cache_printer attribute is always on Expr Node * Final touch: Fix range line can be -1 on if Next Node moved up * Final touch: comment * Final touch: comment * [ci-review] Rector Rectify * Final touch: clean up * Final touch: clean up * only verify against Expr * final touch: decorate the Node as well * Finally: Real solution, immediate connect existing Node after refactor * Really Really Really Final touch: Fix PHPStan notice * Final touch: remove unrelated change * Final touch: clean up unrelated change * Final touch: test to prove that existing addNodeBeforeNode() usage keep working * add test for fixture for keep working with addNodeAfterNode() on Next Stmt * Final touch: better fixture content * [ci-review] Rector Rectify Co-authored-by: GitHub Action <action@github.com>
rectorphp · Nov 20, 2022 · eddc376 · eddc376
1 parent 23c2282
commit eddc376
Show file tree

Hide file tree

Showing 9 changed files with 195 additions and 106 deletions.
diff --git a/...tor/FuncCall/NonVariableToVariableOnFunctionCallRector/Fixture/has_prev_next_stmt.php.inc b/...tor/FuncCall/NonVariableToVariableOnFunctionCallRector/Fixture/has_prev_next_stmt.php.inc
@@ -0,0 +1,26 @@
+<?php
+
+namespace Rector\Tests\Php70\Rector\FuncCall\NonVariableToVariableOnFunctionCallRector\Fixture;
+
+function hasPrevNextStmt()
+{
+    echo 'prev stmt';
+    reset($var = []);
+    echo 'next stmt';
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\Php70\Rector\FuncCall\NonVariableToVariableOnFunctionCallRector\Fixture;
+
+function hasPrevNextStmt()
+{
+    echo 'prev stmt';
+    $var = [];
+    reset($var);
+    echo 'next stmt';
+}
+
+?>
diff --git a/src/NodeDecorator/CreatedByRuleDecorator.php b/src/NodeDecorator/CreatedByRuleDecorator.php
@@ -5,15 +5,22 @@
 namespace Rector\Core\NodeDecorator;
 
 use PhpParser\Node;
+use Rector\Core\Contract\Rector\RectorInterface;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 
 final class CreatedByRuleDecorator
 {
     /**
      * @param array<Node>|Node $node
+     * @param class-string<RectorInterface> $rectorClass
      */
     public function decorate(array | Node $node, Node $originalNode, string $rectorClass): void
     {
+        if ($node instanceof Node && $node === $originalNode) {
+            $this->createByRule($node, $rectorClass);
+            return;
+        }
+
         if ($node instanceof Node) {
             $node = [$node];
         }
@@ -27,19 +34,22 @@ public function decorate(array | Node $node, Node $originalNode, string $rectorC
         $this->createByRule($originalNode, $rectorClass);
     }
 
+    /**
+     * @param class-string<RectorInterface> $rectorClass
+     */
     private function createByRule(Node $node, string $rectorClass): void
     {
+        /** @var class-string<RectorInterface>[] $createdByRule */
         $createdByRule = $node->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
-        $lastRectorRuleKey = array_key_last($createdByRule);
 
         // empty array, insert
-        if ($lastRectorRuleKey === null) {
+        if ($createdByRule === []) {
             $node->setAttribute(AttributeKey::CREATED_BY_RULE, [$rectorClass]);
             return;
         }
 
         // consecutive, no need to refill
-        if ($createdByRule[$lastRectorRuleKey] === $rectorClass) {
+        if (end($createdByRule) === $rectorClass) {
             return;
         }
 

diff --git a/src/ProcessAnalyzer/RectifiedAnalyzer.php b/src/ProcessAnalyzer/RectifiedAnalyzer.php
@@ -6,57 +6,28 @@
 
 use PhpParser\Node;
 use Rector\Core\Contract\Rector\RectorInterface;
-use Rector\Core\PhpParser\Comparing\NodeComparator;
-use Rector\Core\ValueObject\RectifiedNode;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 
 /**
- * This service verify if the Node already rectified with same Rector rule before current Rector rule with condition
+ * This service verify if the Node:
  *
- *        Same Rector Rule <-> Same Node <-> Same File
- *
- *  For both non-consecutive or consecutive order.
+ *      - already applied same Rector rule before current Rector rule on last previous Rector rule.
+ *      - just re-printed but token start still >= 0
  */
 final class RectifiedAnalyzer
 {
-    /**
-     * @var array<string, RectifiedNode|null>
-     */
-    private array $previousFileWithNodes = [];
-
-    public function __construct(
-        private readonly NodeComparator $nodeComparator
-    ) {
-    }
-
     /**
      * @param class-string<RectorInterface> $rectorClass
      */
-    public function verify(string $rectorClass, Node $node, string $filePath): ?RectifiedNode
+    public function hasRectified(string $rectorClass, Node $node): bool
     {
         $originalNode = $node->getAttribute(AttributeKey::ORIGINAL_NODE);
 
         if ($this->hasConsecutiveCreatedByRule($rectorClass, $node, $originalNode)) {
-            return new RectifiedNode($rectorClass, $node);
-        }
-
-        if (! isset($this->previousFileWithNodes[$filePath])) {
-            $this->previousFileWithNodes[$filePath] = new RectifiedNode($rectorClass, $node);
-            return null;
-        }
-
-        /** @var RectifiedNode $rectifiedNode */
-        $rectifiedNode = $this->previousFileWithNodes[$filePath];
-        if ($this->shouldContinue($rectifiedNode, $rectorClass, $node, $originalNode)) {
-            return null;
-        }
-
-        if ($this->previousFileWithNodes[$filePath]->getNode() === $node) {
-            // re-set to refill next
-            $this->previousFileWithNodes[$filePath] = null;
+            return true;
         }
 
-        return $rectifiedNode;
+        return $this->isJustReprintedOverlappedTokenStart($node, $originalNode);
     }
 
     /**
@@ -65,48 +36,31 @@ public function verify(string $rectorClass, Node $node, string $filePath): ?Rect
     private function hasConsecutiveCreatedByRule(string $rectorClass, Node $node, ?Node $originalNode): bool
     {
         $createdByRuleNode = $originalNode ?? $node;
+        /** @var class-string<RectorInterface>[] $createdByRule */
         $createdByRule = $createdByRuleNode->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
 
-        $lastRectorRuleKey = array_key_last($createdByRule);
-        if ($lastRectorRuleKey === null) {
+        if ($createdByRule === []) {
             return false;
         }
 
-        return $createdByRule[$lastRectorRuleKey] === $rectorClass;
+        return end($createdByRule) === $rectorClass;
     }
 
-    /**
-     * @param class-string<RectorInterface> $rectorClass
-     */
-    private function shouldContinue(
-        RectifiedNode $rectifiedNode,
-        string $rectorClass,
-        Node $node,
-        ?Node $originalNode
-    ): bool {
-        $rectifiedNodeClass = $rectifiedNode->getRectorClass();
-        $rectifiedNodeNode = $rectifiedNode->getNode();
-
-        if ($rectifiedNodeClass === $rectorClass && $rectifiedNodeNode === $node) {
-            /**
-             * allow to revisit the Node with same Rector rule if Node is changed by other rule
-             */
-            return ! $this->nodeComparator->areNodesEqual($originalNode, $node);
-        }
-
+    private function isJustReprintedOverlappedTokenStart(Node $node, ?Node $originalNode): bool
+    {
         if ($originalNode instanceof Node) {
-            return true;
+            return false;
         }
 
         $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE);
 
         if (! $parentNode instanceof Node) {
-            return true;
+            return false;
         }
 
         $parentOriginalNode = $parentNode->getAttribute(AttributeKey::ORIGINAL_NODE);
         if ($parentOriginalNode instanceof Node) {
-            return true;
+            return false;
         }
 
         /**
@@ -116,6 +70,6 @@ private function shouldContinue(
          * - Parent Node's original node is null
          */
         $startTokenPos = $node->getStartTokenPos();
-        return $startTokenPos < 0;
+        return $startTokenPos >= 0;
     }
 }
diff --git a/src/Rector/AbstractRector.php b/src/Rector/AbstractRector.php
@@ -36,7 +36,6 @@
 use Rector\Core\ProcessAnalyzer\RectifiedAnalyzer;
 use Rector\Core\Provider\CurrentFileProvider;
 use Rector\Core\ValueObject\Application\File;
-use Rector\Core\ValueObject\RectifiedNode;
 use Rector\NodeNameResolver\NodeNameResolver;
 use Rector\NodeRemoval\NodeRemover;
 use Rector\NodeTypeResolver\Node\AttributeKey;
@@ -225,7 +224,7 @@ final public function enterNode(Node $node)
 
         $originalNode ??= $node;
 
-        /** @var Node[]|Node $refactoredNode */
+        /** @var non-empty-array<Node>|Node $refactoredNode */
         $this->createdByRuleDecorator->decorate($refactoredNode, $originalNode, static::class);
 
         $rectorWithLineChange = new RectorWithLineChange($this::class, $originalNode->getLine());
@@ -241,11 +240,11 @@ final public function enterNode(Node $node)
             $originalNodeHash = spl_object_hash($originalNode);
             $this->nodesToReturn[$originalNodeHash] = $refactoredNode;
 
-            $firstNodeKey = array_key_first($refactoredNode);
-            $this->mirrorComments($refactoredNode[$firstNodeKey], $originalNode);
+            $firstNode = current($refactoredNode);
+            $this->mirrorComments($firstNode, $originalNode);
 
             $this->updateAndconnectParentNodes($refactoredNode, $parentNode);
-            $this->connectNodes($refactoredNode);
+            $this->connectNodes($refactoredNode, $node);
             $this->refreshScopeNodes($refactoredNode, $filePath, $currentScope);
 
             // will be replaced in leaveNode() the original node must be passed
@@ -257,6 +256,7 @@ final public function enterNode(Node $node)
             : $refactoredNode;
 
         $this->updateAndconnectParentNodes($refactoredNode, $parentNode);
+        $this->connectNodes([$refactoredNode], $node);
         $this->refreshScopeNodes($refactoredNode, $filePath, $currentScope);
 
         // is equals node type? return node early
@@ -401,8 +401,7 @@ private function shouldSkipCurrentNode(Node $node): bool
             return true;
         }
 
-        $rectifiedNode = $this->rectifiedAnalyzer->verify(static::class, $node, $this->file->getFilePath());
-        return $rectifiedNode instanceof RectifiedNode;
+        return $this->rectifiedAnalyzer->hasRectified(static::class, $node);
     }
 
     /**
@@ -427,10 +426,28 @@ private function updateAndconnectParentNodes(array | Node $node, ?Node $parentNo
     }
 
     /**
-     * @param Node[] $nodes
+     * @param non-empty-array<Node> $nodes
      */
-    private function connectNodes(array $nodes): void
+    private function connectNodes(array $nodes, Node $node): void
     {
+        $firstNode = current($nodes);
+        $firstNodePreviousNode = $firstNode->getAttribute(AttributeKey::PREVIOUS_NODE);
+
+        if (! $firstNodePreviousNode instanceof Node && $node->hasAttribute(AttributeKey::PREVIOUS_NODE)) {
+            /** @var Node $previousNode */
+            $previousNode = $node->getAttribute(AttributeKey::PREVIOUS_NODE);
+            $nodes = [$previousNode, ...$nodes];
+        }
+
+        $lastNode = end($nodes);
+        $lastNodeNextNode = $lastNode->getAttribute(AttributeKey::NEXT_NODE);
+
+        if (! $lastNodeNextNode instanceof Node && $node->hasAttribute(AttributeKey::NEXT_NODE)) {
+            /** @var Node $nextNode */
+            $nextNode = $node->getAttribute(AttributeKey::NEXT_NODE);
+            $nodes = [...$nodes, $nextNode];
+        }
+
         $nodeTraverser = new NodeTraverser();
         $nodeTraverser->addVisitor(new NodeConnectingVisitor());
         $nodeTraverser->traverse($nodes);

diff --git a/src/ValueObject/RectifiedNode.php b/src/ValueObject/RectifiedNode.php
diff --git a/tests/Issues/AddNodeAfterNodeStmt/AddNodeAfterNodeStmtTest.php b/tests/Issues/AddNodeAfterNodeStmt/AddNodeAfterNodeStmtTest.php
@@ -0,0 +1,32 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues\AddNodeAfterNodeStmt;
+
+use Iterator;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+
+final class AddNodeAfterNodeStmtTest extends AbstractRectorTestCase
+{
+    /**
+     * @dataProvider provideData()
+     */
+    public function test(string $filePath): void
+    {
+        $this->doTestFile($filePath);
+    }
+
+    /**
+     * @return Iterator<array<string>>
+     */
+    public function provideData(): Iterator
+    {
+        return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/configured_rule.php';
+    }
+}
diff --git a/tests/Issues/AddNodeAfterNodeStmt/Fixture/fixture.php.inc b/tests/Issues/AddNodeAfterNodeStmt/Fixture/fixture.php.inc
@@ -0,0 +1,32 @@
+<?php
+
+namespace Rector\Core\Tests\Issues\AddNodeAfterNodeStmt\Fixture;
+
+final class Fixture
+{
+    public function run()
+    {
+        if ($a === 1) {
+        }
+        echo 'existing next stmt after if';
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Core\Tests\Issues\AddNodeAfterNodeStmt\Fixture;
+
+final class Fixture
+{
+    public function run()
+    {
+        if ($a === 1) {
+        }
+        echo 'this is new stmt after if';
+        echo 'existing next stmt after if';
+    }
+}
+
+?>