perf: single-pass FileProcessor (drop do-while reprint loop)

bench/rerun_class_rectors_bench.php

@@ -0,0 +1,91 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * Benchmark for the "re-offer Class_ after child traversal" change in RectorNodeTraverser.
+ *
+ * The traverser is pre-order: a Class_ rule runs before the class children are visited, so a rule
+ * whose precondition is only met by a descendant removal (e.g. an emptied method body) would miss
+ * it in a single pass. The change re-runs the Class_ rules once, after the children settle.
+ *
+ * The re-offer runs the Class_ rules once more per class. This bench isolates that added cost:
+ * a file of many classes, one Class_ rule registered (RemoveEmptyClassMethodRector), each class
+ * re-offered once. It is the price of restoring Class_ convergence in a single pass, and is far
+ * cheaper than the whole-tree do-while re-traversal it replaces (that re-ran every rule on every
+ * node until stable; this re-runs only Class_ rules, only on classes, exactly once).
+ *
+ * Run on both branches to compare:
+ *   php bench/rerun_class_rectors_bench.php
+ *
+ * Reports CPU time (user+sys); CPU is the reliable metric, wall is noisy under concurrent load.
+ */
+
+use Rector\Application\RectorRegistry;
+use Rector\DeadCode\Rector\ClassMethod\RemoveEmptyClassMethodRector;
+use Rector\DependencyInjection\LazyContainerFactory;
+use Rector\PhpParser\NodeTraverser\RectorNodeTraverser;
+use Rector\Testing\TestingParser\TestingParser;
+use Rector\ValueObject\Application\File;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+const CLASSES = 400; // classes in the source file, each with non-empty methods
+const METHODS = 3;   // methods per class
+const REPEAT = 200;  // traversals of the whole file
+
+$rectorConfig = (new LazyContainerFactory())->create();
+$rectorConfig->boot();
+
+/** @var RectorRegistry $rectorRegistry */
+$rectorRegistry = $rectorConfig->make(RectorRegistry::class);
+$rectorRegistry->refreshRectors([$rectorConfig->make(RemoveEmptyClassMethodRector::class)]);
+
+/** @var RectorNodeTraverser $rectorNodeTraverser */
+$rectorNodeTraverser = $rectorConfig->make(RectorNodeTraverser::class);
+
+/** @var TestingParser $testingParser */
+$testingParser = $rectorConfig->make(TestingParser::class);
+
+// build a file of many classes, every method non-empty so no node is removed;
+// the re-offer runs the rule once per class and returns null - pure added overhead
+$source = "<?php\n\nnamespace Bench;\n";
+for ($c = 0; $c < CLASSES; ++$c) {
+    $source .= "\nclass SomeClass{$c}\n{\n";
+    for ($m = 0; $m < METHODS; ++$m) {
+        $source .= "    public function method{$m}(int \$value): int\n    {\n        return \$value + {$m};\n    }\n";
+    }
+
+    $source .= "}\n";
+}
+
+$sourceFilePath = tempnam(sys_get_temp_dir(), 'bench_') . '.php';
+file_put_contents($sourceFilePath, $source);
+
+$file = new File($sourceFilePath, $source);
+$stmts = $testingParser->parseFileToDecoratedNodes($sourceFilePath);
+
+$cpuSeconds = static function (): float {
+    $usage = getrusage();
+    return $usage['ru_utime.tv_sec'] + $usage['ru_utime.tv_usec'] / 1_000_000
+        + $usage['ru_stime.tv_sec'] + $usage['ru_stime.tv_usec'] / 1_000_000;
+};
+
+// warm up caches once, excluded from timing
+$rectorNodeTraverser->traverse($stmts, $file);
+
+$wallStart = microtime(true);
+$cpuStart = $cpuSeconds();
+
+for ($repeat = 0; $repeat < REPEAT; ++$repeat) {
+    $rectorNodeTraverser->traverse($stmts, $file);
+}
+
+$cpu = $cpuSeconds() - $cpuStart;
+$wall = microtime(true) - $wallStart;
+
+@unlink($sourceFilePath);
+
+printf('classes: %d, methods/class: %d, traversals: %d%s', CLASSES, METHODS, REPEAT, PHP_EOL);
+printf('CPU:  %.4f s  (%.4f ms/traversal)%s', $cpu, $cpu / REPEAT * 1_000, PHP_EOL);
+printf('wall: %.4f s%s', $wall, PHP_EOL);

src/Application/FileProcessor.php

@@ -56,65 +56,42 @@ 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;
-            }
+        // no rule recorded a change: the print would just reproduce the original content,
+        // so skip the expensive format-preserving print entirely and cache the file
+        if ($file->getRectorWithLineChanges() === []) {
+            $this->changedFilesDetector->addCacheableFile($filePath);
+            return new FileProcessResult([], null, false);
+        }
 
-            $fileHasChanged = true;
-        } while (true);
+        // 4. print to file or string; this updates the file content and resolves
+        // whether the file really changed (line changes can still print identically)
+        $this->printFile($file, $configuration, $filePath);
 
-        // 5. add as cacheable if not changed at all
-        if (! $fileHasChanged) {
+        // content is identical despite reported line changes, so it is cacheable
+        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);
         }
 
-        $rectorWithLineChanges = $file->getRectorWithLineChanges();
-        if ($file->hasChanged() || $rectorWithLineChanges !== []) {
-            $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges(
-                $configuration->shouldShowDiffs(),
-                $file,
-                $file->getOriginalFileContent(),
-                $file->getFileContent(),
-                $file->getRectorWithLineChanges()
-            );
-
-            $file->setFileDiff($currentFileDiff);
-        }
+        $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

@@ -8,6 +8,7 @@
 use PhpParser\Node;
 use PhpParser\Node\Expr;
 use PhpParser\Node\Stmt;
+use PhpParser\Node\Stmt\Class_;
 use PhpParser\NodeVisitor;
 use Rector\Application\RectorRegistry;
 use Rector\Rector\RectorRunner;
@@ -76,6 +77,11 @@ private function traverseNode(Node $node, File $file): void
             }
 
             $this->traverseNode($subNode, $file);
+            // a child mutation (e.g. an emptied method body, or a rename that now duplicates)
+            // can make a Class_ rule newly apply; re-offer the class once children are settled.
+            // this substitutes for the former whole-tree do-while loop in FileProcessor
+            $node->{$name} = $this->reRunClassRectors($subNode, $file);
+            $subNode = $node->{$name};
 
             if ($this->stopTraversal) {
                 break;
@@ -126,6 +132,10 @@ private function traverseArray(array $nodes, File $file): array
             }
 
             $this->traverseNode($node, $file);
+
+            // re-offer a Class_ after its children settle (see traverseNode)
+            $nodes[$i] = $node = $this->reRunClassRectors($node, $file);
+
             if ($this->stopTraversal) {
                 break;
             }
@@ -140,6 +150,29 @@ private function traverseArray(array $nodes, File $file): array
         return $nodes;
     }
 
+    /**
+     * Re-run Class_ rules once, after its children are traversed, so a rule whose
+     * precondition is only met by a descendant removal still fires in a single pass.
+     */
+    private function reRunClassRectors(Node $node, File $file): Node
+    {
+        if (! $node instanceof Class_) {
+            return $node;
+        }
+
+        foreach ($this->rectorRegistry->forNodeAndFile($node, $file) as $currentNodeVisitor) {
+            $return = $this->rectorRunner->run($currentNodeVisitor, $node);
+            if (! $return instanceof Node) {
+                continue;
+            }
+
+            $this->ensureReplacementReasonable($node, $return);
+            $node = $return;
+        }
+
+        return $node;
+    }
+
     private function ensureReplacementReasonable(Node $old, Node $new): void
     {
         if ($old instanceof Stmt) {

src/ValueObject/Application/File.php

@@ -87,11 +87,6 @@ public function hasChanged(): bool
         return $this->hasChanged;
     }
 
-    public function changeHasChanged(bool $status): void
-    {
-        $this->hasChanged = $status;
-    }
-
     public function setFileDiff(FileDiff $fileDiff): void
     {
         $this->fileDiff = $fileDiff;