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/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(); diff --git a/src/Application/ChangedNodeScopeRefresher.php b/src/Application/ChangedNodeScopeRefresher.php index 2fff52d0abb..954edadc8e4 100644 --- a/src/Application/ChangedNodeScopeRefresher.php +++ b/src/Application/ChangedNodeScopeRefresher.php @@ -15,14 +15,19 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\Closure; +use PhpParser\Node\Expr\ConstFetch; use PhpParser\Node\Expr\New_; +use PhpParser\Node\Name; use PhpParser\Node\Param; use PhpParser\Node\PropertyItem; use PhpParser\Node\StaticVar; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\Declare_; +use PhpParser\Node\Stmt\Else_; +use PhpParser\Node\Stmt\ElseIf_; use PhpParser\Node\Stmt\Expression; +use PhpParser\Node\Stmt\If_; use PhpParser\Node\Stmt\Property; use PhpParser\Node\Stmt\Static_; use PhpParser\Node\Stmt\Use_; @@ -80,6 +85,22 @@ public function refresh(Node $node, string $filePath, ?MutatingScope $mutatingSc */ private function resolveStmts(Node $node): array { + // Else_/ElseIf_ are not processed standalone by PHPStan, only as part of an If_, so their body + // stays unscoped; wrap into an If_ so processNodes() scopes the real statements they hold + if ($node instanceof Else_) { + return [ + new If_(new ConstFetch(new Name('true')), [ + 'stmts' => $node->stmts, + ])]; + } + + if ($node instanceof ElseIf_) { + return [ + new If_($node->cond, [ + 'stmts' => $node->stmts, + ])]; + } + if ($node instanceof Stmt) { return [$node]; } 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/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php b/src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php index 780e7c2e0ee..ef8994833c7 100644 --- a/src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php +++ b/src/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php @@ -91,6 +91,7 @@ use PHPStan\Node\VirtualNode; use PHPStan\Parser\ParserErrorsException; use PHPStan\PhpDocParser\Parser\ParserException; +use PHPStan\Reflection\ClassReflection; use PHPStan\Reflection\ReflectionProvider; use PHPStan\ShouldNotHappenException; use PHPStan\Type\ObjectType; @@ -636,6 +637,13 @@ private function resolveClassOrInterfaceScope( } catch (ShouldNotHappenException) { } + // enterClass() throws when the scope is already inside a class, which happens when a changed + // class node is re-processed on a later pass; keep the in-class scope instead of dropping it + $currentClassReflection = $mutatingScope->getClassReflection(); + if ($currentClassReflection instanceof ClassReflection && $currentClassReflection->getName() === $classReflection->getName()) { + return $mutatingScope; + } + $context = $this->privatesAccessor->getPrivateProperty($mutatingScope, 'context'); $this->privatesAccessor->setPrivateProperty($context, 'classReflection', null); 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);