From f82e8b7b8162bceb820426af5e430bd830e88fd6 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 22 Jun 2026 10:49:37 +0200 Subject: [PATCH] [skip] mark class/path skip used only when rule would change the file Previously a skip was marked used the moment a rule's node matched it, even if refactor() would return null. Now the skipped rule is probed on a deep clone and the skip is marked used only when it would actually change the file; the real node is left untouched. --- .../skip_unused_no_old_class.php.inc | 7 +++ .../skip_used_renames_old_class.php.inc | 7 +++ .../SkipUsedTrackingTest.php | 46 +++++++++++++++++++ .../skip_used_tracking_configured_rule.php | 20 ++++++++ src/Rector/AbstractRector.php | 28 ++++++++++- src/Skipper/SkipVoter/ClassSkipVoter.php | 5 +- src/Skipper/Skipper/SkipSkipper.php | 18 ++++---- src/Skipper/Skipper/Skipper.php | 36 ++++++++++----- src/Skipper/ValueObject/SkipMatch.php | 28 +++++++++++ 9 files changed, 171 insertions(+), 24 deletions(-) create mode 100644 rules-tests/Renaming/Rector/Name/RenameClassRector/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc create mode 100644 rules-tests/Renaming/Rector/Name/RenameClassRector/FixtureSkipUsedTracking/skip_used_renames_old_class.php.inc create mode 100644 rules-tests/Renaming/Rector/Name/RenameClassRector/SkipUsedTrackingTest.php create mode 100644 rules-tests/Renaming/Rector/Name/RenameClassRector/config/skip_used_tracking_configured_rule.php create mode 100644 src/Skipper/ValueObject/SkipMatch.php diff --git a/rules-tests/Renaming/Rector/Name/RenameClassRector/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc b/rules-tests/Renaming/Rector/Name/RenameClassRector/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc new file mode 100644 index 00000000000..ec4ed767c3e --- /dev/null +++ b/rules-tests/Renaming/Rector/Name/RenameClassRector/FixtureSkipUsedTracking/skip_unused_no_old_class.php.inc @@ -0,0 +1,7 @@ +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'; + } +} 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 new file mode 100644 index 00000000000..487d0cde776 --- /dev/null +++ b/rules-tests/Renaming/Rector/Name/RenameClassRector/config/skip_used_tracking_configured_rule.php @@ -0,0 +1,20 @@ +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*'], + ]); +}; diff --git a/src/Rector/AbstractRector.php b/src/Rector/AbstractRector.php index 5dd55bf149e..abb90e8f46f 100644 --- a/src/Rector/AbstractRector.php +++ b/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[] diff --git a/src/Skipper/SkipVoter/ClassSkipVoter.php b/src/Skipper/SkipVoter/ClassSkipVoter.php index 46f20c3c165..4e0d4264d7c 100644 --- a/src/Skipper/SkipVoter/ClassSkipVoter.php +++ b/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); } } diff --git a/src/Skipper/Skipper/SkipSkipper.php b/src/Skipper/Skipper/SkipSkipper.php index cc654a67e94..dab6e328ece 100644 --- a/src/Skipper/Skipper/SkipSkipper.php +++ b/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 $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; } } diff --git a/src/Skipper/Skipper/Skipper.php b/src/Skipper/Skipper/Skipper.php index e7ff1645d25..bf4fae8e428 100644 --- a/src/Skipper/Skipper/Skipper.php +++ b/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 $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 $rectorClass + */ + public function shouldSkipCurrentNode(string $rectorClass, Node $node): bool + { return $this->rectifiedAnalyzer->hasRectified($rectorClass, $node); } } diff --git a/src/Skipper/ValueObject/SkipMatch.php b/src/Skipper/ValueObject/SkipMatch.php new file mode 100644 index 00000000000..139e9a521c1 --- /dev/null +++ b/src/Skipper/ValueObject/SkipMatch.php @@ -0,0 +1,28 @@ +skippedClass; + } + + public function getMatchedPath(): ?string + { + return $this->matchedPath; + } +}