diff --git a/config/set/early-return.php b/config/set/early-return.php index 4e41135f971..64bfa28f321 100644 --- a/config/set/early-return.php +++ b/config/set/early-return.php @@ -3,12 +3,8 @@ declare(strict_types=1); use Rector\Config\RectorConfig; -use Rector\EarlyReturn\Rector\Foreach_\ChangeNestedForeachIfsToEarlyContinueRector; use Rector\EarlyReturn\Rector\If_\ChangeNestedIfsToEarlyReturnRector; return static function (RectorConfig $rectorConfig): void { - $rectorConfig->rules([ - ChangeNestedForeachIfsToEarlyContinueRector::class, - ChangeNestedIfsToEarlyReturnRector::class, - ]); + $rectorConfig->rules([ChangeNestedIfsToEarlyReturnRector::class]); }; diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/ChangeNestedForeachIfsToEarlyContinueRectorTest.php b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/ChangeNestedForeachIfsToEarlyContinueRectorTest.php deleted file mode 100644 index aebd3138909..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/ChangeNestedForeachIfsToEarlyContinueRectorTest.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/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/comment_inside_if_statement.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/comment_inside_if_statement.php.inc deleted file mode 100644 index b0dacbd139d..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/comment_inside_if_statement.php.inc +++ /dev/null @@ -1,46 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/fixture.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/fixture.php.inc deleted file mode 100644 index 5b63ee50c16..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/fixture.php.inc +++ /dev/null @@ -1,45 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs.php.inc deleted file mode 100644 index 6242b570bc8..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs.php.inc +++ /dev/null @@ -1,49 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_OR_both_true.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_OR_both_true.php.inc deleted file mode 100644 index a9d83b8bda0..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_OR_both_true.php.inc +++ /dev/null @@ -1,54 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_OR_both_true2.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_OR_both_true2.php.inc deleted file mode 100644 index 55068bebd51..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_OR_both_true2.php.inc +++ /dev/null @@ -1,60 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_bool_oprnds.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_bool_oprnds.php.inc deleted file mode 100644 index 1af16cc0c24..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/multi_exprs_with_bool_oprnds.php.inc +++ /dev/null @@ -1,57 +0,0 @@ - ------ - diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/skip_nested_with_or_root_if.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/skip_nested_with_or_root_if.php.inc deleted file mode 100644 index 2832dd0ced8..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/skip_nested_with_or_root_if.php.inc +++ /dev/null @@ -1,19 +0,0 @@ - $value) { - if ($key < 7000 || $key == 10000) { - if (array_key_exists($key, $arrayTwo)) { - $executed = true; - } - } - } - return $executed; - } -} diff --git a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/skip_single_line.php.inc b/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/skip_single_line.php.inc deleted file mode 100644 index 61995904c64..00000000000 --- a/rules-tests/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector/Fixture/skip_single_line.php.inc +++ /dev/null @@ -1,17 +0,0 @@ -withRules([ChangeNestedForeachIfsToEarlyContinueRector::class]); diff --git a/rules/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector.php b/rules/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector.php index 9e114ab26ff..d7b6acaaa25 100644 --- a/rules/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector.php +++ b/rules/EarlyReturn/Rector/Foreach_/ChangeNestedForeachIfsToEarlyContinueRector.php @@ -5,33 +5,18 @@ namespace Rector\EarlyReturn\Rector\Foreach_; use PhpParser\Node; -use PhpParser\Node\Expr; -use PhpParser\Node\Expr\BinaryOp\BooleanAnd; -use PhpParser\Node\Expr\BinaryOp\BooleanOr; -use PhpParser\Node\Expr\BinaryOp\Equal; -use PhpParser\Node\Expr\BinaryOp\NotEqual; -use PhpParser\Node\Expr\BooleanNot; -use PhpParser\Node\Stmt\Continue_; use PhpParser\Node\Stmt\Foreach_; -use PhpParser\Node\Stmt\If_; -use Rector\EarlyReturn\NodeTransformer\ConditionInverter; -use Rector\NodeManipulator\IfManipulator; -use Rector\NodeTypeResolver\Node\AttributeKey; +use Rector\Configuration\Deprecation\Contract\DeprecatedInterface; +use Rector\Exception\ShouldNotHappenException; use Rector\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; /** - * @see \Rector\Tests\EarlyReturn\Rector\Foreach_\ChangeNestedForeachIfsToEarlyContinueRector\ChangeNestedForeachIfsToEarlyContinueRectorTest + * @deprecated This rule is deprecated, as inverting nested ifs to continue makes the code harder to read and understand, and depends on the context. */ -final class ChangeNestedForeachIfsToEarlyContinueRector extends AbstractRector +final class ChangeNestedForeachIfsToEarlyContinueRector 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 foreach with continue', [ @@ -91,121 +76,9 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - $nestedIfsWithOnlyNonReturn = $this->ifManipulator->collectNestedIfsWithNonBreaking($node); - if (count($nestedIfsWithOnlyNonReturn) < 2) { - return null; - } - - foreach ($nestedIfsWithOnlyNonReturn as $nestedIfWithOnlyNonReturn) { - $nestedIfWithOnlyNonReturn->cond->setAttribute(AttributeKey::ORIGINAL_NODE, null); - } - - return $this->processNestedIfsWithNonBreaking($node, $nestedIfsWithOnlyNonReturn); - } - - /** - * @param If_[] $nestedIfsWithOnlyReturn - */ - private function processNestedIfsWithNonBreaking(Foreach_ $foreach, array $nestedIfsWithOnlyReturn): Foreach_ - { - // add nested if openly after this - $nestedIfsWithOnlyReturnCount = count($nestedIfsWithOnlyReturn); - - // clear - $foreach->stmts = []; - - foreach ($nestedIfsWithOnlyReturn as $key => $nestedIfWithOnlyReturn) { - // last item → the return node - if ($nestedIfsWithOnlyReturnCount === $key + 1) { - $finalReturn = clone $nestedIfWithOnlyReturn; - - $this->addInvertedIfStmtWithContinue($nestedIfWithOnlyReturn, $foreach); - - // should skip for weak inversion - if ($this->isBooleanOrWithWeakComparison($nestedIfWithOnlyReturn->cond)) { - continue; - } - - $foreach->stmts = array_merge($foreach->stmts, $finalReturn->stmts); - } else { - $this->addInvertedIfStmtWithContinue($nestedIfWithOnlyReturn, $foreach); - } - } - - return $foreach; - } - - private function addInvertedIfStmtWithContinue(If_ $onlyReturnIf, Foreach_ $foreach): void - { - $invertedCondExpr = $this->conditionInverter->createInvertedCondition($onlyReturnIf->cond); - - // special case - if ($invertedCondExpr instanceof BooleanNot && $invertedCondExpr->expr instanceof BooleanAnd) { - $leftExpr = $this->negateOrDeNegate($invertedCondExpr->expr->left); - $foreach->stmts[] = $this->createIfContinue($leftExpr); - - $rightExpr = $this->negateOrDeNegate($invertedCondExpr->expr->right); - $foreach->stmts[] = $this->createIfContinue($rightExpr); - - return; - } - - // should skip for weak inversion - if ($this->isBooleanOrWithWeakComparison($onlyReturnIf->cond)) { - $foreach->stmts[] = $onlyReturnIf; - - return; - } - - $onlyReturnIf->setAttribute(AttributeKey::ORIGINAL_NODE, null); - - $onlyReturnIf->cond = $invertedCondExpr; - $onlyReturnIf->stmts = [new Continue_()]; - - $foreach->stmts[] = $onlyReturnIf; - } - - /** - * Matches: - * $a == 1 || $b == 1 - * - * Skips: - * $a === 1 || $b === 2 - */ - private function isBooleanOrWithWeakComparison(Expr $expr): bool - { - if (! $expr instanceof BooleanOr) { - return false; - } - - if ($expr->left instanceof Equal) { - return true; - } - - if ($expr->left instanceof NotEqual) { - return true; - } - - if ($expr->right instanceof Equal) { - return true; - } - - return $expr->right instanceof NotEqual; - } - - private function negateOrDeNegate(Expr $expr): Expr - { - if ($expr instanceof BooleanNot) { - return $expr->expr; - } - - return new BooleanNot($expr); - } - - private function createIfContinue(Expr $expr): If_ - { - return new If_($expr, [ - 'stmts' => [new Continue_()], - ]); + throw new ShouldNotHappenException(sprintf( + '"%s" rule is deprecated, as inverting nested ifs to continue makes the code harder to read and understand', + self::class + )); } } diff --git a/src/NodeManipulator/IfManipulator.php b/src/NodeManipulator/IfManipulator.php index 92f239ff3ed..f9ba2a3eb5c 100644 --- a/src/NodeManipulator/IfManipulator.php +++ b/src/NodeManipulator/IfManipulator.php @@ -6,23 +6,18 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\Assign; -use PhpParser\Node\Expr\BinaryOp\BooleanOr; use PhpParser\Node\Expr\BinaryOp\NotIdentical; -use PhpParser\Node\Expr\Exit_; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\Else_; -use PhpParser\Node\Stmt\Foreach_; use PhpParser\Node\Stmt\If_; use PhpParser\Node\Stmt\Return_; use Rector\PhpParser\Comparing\NodeComparator; -use Rector\PhpParser\Node\BetterNodeFinder; use Rector\PhpParser\Node\Value\ValueResolver; final readonly class IfManipulator { public function __construct( - private BetterNodeFinder $betterNodeFinder, private StmtsManipulator $stmtsManipulator, private ValueResolver $valueResolver, private NodeComparator $nodeComparator @@ -114,50 +109,6 @@ public function isIfAndElseWithSameVariableAssignAsLastStmts(If_ $if, Expr $desi return $this->nodeComparator->areNodesEqual($desiredExpr, $lastElseNode->var); } - /** - * @return If_[] - */ - public function collectNestedIfsWithNonBreaking(Foreach_ $foreach): array - { - if (count($foreach->stmts) !== 1) { - return []; - } - - $onlyForeachStmt = $foreach->stmts[0]; - if (! $onlyForeachStmt instanceof If_) { - return []; - } - - if ($onlyForeachStmt->cond instanceof BooleanOr) { - return []; - } - - $ifs = []; - - $currentIf = $onlyForeachStmt; - while ($this->isIfWithOnlyStmtIf($currentIf)) { - $ifs[] = $currentIf; - - /** @var If_ $currentIf */ - $currentIf = $currentIf->stmts[0]; - } - - // IfManipulator is not build to handle elseif and else - if (! $this->isIfWithoutElseAndElseIfs($currentIf)) { - return []; - } - - if ($this->betterNodeFinder->hasInstancesOf($currentIf->stmts, [Return_::class, Exit_::class])) { - return []; - } - - // last if is with the expression - - $ifs[] = $currentIf; - - return $ifs; - } - /** * @param class-string $stmtClass */