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/Application/FileProcessor.php b/src/Application/FileProcessor.php index 2d65ae111ad..e0df8c26903 100644 --- a/src/Application/FileProcessor.php +++ b/src/Application/FileProcessor.php @@ -56,65 +56,35 @@ public function processFile(File $file, Configuration $configuration): FileProce return new FileProcessResult([$parsingSystemError], null, false); } - $fileHasChanged = false; $filePath = $file->getFilePath(); - do { - $file->changeHasChanged(false); + // 1. change nodes with Rector Rules + $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file); - // every rule and post rector records a line change when it mutates the AST - $changeCountBeforePass = count($file->getRectorWithLineChanges()); + // 2. apply post rectors + $postNewStmts = $this->postFileProcessor->traverse($newStmts, $file); - // 1. change nodes with Rector Rules - $newStmts = $this->rectorNodeTraverser->traverse($file->getNewStmts(), $file); + // 3. this is needed for new tokens added in "afterTraverse()" + $file->changeNewStmts($postNewStmts); - // 2. apply post rectors - $postNewStmts = $this->postFileProcessor->traverse($newStmts, $file); - - // 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); - - // no change in current iteration, stop - if (! $file->hasChanged()) { - break; - } - - $fileHasChanged = true; - } while (true); - - // 5. add as cacheable if not changed at all - if (! $fileHasChanged) { + // no change, stop + if (! $file->hasChanged()) { $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); + return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged()); } - $rectorWithLineChanges = $file->getRectorWithLineChanges(); - if ($file->hasChanged() || $rectorWithLineChanges !== []) { - $currentFileDiff = $this->fileDiffFactory->createFileDiffWithLineChanges( - $configuration->shouldShowDiffs(), - $file, - $file->getOriginalFileContent(), - $file->getFileContent(), - $file->getRectorWithLineChanges() - ); - - $file->setFileDiff($currentFileDiff); - } + // 4. print to file or string + $this->printFile($file, $configuration, $filePath); + + $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/PhpParser/NodeTraverser/RectorNodeTraverser.php b/src/PhpParser/NodeTraverser/RectorNodeTraverser.php index f45faa2dcdf..06a9539d574 100644 --- a/src/PhpParser/NodeTraverser/RectorNodeTraverser.php +++ b/src/PhpParser/NodeTraverser/RectorNodeTraverser.php @@ -51,31 +51,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 +76,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 +100,75 @@ private function traverseArray(array $nodes, File $file): array return $nodes; } + /** + * 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) + * @return NodeVisitor::REMOVE_NODE|Node|Node[] + */ + private function refactorNode(Node $node, File $file, bool $allowListReturn): int|Node|array + { + $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; + } + + if ($return::class !== $node::class) { + // stop traversing as node type changed and visitors won't work + return $return; + } + + $this->traverseNode($return, $file); + + return $return; + } + + /** + * 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 ensureReplacementReasonable(Node $old, Node $new): void { if ($old instanceof Stmt) { 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 { 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/RepeatedTwice/RepeatedTwiceTest.php b/tests/Issues/RepeatedTwice/RepeatedTwiceTest.php new file mode 100644 index 00000000000..e88f8fa03a5 --- /dev/null +++ b/tests/Issues/RepeatedTwice/RepeatedTwiceTest.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/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, + ]);