warn about infinite loop node modification (#51)

rectorphp · May 16, 2021 · a60e3ac · a60e3ac
1 parent eecf18f
commit a60e3ac
Show file tree

Hide file tree

Showing 14 changed files with 241 additions and 23 deletions.
diff --git a/packages/NodeTypeResolver/Node/AttributeKey.php b/packages/NodeTypeResolver/Node/AttributeKey.php
@@ -191,4 +191,10 @@ final class AttributeKey
      * @var string
      */
     public const HAS_PHP_DOC_INFO_JUST_CHANGED = 'has_php_doc_info_just_changed';
+
+    /**
+     * Helps with infinite loop detection
+     * @var string
+     */
+    public const CREATED_BY_RULE = 'created_by_rule';
 }
diff --git a/packages/PostRector/Collector/NodesToAddCollector.php b/packages/PostRector/Collector/NodesToAddCollector.php
@@ -47,7 +47,7 @@ public function addNodeBeforeNode(Node $addedNode, Node $positionNode): void
             throw new ShouldNotHappenException($message);
         }
 
-        $position = $this->resolveNearestExpressionPosition($positionNode);
+        $position = $this->resolveNearestStmtPosition($positionNode);
         $this->nodesToAddBefore[$position][] = $this->wrapToExpression($addedNode);
 
         $this->rectorChangeCollector->notifyNodeFileInfo($positionNode);
@@ -58,7 +58,7 @@ public function addNodeBeforeNode(Node $addedNode, Node $positionNode): void
      */
     public function addNodesAfterNode(array $addedNodes, Node $positionNode): void
     {
-        $position = $this->resolveNearestExpressionPosition($positionNode);
+        $position = $this->resolveNearestStmtPosition($positionNode);
         foreach ($addedNodes as $addedNode) {
             // prevent fluent method weird indent
             $addedNode->setAttribute(AttributeKey::ORIGINAL_NODE, null);
@@ -70,7 +70,7 @@ public function addNodesAfterNode(array $addedNodes, Node $positionNode): void
 
     public function addNodeAfterNode(Node $addedNode, Node $positionNode): void
     {
-        $position = $this->resolveNearestExpressionPosition($positionNode);
+        $position = $this->resolveNearestStmtPosition($positionNode);
         $this->nodesToAddAfter[$position][] = $this->wrapToExpression($addedNode);
 
         $this->rectorChangeCollector->notifyNodeFileInfo($positionNode);
@@ -118,13 +118,14 @@ public function addNodesBeforeNode(array $newNodes, Node $positionNode): void
         $this->rectorChangeCollector->notifyNodeFileInfo($positionNode);
     }
 
-    private function resolveNearestExpressionPosition(Node $node): string
+    private function resolveNearestStmtPosition(Node $node): string
     {
-        if ($node instanceof Expression || $node instanceof Stmt) {
+        if ($node instanceof Stmt) {
             return spl_object_hash($node);
         }
 
         $currentStmt = $node->getAttribute(AttributeKey::CURRENT_STATEMENT);
+
         if ($currentStmt instanceof Stmt) {
             return spl_object_hash($currentStmt);
         }
@@ -134,7 +135,7 @@ private function resolveNearestExpressionPosition(Node $node): string
             return spl_object_hash($parent);
         }
 
-        $foundNode = $this->betterNodeFinder->findParentTypes($node, [Expression::class, Stmt::class]);
+        $foundNode = $this->betterNodeFinder->findParentType($node, Stmt::class);
 
         if (! $foundNode instanceof Stmt) {
             $printedNode = $this->betterStandardPrinter->print($node);

diff --git a/phpstan.neon b/phpstan.neon
@@ -518,3 +518,26 @@ parameters:
         - '#Parameter \#1 \$type of method Rector\\BetterPhpDocParser\\PhpDocInfo\\PhpDocInfo<PHPStan\\PhpDocParser\\Ast\\Node\>\:\:removeByType\(\) expects class\-string<PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocTagValueNode\>, class\-string<PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocTagValueNode\>\|PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocTagValueNode given#'
 
         - '#Method Rector\\Core\\Application\\VersionResolver\:\:resolvePackageData\(\) return type has no value type specified in iterable type array#'
+        - '#Cognitive complexity for "Rector\\Core\\Rector\\AbstractRector\:\:enterNode\(\)" is \d+, keep it under 9#'
+
+        -
+            message: '#Class cognitive complexity is 33, keep it under 30#'
+            path: src/Rector/AbstractRector.php
+
+
+        -
+            message: '#Method call argument on position 0 must use constant \(e\.g\. "Option\:\:NAME"\) over value#'
+            path: src/Rector/AbstractRector.php
+
+        -
+            message: '#Namespace "Rector\\Core\\Rector" is only reserved for "Rector"\. Move the class somewhere else#'
+            path: src/Rector/AbstractRector.php
+
+        -
+            message: '#Anonymous class is not allowed#'
+            path: src/Rector/AbstractRector.php
+
+        -
+            message: '#Class cognitive complexity is 38, keep it under 30#'
+            path: src/Rector/AbstractRector.php
+
diff --git a/rules/DeadCode/Rector/If_/UnwrapFutureCompatibleIfPhpVersionRector.php b/rules/DeadCode/Rector/If_/UnwrapFutureCompatibleIfPhpVersionRector.php
@@ -65,7 +65,7 @@ public function getNodeTypes(): array
      */
     public function refactor(Node $node): ?array
     {
-        if ((bool) $node->elseifs) {
+        if ($node->elseifs) {
             return null;
         }
 

diff --git a/rules/DowngradePhp80/Rector/NullsafeMethodCall/DowngradeNullsafeToTernaryOperatorRector.php b/rules/DowngradePhp80/Rector/NullsafeMethodCall/DowngradeNullsafeToTernaryOperatorRector.php
@@ -69,6 +69,7 @@ public function refactor(Node $node): ?Node
             ? new MethodCall($variable, $node->name, $node->args)
             : new PropertyFetch($variable, $node->name);
 
-        return new Ternary(new Assign($variable, $node->var), $called, $this->nodeFactory->createNull());
+        $assign = new Assign($variable, $node->var);
+        return new Ternary($assign, $called, $this->nodeFactory->createNull());
     }
 }
diff --git a/rules/RemovingStatic/Rector/Class_/StaticTypeToSetterInjectionRector.php b/rules/RemovingStatic/Rector/Class_/StaticTypeToSetterInjectionRector.php
@@ -157,7 +157,6 @@ function (Node $node) use ($objectType): bool {
             $this->phpDocTypeChanger->changeVarType($phpDocInfo, $objectType);
 
             $class->stmts = array_merge([$entityFactoryProperty, $setEntityFactoryMethod], $class->stmts);
-
             break;
         }
 

diff --git a/src/Application/FileProcessor/PhpFileProcessor.php b/src/Application/FileProcessor/PhpFileProcessor.php
@@ -163,6 +163,8 @@ private function tryCatchWrapper(File $file, callable $callback, string $phase):
             }
 
             $callback($file);
+        } catch (ShouldNotHappenException $shouldNotHappenException) {
+            throw $shouldNotHappenException;
         } catch (AnalysedCodeException $analysedCodeException) {
             // inform about missing classes in tests
             if (StaticPHPUnitEnvironment::isPHPUnitRun()) {
@@ -190,9 +192,9 @@ private function printFile(File $file): void
             return;
         }
 
-        $newContent = $this->configuration->isDryRun() ? $this->formatPerservingPrinter->printParsedStmstAndTokensToString(
-            $file
-        ) : $this->formatPerservingPrinter->printParsedStmstAndTokens($file);
+        $newContent = $this->configuration->isDryRun()
+            ? $this->formatPerservingPrinter->printParsedStmstAndTokensToString($file)
+            : $this->formatPerservingPrinter->printParsedStmstAndTokens($file);
 
         $file->changeFileContent($newContent);
         $this->fileDiffFileDecorator->decorate([$file]);

diff --git a/src/Exception/NodeTraverser/InfiniteLoopTraversingException.php b/src/Exception/NodeTraverser/InfiniteLoopTraversingException.php
@@ -0,0 +1,11 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Exception\NodeTraverser;
+
+use Exception;
+
+final class InfiniteLoopTraversingException extends Exception
+{
+}
diff --git a/src/Rector/AbstractRector.php b/src/Rector/AbstractRector.php
@@ -25,6 +25,7 @@
 use Rector\Core\Configuration\CurrentNodeProvider;
 use Rector\Core\Configuration\Option;
 use Rector\Core\Contract\Rector\PhpRectorInterface;
+use Rector\Core\Exception\NodeTraverser\InfiniteLoopTraversingException;
 use Rector\Core\Exception\ShouldNotHappenException;
 use Rector\Core\Exclusion\ExclusionManager;
 use Rector\Core\Logging\CurrentRectorProvider;
@@ -38,6 +39,7 @@
 use Rector\Core\Provider\CurrentFileProvider;
 use Rector\Core\ValueObject\Application\File;
 use Rector\Core\ValueObject\ProjectType;
+use Rector\DowngradePhp80\Rector\NullsafeMethodCall\DowngradeNullsafeToTernaryOperatorRector;
 use Rector\NodeCollector\NodeCollector\NodeRepository;
 use Rector\NodeNameResolver\NodeNameResolver;
 use Rector\NodeRemoval\NodeRemover;
@@ -135,7 +137,7 @@ abstract class AbstractRector extends NodeVisitorAbstract implements PhpRectorIn
     private ChangedNodeAnalyzer $changedNodeAnalyzer;
 
     /**
-     * @var array<string, Node[]>
+     * @var array<string, Node[]|Node>
      */
     private array $nodesToReturn = [];
 
@@ -219,7 +221,7 @@ public function beforeTraverse(array $nodes): ?array
     }
 
     /**
-     * @return Expression|Node|Node[]|null
+     * @return Expression|Node|Node[]|int|null
      */
     final public function enterNode(Node $node)
     {
@@ -248,7 +250,7 @@ final public function enterNode(Node $node)
             $originalNodeHash = spl_object_hash($originalNode);
             $this->nodesToReturn[$originalNodeHash] = $node;
 
-            if (($node !== []) > 0) {
+            if ($node !== []) {
                 $firstNodeKey = array_key_first($node);
                 $this->mirrorComments($node[$firstNodeKey], $originalNode);
             }
@@ -267,25 +269,72 @@ final public function enterNode(Node $node)
             $rectorWithLineChange = new RectorWithLineChange($this, $originalNode->getLine());
             $this->file->addRectorClassWithLine($rectorWithLineChange);
 
-            // update parents relations
-            $this->connectParentNodes($node);
+            // update parents relations - must run before connectParentNodes()
             $this->mirrorAttributes($originalAttributes, $node);
+            $this->connectParentNodes($node);
+
+            // is different node type? do not traverse children to avoid looping
+            if (get_class($originalNode) !== get_class($node)) {
+                $createdByRule = $originalNode->getAttribute(AttributeKey::CREATED_BY_RULE);
+                // special case
+                if ($createdByRule === static::class && static::class !== DowngradeNullsafeToTernaryOperatorRector::class) {
+                    // does it contain the same node type as input?
+                    $hasNestedOriginalNodeType = $this->betterNodeFinder->findInstanceOf(
+                        $node,
+                        get_class($originalNode)
+                    );
+                    if ($hasNestedOriginalNodeType !== []) {
+                        throw new InfiniteLoopTraversingException(static::class);
+                    }
+                }
+
+                // hacking :)
+                $nodeTraverser = new NodeTraverser();
+                $nodeTraverser->addVisitor(new class(static::class) extends NodeVisitorAbstract {
+                    public function __construct(
+                        private string $rectorClass
+                    ) {
+                    }
+
+                    public function enterNode(Node $node)
+                    {
+                        $node->setAttribute(AttributeKey::CREATED_BY_RULE, $this->rectorClass);
+                        return $node;
+                    }
+                });
+                $nodeTraverser->traverse([$originalNode]);
+
+                // search "infinite recursion" in https://github.com/nikic/PHP-Parser/blob/master/doc/component/Walking_the_AST.markdown
+                $originalNodeHash = spl_object_hash($originalNode);
+
+                if ($originalNode instanceof Stmt && $node instanceof Expr) {
+                    $node = new Expression($node);
+                }
+
+                $this->nodesToReturn[$originalNodeHash] = $node;
+
+                return $node;
+            }
         }
 
-        // if stmt ("$value;") was replaced by expr ("$value"), add the ending ";" (Expression) to prevent breaking the code
+        // if Stmt ("$value;") was replaced by Expr ("$value"), add Expression (the ending ";") to prevent breaking the code
         if ($originalNode instanceof Stmt && $node instanceof Expr) {
-            return new Expression($node);
+            $node = new Expression($node);
         }
 
         return $node;
     }
 
+    /**
+     * Replacing nodes in leaveNode() method avoids infinite recursion
+     * see"infinite recursion" in https://github.com/nikic/PHP-Parser/blob/master/doc/component/Walking_the_AST.markdown
+     */
     public function leaveNode(Node $node)
     {
         $objectHash = spl_object_hash($node);
 
-        // update parents relations
-        return $this->nodesToReturn[$objectHash] ?? null;
+        // update parents relations!!!
+        return $this->nodesToReturn[$objectHash] ?? $node;
     }
 
     protected function isName(Node $node, string $name): bool

diff --git a/tests/Issues/FixtureInfiniteLoop/de_morgan.php.inc b/tests/Issues/FixtureInfiniteLoop/de_morgan.php.inc
@@ -0,0 +1,37 @@
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\BooleanNot\SimplifyDeMorganBinaryRector\Fixture;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr;
+
+class MultiBinary
+{
+    public function run(Node $node)
+    {
+        return !($node instanceof Node\Name || $node instanceof Expr\StaticCall || $node instanceof Expr\Array_);
+
+        return !($node instanceof Node\Name || $node instanceof Expr\StaticCall);
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\CodeQuality\Rector\BooleanNot\SimplifyDeMorganBinaryRector\Fixture;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr;
+
+class MultiBinary
+{
+    public function run(Node $node)
+    {
+        return !($node instanceof Node\Name || $node instanceof Expr\StaticCall || $node instanceof Expr\Array_);
+
+        return !$node instanceof Node\Name && !$node instanceof Expr\StaticCall;
+    }
+}
+
+?>
diff --git a/tests/Issues/FixtureInfiniteLoop/some_method_call_infinity.php.inc b/tests/Issues/FixtureInfiniteLoop/some_method_call_infinity.php.inc
@@ -0,0 +1,13 @@
+<?php
+
+$dateTime = new DateTime();
+$dateTime->modify('+1');
+
+?>
+-----
+<?php
+
+$dateTime = new DateTime();
+$dateTime = $dateTime->modify('+1');
+
+?>
diff --git a/tests/Issues/InfiniteLoopTest.php b/tests/Issues/InfiniteLoopTest.php
@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Core\Tests\Issues;
+
+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__ . '/FixtureInfiniteLoop/some_method_call_infinity.php.inc');
+        $this->doTestFileInfo($fixtureFileInfo);
+    }
+
+    public function testPass(): void
+    {
+        $fixtureFileInfo = new SmartFileInfo(__DIR__ . '/FixtureInfiniteLoop/de_morgan.php.inc');
+        $this->doTestFileInfo($fixtureFileInfo);
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/infinite_loop.php';
+    }
+}