From 6132a78dc2c2ed017b0e44a01a355f84f31f748f Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sat, 17 Oct 2026 17:11:21 +0200 Subject: [PATCH 1/2] decouple node traversal from skipper, simplify RectorNodeTraverser Split forNode() from forNodeAndFile() so the per-file skip lives in RectorRegistry, drop the created-by-rule reprocessing guard, and keep the traverser marker-free. Adjust affected tests. --- .../Fixture/with_interface.php.inc | 18 +++--- .../Source/ImplementedInterface.php | 10 +++ .../SkipUsedTrackingTest.php | 10 +-- .../skip_used_tracking_configured_rule.php | 3 +- .../ChangeMethodVisibilityRector.php | 8 +-- src/Application/FileProcessor.php | 2 +- src/Application/RectorRegistry.php | 18 ++++++ src/NodeDecorator/CreatedByRuleDecorator.php | 63 ------------------- .../NodeTraverser/RectorNodeTraverser.php | 20 +++--- src/Rector/RectorRunner.php | 46 -------------- ...FullyQualifiedIdentifierDocblockRector.php | 7 +++ .../config/configured_rule.php | 5 +- .../StopTraverseOnTypeChangeTest.php | 4 +- 13 files changed, 66 insertions(+), 148 deletions(-) create mode 100644 rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Source/ImplementedInterface.php delete mode 100644 src/NodeDecorator/CreatedByRuleDecorator.php diff --git a/rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Fixture/with_interface.php.inc b/rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Fixture/with_interface.php.inc index 890def08d1f..3cca3a9297a 100644 --- a/rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Fixture/with_interface.php.inc +++ b/rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Fixture/with_interface.php.inc @@ -1,7 +1,11 @@ ----- diff --git a/rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Source/ImplementedInterface.php b/rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Source/ImplementedInterface.php new file mode 100644 index 00000000000..ff8ab30204a --- /dev/null +++ b/rules-tests/DeadCode/Rector/ClassMethod/RemoveEmptyClassMethodRector/Source/ImplementedInterface.php @@ -0,0 +1,10 @@ +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'); $skipper = $this->make(Skipper::class); @@ -33,11 +31,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/rules/Visibility/Rector/ClassMethod/ChangeMethodVisibilityRector.php b/rules/Visibility/Rector/ClassMethod/ChangeMethodVisibilityRector.php index 715e7722e0e..132ad4e8fc7 100644 --- a/rules/Visibility/Rector/ClassMethod/ChangeMethodVisibilityRector.php +++ b/rules/Visibility/Rector/ClassMethod/ChangeMethodVisibilityRector.php @@ -133,12 +133,6 @@ public function configure(array $configuration): void private function hasMatchingMethodName(ClassMethod $classMethod): bool { - foreach ($this->methodVisibilities as $methodVisibility) { - if ($this->isName($classMethod, $methodVisibility->getMethod())) { - return true; - } - } - - return false; + return array_any($this->methodVisibilities, fn(ChangeMethodVisibility $methodVisibility): bool => $this->isName($classMethod, $methodVisibility->getMethod())); } } 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..feda7d61d13 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,21 @@ public function forNode(Node $node): array return $this->cache[$nodeClass]; } + /** + * @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/NodeDecorator/CreatedByRuleDecorator.php b/src/NodeDecorator/CreatedByRuleDecorator.php deleted file mode 100644 index 0843e006e6e..00000000000 --- a/src/NodeDecorator/CreatedByRuleDecorator.php +++ /dev/null @@ -1,63 +0,0 @@ -|Node $node - * @param class-string $rectorClass - */ - public function decorate(array|Node $node, Node $originalNode, string $rectorClass): void - { - if ($node instanceof Node && $node === $originalNode) { - $this->createByRule($node, $rectorClass); - return; - } - - if ($node instanceof Node) { - $node = [$node]; - } - - foreach ($node as $singleNode) { - if ($singleNode::class === $originalNode::class) { - $this->createByRule($singleNode, $rectorClass); - } - } - - $this->createByRule($originalNode, $rectorClass); - } - - /** - * @param class-string $rectorClass - */ - private function createByRule(Node $node, string $rectorClass): void - { - /** @var class-string[] $createdByRule */ - $createdByRule = $node->getAttribute(AttributeKey::CREATED_BY_RULE) ?? []; - - // empty array, insert - if ($createdByRule === []) { - $node->setAttribute(AttributeKey::CREATED_BY_RULE, [$rectorClass]); - return; - } - - // consecutive, no need to refill - if (end($createdByRule) === $rectorClass) { - return; - } - - // filter out when exists, then append - $createdByRule = array_filter( - $createdByRule, - static fn (string $rectorRule): bool => $rectorRule !== $rectorClass - ); - $node->setAttribute(AttributeKey::CREATED_BY_RULE, [...$createdByRule, $rectorClass]); - } -} 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..ff17126bbfe 100644 --- a/src/Rector/RectorRunner.php +++ b/src/Rector/RectorRunner.php @@ -5,20 +5,14 @@ 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; use Rector\ChangesReporting\ValueObject\RectorWithLineChange; use Rector\Contract\Rector\RectorInterface; use Rector\Exception\ShouldNotHappenException; -use Rector\NodeDecorator\CreatedByRuleDecorator; use Rector\NodeTypeResolver\Node\AttributeKey; -use Rector\Skipper\Skipper\Skipper; -use Rector\Skipper\ValueObject\SkipMatch; use Rector\ValueObject\Application\File; /** @@ -42,8 +36,6 @@ CODE_SAMPLE; public function __construct( - private Skipper $skipper, - private CreatedByRuleDecorator $createdByRuleDecorator, private ChangedNodeScopeRefresher $changedNodeScopeRefresher, private CurrentFileProvider $currentFileProvider, ) { @@ -57,23 +49,6 @@ public function run(RectorInterface $rector, Node $node): int|Node|null|array $file = $this->getFile(); $filePath = $file->getFilePath(); - // node already changed by this rule in a previous pass → hard skip - if ($this->skipper->shouldSkipCurrentNode($rector::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($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; @@ -90,8 +65,6 @@ public function run(RectorInterface $rector, Node $node): int|Node|null|array } if (is_int($refactoredNodeOrState)) { - $this->createdByRuleDecorator->decorate($node, $originalNode, $rector::class); - // only remove node is supported if ($refactoredNodeOrState !== NodeVisitor::REMOVE_NODE) { throw new ShouldNotHappenException(sprintf('Unsupported state "%d" returned from "%s".', $refactoredNodeOrState, $rector::class)); @@ -119,23 +92,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[] @@ -149,8 +105,6 @@ private function postRefactorProcess( string $filePath ): Node|array { /** @var non-empty-array|Node $refactoredNode */ - $this->createdByRuleDecorator->decorate($refactoredNode, $originalNode, $rector::class); - $rectorWithLineChange = new RectorWithLineChange($rector::class, $originalNode->getStartLine()); $file->addRectorClassWithLine($rectorWithLineChange); diff --git a/tests/Issues/ImportFullyQualifiedIdentifierDocblock/Source/AddFullyQualifiedIdentifierDocblockRector.php b/tests/Issues/ImportFullyQualifiedIdentifierDocblock/Source/AddFullyQualifiedIdentifierDocblockRector.php index 5657dd4d7c8..260fe9f4fc4 100644 --- a/tests/Issues/ImportFullyQualifiedIdentifierDocblock/Source/AddFullyQualifiedIdentifierDocblockRector.php +++ b/tests/Issues/ImportFullyQualifiedIdentifierDocblock/Source/AddFullyQualifiedIdentifierDocblockRector.php @@ -36,8 +36,15 @@ public function getNodeTypes(): array public function refactor(Node $node) { $phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($node); + + // make rule idempotent, do not add if already present + if ($phpDocInfo->getVarTagValueNode() instanceof VarTagValueNode) { + return null; + } + $varTagValueNode = new VarTagValueNode(new FullyQualifiedIdentifierTypeNode('DateTime'), '', ''); + $phpDocInfo->addTagValueNode($varTagValueNode); $this->docBlockUpdater->updateRefactoredNodeWithPhpDocInfo($node); diff --git a/tests/Issues/ImportFullyQualifiedIdentifierDocblock/config/configured_rule.php b/tests/Issues/ImportFullyQualifiedIdentifierDocblock/config/configured_rule.php index f1825a9c5a5..00905124b4c 100644 --- a/tests/Issues/ImportFullyQualifiedIdentifierDocblock/config/configured_rule.php +++ b/tests/Issues/ImportFullyQualifiedIdentifierDocblock/config/configured_rule.php @@ -7,7 +7,10 @@ use Rector\Tests\Issues\ImportFullyQualifiedIdentifierDocblock\Source\AddFullyQualifiedIdentifierDocblockRector; return RectorConfig::configure() - ->withRules([AddFullyQualifiedIdentifierDocblockRector::class, RemoveUnusedConstructorParamRector::class]) + ->withRules([ + AddFullyQualifiedIdentifierDocblockRector::class, + RemoveUnusedConstructorParamRector::class, + ]) ->withImportNames() ->withSkip([ RemoveUnusedConstructorParamRector::class => [ 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); From 13a1fdbb6ed54a2cd983308c57ac617d1e70fd51 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sat, 17 Oct 2026 17:11:21 +0200 Subject: [PATCH 2/2] make NullToStrictString and ReplaceArgumentDefaultValue idempotent Skip args already cast to the target type and constants already at the target, so repeated passes neither nest casts nor rewrite stable code. Hoist the cheap limit-arg guard before scope fetch in the preg_split rule. --- .../ArgumentDefaultValueReplacer.php | 3 ++- .../NullToStrictStringIntConverter.php | 25 +++++++++++++++++++ ...trictIntPregSlitFuncCallLimitArgRector.php | 10 ++++---- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/rules/Arguments/ArgumentDefaultValueReplacer.php b/rules/Arguments/ArgumentDefaultValueReplacer.php index 3e92627089a..fc95bdd9244 100644 --- a/rules/Arguments/ArgumentDefaultValueReplacer.php +++ b/rules/Arguments/ArgumentDefaultValueReplacer.php @@ -170,8 +170,9 @@ private function processArgs( && str_contains($replaceArgumentDefaultValue->getValueAfter(), '::')) { [$targetClass, $targetConstant] = explode('::', $replaceArgumentDefaultValue->getValueAfter()); $type = $this->nodeTypeResolver->getType($particularArg->value->class); + // already the target constant, possibly via self::/subclass that inherits it → no-op if ($type instanceof FullyQualifiedObjectType - && $type->getClassName() === $targetClass + && ($type->getClassName() === $targetClass || $type->isInstanceOf($targetClass)->yes()) && $particularArg->value->name instanceof Identifier && $particularArg->value->name->toString() === $targetConstant) { return null; diff --git a/rules/Php81/NodeManipulator/NullToStrictStringIntConverter.php b/rules/Php81/NodeManipulator/NullToStrictStringIntConverter.php index 6a2c3454d66..862c5a48868 100644 --- a/rules/Php81/NodeManipulator/NullToStrictStringIntConverter.php +++ b/rules/Php81/NodeManipulator/NullToStrictStringIntConverter.php @@ -98,6 +98,11 @@ public function convertIfNull( return $funcCall; } + // already cast to the target type on a previous run, re-casting would nest a redundant cast + if ($this->isAlreadyCastToTargetType($argValue, $targetType)) { + return null; + } + $wrapInParentheses = false; if ($argValue instanceof Ternary && $argValue->cond instanceof CastString_) { $wrapInParentheses = true; @@ -118,6 +123,26 @@ public function convertIfNull( return $funcCall; } + /** + * Structural check (immune to stale resolved types on re-printed nodes): the value is already a + * cast/literal of the target type, or a ternary whose both branches are. + */ + private function isAlreadyCastToTargetType(Expr $expr, string $targetType): bool + { + if ($expr instanceof Ternary) { + // short ternary "A ?: B" keeps the left value in cond; full ternary uses if + $left = $expr->if ?? $expr->cond; + return $this->isAlreadyCastToTargetType($left, $targetType) + && $this->isAlreadyCastToTargetType($expr->else, $targetType); + } + + if ($targetType === 'string') { + return $expr instanceof CastString_ || $expr instanceof String_ || $expr instanceof InterpolatedString; + } + + return $expr instanceof CastInt_ || $expr instanceof Int_; + } + private function shouldSkipValue(Expr $expr, Scope $scope, bool $isTrait, string $targetType): bool { // array dim fetch value is mixed, null is not known - skip to avoid wrong (string) cast diff --git a/rules/Php81/Rector/FuncCall/NullToStrictIntPregSlitFuncCallLimitArgRector.php b/rules/Php81/Rector/FuncCall/NullToStrictIntPregSlitFuncCallLimitArgRector.php index 62a7279c887..5a7acb3f72e 100644 --- a/rules/Php81/Rector/FuncCall/NullToStrictIntPregSlitFuncCallLimitArgRector.php +++ b/rules/Php81/Rector/FuncCall/NullToStrictIntPregSlitFuncCallLimitArgRector.php @@ -80,11 +80,6 @@ public function refactor(Node $node): ?Node return null; } - $scope = $node->getAttribute(AttributeKey::SCOPE); - if (! $scope instanceof Scope) { - return null; - } - $args = $node->getArgs(); $position = $this->argsAnalyzer->resolveArgPosition($args, 'limit', 2); @@ -92,6 +87,11 @@ public function refactor(Node $node): ?Node return null; } + $scope = $node->getAttribute(AttributeKey::SCOPE); + if (! $scope instanceof Scope) { + return null; + } + $classReflection = $scope->getClassReflection(); $isTrait = $classReflection instanceof ClassReflection && $classReflection->isTrait();