From b70eb135c8d4a021acb021e749cdae136be4e79c Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 25 Oct 2026 12:11:58 +0100 Subject: [PATCH 1/5] [perf][expr] drop do while from FileProcessor --- src/Application/FileProcessor.php | 99 +++++++++++++++------------- src/ValueObject/Application/File.php | 15 +++-- 2 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/Application/FileProcessor.php b/src/Application/FileProcessor.php index 2d65ae111ad..4a609e45c9a 100644 --- a/src/Application/FileProcessor.php +++ b/src/Application/FileProcessor.php @@ -56,65 +56,74 @@ public function processFile(File $file, Configuration $configuration): FileProce return new FileProcessResult([$parsingSystemError], null, false); } - $fileHasChanged = false; + // $fileHasChanged = false; $filePath = $file->getFilePath(); - do { - $file->changeHasChanged(false); + // do { + // $file->changeHasChanged(false); - // every rule and post rector records a line change when it mutates the AST - $changeCountBeforePass = count($file->getRectorWithLineChanges()); + // every rule and post rector records a line change when it mutates the AST + // $changeCountBeforePass = count($file->getRectorWithLineChanges()); - // 1. change nodes with Rector Rules - $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file); + // 1. change nodes with Rector Rules + $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file); - // 2. apply post rectors - $postNewStmts = $this->postFileProcessor->traverse($newStmts, $file); + // 2. apply post rectors + $postNewStmts = $this->postFileProcessor->traverse($newStmts, $file); - // 3. this is needed for new tokens added in "afterTraverse()" - $file->changeNewStmts($postNewStmts); + // 3. this is needed for new tokens added in "afterTraverse()" + $file->changeNewStmts($postNewStmts); - // file never changed and this pass changed nothing: the print would just reproduce the - // original content, so skip the expensive format-preserving print entirely. - // once the file has changed we keep printing every pass, as a reprint can still - // stabilize the output (e.g. drop a leftover trailing comma) - if (! $fileHasChanged && count($file->getRectorWithLineChanges()) === $changeCountBeforePass) { - break; - } - - // 4. print to file or string - // important to detect if file has changed - $this->printFile($file, $configuration, $filePath); + // file never changed and this pass changed nothing: the print would just reproduce the + // original content, so skip the expensive format-preserving print entirely. + // once the file has changed we keep printing every pass, as a reprint can still + // stabilize the output (e.g. drop a leftover trailing comma) + // if (! $fileHasChanged && count($file->getRectorWithLineChanges()) === $changeCountBeforePass) { + // break; + // } - // no change in current iteration, stop - if (! $file->hasChanged()) { - break; - } + // if (! $file->hasChanged()) { + // continue; - $fileHasChanged = true; - } while (true); + // 4. print to file or string + // important to detect if file has changed + // $this->printFile($file, $configuration, $filePath); - // 5. add as cacheable if not changed at all - if (! $fileHasChanged) { + // no change in current iteration, stop + if (! $file->hasChanged()) { $this->changedFilesDetector->addCacheableFile($filePath); + return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged()); + + // break; + } else { - // when changed, set final status changed to true - // to ensure it make sense to verify in next process when needed - $file->changeHasChanged(true); + $this->printFile($file, $configuration, $filePath); } - $rectorWithLineChanges = $file->getRectorWithLineChanges(); - if ($file->hasChanged() || $rectorWithLineChanges !== []) { - $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges( - $configuration->shouldShowDiffs(), - $file, - $file->getOriginalFileContent(), - $file->getFileContent(), - $file->getRectorWithLineChanges() - ); - - $file->setFileDiff($currentFileDiff); - } + // $fileHasChanged = true; + // } while (true); + + // 5. add as cacheable if not changed at all + // if (! $fileHasChanged) { + // $this->changedFilesDetector->addCacheableFile($filePath); + // } else { + // // when changed, set final status changed to true + // // to ensure it make sense to verify in next process when needed + // $file->changeHasChanged(true); + // } + + $file->getRectorWithLineChanges(); + // if ($file->hasChanged() || $rectorWithLineChanges !== []) { + $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges( + $configuration->shouldShowDiffs(), + $file, + $file->getOriginalFileContent(), + $file->getFileContent(), + $file->getRectorWithLineChanges() + ); + + $file->setFileDiff($currentFileDiff); + // } return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged()); } diff --git a/src/ValueObject/Application/File.php b/src/ValueObject/Application/File.php index 5190e72963b..be9ca878fcc 100644 --- a/src/ValueObject/Application/File.php +++ b/src/ValueObject/Application/File.php @@ -17,7 +17,7 @@ final class File { - private bool $hasChanged = false; + // private bool $hasChanged = false; private readonly string $originalFileContent; @@ -74,7 +74,7 @@ public function changeFileContent(string $newFileContent): void } $this->fileContent = $newFileContent; - $this->hasChanged = true; + // $this->hasChanged = true; } public function getOriginalFileContent(): string @@ -84,13 +84,14 @@ public function getOriginalFileContent(): string public function hasChanged(): bool { - return $this->hasChanged; + return $this->rectorWithLineChanges !== []; + // return $this->hasChanged; } - public function changeHasChanged(bool $status): void - { - $this->hasChanged = $status; - } + // public function changeHasChanged(bool $status): void + // { + // $this->hasChanged = $status; + // } public function setFileDiff(FileDiff $fileDiff): void { From 701bebe1a28d157e2cd359248f481ba123aa64b9 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Mon, 26 Oct 2026 08:14:45 +0700 Subject: [PATCH 2/5] [Tests] Add test for rule chain that needs 3 FileProcessor passes --- src/Application/FileProcessor.php | 2 +- .../Fixture/fixture.php.inc | 35 ++++++++ .../RepeatedThreeTimesTest.php | 34 ++++++++ .../NarrowArrayCollectionDocblockRector.php | 78 +++++++++++++++++ .../Source/NarrowParamUnionDocblockRector.php | 85 +++++++++++++++++++ .../Source/ParamTypeFromDocblockRector.php | 69 +++++++++++++++ .../Source/SomeArrayCollection.php | 9 ++ .../Source/SomeCollection.php | 9 ++ .../config/configured_rule.php | 20 +++++ 9 files changed, 340 insertions(+), 1 deletion(-) create mode 100644 tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc create mode 100644 tests/Issues/RepeatedThreeTimes/RepeatedThreeTimesTest.php create mode 100644 tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php create mode 100644 tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php create mode 100644 tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php create mode 100644 tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php create mode 100644 tests/Issues/RepeatedThreeTimes/Source/SomeCollection.php create mode 100644 tests/Issues/RepeatedThreeTimes/config/configured_rule.php diff --git a/src/Application/FileProcessor.php b/src/Application/FileProcessor.php index 4a609e45c9a..ecef9f370f3 100644 --- a/src/Application/FileProcessor.php +++ b/src/Application/FileProcessor.php @@ -112,7 +112,7 @@ public function processFile(File $file, Configuration $configuration): FileProce // $file->changeHasChanged(true); // } - $file->getRectorWithLineChanges(); + // $file->getRectorWithLineChanges(); // if ($file->hasChanged() || $rectorWithLineChanges !== []) { $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges( $configuration->shouldShowDiffs(), diff --git a/tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc b/tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc new file mode 100644 index 00000000000..fd64bc884cc --- /dev/null +++ b/tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc @@ -0,0 +1,35 @@ + +----- + diff --git a/tests/Issues/RepeatedThreeTimes/RepeatedThreeTimesTest.php b/tests/Issues/RepeatedThreeTimes/RepeatedThreeTimesTest.php new file mode 100644 index 00000000000..d2e454bc70a --- /dev/null +++ b/tests/Issues/RepeatedThreeTimes/RepeatedThreeTimesTest.php @@ -0,0 +1,34 @@ +doTestFile($filePath); + } + + public static function provideData(): Iterator + { + return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php b/tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php new file mode 100644 index 00000000000..5e71571fa68 --- /dev/null +++ b/tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php @@ -0,0 +1,78 @@ +> + */ + public function getNodeTypes(): array + { + return [ClassMethod::class]; + } + + /** + * @param ClassMethod $node + */ + public function refactor(Node $node): ?ClassMethod + { + $phpDocInfo = $this->phpDocInfoFactory->createFromNode($node); + if ($phpDocInfo === null) { + return null; + } + + $hasChanged = false; + foreach ($phpDocInfo->getParamTagValueNodes() as $paramTagValueNode) { + if (! $paramTagValueNode->type instanceof UnionTypeNode) { + continue; + } + + foreach ($paramTagValueNode->type->types as $key => $unionedTypeNode) { + $unionedType = $this->staticTypeMapper->mapPHPStanPhpDocTypeNodeToPHPStanType($unionedTypeNode, $node); + if (! $unionedType->equals(new ObjectType(SomeArrayCollection::class))) { + continue; + } + + $paramTagValueNode->type->types[$key] = new FullyQualifiedIdentifierTypeNode(SomeCollection::class); + $hasChanged = true; + } + } + + if (! $hasChanged) { + return null; + } + + $this->docBlockUpdater->updateRefactoredNodeWithPhpDocInfo($node); + + return $node; + } +} diff --git a/tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php b/tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php new file mode 100644 index 00000000000..e1eb2d6874e --- /dev/null +++ b/tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php @@ -0,0 +1,85 @@ +> + */ + public function getNodeTypes(): array + { + return [ClassMethod::class]; + } + + /** + * @param ClassMethod $node + */ + public function refactor(Node $node): ?ClassMethod + { + $phpDocInfo = $this->phpDocInfoFactory->createFromNode($node); + if ($phpDocInfo === null) { + return null; + } + + $hasChanged = false; + foreach ($phpDocInfo->getParamTagValueNodes() as $paramTagValueNode) { + if (! $paramTagValueNode->type instanceof UnionTypeNode) { + continue; + } + + if (count($paramTagValueNode->type->types) !== 2) { + continue; + } + + [$firstTypeNode, $secondTypeNode] = $paramTagValueNode->type->types; + if (! $firstTypeNode instanceof IdentifierTypeNode || $firstTypeNode->name !== 'array') { + continue; + } + + $secondType = $this->staticTypeMapper->mapPHPStanPhpDocTypeNodeToPHPStanType($secondTypeNode, $node); + if (! $secondType->equals(new ObjectType(SomeCollection::class))) { + continue; + } + + $paramTagValueNode->type = $secondTypeNode; + $hasChanged = true; + } + + if (! $hasChanged) { + return null; + } + + $this->docBlockUpdater->updateRefactoredNodeWithPhpDocInfo($node); + + return $node; + } +} diff --git a/tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php b/tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php new file mode 100644 index 00000000000..9fe75a05539 --- /dev/null +++ b/tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php @@ -0,0 +1,69 @@ +> + */ + public function getNodeTypes(): array + { + return [ClassMethod::class]; + } + + /** + * @param ClassMethod $node + */ + public function refactor(Node $node): ?ClassMethod + { + $phpDocInfo = $this->phpDocInfoFactory->createFromNode($node); + if ($phpDocInfo === null) { + return null; + } + + $hasChanged = false; + foreach ($node->params as $param) { + if ($param->type instanceof Node) { + continue; + } + + $paramType = $phpDocInfo->getParamType($this->getName($param)); + if (! $paramType->equals(new ObjectType(SomeCollection::class))) { + continue; + } + + $param->type = new FullyQualified(SomeCollection::class); + $hasChanged = true; + } + + if (! $hasChanged) { + return null; + } + + return $node; + } +} diff --git a/tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php b/tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php new file mode 100644 index 00000000000..566eadf74dd --- /dev/null +++ b/tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php @@ -0,0 +1,9 @@ +withImportNames() + // registered in reverse order of dependency: + // 3rd pass: native param type from @param SomeCollection + // 2nd pass: @param array|SomeCollection to @param SomeCollection + // 1st pass: @param array|SomeArrayCollection to @param array|SomeCollection + ->withRules([ + ParamTypeFromDocblockRector::class, + NarrowParamUnionDocblockRector::class, + NarrowArrayCollectionDocblockRector::class, + ]); From 58f813be0fafb22f4bc40b776f6a29046c952084 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 26 Oct 2026 13:46:50 +0100 Subject: [PATCH 3/5] wip --- .../Rector/If_/RemoveDeadInstanceOfRector.php | 9 +- .../NodeTraverser/RectorNodeTraverser.php | 176 +++++++++++++----- 2 files changed, 134 insertions(+), 51 deletions(-) diff --git a/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php b/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php index 35916f6bb3b..bdba710444e 100644 --- a/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php +++ b/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php @@ -21,6 +21,8 @@ use PHPStan\Reflection\ClassReflection; use PHPStan\Type\MixedType; use PHPStan\Type\ObjectType; +use Rector\CodeQuality\Rector\If_\ExplicitNullableObjectInstanceRector; +use Rector\Contract\Rector\OrderedRectorInterface; use Rector\NodeManipulator\IfManipulator; use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\Rector\AbstractRector; @@ -31,7 +33,7 @@ /** * @see \Rector\Tests\DeadCode\Rector\If_\RemoveDeadInstanceOfRector\RemoveDeadInstanceOfRectorTest */ -final class RemoveDeadInstanceOfRector extends AbstractRector +final class RemoveDeadInstanceOfRector extends AbstractRector implements OrderedRectorInterface { public function __construct( private readonly IfManipulator $ifManipulator, @@ -72,6 +74,11 @@ public function getNodeTypes(): array return [If_::class]; } + public function precededRule(): string + { + return ExplicitNullableObjectInstanceRector::class; + } + /** * @param If_ $node * @return Stmt[]|null|NodeVisitor::REMOVE_NODE|If_ diff --git a/src/PhpParser/NodeTraverser/RectorNodeTraverser.php b/src/PhpParser/NodeTraverser/RectorNodeTraverser.php index f45faa2dcdf..70cc7868372 100644 --- a/src/PhpParser/NodeTraverser/RectorNodeTraverser.php +++ b/src/PhpParser/NodeTraverser/RectorNodeTraverser.php @@ -10,16 +10,24 @@ use PhpParser\Node\Stmt; use PhpParser\NodeVisitor; use Rector\Application\RectorRegistry; +use Rector\Exception\ShouldNotHappenException; +use Rector\PhpParser\Printer\BetterStandardPrinter; use Rector\Rector\RectorRunner; use Rector\ValueObject\Application\File; final class RectorNodeTraverser { + /** + * Safety cap on local re-visits of a single node, so a rule that never settles cannot loop forever. + */ + private const int MAX_REVISITS = 50; + private bool $stopTraversal; public function __construct( private readonly RectorRunner $rectorRunner, private readonly RectorRegistry $rectorRegistry, + private readonly BetterStandardPrinter $betterStandardPrinter, ) { } @@ -51,31 +59,8 @@ private function traverseNode(Node $node, File $file): void continue; } - $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($subNode, $file); - - foreach ($currentNodeVisitors as $currentNodeVisitor) { - $return = $this->rectorRunner->run($currentNodeVisitor, $subNode); - if ($return === null) { - continue; - } - - if ($return instanceof Node) { - $originalSubNodeClass = $subNode::class; - - $this->ensureReplacementReasonable($subNode, $return); - $subNode = $return; - $node->{$name} = $return; - - if ($originalSubNodeClass !== $subNode::class) { - // stop traversing as node type changed and visitors won't work - continue 2; - } - } else { - throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return)); - } - } - - $this->traverseNode($subNode, $file); + // a single sub-node slot cannot hold a list or be removed + $node->{$name} = $this->refactorNode($subNode, $file, false); if ($this->stopTraversal) { break; @@ -99,33 +84,16 @@ private function traverseArray(array $nodes, File $file): array continue; } - $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($node, $file); - - foreach ($currentNodeVisitors as $currentNodeVisitor) { - $return = $this->rectorRunner->run($currentNodeVisitor, $node); - if ($return !== null) { - if ($return instanceof Node) { - $originalNodeNodeClass = $node::class; - $this->ensureReplacementReasonable($node, $return); - $nodes[$i] = $node = $return; - - if ($originalNodeNodeClass !== $return::class) { - // stop traversing as node type changed and visitors won't work - continue 2; - } - } elseif (\is_array($return)) { - $doNodes[] = [$i, $return]; - continue 2; - } elseif ($return === NodeVisitor::REMOVE_NODE) { - $doNodes[] = [$i, []]; - continue 2; - } else { - throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return)); - } - } + $result = $this->refactorNode($node, $file, true); + + if (\is_array($result)) { + $doNodes[] = [$i, $result]; + } elseif ($result === NodeVisitor::REMOVE_NODE) { + $doNodes[] = [$i, []]; + } else { + $nodes[$i] = $result; } - $this->traverseNode($node, $file); if ($this->stopTraversal) { break; } @@ -140,6 +108,114 @@ private function traverseArray(array $nodes, File $file): array return $nodes; } + /** + * Applies the node's rules and traverses into its children, then re-applies its rules while a + * descendant keeps changing - so a rule that depends on a change made deeper in the tree (ordered + * via OrderedRectorInterface, or a plain multi-pass chain) converges within a single file pass. + * + * Termination is structural: the loop stops once a round reports no change, or reports a change + * that leaves the node's printed form identical (a rule that keeps returning the same result). + * + * @param bool $allowListReturn whether an array / REMOVE_NODE return is valid here (only for a + * node that lives inside a list, not a single sub-node slot) + * @return NodeVisitor::REMOVE_NODE|Node|Node[] + */ + private function refactorNode(Node $node, File $file, bool $allowListReturn): int|Node|array + { + $lastPrint = null; + $revisits = 0; + + while (true) { + $changeCountBefore = $this->changeCount($file); + $return = $this->applyRectors($node, $file); + + if (\is_array($return) || $return === NodeVisitor::REMOVE_NODE) { + if (! $allowListReturn) { + throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return)); + } + + // a list / removal replaces the node in its parent list; its children are not traversed + return $return; + } + + $typeChanged = $return::class !== $node::class; + $node = $return; + + if ($typeChanged) { + // stop traversing as node type changed and visitors won't work + return $node; + } + + $this->traverseNode($node, $file); + if ($this->stopTraversal) { + return $node; + } + + // no rule (here or deeper) reported a change this round: the subtree is stable + if ($this->changeCount($file) === $changeCountBefore) { + return $node; + } + + // a change was reported; keep going only while it actually reshapes the node + $printNow = $this->betterStandardPrinter->print($node); + if ($printNow === $lastPrint) { + return $node; + } + + $lastPrint = $printNow; + + if (++$revisits >= self::MAX_REVISITS) { + throw new ShouldNotHappenException( + sprintf('Node "%s" did not converge after %d re-visits', $node->getType(), $revisits) + ); + } + } + } + + /** + * Runs every rule subscribed to this node, in order. A rule returning a same-type node keeps the + * remaining rules running on the new node; a type change, an array or REMOVE_NODE stops the chain + * and is handed back to the caller. + * + * @return NodeVisitor::REMOVE_NODE|Node|Node[] + */ + private function applyRectors(Node $node, File $file): int|Node|array + { + $currentNodeVisitors = $this->rectorRegistry->forNodeAndFile($node, $file); + + foreach ($currentNodeVisitors as $currentNodeVisitor) { + $return = $this->rectorRunner->run($currentNodeVisitor, $node); + if ($return === null) { + continue; + } + + if ($return instanceof Node) { + $originalNodeClass = $node::class; + $this->ensureReplacementReasonable($node, $return); + $node = $return; + + if ($originalNodeClass !== $node::class) { + return $node; + } + + continue; + } + + if (\is_array($return) || $return === NodeVisitor::REMOVE_NODE) { + return $return; + } + + throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return)); + } + + return $node; + } + + private function changeCount(File $file): int + { + return count($file->getRectorWithLineChanges()); + } + private function ensureReplacementReasonable(Node $old, Node $new): void { if ($old instanceof Stmt) { From 42e429cffb34abdc77a69196c47c0b489c1c22e7 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 26 Oct 2026 17:30:11 +0100 Subject: [PATCH 4/5] [Tests] replace bespoke RepeatedThreeTimes rules with real-rule RepeatedTwice chain Drop the three hand-written Source rules that duplicated rule logic in tests; use real AddParamTypeFromPropertyTypeRector + ReturnTypeFromStrictParamRector instead. --- .../Fixture/fixture.php.inc | 35 -------- .../NarrowArrayCollectionDocblockRector.php | 78 ----------------- .../Source/NarrowParamUnionDocblockRector.php | 85 ------------------- .../Source/ParamTypeFromDocblockRector.php | 69 --------------- .../Source/SomeArrayCollection.php | 9 -- .../Source/SomeCollection.php | 9 -- .../config/configured_rule.php | 20 ----- .../RepeatedTwice/Fixture/fixture.php.inc | 35 ++++++++ .../RepeatedTwiceTest.php} | 8 +- .../RepeatedTwice/config/configured_rule.php | 16 ++++ 10 files changed, 55 insertions(+), 309 deletions(-) delete mode 100644 tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc delete mode 100644 tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php delete mode 100644 tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php delete mode 100644 tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php delete mode 100644 tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php delete mode 100644 tests/Issues/RepeatedThreeTimes/Source/SomeCollection.php delete mode 100644 tests/Issues/RepeatedThreeTimes/config/configured_rule.php create mode 100644 tests/Issues/RepeatedTwice/Fixture/fixture.php.inc rename tests/Issues/{RepeatedThreeTimes/RepeatedThreeTimesTest.php => RepeatedTwice/RepeatedTwiceTest.php} (70%) create mode 100644 tests/Issues/RepeatedTwice/config/configured_rule.php diff --git a/tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc b/tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc deleted file mode 100644 index fd64bc884cc..00000000000 --- a/tests/Issues/RepeatedThreeTimes/Fixture/fixture.php.inc +++ /dev/null @@ -1,35 +0,0 @@ - ------ - diff --git a/tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php b/tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php deleted file mode 100644 index 5e71571fa68..00000000000 --- a/tests/Issues/RepeatedThreeTimes/Source/NarrowArrayCollectionDocblockRector.php +++ /dev/null @@ -1,78 +0,0 @@ -> - */ - public function getNodeTypes(): array - { - return [ClassMethod::class]; - } - - /** - * @param ClassMethod $node - */ - public function refactor(Node $node): ?ClassMethod - { - $phpDocInfo = $this->phpDocInfoFactory->createFromNode($node); - if ($phpDocInfo === null) { - return null; - } - - $hasChanged = false; - foreach ($phpDocInfo->getParamTagValueNodes() as $paramTagValueNode) { - if (! $paramTagValueNode->type instanceof UnionTypeNode) { - continue; - } - - foreach ($paramTagValueNode->type->types as $key => $unionedTypeNode) { - $unionedType = $this->staticTypeMapper->mapPHPStanPhpDocTypeNodeToPHPStanType($unionedTypeNode, $node); - if (! $unionedType->equals(new ObjectType(SomeArrayCollection::class))) { - continue; - } - - $paramTagValueNode->type->types[$key] = new FullyQualifiedIdentifierTypeNode(SomeCollection::class); - $hasChanged = true; - } - } - - if (! $hasChanged) { - return null; - } - - $this->docBlockUpdater->updateRefactoredNodeWithPhpDocInfo($node); - - return $node; - } -} diff --git a/tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php b/tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php deleted file mode 100644 index e1eb2d6874e..00000000000 --- a/tests/Issues/RepeatedThreeTimes/Source/NarrowParamUnionDocblockRector.php +++ /dev/null @@ -1,85 +0,0 @@ -> - */ - public function getNodeTypes(): array - { - return [ClassMethod::class]; - } - - /** - * @param ClassMethod $node - */ - public function refactor(Node $node): ?ClassMethod - { - $phpDocInfo = $this->phpDocInfoFactory->createFromNode($node); - if ($phpDocInfo === null) { - return null; - } - - $hasChanged = false; - foreach ($phpDocInfo->getParamTagValueNodes() as $paramTagValueNode) { - if (! $paramTagValueNode->type instanceof UnionTypeNode) { - continue; - } - - if (count($paramTagValueNode->type->types) !== 2) { - continue; - } - - [$firstTypeNode, $secondTypeNode] = $paramTagValueNode->type->types; - if (! $firstTypeNode instanceof IdentifierTypeNode || $firstTypeNode->name !== 'array') { - continue; - } - - $secondType = $this->staticTypeMapper->mapPHPStanPhpDocTypeNodeToPHPStanType($secondTypeNode, $node); - if (! $secondType->equals(new ObjectType(SomeCollection::class))) { - continue; - } - - $paramTagValueNode->type = $secondTypeNode; - $hasChanged = true; - } - - if (! $hasChanged) { - return null; - } - - $this->docBlockUpdater->updateRefactoredNodeWithPhpDocInfo($node); - - return $node; - } -} diff --git a/tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php b/tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php deleted file mode 100644 index 9fe75a05539..00000000000 --- a/tests/Issues/RepeatedThreeTimes/Source/ParamTypeFromDocblockRector.php +++ /dev/null @@ -1,69 +0,0 @@ -> - */ - public function getNodeTypes(): array - { - return [ClassMethod::class]; - } - - /** - * @param ClassMethod $node - */ - public function refactor(Node $node): ?ClassMethod - { - $phpDocInfo = $this->phpDocInfoFactory->createFromNode($node); - if ($phpDocInfo === null) { - return null; - } - - $hasChanged = false; - foreach ($node->params as $param) { - if ($param->type instanceof Node) { - continue; - } - - $paramType = $phpDocInfo->getParamType($this->getName($param)); - if (! $paramType->equals(new ObjectType(SomeCollection::class))) { - continue; - } - - $param->type = new FullyQualified(SomeCollection::class); - $hasChanged = true; - } - - if (! $hasChanged) { - return null; - } - - return $node; - } -} diff --git a/tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php b/tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php deleted file mode 100644 index 566eadf74dd..00000000000 --- a/tests/Issues/RepeatedThreeTimes/Source/SomeArrayCollection.php +++ /dev/null @@ -1,9 +0,0 @@ -withImportNames() - // registered in reverse order of dependency: - // 3rd pass: native param type from @param SomeCollection - // 2nd pass: @param array|SomeCollection to @param SomeCollection - // 1st pass: @param array|SomeArrayCollection to @param array|SomeCollection - ->withRules([ - ParamTypeFromDocblockRector::class, - NarrowParamUnionDocblockRector::class, - NarrowArrayCollectionDocblockRector::class, - ]); diff --git a/tests/Issues/RepeatedTwice/Fixture/fixture.php.inc b/tests/Issues/RepeatedTwice/Fixture/fixture.php.inc new file mode 100644 index 00000000000..bd2a32a49fa --- /dev/null +++ b/tests/Issues/RepeatedTwice/Fixture/fixture.php.inc @@ -0,0 +1,35 @@ +name = $name; + + return $name; + } +} + +?> +----- +name = $name; + + return $name; + } +} + +?> diff --git a/tests/Issues/RepeatedThreeTimes/RepeatedThreeTimesTest.php b/tests/Issues/RepeatedTwice/RepeatedTwiceTest.php similarity index 70% rename from tests/Issues/RepeatedThreeTimes/RepeatedThreeTimesTest.php rename to tests/Issues/RepeatedTwice/RepeatedTwiceTest.php index d2e454bc70a..e88f8fa03a5 100644 --- a/tests/Issues/RepeatedThreeTimes/RepeatedThreeTimesTest.php +++ b/tests/Issues/RepeatedTwice/RepeatedTwiceTest.php @@ -2,19 +2,19 @@ declare(strict_types=1); -namespace Rector\Tests\Issues\RepeatedThreeTimes; +namespace Rector\Tests\Issues\RepeatedTwice; use Iterator; use PHPUnit\Framework\Attributes\DataProvider; use Rector\Testing\PHPUnit\AbstractRectorTestCase; /** - * Each rule depends on the output of the rule registered after it, - * so the file only reaches its final state on the 3rd pass + * The return-type rule depends on the output of the param-type rule registered after it, + * so the file only reaches its final state on the 2nd pass * * @see https://github.com/rectorphp/rector-src/pull/8826 */ -final class RepeatedThreeTimesTest extends AbstractRectorTestCase +final class RepeatedTwiceTest extends AbstractRectorTestCase { #[DataProvider('provideData')] public function test(string $filePath): void diff --git a/tests/Issues/RepeatedTwice/config/configured_rule.php b/tests/Issues/RepeatedTwice/config/configured_rule.php new file mode 100644 index 00000000000..e2bb026c1bc --- /dev/null +++ b/tests/Issues/RepeatedTwice/config/configured_rule.php @@ -0,0 +1,16 @@ +withRules([ + ReturnTypeFromStrictParamRector::class, + AddParamTypeFromPropertyTypeRector::class, + ]); From 009889b11821be3694833e2cc8f1233ac4579ae8 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 26 Oct 2026 17:42:51 +0100 Subject: [PATCH 5/5] [perf][expr] remove leftover commented do-while block --- src/Application/FileProcessor.php | 45 +---------- .../NodeTraverser/RectorNodeTraverser.php | 75 ++++--------------- 2 files changed, 17 insertions(+), 103 deletions(-) diff --git a/src/Application/FileProcessor.php b/src/Application/FileProcessor.php index ecef9f370f3..e0df8c26903 100644 --- a/src/Application/FileProcessor.php +++ b/src/Application/FileProcessor.php @@ -56,15 +56,8 @@ public function processFile(File $file, Configuration $configuration): FileProce return new FileProcessResult([$parsingSystemError], null, false); } - // $fileHasChanged = false; $filePath = $file->getFilePath(); - // do { - // $file->changeHasChanged(false); - - // every rule and post rector records a line change when it mutates the AST - // $changeCountBeforePass = count($file->getRectorWithLineChanges()); - // 1. change nodes with Rector Rules $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file); @@ -74,46 +67,15 @@ public function processFile(File $file, Configuration $configuration): FileProce // 3. this is needed for new tokens added in "afterTraverse()" $file->changeNewStmts($postNewStmts); - // file never changed and this pass changed nothing: the print would just reproduce the - // original content, so skip the expensive format-preserving print entirely. - // once the file has changed we keep printing every pass, as a reprint can still - // stabilize the output (e.g. drop a leftover trailing comma) - // if (! $fileHasChanged && count($file->getRectorWithLineChanges()) === $changeCountBeforePass) { - // break; - // } - - // if (! $file->hasChanged()) { - // continue; - - // 4. print to file or string - // important to detect if file has changed - // $this->printFile($file, $configuration, $filePath); - - // no change in current iteration, stop + // no change, stop if (! $file->hasChanged()) { $this->changedFilesDetector->addCacheableFile($filePath); return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged()); - - // break; - - } else { - $this->printFile($file, $configuration, $filePath); } - // $fileHasChanged = true; - // } while (true); - - // 5. add as cacheable if not changed at all - // if (! $fileHasChanged) { - // $this->changedFilesDetector->addCacheableFile($filePath); - // } else { - // // when changed, set final status changed to true - // // to ensure it make sense to verify in next process when needed - // $file->changeHasChanged(true); - // } + // 4. print to file or string + $this->printFile($file, $configuration, $filePath); - // $file->getRectorWithLineChanges(); - // if ($file->hasChanged() || $rectorWithLineChanges !== []) { $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges( $configuration->shouldShowDiffs(), $file, @@ -123,7 +85,6 @@ public function processFile(File $file, Configuration $configuration): FileProce ); $file->setFileDiff($currentFileDiff); - // } return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged()); } diff --git a/src/PhpParser/NodeTraverser/RectorNodeTraverser.php b/src/PhpParser/NodeTraverser/RectorNodeTraverser.php index 70cc7868372..06a9539d574 100644 --- a/src/PhpParser/NodeTraverser/RectorNodeTraverser.php +++ b/src/PhpParser/NodeTraverser/RectorNodeTraverser.php @@ -10,24 +10,16 @@ use PhpParser\Node\Stmt; use PhpParser\NodeVisitor; use Rector\Application\RectorRegistry; -use Rector\Exception\ShouldNotHappenException; -use Rector\PhpParser\Printer\BetterStandardPrinter; use Rector\Rector\RectorRunner; use Rector\ValueObject\Application\File; final class RectorNodeTraverser { - /** - * Safety cap on local re-visits of a single node, so a rule that never settles cannot loop forever. - */ - private const int MAX_REVISITS = 50; - private bool $stopTraversal; public function __construct( private readonly RectorRunner $rectorRunner, private readonly RectorRegistry $rectorRegistry, - private readonly BetterStandardPrinter $betterStandardPrinter, ) { } @@ -109,12 +101,7 @@ private function traverseArray(array $nodes, File $file): array } /** - * Applies the node's rules and traverses into its children, then re-applies its rules while a - * descendant keeps changing - so a rule that depends on a change made deeper in the tree (ordered - * via OrderedRectorInterface, or a plain multi-pass chain) converges within a single file pass. - * - * Termination is structural: the loop stops once a round reports no change, or reports a change - * that leaves the node's printed form identical (a rule that keeps returning the same result). + * Applies the node's rules, then traverses into its children. * * @param bool $allowListReturn whether an array / REMOVE_NODE return is valid here (only for a * node that lives inside a list, not a single sub-node slot) @@ -122,54 +109,25 @@ private function traverseArray(array $nodes, File $file): array */ private function refactorNode(Node $node, File $file, bool $allowListReturn): int|Node|array { - $lastPrint = null; - $revisits = 0; + $return = $this->applyRectors($node, $file); - while (true) { - $changeCountBefore = $this->changeCount($file); - $return = $this->applyRectors($node, $file); - - if (\is_array($return) || $return === NodeVisitor::REMOVE_NODE) { - if (! $allowListReturn) { - throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return)); - } - - // a list / removal replaces the node in its parent list; its children are not traversed - return $return; - } - - $typeChanged = $return::class !== $node::class; - $node = $return; - - if ($typeChanged) { - // stop traversing as node type changed and visitors won't work - return $node; + if (\is_array($return) || $return === NodeVisitor::REMOVE_NODE) { + if (! $allowListReturn) { + throw new LogicException('RectorRunner::run() returned invalid value of type ' . gettype($return)); } - $this->traverseNode($node, $file); - if ($this->stopTraversal) { - return $node; - } - - // no rule (here or deeper) reported a change this round: the subtree is stable - if ($this->changeCount($file) === $changeCountBefore) { - return $node; - } + // a list / removal replaces the node in its parent list; its children are not traversed + return $return; + } - // a change was reported; keep going only while it actually reshapes the node - $printNow = $this->betterStandardPrinter->print($node); - if ($printNow === $lastPrint) { - return $node; - } + if ($return::class !== $node::class) { + // stop traversing as node type changed and visitors won't work + return $return; + } - $lastPrint = $printNow; + $this->traverseNode($return, $file); - if (++$revisits >= self::MAX_REVISITS) { - throw new ShouldNotHappenException( - sprintf('Node "%s" did not converge after %d re-visits', $node->getType(), $revisits) - ); - } - } + return $return; } /** @@ -211,11 +169,6 @@ private function applyRectors(Node $node, File $file): int|Node|array return $node; } - private function changeCount(File $file): int - { - return count($file->getRectorWithLineChanges()); - } - private function ensureReplacementReasonable(Node $old, Node $new): void { if ($old instanceof Stmt) {