[perf] Decouple skipper from node run, drop clone-probe double run (#8714)

Author: TomasVotruba

rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php

@@ -17,27 +17,25 @@
  */
 final class SkipUsedTrackingTest extends AbstractRectorTestCase
 {
-    public function testMarksSkipUsedOnlyWhenRuleWouldChangeFile(): void
+    public function testMarksSkipUsedForEveryMatchedFile(): void
     {
-        // file uses OldClass → rule would rename it → its skip is what prevents the change
+        // file uses OldClass → rule matches the path and is skipped → its skip mask is marked used
         $this->doTestFile(__DIR__ . '/FixtureSkipUsedTracking/skip_used_renames_old_class.php.inc');
 
         // doTestFile() only cleans up the last processed temp file, so remove this one explicitly
         FileSystem::delete(__DIR__ . '/FixtureSkipUsedTracking/skip_used_renames_old_class.php');
 
-        // file does not use OldClass → rule would not change it → its skip is unnecessary
+        // file does not use OldClass, but its path still matches a skip mask → skip is applied and marked used
         $this->doTestFile(__DIR__ . '/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc');
 
         $skipper = $this->make(Skipper::class);
         $usedSkips = $skipper->provideUsedSkips();
 
         $usedPaths = $usedSkips[RenameClassRector::class] ?? [];
 
-        // both skip masks match a processed file, so both are marked used
+        // the rule is skipped once per file whose path matches, so every matched mask is marked used
         $this->assertContains('*skip_used_renames_old_class*', $usedPaths);
-
-        // the skip on a file the rule would not have touched is never marked used
-        $this->assertNotContains('*skip_unused_no_old_class*', $usedPaths);
+        $this->assertContains('*skip_unused_no_old_class*', $usedPaths);
     }
 
     public function provideConfigFilePath(): string

rules-tests/Renaming/Rector/Name/RenameClassRector/config/skip_used_tracking_configured_rule.php

@@ -12,8 +12,7 @@
         OldClass::class => NewClass::class,
     ]);
 
-    // both paths are skipped, but only the first file actually uses OldClass, so only its skip
-    // prevents a real change - the second one would not have changed anyway
+    // both paths are skipped; the rule is skipped once per matching file, so both masks are marked used
     $rectorConfig->skip([
         RenameClassRector::class => ['*skip_used_renames_old_class*', '*skip_unused_no_old_class*'],
     ]);

src/Application/FileProcessor.php

@@ -63,7 +63,7 @@ public function processFile(File $file, Configuration $configuration): FileProce
             $file->changeHasChanged(false);
 
             // 1. change nodes with Rector Rules
-            $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts());
+            $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file);
 
             // 2. apply post rectors
             $postNewStmts = $this->postFileProcessor->traverse($newStmts, $file);

src/Application/RectorRegistry.php

@@ -7,6 +7,8 @@
 use PhpParser\Node;
 use Rector\Configuration\ConfigurationRuleFilter;
 use Rector\Contract\Rector\RectorInterface;
+use Rector\Skipper\Skipper\Skipper;
+use Rector\ValueObject\Application\File;
 use Rector\VersionBonding\ComposerPackageConstraintFilter;
 use Rector\VersionBonding\PhpVersionedFilter;
 
@@ -34,6 +36,7 @@ public function __construct(
         private readonly PhpVersionedFilter $phpVersionedFilter,
         private readonly ComposerPackageConstraintFilter $composerPackageConstraintFilter,
         private readonly ConfigurationRuleFilter $configurationRuleFilter,
+        private readonly Skipper $skipper,
         private array $rectors,
     ) {
     }
@@ -75,6 +78,28 @@ public function forNode(Node $node): array
         return $this->cache[$nodeClass];
     }
 
+    /**
+     * Filters out rectors skipped for this file before they ever run, so the skip no longer needs a
+     * second refactor() probe on a clone to learn whether it mattered.
+     *
+     * Known limit: a configured skip is marked used as soon as its path matches the file, even if the
+     * rule would not have changed it. This drops precise unused-skip reporting in exchange for not
+     * running the rule twice per skipped node.
+     *
+     * @return RectorInterface[]
+     */
+    public function forNodeAndFile(Node $node, File $file): array
+    {
+        $rectors = $this->forNode($node);
+
+        $filePath = $file->getFilePath();
+
+        return array_filter(
+            $rectors,
+            fn (RectorInterface $rector): bool => ! $this->skipper->shouldSkipRectorAndFile($rector, $filePath)
+        );
+    }
+
     /**
      * Filtering must happen lazily, after the configuration is set in ProcessCommand::execute(),
      * otherwise options like --only are not yet available and get ignored.

src/PhpParser/NodeTraverser/RectorNodeTraverser.php

@@ -11,6 +11,7 @@
 use PhpParser\NodeVisitor;
 use Rector\Application\RectorRegistry;
 use Rector\Rector\RectorRunner;
+use Rector\ValueObject\Application\File;
 
 final class RectorNodeTraverser
 {
@@ -26,19 +27,19 @@ public function __construct(
      * @param Node[] $nodes
      * @return Node[]
      */
-    public function traverse(array $nodes): array
+    public function traverse(array $nodes, File $file): array
     {
         $this->stopTraversal = false;
 
-        return $this->traverseArray($nodes);
+        return $this->traverseArray($nodes, $file);
     }
 
-    private function traverseNode(Node $node): void
+    private function traverseNode(Node $node, File $file): void
     {
         foreach ($node->getSubNodeNames() as $name) {
             $subNode = $node->{$name};
             if (\is_array($subNode)) {
-                $node->{$name} = $this->traverseArray($subNode);
+                $node->{$name} = $this->traverseArray($subNode, $file);
                 if ($this->stopTraversal) {
                     break;
                 }
@@ -50,7 +51,7 @@ private function traverseNode(Node $node): void
                 continue;
             }
 
-            $currentNodeVisitors = $this->rectorRegistry->forNode($subNode);
+            $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($subNode, $file);
 
             foreach ($currentNodeVisitors as $currentNodeVisitor) {
                 $return = $this->rectorRunner->run($currentNodeVisitor, $subNode);
@@ -74,7 +75,8 @@ private function traverseNode(Node $node): void
                 }
             }
 
-            $this->traverseNode($subNode);
+            $this->traverseNode($subNode, $file);
+
             if ($this->stopTraversal) {
                 break;
             }
@@ -85,7 +87,7 @@ private function traverseNode(Node $node): void
      * @param array<Node|null> $nodes The null can be in case of empty list(, , )
      * @return Node[]
      */
-    private function traverseArray(array $nodes): array
+    private function traverseArray(array $nodes, File $file): array
     {
         $doNodes = [];
         foreach ($nodes as $i => $node) {
@@ -97,7 +99,7 @@ private function traverseArray(array $nodes): array
                 continue;
             }
 
-            $currentNodeVisitors = $this->rectorRegistry->forNode($node);
+            $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($node, $file);
 
             foreach ($currentNodeVisitors as $currentNodeVisitor) {
                 $return = $this->rectorRunner->run($currentNodeVisitor, $node);
@@ -123,7 +125,7 @@ private function traverseArray(array $nodes): array
                 }
             }
 
-            $this->traverseNode($node);
+            $this->traverseNode($node, $file);
             if ($this->stopTraversal) {
                 break;
             }

src/Rector/RectorRunner.php

@@ -5,10 +5,7 @@
 namespace Rector\Rector;
 
 use PhpParser\Node;
-use PhpParser\NodeTraverser;
 use PhpParser\NodeVisitor;
-use PhpParser\NodeVisitor\CloningVisitor;
-use PhpParser\NodeVisitorAbstract;
 use PHPStan\Analyser\MutatingScope;
 use Rector\Application\ChangedNodeScopeRefresher;
 use Rector\Application\Provider\CurrentFileProvider;
@@ -18,7 +15,6 @@
 use Rector\NodeDecorator\CreatedByRuleDecorator;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\Skipper\Skipper\Skipper;
-use Rector\Skipper\ValueObject\SkipMatch;
 use Rector\ValueObject\Application\File;
 
 /**
@@ -62,18 +58,6 @@ public function run(RectorInterface $rector, Node $node): int|Node|null|array
             return null;
         }
 
-        // class/path skip is configured for this rule and file: run the rule on a deep clone to learn
-        // whether it would actually have changed anything. Only a skip that prevents a real change
-        // counts as used; the original node is left untouched, so the file stays skipped either way.
-        $skipMatch = $this->skipper->matchSkip($rector, $filePath);
-        if ($skipMatch instanceof SkipMatch) {
-            if ($rector->refactor($this->cloneNode($node)) !== null) {
-                $this->skipper->markSkipUsed($skipMatch);
-            }
-
-            return null;
-        }
-
         // ensure origNode pulled before refactor to avoid changed during refactor, ref https://3v4l.org/YMEGN
         $originalNode = $node->getAttribute(AttributeKey::ORIGINAL_NODE) ?? $node;
 
@@ -119,23 +103,6 @@ private function getFile(): File
         return $file;
     }
 
-    /**
-     * Deep clone, so a skipped rule can be probed on the clone without mutating the real node.
-     */
-    private function cloneNode(Node $node): Node
-    {
-        $phpDocInfoRemovingNodeVisitor = new class() extends NodeVisitorAbstract {
-            public function enterNode(Node $node): Node
-            {
-                $node->setAttribute(AttributeKey::PHP_DOC_INFO, null);
-                return $node;
-            }
-        };
-
-        $nodeTraverser = new NodeTraverser(new CloningVisitor(), $phpDocInfoRemovingNodeVisitor);
-        return $nodeTraverser->traverse([$node])[0];
-    }
-
     /**
      * @param Node|Node[] $refactoredNode
      * @return Node|Node[]

tests/PhpParser/NodeTraverser/StopTraverseOnTypeChange/StopTraverseOnTypeChangeTest.php

@@ -38,9 +38,9 @@ protected function setUp(): void
     public function testGetVisitorsForNodeWhenNoVisitorsAvailable(): void
     {
         // must be cloned + Scope set to allow node replacement
-        $nodes = $this->testingParser->parseFileToDecoratedNodes(__DIR__ . '/Fixture/SimpleClass.php');
+        $file = $this->testingParser->parseFilePathToFile(__DIR__ . '/Fixture/SimpleClass.php');
 
-        $changedNodes = $this->rectorNodeTraverser->traverse($nodes);
+        $changedNodes = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file);
 
         $nodeFinder = new NodeFinder();
         $classes = $nodeFinder->findInstanceOf($changedNodes, Class_::class);