[skip] mark class/path skip used only when rule would change the file

rules-tests/Renaming/Rector/Name/RenameClassRector/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc

@@ -0,0 +1,7 @@
+<?php
+
+namespace Rector\Tests\Renaming\Rector\Name\RenameClassRector\FixtureSkipUsedTracking;
+
+class SkipUnusedNoOldClass
+{
+}

rules-tests/Renaming/Rector/Name/RenameClassRector/FixtureSkipUsedTracking/skip_used_renames_old_class.php.inc

@@ -0,0 +1,7 @@
+<?php
+
+namespace Rector\Tests\Renaming\Rector\Name\RenameClassRector\FixtureSkipUsedTracking;
+
+class SkipUsedRenamesOldClass extends \Rector\Tests\Renaming\Rector\Name\RenameClassRector\Source\OldClass
+{
+}

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

@@ -0,0 +1,46 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Renaming\Rector\Name\RenameClassRector;
+
+use Nette\Utils\FileSystem;
+use Rector\Renaming\Rector\Name\RenameClassRector;
+use Rector\Skipper\Skipper\UsedSkipCollector;
+use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+
+/**
+ * A class/path skip is only "used" when the rule would actually have changed the skipped file.
+ *
+ * @see RenameClassRector
+ */
+final class SkipUsedTrackingTest extends AbstractRectorTestCase
+{
+    public function testMarksSkipUsedOnlyWhenRuleWouldChangeFile(): void
+    {
+        // file uses OldClass → rule would rename it → its skip is what prevents the change
+        $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
+        $this->doTestFile(__DIR__ . '/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc');
+
+        $usedSkipCollector = $this->make(UsedSkipCollector::class);
+        $usedSkips = $usedSkipCollector->provide();
+
+        $usedPaths = $usedSkips[RenameClassRector::class] ?? [];
+
+        // the skip that actually prevented a rename 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);
+    }
+
+    public function provideConfigFilePath(): string
+    {
+        return __DIR__ . '/config/skip_used_tracking_configured_rule.php';
+    }
+}

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

@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+use Rector\Config\RectorConfig;
+use Rector\Renaming\Rector\Name\RenameClassRector;
+use Rector\Tests\Renaming\Rector\Name\RenameClassRector\Source\NewClass;
+use Rector\Tests\Renaming\Rector\Name\RenameClassRector\Source\OldClass;
+
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->ruleWithConfiguration(RenameClassRector::class, [
+        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
+    $rectorConfig->skip([
+        RenameClassRector::class => ['*skip_used_renames_old_class*', '*skip_unused_no_old_class*'],
+    ]);
+};

src/Rector/AbstractRector.php

@@ -14,7 +14,9 @@
 use PhpParser\Node\Stmt\Interface_;
 use PhpParser\Node\Stmt\Property;
 use PhpParser\Node\Stmt\Trait_;
+use PhpParser\NodeTraverser;
 use PhpParser\NodeVisitor;
+use PhpParser\NodeVisitor\CloningVisitor;
 use PhpParser\NodeVisitorAbstract;
 use PHPStan\Analyser\MutatingScope;
 use PHPStan\Type\ObjectType;
@@ -34,6 +36,7 @@
 use Rector\PhpParser\Comparing\NodeComparator;
 use Rector\PhpParser\Node\NodeFactory;
 use Rector\Skipper\Skipper\Skipper;
+use Rector\Skipper\ValueObject\SkipMatch;
 use Rector\ValueObject\Application\File;
 
 abstract class AbstractRector extends NodeVisitorAbstract implements RectorInterface
@@ -132,7 +135,21 @@ final public function enterNode(Node $node): int|Node|null|array
         }
 
         $filePath = $this->file->getFilePath();
-        if ($this->skipper->shouldSkipCurrentNode($this, $filePath, static::class, $node)) {
+
+        // node already changed by this rule in a previous pass → hard skip
+        if ($this->skipper->shouldSkipCurrentNode(static::class, $node)) {
+            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($this, $filePath);
+        if ($skipMatch instanceof SkipMatch) {
+            if ($this->refactor($this->cloneNode($node)) !== null) {
+                $this->skipper->markSkipUsed($skipMatch);
+            }
+
             return null;
         }
 
@@ -258,6 +275,15 @@ protected function mirrorComments(Node $newNode, Node $oldNode): void
         $this->commentsMerger->mirrorComments($newNode, $oldNode);
     }
 
+    /**
+     * Deep clone, so a skipped rule can be probed on the clone without mutating the real node.
+     */
+    private function cloneNode(Node $node): Node
+    {
+        $nodeTraverser = new NodeTraverser(new CloningVisitor());
+        return $nodeTraverser->traverse([$node])[0];
+    }
+
     /**
      * @param Node|Node[] $refactoredNode
      * @return Node|Node[]

src/Skipper/SkipVoter/ClassSkipVoter.php

@@ -7,6 +7,7 @@
 use PHPStan\Reflection\ReflectionProvider;
 use Rector\Skipper\SkipCriteriaResolver\SkippedClassResolver;
 use Rector\Skipper\Skipper\SkipSkipper;
+use Rector\Skipper\ValueObject\SkipMatch;
 
 final readonly class ClassSkipVoter
 {
@@ -26,9 +27,9 @@ public function match(string | object $element): bool
         return $this->reflectionProvider->hasClass($element);
     }
 
-    public function shouldSkip(string | object $element, string $filePath): bool
+    public function matchSkip(string | object $element, string $filePath): ?SkipMatch
     {
         $skippedClasses = $this->skippedClassResolver->resolve();
-        return $this->skipSkipper->doesMatchSkip($element, $filePath, $skippedClasses);
+        return $this->skipSkipper->match($element, $filePath, $skippedClasses);
     }
 }

src/Skipper/Skipper/SkipSkipper.php

@@ -5,19 +5,19 @@
 namespace Rector\Skipper\Skipper;
 
 use Rector\Skipper\Matcher\FileInfoMatcher;
+use Rector\Skipper\ValueObject\SkipMatch;
 
 final readonly class SkipSkipper
 {
     public function __construct(
-        private FileInfoMatcher $fileInfoMatcher,
-        private UsedSkipCollector $usedSkipCollector
+        private FileInfoMatcher $fileInfoMatcher
     ) {
     }
 
     /**
      * @param array<string, string[]|null> $skippedClasses
      */
-    public function doesMatchSkip(object | string $checker, string $filePath, array $skippedClasses): bool
+    public function match(object | string $checker, string $filePath, array $skippedClasses): ?SkipMatch
     {
         foreach ($skippedClasses as $skippedClass => $skippedFiles) {
             if (! is_a($checker, $skippedClass, true)) {
@@ -26,19 +26,17 @@ public function doesMatchSkip(object | string $checker, string $filePath, array
 
             // skip everywhere
             if (! is_array($skippedFiles)) {
-                $this->usedSkipCollector->markUsed($skippedClass);
-                return true;
+                return new SkipMatch($skippedClass, null);
             }
 
-            // mark the specific matched path used, scoped to its rule - the same path can be skipped
-            // under multiple rules, so the path is nested under its rule, not tracked on its own
+            // the same path can be skipped under multiple rules, so the matched path is reported
+            // scoped to its rule, not tracked on its own
             $matchedPath = $this->fileInfoMatcher->matchPattern($filePath, $skippedFiles);
             if ($matchedPath !== null) {
-                $this->usedSkipCollector->markUsed($skippedClass, $matchedPath);
-                return true;
+                return new SkipMatch($skippedClass, $matchedPath);
             }
         }
 
-        return false;
+        return null;
     }
 }

src/Skipper/Skipper/Skipper.php

@@ -8,6 +8,7 @@
 use Rector\Contract\Rector\RectorInterface;
 use Rector\ProcessAnalyzer\RectifiedAnalyzer;
 use Rector\Skipper\SkipVoter\ClassSkipVoter;
+use Rector\Skipper\ValueObject\SkipMatch;
 
 /**
  * @api
@@ -19,6 +20,7 @@ public function __construct(
         private RectifiedAnalyzer $rectifiedAnalyzer,
         private PathSkipper $pathSkipper,
         private ClassSkipVoter $classSkipVoter,
+        private UsedSkipCollector $usedSkipCollector,
     ) {
     }
 
@@ -34,26 +36,38 @@ public function shouldSkipFilePath(string $filePath): bool
 
     public function shouldSkipElementAndFilePath(string | object $element, string $filePath): bool
     {
-        if (! $this->classSkipVoter->match($element)) {
+        $skipMatch = $this->matchSkip($element, $filePath);
+        if (! $skipMatch instanceof SkipMatch) {
             return false;
         }
 
-        return $this->classSkipVoter->shouldSkip($element, $filePath);
+        $this->markSkipUsed($skipMatch);
+        return true;
     }
 
     /**
-     * @param class-string<RectorInterface> $rectorClass
+     * Match a class/path skip without marking it used. Callers that can only tell whether the skip
+     * actually prevented a change later on must mark it used themselves via markSkipUsed().
      */
-    public function shouldSkipCurrentNode(
-        string | object $element,
-        string $filePath,
-        string $rectorClass,
-        Node $node
-    ): bool {
-        if ($this->shouldSkipElementAndFilePath($element, $filePath)) {
-            return true;
+    public function matchSkip(string | object $element, string $filePath): ?SkipMatch
+    {
+        if (! $this->classSkipVoter->match($element)) {
+            return null;
         }
 
+        return $this->classSkipVoter->matchSkip($element, $filePath);
+    }
+
+    public function markSkipUsed(SkipMatch $skipMatch): void
+    {
+        $this->usedSkipCollector->markUsed($skipMatch->getSkippedClass(), $skipMatch->getMatchedPath());
+    }
+
+    /**
+     * @param class-string<RectorInterface> $rectorClass
+     */
+    public function shouldSkipCurrentNode(string $rectorClass, Node $node): bool
+    {
         return $this->rectifiedAnalyzer->hasRectified($rectorClass, $node);
     }
 }

src/Skipper/ValueObject/SkipMatch.php

@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Skipper\ValueObject;
+
+/**
+ * Result of a matched class/path skip: the skipped class and the concrete path pattern
+ * that matched, if the skip is scoped to specific paths.
+ */
+final readonly class SkipMatch
+{
+    public function __construct(
+        private string $skippedClass,
+        private ?string $matchedPath
+    ) {
+    }
+
+    public function getSkippedClass(): string
+    {
+        return $this->skippedClass;
+    }
+
+    public function getMatchedPath(): ?string
+    {
+        return $this->matchedPath;
+    }
+}