[perf][expr] drop do while from FileProcessor

rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php

@@ -21,6 +21,8 @@
 use PHPStan\Reflection\ClassReflection;
 use PHPStan\Type\MixedType;
 use PHPStan\Type\ObjectType;
+use Rector\CodeQuality\Rector\If_\ExplicitNullableObjectInstanceRector;
+use Rector\Contract\Rector\OrderedRectorInterface;
 use Rector\NodeManipulator\IfManipulator;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\Rector\AbstractRector;
@@ -31,7 +33,7 @@
 /**
  * @see \Rector\Tests\DeadCode\Rector\If_\RemoveDeadInstanceOfRector\RemoveDeadInstanceOfRectorTest
  */
-final class RemoveDeadInstanceOfRector extends AbstractRector
+final class RemoveDeadInstanceOfRector extends AbstractRector implements OrderedRectorInterface
 {
     public function __construct(
         private readonly IfManipulator $ifManipulator,
@@ -72,6 +74,11 @@ public function getNodeTypes(): array
         return [If_::class];
     }
 
+    public function precededRule(): string
+    {
+        return ExplicitNullableObjectInstanceRector::class;
+    }
+
     /**
      * @param If_ $node
      * @return Stmt[]|null|NodeVisitor::REMOVE_NODE|If_

src/Application/FileProcessor.php

@@ -56,65 +56,35 @@ public function processFile(File $file, Configuration $configuration): FileProce
             return new FileProcessResult([$parsingSystemError], null, false);
         }
 
-        $fileHasChanged = false;
         $filePath = $file->getFilePath();
 
-        do {
-            $file->changeHasChanged(false);
+        // 1. change nodes with Rector Rules
+        $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file);
 
-            // every rule and post rector records a line change when it mutates the AST
-            $changeCountBeforePass = count($file->getRectorWithLineChanges());
+        // 2. apply post rectors
+        $postNewStmts = $this->postFileProcessor->traverse($newStmts, $file);
 
-            // 1. change nodes with Rector Rules
-            $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file);
+        // 3. this is needed for new tokens added in "afterTraverse()"
+        $file->changeNewStmts($postNewStmts);
 
-            // 2. apply post rectors
-            $postNewStmts = $this->postFileProcessor->traverse($newStmts, $file);
-
-            // 3. this is needed for new tokens added in "afterTraverse()"
-            $file->changeNewStmts($postNewStmts);
-
-            // file never changed and this pass changed nothing: the print would just reproduce the
-            // original content, so skip the expensive format-preserving print entirely.
-            // once the file has changed we keep printing every pass, as a reprint can still
-            // stabilize the output (e.g. drop a leftover trailing comma)
-            if (! $fileHasChanged && count($file->getRectorWithLineChanges()) === $changeCountBeforePass) {
-                break;
-            }
-
-            // 4. print to file or string
-            // important to detect if file has changed
-            $this->printFile($file, $configuration, $filePath);
-
-            // no change in current iteration, stop
-            if (! $file->hasChanged()) {
-                break;
-            }
-
-            $fileHasChanged = true;
-        } while (true);
-
-        // 5. add as cacheable if not changed at all
-        if (! $fileHasChanged) {
+        // no change, stop
+        if (! $file->hasChanged()) {
             $this->changedFilesDetector->addCacheableFile($filePath);
-        } else {
-            // when changed, set final status changed to true
-            // to ensure it make sense to verify in next process when needed
-            $file->changeHasChanged(true);
+            return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged());
         }
 
-        $rectorWithLineChanges = $file->getRectorWithLineChanges();
-        if ($file->hasChanged() || $rectorWithLineChanges !== []) {
-            $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges(
-                $configuration->shouldShowDiffs(),
-                $file,
-                $file->getOriginalFileContent(),
-                $file->getFileContent(),
-                $file->getRectorWithLineChanges()
-            );
-
-            $file->setFileDiff($currentFileDiff);
-        }
+        // 4. print to file or string
+        $this->printFile($file, $configuration, $filePath);
+
+        $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges(
+            $configuration->shouldShowDiffs(),
+            $file,
+            $file->getOriginalFileContent(),
+            $file->getFileContent(),
+            $file->getRectorWithLineChanges()
+        );
+
+        $file->setFileDiff($currentFileDiff);
 
         return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged());
     }

src/PhpParser/NodeTraverser/RectorNodeTraverser.php

@@ -51,31 +51,8 @@ private function traverseNode(Node $node, File $file): void
                 continue;
             }
 
-            $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($subNode, $file);
-
-            foreach ($currentNodeVisitors as $currentNodeVisitor) {
-                $return = $this->rectorRunner->run($currentNodeVisitor, $subNode);
-                if ($return === null) {
-                    continue;
-                }
-
-                if ($return instanceof Node) {
-                    $originalSubNodeClass = $subNode::class;
-
-                    $this->ensureReplacementReasonable($subNode, $return);
-                    $subNode = $return;
-                    $node->{$name} = $return;
-
-                    if ($originalSubNodeClass !== $subNode::class) {
-                        // stop traversing as node type changed and visitors won't work
-                        continue 2;
-                    }
-                } else {
-                    throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return));
-                }
-            }
-
-            $this->traverseNode($subNode, $file);
+            // a single sub-node slot cannot hold a list or be removed
+            $node->{$name} = $this->refactorNode($subNode, $file, false);
 
             if ($this->stopTraversal) {
                 break;
@@ -99,33 +76,16 @@ private function traverseArray(array $nodes, File $file): array
                 continue;
             }
 
-            $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($node, $file);
-
-            foreach ($currentNodeVisitors as $currentNodeVisitor) {
-                $return = $this->rectorRunner->run($currentNodeVisitor, $node);
-                if ($return !== null) {
-                    if ($return instanceof Node) {
-                        $originalNodeNodeClass = $node::class;
-                        $this->ensureReplacementReasonable($node, $return);
-                        $nodes[$i] = $node = $return;
-
-                        if ($originalNodeNodeClass !== $return::class) {
-                            // stop traversing as node type changed and visitors won't work
-                            continue 2;
-                        }
-                    } elseif (\is_array($return)) {
-                        $doNodes[] = [$i, $return];
-                        continue 2;
-                    } elseif ($return === NodeVisitor::REMOVE_NODE) {
-                        $doNodes[] = [$i, []];
-                        continue 2;
-                    } else {
-                        throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return));
-                    }
-                }
+            $result = $this->refactorNode($node, $file, true);
+
+            if (\is_array($result)) {
+                $doNodes[] = [$i, $result];
+            } elseif ($result === NodeVisitor::REMOVE_NODE) {
+                $doNodes[] = [$i, []];
+            } else {
+                $nodes[$i] = $result;
             }
 
-            $this->traverseNode($node, $file);
             if ($this->stopTraversal) {
                 break;
             }
@@ -140,6 +100,75 @@ private function traverseArray(array $nodes, File $file): array
         return $nodes;
     }
 
+    /**
+     * Applies the node's rules, then traverses into its children.
+     *
+     * @param bool $allowListReturn whether an array / REMOVE_NODE return is valid here (only for a
+     *                              node that lives inside a list, not a single sub-node slot)
+     * @return NodeVisitor::REMOVE_NODE|Node|Node[]
+     */
+    private function refactorNode(Node $node, File $file, bool $allowListReturn): int|Node|array
+    {
+        $return = $this->applyRectors($node, $file);
+
+        if (\is_array($return) || $return === NodeVisitor::REMOVE_NODE) {
+            if (! $allowListReturn) {
+                throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return));
+            }
+
+            // a list / removal replaces the node in its parent list; its children are not traversed
+            return $return;
+        }
+
+        if ($return::class !== $node::class) {
+            // stop traversing as node type changed and visitors won't work
+            return $return;
+        }
+
+        $this->traverseNode($return, $file);
+
+        return $return;
+    }
+
+    /**
+     * Runs every rule subscribed to this node, in order. A rule returning a same-type node keeps the
+     * remaining rules running on the new node; a type change, an array or REMOVE_NODE stops the chain
+     * and is handed back to the caller.
+     *
+     * @return NodeVisitor::REMOVE_NODE|Node|Node[]
+     */
+    private function applyRectors(Node $node, File $file): int|Node|array
+    {
+        $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($node, $file);
+
+        foreach ($currentNodeVisitors as $currentNodeVisitor) {
+            $return = $this->rectorRunner->run($currentNodeVisitor, $node);
+            if ($return === null) {
+                continue;
+            }
+
+            if ($return instanceof Node) {
+                $originalNodeClass = $node::class;
+                $this->ensureReplacementReasonable($node, $return);
+                $node = $return;
+
+                if ($originalNodeClass !== $node::class) {
+                    return $node;
+                }
+
+                continue;
+            }
+
+            if (\is_array($return) || $return === NodeVisitor::REMOVE_NODE) {
+                return $return;
+            }
+
+            throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return));
+        }
+
+        return $node;
+    }
+
     private function ensureReplacementReasonable(Node $old, Node $new): void
     {
         if ($old instanceof Stmt) {

src/ValueObject/Application/File.php

@@ -17,7 +17,7 @@
 
 final class File
 {
-    private bool $hasChanged = false;
+    //    private bool $hasChanged = false;
 
     private readonly string $originalFileContent;
 
@@ -74,7 +74,7 @@ public function changeFileContent(string $newFileContent): void
         }
 
         $this->fileContent = $newFileContent;
-        $this->hasChanged = true;
+        //        $this->hasChanged = true;
     }
 
     public function getOriginalFileContent(): string
@@ -84,13 +84,14 @@ public function getOriginalFileContent(): string
 
     public function hasChanged(): bool
     {
-        return $this->hasChanged;
+        return $this->rectorWithLineChanges !== [];
+        //         return $this->hasChanged;
     }
 
-    public function changeHasChanged(bool $status): void
-    {
-        $this->hasChanged = $status;
-    }
+    //    public function changeHasChanged(bool $status): void
+    //    {
+    //        $this->hasChanged = $status;
+    //    }
 
     public function setFileDiff(FileDiff $fileDiff): void
     {

tests/Issues/RepeatedTwice/Fixture/fixture.php.inc

@@ -0,0 +1,35 @@
+<?php
+
+namespace Rector\Tests\Issues\RepeatedTwice\Fixture;
+
+final class Fixture
+{
+    private string $name;
+
+    public function change($name)
+    {
+        $this->name = $name;
+
+        return $name;
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\Issues\RepeatedTwice\Fixture;
+
+final class Fixture
+{
+    private string $name;
+
+    public function change(string $name): string
+    {
+        $this->name = $name;
+
+        return $name;
+    }
+}
+
+?>

tests/Issues/RepeatedTwice/RepeatedTwiceTest.php

@@ -0,0 +1,34 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Issues\RepeatedTwice;
+
+use Iterator;
+use PHPUnit\Framework\Attributes\DataProvider;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+
+/**
+ * The return-type rule depends on the output of the param-type rule registered after it,
+ * so the file only reaches its final state on the 2nd pass
+ *
+ * @see https://github.com/rectorphp/rector-src/pull/8826
+ */
+final class RepeatedTwiceTest extends AbstractRectorTestCase
+{
+    #[DataProvider('provideData')]
+    public function test(string $filePath): void
+    {
+        $this->doTestFile($filePath);
+    }
+
+    public static function provideData(): Iterator
+    {
+        return self::yieldFilesFromDirectory(__DIR__ . '/Fixture');
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/configured_rule.php';
+    }
+}

tests/Issues/RepeatedTwice/config/configured_rule.php

@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\Config\RectorConfig;
+use Rector\TypeDeclaration\Rector\ClassMethod\AddParamTypeFromPropertyTypeRector;
+use Rector\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictParamRector;
+
+return RectorConfig::configure()
+    // registered in reverse order of dependency:
+    // 2nd pass: native return type from the now-typed param
+    // 1st pass: native param type from the typed property
+    ->withRules([
+        ReturnTypeFromStrictParamRector::class,
+        AddParamTypeFromPropertyTypeRector::class,
+    ]);