From 8f1478ba90eaed798b50ea4138e39423123d5942 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 19 Oct 2026 11:40:08 +0200 Subject: [PATCH] Decouple skipper from node run, drop clone-probe double run Move class/path skip filtering into RectorRegistry::forNodeAndFile so skipped rules never run, instead of re-running refactor() on a deep clone just to decide whether a skip was used. The skip is marked used on path match. --- .../SkipUsedTrackingTest.php | 12 +++---- .../skip_used_tracking_configured_rule.php | 3 +- src/Application/FileProcessor.php | 2 +- src/Application/RectorRegistry.php | 25 ++++++++++++++ .../NodeTraverser/RectorNodeTraverser.php | 20 ++++++----- src/Rector/RectorRunner.php | 33 ------------------- .../StopTraverseOnTypeChangeTest.php | 4 +-- 7 files changed, 45 insertions(+), 54 deletions(-) diff --git a/rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php b/rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php index a658750c742..1e6da1876fe 100644 --- a/rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php +++ b/rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php @@ -17,15 +17,15 @@ */ 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); @@ -33,11 +33,9 @@ public function testMarksSkipUsedOnlyWhenRuleWouldChangeFile(): void $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 diff --git a/rules-tests/Renaming/Rector/Name/RenameClassRector/config/skip_used_tracking_configured_rule.php b/rules-tests/Renaming/Rector/Name/RenameClassRector/config/skip_used_tracking_configured_rule.php index 487d0cde776..7050700dcdf 100644 --- a/rules-tests/Renaming/Rector/Name/RenameClassRector/config/skip_used_tracking_configured_rule.php +++ b/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*'], ]); diff --git a/src/Application/FileProcessor.php b/src/Application/FileProcessor.php index 1fc05375392..91f2ec44a1c 100644 --- a/src/Application/FileProcessor.php +++ b/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); diff --git a/src/Application/RectorRegistry.php b/src/Application/RectorRegistry.php index d5dd8d8f0fb..c251d01d43f 100644 --- a/src/Application/RectorRegistry.php +++ b/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. diff --git a/src/PhpParser/NodeTraverser/RectorNodeTraverser.php b/src/PhpParser/NodeTraverser/RectorNodeTraverser.php index a4f85d9bb7d..f45faa2dcdf 100644 --- a/src/PhpParser/NodeTraverser/RectorNodeTraverser.php +++ b/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 $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; } diff --git a/src/Rector/RectorRunner.php b/src/Rector/RectorRunner.php index e0690dc744d..5f0e8224aa3 100644 --- a/src/Rector/RectorRunner.php +++ b/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[] diff --git a/tests/PhpParser/NodeTraverser/StopTraverseOnTypeChange/StopTraverseOnTypeChangeTest.php b/tests/PhpParser/NodeTraverser/StopTraverseOnTypeChange/StopTraverseOnTypeChangeTest.php index 264eef77dd4..062696559d7 100644 --- a/tests/PhpParser/NodeTraverser/StopTraverseOnTypeChange/StopTraverseOnTypeChangeTest.php +++ b/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);