[Core] Refactor InfiniteLoopValidator (#1771)

* [Core] Refactor InfiniteLoopValidator * pass rector class * check found * verify rector class * remove InfiniteLoopTraversingException * remove manual check on rules * test possibly null already transformed for DowngradeJsonDecodeNullAssociativeArgRector * [ci-review] Rector Rectify * [ci-review] Rector Rectify * [ci-review] Rector Rectify * test for skip some method call infinity already transformed * finally, created by rule check can be cleaned up * clean up ALREADY_CHANGED_ON_COUNT attribute check on CountOnNullRector Co-authored-by: GitHub Action <action@github.com>
rectorphp · Feb 6, 2022 · ca5b89b · ca5b89b
1 parent aa4f454
commit ca5b89b
Show file tree

Hide file tree

Showing 13 changed files with 79 additions and 141 deletions.
diff --git a/config/services.php b/config/services.php
@@ -72,7 +72,6 @@
             __DIR__ . '/../src/PhpParser/ValueObject',
             __DIR__ . '/../src/functions',
             __DIR__ . '/../src/constants.php',
-            __DIR__ . '/../src/PhpParser/NodeVisitor/CreatedByRuleNodeVisitor.php',
         ]);
 
     $services->alias(Application::class, ConsoleApplication::class);

diff --git a/...JsonDecodeNullAssociativeArgRector/Fixture/skip_possibly_null_already_transformed.php.inc b/...JsonDecodeNullAssociativeArgRector/Fixture/skip_possibly_null_already_transformed.php.inc
@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\DowngradePhp72\Rector\FuncCall\DowngradeJsonDecodeNullAssociativeArgRector\Fixture;
+
+class SkipPossiblyNullAlreadyTransformed
+{
+    function run(string $json, ?bool $associative)
+    {
+        if ($associative === null) {
+            $associative = true;
+        }
+        $value = json_decode($json, $associative);
+    }
+}
+
+?>
diff --git a/rules/DowngradePhp70/Rector/Instanceof_/DowngradeInstanceofThrowableRector.php b/rules/DowngradePhp70/Rector/Instanceof_/DowngradeInstanceofThrowableRector.php
@@ -70,11 +70,6 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        $createdByRule = $node->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
-        if (in_array(self::class, $createdByRule, true)) {
-            return null;
-        }
-
         // Ensure the refactoring is idempotent.
         if ($this->isAlreadyTransformed($node)) {
             return null;

diff --git a/rules/DowngradePhp72/Rector/FuncCall/DowngradeJsonDecodeNullAssociativeArgRector.php b/rules/DowngradePhp72/Rector/FuncCall/DowngradeJsonDecodeNullAssociativeArgRector.php
@@ -89,11 +89,6 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        $createdByRule = $node->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
-        if (in_array(self::class, $createdByRule, true)) {
-            return null;
-        }
-
         $args = $node->getArgs();
         if ($this->argsAnalyzer->hasNamedArg($args)) {
             return null;

diff --git a/rules/DowngradePhp80/Rector/FuncCall/DowngradeArrayFilterNullableCallbackRector.php b/rules/DowngradePhp80/Rector/FuncCall/DowngradeArrayFilterNullableCallbackRector.php
@@ -27,7 +27,6 @@
 use PHPStan\Type\StringType;
 use Rector\Core\NodeAnalyzer\ArgsAnalyzer;
 use Rector\Core\Rector\AbstractRector;
-use Rector\NodeTypeResolver\Node\AttributeKey;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
 
@@ -101,11 +100,6 @@ public function refactor(Node $node): FuncCall|Ternary|null
             return null;
         }
 
-        $createdByRule = $node->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
-        if (in_array(self::class, $createdByRule, true)) {
-            return null;
-        }
-
         // direct null check ConstFetch
         if ($args[1]->value instanceof ConstFetch && $this->valueResolver->isNull($args[1]->value)) {
             $args = [$args[0]];

diff --git a/rules/Php71/Rector/FuncCall/CountOnNullRector.php b/rules/Php71/Rector/FuncCall/CountOnNullRector.php
@@ -40,11 +40,6 @@
  */
 final class CountOnNullRector extends AbstractRector implements MinPhpVersionInterface
 {
-    /**
-     * @var string
-     */
-    private const ALREADY_CHANGED_ON_COUNT = 'already_changed_on_count';
-
     public function __construct(
         private readonly CountableTypeAnalyzer $countableTypeAnalyzer,
         private readonly CountableAnalyzer $countableAnalyzer,
@@ -121,10 +116,8 @@ public function refactor(Node $node): ?Node
 
         if ($this->nodeTypeResolver->isNullableType($countedNode) || $countedType instanceof NullType) {
             $identical = new Identical($countedNode, $this->nodeFactory->createNull());
-            $ternary = new Ternary($identical, new LNumber(0), $node);
-            // prevent infinity loop re-resolution
-            $node->setAttribute(self::ALREADY_CHANGED_ON_COUNT, true);
-            return $ternary;
+
+            return new Ternary($identical, new LNumber(0), $node);
         }
 
         if ($this->phpVersionProvider->isAtLeastPhpVersion(PhpVersionFeature::IS_COUNTABLE)) {
@@ -137,9 +130,6 @@ public function refactor(Node $node): ?Node
             ), $instanceof);
         }
 
-        // prevent infinity loop re-resolution
-        $node->setAttribute(self::ALREADY_CHANGED_ON_COUNT, true);
-
         return new Ternary($conditionNode, $node, new LNumber(0));
     }
 
@@ -182,13 +172,6 @@ private function shouldSkip(FuncCall $funcCall): bool
             return true;
         }
 
-        $alreadyChangedOnCount = (bool) $funcCall->getAttribute(self::ALREADY_CHANGED_ON_COUNT, false);
-
-        // check if it has some condition before already, if so, probably it's already handled
-        if ($alreadyChangedOnCount) {
-            return true;
-        }
-
         $parentNode = $funcCall->getAttribute(AttributeKey::PARENT_NODE);
         if ($parentNode instanceof Ternary) {
             return true;

diff --git a/src/Exception/NodeTraverser/InfiniteLoopTraversingException.php b/src/Exception/NodeTraverser/InfiniteLoopTraversingException.php
diff --git a/src/NodeDecorator/CreatedByRuleDecorator.php b/src/NodeDecorator/CreatedByRuleDecorator.php
@@ -9,7 +9,23 @@
 
 final class CreatedByRuleDecorator
 {
-    public function decorate(Node $node, string $rectorClass): void
+    /**
+     * @param array<Node>|Node $node
+     */
+    public function decorate(array | Node $node, Node $originalNode, string $rectorClass): void
+    {
+        if ($node instanceof Node) {
+            $node = [$node];
+        }
+
+        foreach ($node as $singleNode) {
+            $this->createByRule($singleNode, $rectorClass);
+        }
+
+        $this->createByRule($originalNode, $rectorClass);
+    }
+
+    private function createByRule(Node $node, string $rectorClass): void
     {
         $mergeCreatedByRule = array_merge($node->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [], [$rectorClass]);
         $mergeCreatedByRule = array_unique($mergeCreatedByRule);

diff --git a/src/PhpParser/NodeVisitor/CreatedByRuleNodeVisitor.php b/src/PhpParser/NodeVisitor/CreatedByRuleNodeVisitor.php
diff --git a/src/Rector/AbstractRector.php b/src/Rector/AbstractRector.php
@@ -233,24 +233,31 @@ final public function enterNode(Node $node)
 
         $originalNode ??= clone $node;
 
+        if (! $this->infiniteLoopValidator->isValid($node, $originalNode, static::class)) {
+            return null;
+        }
+
         $node = $this->refactor($node);
 
         // nothing to change → continue
-        if ($node === null) {
+        if ($this->isNothingToChange($node)) {
+            return null;
+        }
+
+        /** @var Node|array<Node> $node */
+        if (! $this->infiniteLoopValidator->isValid($node, $originalNode, static::class)) {
             return null;
         }
 
         /** @var Node $originalNode */
         if (is_array($node)) {
-            $this->createdByRule($node, $originalNode);
+            $this->createdByRuleDecorator->decorate($node, $originalNode, static::class);
 
             $originalNodeHash = spl_object_hash($originalNode);
             $this->nodesToReturn[$originalNodeHash] = $node;
 
-            if ($node !== []) {
-                $firstNodeKey = array_key_first($node);
-                $this->mirrorComments($node[$firstNodeKey], $originalNode);
-            }
+            $firstNodeKey = array_key_first($node);
+            $this->mirrorComments($node[$firstNodeKey], $originalNode);
 
             // will be replaced in leaveNode() the original node must be passed
             return $originalNode;
@@ -268,16 +275,13 @@ final public function enterNode(Node $node)
         $this->mirrorAttributes($originalAttributes, $node);
         $this->connectParentNodes($node);
 
+        $this->createdByRuleDecorator->decorate($node, $originalNode, static::class);
+
         // is equals node type? return node early
         if ($originalNode::class === $node::class) {
-            $this->createdByRule($node, $originalNode);
             return $node;
         }
 
-        // is different node type? do not traverse children to avoid looping
-        $this->infiniteLoopValidator->process($node, $originalNode, static::class);
-        $this->createdByRuleDecorator->decorate($node, static::class);
-
         // search "infinite recursion" in https://github.com/nikic/PHP-Parser/blob/master/doc/component/Walking_the_AST.markdown
         $originalNodeHash = spl_object_hash($originalNode);
 
@@ -433,19 +437,15 @@ protected function removeNodes(array $nodes): void
     }
 
     /**
-     * @param array<Node>|Node $node
+     * @param Node|array<Node>|null $node
      */
-    private function createdByRule(array | Node $node, Node $originalNode): void
+    private function isNothingToChange(array|Node|null $node): bool
     {
-        if ($node instanceof Node) {
-            $node = [$node];
-        }
-
-        foreach ($node as $singleNode) {
-            $this->createdByRuleDecorator->decorate($singleNode, static::class);
+        if ($node === null) {
+            return true;
         }
 
-        $this->createdByRuleDecorator->decorate($originalNode, static::class);
+        return $node === [];
     }
 
     /**

diff --git a/src/Validation/InfiniteLoopValidator.php b/src/Validation/InfiniteLoopValidator.php
@@ -5,69 +5,41 @@
 namespace Rector\Core\Validation;
 
 use PhpParser\Node;
-use PhpParser\NodeTraverser;
-use Rector\Core\Contract\Rector\RectorInterface;
-use Rector\Core\Exception\NodeTraverser\InfiniteLoopTraversingException;
-use Rector\Core\NodeDecorator\CreatedByRuleDecorator;
+use Rector\Core\PhpParser\Comparing\NodeComparator;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
-use Rector\Core\PhpParser\NodeVisitor\CreatedByRuleNodeVisitor;
-use Rector\DowngradePhp74\Rector\ArrowFunction\ArrowFunctionToAnonymousFunctionRector;
-use Rector\DowngradePhp80\Rector\NullsafeMethodCall\DowngradeNullsafeToTernaryOperatorRector;
 use Rector\NodeTypeResolver\Node\AttributeKey;
-use Rector\Php74\Rector\Closure\ClosureToArrowFunctionRector;
 
 final class InfiniteLoopValidator
 {
-    /**
-     * @var array<class-string<RectorInterface>>
-     */
-    private const ALLOWED_INFINITE_RECTOR_CLASSES = [
-        DowngradeNullsafeToTernaryOperatorRector::class,
-        ArrowFunctionToAnonymousFunctionRector::class,
-        ClosureToArrowFunctionRector::class,
-    ];
-
     public function __construct(
-        private readonly BetterNodeFinder $betterNodeFinder,
-        private readonly CreatedByRuleDecorator $createdByRuleDecorator
+        private readonly NodeComparator $nodeComparator,
+        private readonly BetterNodeFinder $betterNodeFinder
     ) {
     }
 
     /**
-     * @param class-string<RectorInterface> $rectorClass
+     * @param Node|array<Node> $node
      */
-    public function process(Node $node, Node $originalNode, string $rectorClass): void
+    public function isValid(Node|array $node, Node $originalNode, string $rectorClass): bool
     {
-        if (in_array($rectorClass, self::ALLOWED_INFINITE_RECTOR_CLASSES, true)) {
-            return;
+        if ($this->nodeComparator->areNodesEqual($node, $originalNode)) {
+            return true;
         }
 
-        $createdByRule = $originalNode->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
+        $isFound = (bool) $this->betterNodeFinder->findFirst(
+            $node,
+            fn (Node $subNode): bool => $this->nodeComparator->areNodesEqual($node, $subNode)
+        );
 
-        // special case
-        if (in_array($rectorClass, $createdByRule, true)) {
-            // does it contain the same node type as input?
-            $originalNodeClass = $originalNode::class;
-
-            $hasNestedOriginalNodeType = $this->betterNodeFinder->findInstanceOf($node, $originalNodeClass);
-            if ($hasNestedOriginalNodeType !== []) {
-                throw new InfiniteLoopTraversingException($rectorClass);
-            }
+        if (! $isFound) {
+            return true;
         }
 
-        $this->decorateNode($originalNode, $rectorClass);
-    }
-
-    /**
-     * @param class-string<RectorInterface> $rectorClass
-     */
-    private function decorateNode(Node $node, string $rectorClass): void
-    {
-        $nodeTraverser = new NodeTraverser();
-
-        $createdByRuleNodeVisitor = new CreatedByRuleNodeVisitor($this->createdByRuleDecorator, $rectorClass);
-        $nodeTraverser->addVisitor($createdByRuleNodeVisitor);
+        $createdByRule = $originalNode->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
+        if ($createdByRule === []) {
+            return true;
+        }
 
-        $nodeTraverser->traverse([$node]);
+        return ! in_array($rectorClass, $createdByRule, true);
     }
 }
diff --git a/tests/Issues/InfiniteLoop/Fixture/skip_some_method_call_infinity_already_transformed.php.inc b/tests/Issues/InfiniteLoop/Fixture/skip_some_method_call_infinity_already_transformed.php.inc
@@ -0,0 +1,4 @@
+<?php
+
+$dateTime = new DateTime();
+$dateTime = $dateTime->modify('+1');
diff --git a/tests/Issues/InfiniteLoop/InfiniteLoopTest.php b/tests/Issues/InfiniteLoop/InfiniteLoopTest.php
@@ -4,16 +4,13 @@
 
 namespace Rector\Core\Tests\Issues\InfiniteLoop;
 
-use Rector\Core\Exception\NodeTraverser\InfiniteLoopTraversingException;
 use Rector\Testing\PHPUnit\AbstractRectorTestCase;
 use Symplify\SmartFileSystem\SmartFileInfo;
 
 final class InfiniteLoopTest extends AbstractRectorTestCase
 {
     public function testException(): void
     {
-        $this->expectException(InfiniteLoopTraversingException::class);
-
         $fixtureFileInfo = new SmartFileInfo(__DIR__ . '/Fixture/some_method_call_infinity.php.inc');
         $this->doTestFileInfo($fixtureFileInfo);
     }