From 94633747fe5d645dd1f2803ebff05ca9173eda3c Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 19 Oct 2026 19:20:04 +0200 Subject: [PATCH 1/2] Enforce single-pass rule application in tests, make 7 rules idempotent Add a test-only guard in FileProcessor that fails a rule which changes a file again after it already changed it in a previous pass. Such a rule is not finished in a single pass and relies on the do-while loop re-running it, which masks incomplete rules. The guard is gated by the RECTOR_ENFORCE_SINGLE_PASS env (set in phpunit.xml), so sibling packages pulling rector-src as dev-main are not affected. Post rectors are excluded, as import handling legitimately converges over passes. Make the 7 rules that depended on the re-run resolve their nested or chained case inside refactor(): - ReplaceConstantBooleanNotRector - fold the whole !! negation chain - RemoveDeadZeroAndOneOperationRector - reduce the returned operand chain - PowToExpRector - convert nested pow() calls - TernaryToNullCoalescingRector - convert a nested coalescable else - ClosureToArrowFunctionRector - convert a nested closure body - ReplaceBlockToItsStmtsRector - flatten nested blocks - RemoveAlwaysElseRector - split nested always-else in the returned stmts --- phpunit.xml | 2 + .../ReplaceConstantBooleanNotRector.php | 23 ++++++-- .../Block/ReplaceBlockToItsStmtsRector.php | 27 ++++++++- .../RemoveDeadZeroAndOneOperationRector.php | 19 ++++++- .../Rector/If_/RemoveAlwaysElseRector.php | 57 +++++++++++++++++++ .../Php56/Rector/FuncCall/PowToExpRector.php | 34 +++++++++-- .../Ternary/TernaryToNullCoalescingRector.php | 8 +++ src/Application/FileProcessor.php | 50 ++++++++++++++++ 8 files changed, 207 insertions(+), 13 deletions(-) diff --git a/phpunit.xml b/phpunit.xml index 37c4eb12d7f..e927ad4806a 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -20,5 +20,7 @@ + + diff --git a/rules/CodeQuality/Rector/BooleanNot/ReplaceConstantBooleanNotRector.php b/rules/CodeQuality/Rector/BooleanNot/ReplaceConstantBooleanNotRector.php index d53136d5f1d..537cbb4088c 100644 --- a/rules/CodeQuality/Rector/BooleanNot/ReplaceConstantBooleanNotRector.php +++ b/rules/CodeQuality/Rector/BooleanNot/ReplaceConstantBooleanNotRector.php @@ -69,14 +69,27 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - if ($this->valueResolver->isFalse($node->expr)) { - return new ConstFetch(new Name('true')); + // unwrap the whole negation chain at once, so nested cases like !!false resolve in a single pass + $negationCount = 0; + $innerExpr = $node; + while ($innerExpr instanceof BooleanNot) { + ++$negationCount; + $innerExpr = $innerExpr->expr; } - if ($this->valueResolver->isTrue($node->expr)) { - return new ConstFetch(new Name('false')); + if ($this->valueResolver->isTrue($innerExpr)) { + $value = true; + } elseif ($this->valueResolver->isFalse($innerExpr)) { + $value = false; + } else { + return null; } - return null; + // every negation flips the literal value + if ($negationCount % 2 === 1) { + $value = ! $value; + } + + return new ConstFetch(new Name($value ? 'true' : 'false')); } } diff --git a/rules/DeadCode/Rector/Block/ReplaceBlockToItsStmtsRector.php b/rules/DeadCode/Rector/Block/ReplaceBlockToItsStmtsRector.php index 4c923942e3c..30eb8fdea25 100644 --- a/rules/DeadCode/Rector/Block/ReplaceBlockToItsStmtsRector.php +++ b/rules/DeadCode/Rector/Block/ReplaceBlockToItsStmtsRector.php @@ -60,6 +60,31 @@ public function refactor(Node $node): int|array return NodeVisitor::REMOVE_NODE; } - return $node->stmts; + $stmts = $this->flattenBlocks($node->stmts); + if ($stmts === []) { + return NodeVisitor::REMOVE_NODE; + } + + return $stmts; + } + + /** + * Flatten nested blocks and drop empty ones in the same pass, as replaced stmts are not re-traversed + * @param Stmt[] $stmts + * @return Stmt[] + */ + private function flattenBlocks(array $stmts): array + { + $flattenedStmts = []; + foreach ($stmts as $stmt) { + if ($stmt instanceof Block) { + $flattenedStmts = [...$flattenedStmts, ...$this->flattenBlocks($stmt->stmts)]; + continue; + } + + $flattenedStmts[] = $stmt; + } + + return $flattenedStmts; } } diff --git a/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php b/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php index 9928ea8e7a0..aceecdd00ae 100644 --- a/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php +++ b/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php @@ -89,7 +89,24 @@ public function refactor(Node $node): ?Node } // -, + - return $this->processBinaryOp($node); + $result = $this->processBinaryOp($node); + if ($result === null) { + return null; + } + + // the chosen operand can be a same-class operation that still carries a dead 0/1 at its own + // top level (e.g. 0 + 5 + 0 returns 0 + 5); such a returned node is skip-marked for this rule, + // so reduce its chain here to finish in a single pass + while ($result instanceof Plus || $result instanceof Minus || $result instanceof Mul || $result instanceof Div) { + $reduced = $this->processBinaryOp($result); + if ($reduced === null || $reduced === $result) { + break; + } + + $result = $reduced; + } + + return $result; } private function processAssignOp(AssignOp $assignOp): ?Expr diff --git a/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php b/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php index c94dd4c42f7..fa7c5e6a6ca 100644 --- a/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php +++ b/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php @@ -73,6 +73,20 @@ public function getNodeTypes(): array * @return Node[]|null */ public function refactor(Node $node): ?array + { + $nodesToReturn = $this->splitAlwaysElse($node); + if ($nodesToReturn === null) { + return null; + } + + // the returned stmts are not re-traversed in this pass, so split nested always-else here too + return $this->splitNestedAlwaysElse($nodesToReturn); + } + + /** + * @return Node[]|null + */ + private function splitAlwaysElse(If_ $node): ?array { if ($this->doesNotLastStatementBreakFlow($node)) { return null; @@ -92,6 +106,49 @@ public function refactor(Node $node): ?array return null; } + /** + * @param Node[] $stmts + * @return Node[] + */ + private function splitNestedAlwaysElse(array $stmts): array + { + $splitStmts = []; + foreach ($stmts as $stmt) { + $this->splitAlwaysElseInSubStmts($stmt); + + if ($stmt instanceof If_) { + $nestedNodesToReturn = $this->splitAlwaysElse($stmt); + if ($nestedNodesToReturn !== null) { + $splitStmts = [...$splitStmts, ...$this->splitNestedAlwaysElse($nestedNodesToReturn)]; + continue; + } + } + + $splitStmts[] = $stmt; + } + + return $splitStmts; + } + + private function splitAlwaysElseInSubStmts(Node $node): void + { + foreach ($node->getSubNodeNames() as $subNodeName) { + $subNode = $node->{$subNodeName}; + if (! is_array($subNode) || $subNode === []) { + continue; + } + + // only statement lists are split; expression/argument lists are left untouched + foreach ($subNode as $subNodeItem) { + if (! $subNodeItem instanceof Node\Stmt) { + continue 2; + } + } + + $node->{$subNodeName} = $this->splitNestedAlwaysElse($subNode); + } + } + /** * @return Node[] */ diff --git a/rules/Php56/Rector/FuncCall/PowToExpRector.php b/rules/Php56/Rector/FuncCall/PowToExpRector.php index d25fe8d549a..c5dc9e4f343 100644 --- a/rules/Php56/Rector/FuncCall/PowToExpRector.php +++ b/rules/Php56/Rector/FuncCall/PowToExpRector.php @@ -5,6 +5,7 @@ namespace Rector\Php56\Rector\FuncCall; use PhpParser\Node; +use PhpParser\Node\Expr; use PhpParser\Node\Expr\BinaryOp\Pow; use PhpParser\Node\Expr\FuncCall; use Rector\NodeAnalyzer\PowOperandAnalyzer; @@ -54,10 +55,24 @@ public function refactor(Node $node): ?Node return null; } - $firstExpr = $node->getArgs()[0] - ->value; - $secondExpr = $node->getArgs()[1] - ->value; + return $this->createPow($node); + } + + public function provideMinPhpVersion(): int + { + return PhpVersionFeature::EXP_OPERATOR; + } + + private function createPow(FuncCall $funcCall): ?Pow + { + $args = $funcCall->getArgs(); + if (count($args) < 2) { + return null; + } + + // convert nested pow() calls first, so pow(pow(...), ...) resolves in a single pass + $firstExpr = $this->createPowFromNested($args[0]->value); + $secondExpr = $this->createPowFromNested($args[1]->value); // ** binds tighter than most operators, so operands with lower precedence must be // wrapped in parentheses to keep the original semantics, e.g. pow(~3, 4) => (~3) ** 4 @@ -72,8 +87,15 @@ public function refactor(Node $node): ?Node return new Pow($firstExpr, $secondExpr); } - public function provideMinPhpVersion(): int + private function createPowFromNested(Expr $expr): Expr { - return PhpVersionFeature::EXP_OPERATOR; + if ($expr instanceof FuncCall && ! $expr->isFirstClassCallable() && $this->isName($expr, 'pow')) { + $pow = $this->createPow($expr); + if ($pow instanceof Pow) { + return $pow; + } + } + + return $expr; } } diff --git a/rules/Php70/Rector/Ternary/TernaryToNullCoalescingRector.php b/rules/Php70/Rector/Ternary/TernaryToNullCoalescingRector.php index e17ae0362a4..bc3a9834ab4 100644 --- a/rules/Php70/Rector/Ternary/TernaryToNullCoalescingRector.php +++ b/rules/Php70/Rector/Ternary/TernaryToNullCoalescingRector.php @@ -171,6 +171,14 @@ private function processTernaryWithIsset(Ternary $ternary, Isset_ $isset): ?Coal return null; } + // a nested coalescable ternary fallback is converted in the same pass; chained ?? needs no parentheses + if ($ternary->else instanceof Ternary) { + $convertedElse = $this->refactor($ternary->else); + if ($convertedElse instanceof Expr) { + return $this->createCoalesce($ternary->if, $convertedElse); + } + } + if (($ternary->else instanceof Ternary || $ternary->else instanceof BinaryOp) && $this->isTernaryParenthesized( $this->getFile(), $ternary->cond, diff --git a/src/Application/FileProcessor.php b/src/Application/FileProcessor.php index 91f2ec44a1c..8d047b05414 100644 --- a/src/Application/FileProcessor.php +++ b/src/Application/FileProcessor.php @@ -22,6 +22,7 @@ use Rector\PhpParser\Parser\RectorParser; use Rector\PhpParser\Printer\BetterStandardPrinter; use Rector\PostRector\Application\PostFileProcessor; +use Rector\PostRector\Contract\Rector\PostRectorInterface; use Rector\Testing\PHPUnit\StaticPHPUnitEnvironment; use Rector\ValueObject\Application\File; use Rector\ValueObject\Configuration; @@ -59,7 +60,9 @@ public function processFile(File $file, Configuration $configuration): FileProce $fileHasChanged = false; $filePath = $file->getFilePath(); + $previousPassRectorClasses = []; do { + $ruleChangesBefore = count($file->getRectorWithLineChanges()); $file->changeHasChanged(false); // 1. change nodes with Rector Rules @@ -75,6 +78,17 @@ public function processFile(File $file, Configuration $configuration): FileProce // important to detect if file has changed $this->printFile($file, $configuration, $filePath); + // in rector-src tests, fail a rule that re-applies to its own previous-pass output; + // such a rule is not finished in a single pass and must handle the nested/chained case itself. + // gated by env so sibling packages pulling rector-src as dev-main are not affected + if (StaticPHPUnitEnvironment::isPHPUnitRun() && getenv('RECTOR_ENFORCE_SINGLE_PASS') === '1') { + $previousPassRectorClasses = $this->ensureRectorsAreIdempotent( + $file, + $ruleChangesBefore, + $previousPassRectorClasses + ); + } + // no change in current iteration, stop if (! $file->hasChanged()) { break; @@ -108,6 +122,42 @@ public function processFile(File $file, Configuration $configuration): FileProce return new FileProcessResult([], $file->getFileDiff(), $file->hasChanged()); } + /** + * @param array $previousPassRectorClasses rule classes that changed the file in the previous pass + * @return array rule classes that changed the file in the current pass + */ + private function ensureRectorsAreIdempotent(File $file, int $ruleChangesBefore, array $previousPassRectorClasses): array + { + $currentPassRectorClasses = []; + $rectorWithLineChanges = $file->getRectorWithLineChanges(); + + foreach ($rectorWithLineChanges as $key => $rectorWithLineChange) { + if ($key < $ruleChangesBefore) { + continue; + } + + $rectorClass = $rectorWithLineChange->getRectorClass(); + + // post rectors (imports) legitimately converge over several passes + if (is_a($rectorClass, PostRectorInterface::class, true)) { + continue; + } + + $currentPassRectorClasses[$rectorClass] = true; + + if (isset($previousPassRectorClasses[$rectorClass])) { + throw new ShouldNotHappenException(sprintf( + 'The "%s" rule changed "%s" again after it already changed it in the previous pass. ' . + 'A rule must reach its final result in a single pass; handle the nested or chained case inside refactor() instead of relying on the file being re-run.', + $rectorClass, + $file->getFilePath() + )); + } + } + + return $currentPassRectorClasses; + } + private function parseFileAndDecorateNodes(File $file): ?SystemError { try { From 31fa11b9891ea7742bc39921a826dec6e00a1b55 Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Mon, 19 Oct 2026 17:22:49 +0000 Subject: [PATCH 2/2] [ci-review] Rector Rectify --- .../Rector/Plus/RemoveDeadZeroAndOneOperationRector.php | 4 ++-- rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php b/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php index aceecdd00ae..2acabcda23a 100644 --- a/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php +++ b/rules/DeadCode/Rector/Plus/RemoveDeadZeroAndOneOperationRector.php @@ -90,7 +90,7 @@ public function refactor(Node $node): ?Node // -, + $result = $this->processBinaryOp($node); - if ($result === null) { + if (!$result instanceof Expr) { return null; } @@ -99,7 +99,7 @@ public function refactor(Node $node): ?Node // so reduce its chain here to finish in a single pass while ($result instanceof Plus || $result instanceof Minus || $result instanceof Mul || $result instanceof Div) { $reduced = $this->processBinaryOp($result); - if ($reduced === null || $reduced === $result) { + if (!$reduced instanceof Expr || $reduced === $result) { break; } diff --git a/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php b/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php index fa7c5e6a6ca..2fa1296f751 100644 --- a/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php +++ b/rules/EarlyReturn/Rector/If_/RemoveAlwaysElseRector.php @@ -4,6 +4,7 @@ namespace Rector\EarlyReturn\Rector\If_; +use PhpParser\Node\Stmt; use PhpParser\Node; use PhpParser\Node\Expr\Exit_; use PhpParser\Node\Expr\Throw_; @@ -140,7 +141,7 @@ private function splitAlwaysElseInSubStmts(Node $node): void // only statement lists are split; expression/argument lists are left untouched foreach ($subNode as $subNodeItem) { - if (! $subNodeItem instanceof Node\Stmt) { + if (! $subNodeItem instanceof Stmt) { continue 2; } }