From bd6830f0a0ef9a766cd97eb8d85355a752e48aa6 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Thu, 6 Aug 2026 01:07:49 +0200 Subject: [PATCH] [EarlyReturn] Deprecate ChangeNestedIfsToEarlyReturnRector --- config/set/early-return.php | 4 +- ...ChangeNestedIfsToEarlyReturnRectorTest.php | 28 ----- .../do_not_remove_previous_if_stmts.php.inc | 44 -------- .../Fixture/fixture.php.inc | 40 ------- .../Fixture/having_elseifs.php.inc | 44 -------- .../Fixture/rector_sample.php.inc | 86 -------------- .../Fixture/skip_single_if.php.inc | 17 --- .../Fixture/three_nesting.php.inc | 45 -------- .../config/configured_rule.php | 9 -- .../ChangeNestedIfsToEarlyReturnRector.php | 105 ++---------------- src/NodeManipulator/IfManipulator.php | 38 ------- 11 files changed, 10 insertions(+), 450 deletions(-) delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/ChangeNestedIfsToEarlyReturnRectorTest.php delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/do_not_remove_previous_if_stmts.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/fixture.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/having_elseifs.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/rector_sample.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/skip_single_if.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/three_nesting.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/config/configured_rule.php diff --git a/config/set/early-return.php b/config/set/early-return.php index 64bfa28f321..4d7a1077583 100644 --- a/config/set/early-return.php +++ b/config/set/early-return.php @@ -3,8 +3,8 @@ declare(strict_types=1); use Rector\Config\RectorConfig; -use Rector\EarlyReturn\Rector\If_\ChangeNestedIfsToEarlyReturnRector; +// note: all early return rules were moved to code quality set or deprecated return static function (RectorConfig $rectorConfig): void { - $rectorConfig->rules([ChangeNestedIfsToEarlyReturnRector::class]); + $rectorConfig->rules([]); }; diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/ChangeNestedIfsToEarlyReturnRectorTest.php b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/ChangeNestedIfsToEarlyReturnRectorTest.php deleted file mode 100644 index 17e5f5d57ae..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/ChangeNestedIfsToEarlyReturnRectorTest.php +++ /dev/null @@ -1,28 +0,0 @@ -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/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/do_not_remove_previous_if_stmts.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/do_not_remove_previous_if_stmts.php.inc deleted file mode 100644 index 57f3e91ba21..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/do_not_remove_previous_if_stmts.php.inc +++ /dev/null @@ -1,44 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/fixture.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/fixture.php.inc deleted file mode 100644 index 0485805543c..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/fixture.php.inc +++ /dev/null @@ -1,40 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/having_elseifs.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/having_elseifs.php.inc deleted file mode 100644 index 5f419d6a0bd..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/having_elseifs.php.inc +++ /dev/null @@ -1,44 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/rector_sample.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/rector_sample.php.inc deleted file mode 100644 index df8d0420610..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/rector_sample.php.inc +++ /dev/null @@ -1,86 +0,0 @@ -cond instanceof Smaller) { - // $a < $b ? -1 - if ($this->getValue($node->if) === -1) { - // $a < $b ? -1 : ( ? : ) - if ($node->else instanceof Ternary) { - // $a < $b ? -1 : ( > ? : ) - if ($node->else->cond instanceof BinaryOp\Greater) { - // $a < $b ? -1 : ($a > $b ? : ) - if ($this->nodeComparator->areNodesEqual($node->cond->left, $node->else->cond->left)) { - if ($this->nodeComparator->areNodesEqual($node->cond->right, $node->else->cond->right)) { - // $a < $b ? -1 : ($a > $b ? 1 : 0) - if ($this->getValue($node->else->if) === 1) { - if ($this->getValue($node->else->else) === 0) { - return new Spaceship($node->cond->left, $node->cond->right); - } - } - } - } - } - } - } - } - - return null; - } -} - -?> ------ -cond instanceof Smaller) { - return null; - } - // $a < $b ? -1 - if ($this->getValue($node->if) !== -1) { - return null; - } - // $a < $b ? -1 : ( ? : ) - if (!$node->else instanceof Ternary) { - return null; - } - // $a < $b ? -1 : ( > ? : ) - if (!$node->else->cond instanceof BinaryOp\Greater) { - return null; - } - // $a < $b ? -1 : ($a > $b ? : ) - if (!$this->nodeComparator->areNodesEqual($node->cond->left, $node->else->cond->left)) { - return null; - } - if (!$this->nodeComparator->areNodesEqual($node->cond->right, $node->else->cond->right)) { - return null; - } - // $a < $b ? -1 : ($a > $b ? 1 : 0) - if ($this->getValue($node->else->if) !== 1) { - return null; - } - if ($this->getValue($node->else->else) === 0) { - return new Spaceship($node->cond->left, $node->cond->right); - } - - return null; - } -} - -?> diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/skip_single_if.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/skip_single_if.php.inc deleted file mode 100644 index b53d70fb82d..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/skip_single_if.php.inc +++ /dev/null @@ -1,17 +0,0 @@ -betterNodeFinder->find((array) $classMethod->stmts, function (Node $node): bool { - if (! $node instanceof FuncCall) { - return false; - } - - return $this->nameResolver->isName($node, 'compact'); - }); - } -} diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/three_nesting.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/three_nesting.php.inc deleted file mode 100644 index 7d37d401203..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/three_nesting.php.inc +++ /dev/null @@ -1,45 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/config/configured_rule.php b/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/config/configured_rule.php deleted file mode 100644 index 5430533faa0..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector/config/configured_rule.php +++ /dev/null @@ -1,9 +0,0 @@ -withRules([ChangeNestedIfsToEarlyReturnRector::class]); diff --git a/rules/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php b/rules/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php index 44b14399cb1..c905a911e64 100644 --- a/rules/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php +++ b/rules/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php @@ -5,28 +5,18 @@ namespace Rector\EarlyReturn\Rector\If_; use PhpParser\Node; -use PhpParser\Node\Expr\BinaryOp\BooleanAnd; -use PhpParser\Node\Expr\BooleanNot; -use PhpParser\Node\Stmt\If_; -use PhpParser\Node\Stmt\Return_; -use Rector\EarlyReturn\NodeTransformer\ConditionInverter; -use Rector\NodeManipulator\IfManipulator; +use Rector\Configuration\Deprecation\Contract\DeprecatedInterface; +use Rector\Exception\ShouldNotHappenException; use Rector\PhpParser\Enum\NodeGroup; use Rector\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; /** - * @see \Rector\Tests\EarlyReturn\Rector\If_\ChangeNestedIfsToEarlyReturnRector\ChangeNestedIfsToEarlyReturnRectorTest + * @deprecated This rule is deprecated, as inverting nested ifs to early return makes the code harder to read and understand, and depends on the context. */ -final class ChangeNestedIfsToEarlyReturnRector extends AbstractRector +final class ChangeNestedIfsToEarlyReturnRector extends AbstractRector implements DeprecatedInterface { - public function __construct( - private readonly ConditionInverter $conditionInverter, - private readonly IfManipulator $ifManipulator - ) { - } - public function getRuleDefinition(): RuleDefinition { return new RuleDefinition('Change nested ifs to early return', [ @@ -76,90 +66,11 @@ public function getNodeTypes(): array return NodeGroup::STMTS_AWARE; } - /** - * @param StmtsAware $node - * @return StmtsAware - */ public function refactor(Node $node): ?Node { - if ($node->stmts === null) { - return null; - } - - foreach ($node->stmts as $key => $stmt) { - if (! $stmt instanceof If_) { - continue; - } - - $nextStmt = $node->stmts[$key + 1] ?? null; - if (! $nextStmt instanceof Return_) { - return null; - } - - $nestedIfsWithOnlyReturn = $this->ifManipulator->collectNestedIfsWithOnlyReturn($stmt); - if ($nestedIfsWithOnlyReturn === []) { - continue; - } - - $newStmts = $this->processNestedIfsWithOnlyReturn($nestedIfsWithOnlyReturn, $nextStmt); - - // replace nested ifs with many separate ifs - array_splice($node->stmts, $key, 1, $newStmts); - - return $node; - } - - return null; - } - - /** - * @param If_[] $nestedIfsWithOnlyReturn - * @return If_[] - */ - private function processNestedIfsWithOnlyReturn(array $nestedIfsWithOnlyReturn, Return_ $nextReturn): array - { - // add nested if openly after this - $nestedIfsWithOnlyReturnCount = count($nestedIfsWithOnlyReturn); - - $newStmts = []; - - /** @var int $key */ - foreach ($nestedIfsWithOnlyReturn as $key => $nestedIfWithOnlyReturn) { - // last item → the return node - if ($nestedIfsWithOnlyReturnCount === $key + 1) { - $newStmts[] = $nestedIfWithOnlyReturn; - } else { - $standaloneIfs = $this->createStandaloneIfsWithReturn($nestedIfWithOnlyReturn, $nextReturn); - $newStmts = [...$newStmts, ...$standaloneIfs]; - } - } - - // $newStmts[] = $nextReturn; - - return $newStmts; - } - - /** - * @return If_[] - */ - private function createStandaloneIfsWithReturn(If_ $onlyReturnIf, Return_ $return): array - { - $invertedCondExpr = $this->conditionInverter->createInvertedCondition($onlyReturnIf->cond); - - // special case - if ($invertedCondExpr instanceof BooleanNot && $invertedCondExpr->expr instanceof BooleanAnd) { - $booleanNotPartIf = new If_(new BooleanNot($invertedCondExpr->expr->left)); - $booleanNotPartIf->stmts = [clone $return]; - - $secondBooleanNotPartIf = new If_(new BooleanNot($invertedCondExpr->expr->right)); - $secondBooleanNotPartIf->stmts = [clone $return]; - - return [$booleanNotPartIf, $secondBooleanNotPartIf]; - } - - $onlyReturnIf->cond = $invertedCondExpr; - $onlyReturnIf->stmts = [$return]; - - return [$onlyReturnIf]; + throw new ShouldNotHappenException(sprintf( + '"%s" rule is deprecated, as inverting nested ifs to early return makes the code harder to read and understand', + self::class + )); } } diff --git a/src/NodeManipulator/IfManipulator.php b/src/NodeManipulator/IfManipulator.php index f9ba2a3eb5c..5fc7679fc12 100644 --- a/src/NodeManipulator/IfManipulator.php +++ b/src/NodeManipulator/IfManipulator.php @@ -49,35 +49,6 @@ public function matchIfNotNullReturnValue(If_ $if): ?Expr return $this->matchComparedAndReturnedNode($if->cond, $insideIfNode); } - /** - * @return If_[] - */ - public function collectNestedIfsWithOnlyReturn(If_ $if): array - { - $ifs = []; - - $currentIf = $if; - while ($this->isIfWithOnlyStmtIf($currentIf)) { - $ifs[] = $currentIf; - - /** @var If_ $currentIf */ - $currentIf = $currentIf->stmts[0]; - } - - if ($ifs === []) { - return []; - } - - if (! $this->hasOnlyStmtOfType($currentIf, Return_::class)) { - return []; - } - - // last if is with the return value - $ifs[] = $currentIf; - - return $ifs; - } - public function isIfAndElseWithSameVariableAssignAsLastStmts(If_ $if, Expr $desiredExpr): bool { if (! $if->else instanceof Else_) { @@ -150,15 +121,6 @@ private function matchComparedAndReturnedNode(NotIdentical $notIdentical, Return return null; } - private function isIfWithOnlyStmtIf(If_ $if): bool - { - if (! $this->isIfWithoutElseAndElseIfs($if)) { - return false; - } - - return $this->hasOnlyStmtOfType($if, If_::class); - } - /** * @param class-string $stmtClass */