diff --git a/config/set/solid/early-return.yaml b/config/set/solid/early-return.yaml index ac936a208f1..df8e864d391 100644 --- a/config/set/solid/early-return.yaml +++ b/config/set/solid/early-return.yaml @@ -1,2 +1,3 @@ services: Rector\SOLID\Rector\If_\RemoveAlwaysElseRector: null + Rector\SOLID\Rector\If_\ChangeNestedIfsToEarlyReturnRector: ~ diff --git a/packages/CodeQuality/src/Rector/BinaryOp/SimplifyDeMorganBinaryRector.php b/packages/CodeQuality/src/Rector/BinaryOp/SimplifyDeMorganBinaryRector.php index e9bb37e58d1..2f12cba2476 100644 --- a/packages/CodeQuality/src/Rector/BinaryOp/SimplifyDeMorganBinaryRector.php +++ b/packages/CodeQuality/src/Rector/BinaryOp/SimplifyDeMorganBinaryRector.php @@ -5,12 +5,10 @@ declare(strict_types=1); namespace Rector\CodeQuality\Rector\BinaryOp; use PhpParser\Node; -use PhpParser\Node\Expr; use PhpParser\Node\Expr\BinaryOp; use PhpParser\Node\Expr\BinaryOp\BooleanAnd; -use PhpParser\Node\Expr\BinaryOp\BooleanOr; use PhpParser\Node\Expr\BooleanNot; -use Rector\PhpParser\Node\AssignAndBinaryMap; +use Rector\PhpParser\Node\Manipulator\BinaryOpManipulator; use Rector\Rector\AbstractRector; use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; @@ -23,13 +21,13 @@ use Rector\RectorDefinition\RectorDefinition; final class SimplifyDeMorganBinaryRector extends AbstractRector { /** - * @var AssignAndBinaryMap + * @var BinaryOpManipulator */ - private $assignAndBinaryMap; + private $binaryOpManipulator; - public function __construct(AssignAndBinaryMap $assignAndBinaryMap) + public function __construct(BinaryOpManipulator $binaryOpManipulator) { - $this->assignAndBinaryMap = $assignAndBinaryMap; + $this->binaryOpManipulator = $binaryOpManipulator; } public function getDefinition(): RectorDefinition @@ -77,40 +75,6 @@ PHP return null; } - $inversedNode = $this->assignAndBinaryMap->getInversed($node->expr); - if ($inversedNode === null) { - if ($node->expr instanceof BooleanOr) { - $inversedNode = BooleanAnd::class; - } else { - return null; - } - } - - // no nesting - if ($node->expr->left instanceof BooleanOr) { - return null; - } - - if ($node->expr->right instanceof BooleanOr) { - return null; - } - - return new $inversedNode($this->inverseNode($node->expr->left), $this->inverseNode($node->expr->right)); - } - - private function inverseNode(Expr $expr): Node - { - if ($expr instanceof BinaryOp) { - $inversedBinaryOp = $this->assignAndBinaryMap->getInversed($expr); - if ($inversedBinaryOp) { - return new $inversedBinaryOp($expr->left, $expr->right); - } - } - - if ($expr instanceof BooleanNot) { - return $expr->expr; - } - - return new BooleanNot($expr); + return $this->binaryOpManipulator->inverseBinaryOp($node->expr); } } diff --git a/packages/CodeQuality/src/Rector/FuncCall/ChangeArrayPushToArrayAssignRector.php b/packages/CodeQuality/src/Rector/FuncCall/ChangeArrayPushToArrayAssignRector.php index 9de076b256e..a0ba93918af 100644 --- a/packages/CodeQuality/src/Rector/FuncCall/ChangeArrayPushToArrayAssignRector.php +++ b/packages/CodeQuality/src/Rector/FuncCall/ChangeArrayPushToArrayAssignRector.php @@ -8,6 +8,7 @@ use PhpParser\Node; use PhpParser\Node\Expr\ArrayDimFetch; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\FuncCall; +use PhpParser\Node\Stmt\Expression; use Rector\Rector\AbstractRector; use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; @@ -71,6 +72,13 @@ PHP $position = 1; while (isset($node->args[$position])) { $assign = new Assign($arrayDimFetch, $node->args[$position]->value); + + // keep comments of first line + if ($position === 1) { + $assign = new Expression($assign); + $assign->setAttribute('comments', $node->getComments()); + } + $this->addNodeAfterNode($assign, $node); ++$position; diff --git a/packages/CodeQuality/tests/Rector/FuncCall/ChangeArrayPushToArrayAssignRector/Fixture/push_multiple.php.inc b/packages/CodeQuality/tests/Rector/FuncCall/ChangeArrayPushToArrayAssignRector/Fixture/push_multiple.php.inc index 0ecad117ed8..fe6c18cc4e3 100644 --- a/packages/CodeQuality/tests/Rector/FuncCall/ChangeArrayPushToArrayAssignRector/Fixture/push_multiple.php.inc +++ b/packages/CodeQuality/tests/Rector/FuncCall/ChangeArrayPushToArrayAssignRector/Fixture/push_multiple.php.inc @@ -7,6 +7,7 @@ class PushMultiple public function run() { $items = []; + // keep comment array_push($items, $item, $item2, $item3); } } @@ -22,6 +23,7 @@ class PushMultiple public function run() { $items = []; + // keep comment $items[] = $item; $items[] = $item2; $items[] = $item3; diff --git a/packages/SOLID/src/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php b/packages/SOLID/src/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php new file mode 100644 index 00000000000..f259859a9f8 --- /dev/null +++ b/packages/SOLID/src/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php @@ -0,0 +1,128 @@ +ifManipulator = $ifManipulator; + $this->binaryOpManipulator = $binaryOpManipulator; + } + + public function getDefinition(): RectorDefinition + { + return new RectorDefinition('Change nested ifs to early return', [ + new CodeSample( + <<<'PHP' +class SomeClass +{ + public function run() + { + if ($value === 5) { + if ($value2 === 10) { + return 'yes'; + } + } + + return 'no'; + } +} +PHP +, + <<<'PHP' +class SomeClass +{ + public function run() + { + if ($value !== 5) { + return 'no'; + } + + if ($value2 === 10) { + return 'yes'; + } + + return 'no'; + } +} +PHP + + ), + ]); + } + + /** + * @return string[] + */ + public function getNodeTypes(): array + { + return [If_::class]; + } + + /** + * @param If_ $node + */ + public function refactor(Node $node): ?Node + { + // A. next node is return + $nextNode = $node->getAttribute(AttributeKey::NEXT_NODE); + if (! $nextNode instanceof Return_) { + return null; + } + + $nestedIfsWithOnlyReturn = $this->ifManipulator->collectNestedIfsWithOnlyReturn($node); + if ($nestedIfsWithOnlyReturn === []) { + return null; + } + + // add nested if openly after this + $nestedIfsWithOnlyReturnCount = count($nestedIfsWithOnlyReturn); + + foreach ($nestedIfsWithOnlyReturn as $key => $nestedIfWithOnlyReturn) { + // last item → the return node + if ($nestedIfsWithOnlyReturnCount === $key + 1) { + $this->addNodeAfterNode($nestedIfWithOnlyReturn, $node); + } else { + $inversedCondition = $this->binaryOpManipulator->inverseCondition($nestedIfWithOnlyReturn->cond); + if ($inversedCondition === null) { + return null; + } + + $nestedIfWithOnlyReturn->cond = $inversedCondition; + $nestedIfWithOnlyReturn->stmts = [clone $nextNode]; + + $this->addNodeAfterNode($nestedIfWithOnlyReturn, $node); + } + } + + $this->removeNode($node); + + return null; + } +} diff --git a/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/ChangeNestedIfsToEarlyReturnRectorTest.php b/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/ChangeNestedIfsToEarlyReturnRectorTest.php new file mode 100644 index 00000000000..d4d72da0bee --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/ChangeNestedIfsToEarlyReturnRectorTest.php @@ -0,0 +1,30 @@ +doTestFile($file); + } + + public function provideDataForTest(): Iterator + { + return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + protected function getRectorClass(): string + { + return ChangeNestedIfsToEarlyReturnRector::class; + } +} diff --git a/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/fixture.php.inc b/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/fixture.php.inc new file mode 100644 index 00000000000..30ca8e8ec48 --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/fixture.php.inc @@ -0,0 +1,40 @@ + +----- + diff --git a/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/three_nesting.php.inc b/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/three_nesting.php.inc new file mode 100644 index 00000000000..ecc38014f52 --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/ChangeNestedIfsToEarlyReturnRector/Fixture/three_nesting.php.inc @@ -0,0 +1,45 @@ + +----- + diff --git a/src/PhpParser/Node/Manipulator/BinaryOpManipulator.php b/src/PhpParser/Node/Manipulator/BinaryOpManipulator.php index 78602e0f713..be200fdff00 100644 --- a/src/PhpParser/Node/Manipulator/BinaryOpManipulator.php +++ b/src/PhpParser/Node/Manipulator/BinaryOpManipulator.php @@ -5,11 +5,25 @@ declare(strict_types=1); namespace Rector\PhpParser\Node\Manipulator; use PhpParser\Node; +use PhpParser\Node\Expr; use PhpParser\Node\Expr\BinaryOp; +use PhpParser\Node\Expr\BinaryOp\BooleanAnd; +use PhpParser\Node\Expr\BinaryOp\BooleanOr; use Rector\Exception\ShouldNotHappenException; +use Rector\PhpParser\Node\AssignAndBinaryMap; final class BinaryOpManipulator { + /** + * @var AssignAndBinaryMap + */ + private $assignAndBinaryMap; + + public function __construct(AssignAndBinaryMap $assignAndBinaryMap) + { + $this->assignAndBinaryMap = $assignAndBinaryMap; + } + /** * Tries to match left or right parts (xor), * returns null or match on first condition and then second condition. No matter what the origin order is. @@ -40,6 +54,47 @@ final class BinaryOpManipulator return null; } + public function inverseBinaryOp(BinaryOp $binaryOp): ?BinaryOp + { + // no nesting + if ($binaryOp->left instanceof BooleanOr) { + return null; + } + + if ($binaryOp->right instanceof BooleanOr) { + return null; + } + + $inversedNodeClass = $this->resolveInversedNodeClass($binaryOp); + if ($inversedNodeClass === null) { + return null; + } + + $firstInversedNode = $this->inverseNode($binaryOp->left); + $secondInversedNode = $this->inverseNode($binaryOp->right); + + return new $inversedNodeClass($firstInversedNode, $secondInversedNode); + } + + public function inverseCondition(BinaryOp $binaryOp): ?BinaryOp + { + // no nesting + if ($binaryOp->left instanceof BooleanOr) { + return null; + } + + if ($binaryOp->right instanceof BooleanOr) { + return null; + } + + $inversedNodeClass = $this->resolveInversedNodeClass($binaryOp); + if ($inversedNodeClass === null) { + return null; + } + + return new $inversedNodeClass($binaryOp->left, $binaryOp->right); + } + private function validateCondition($firstCondition): void { if (is_callable($firstCondition)) { @@ -66,4 +121,34 @@ final class BinaryOpManipulator return is_a($node, $condition, true); }; } + + private function inverseNode(Expr $expr): Node + { + if ($expr instanceof BinaryOp) { + $inversedBinaryOp = $this->assignAndBinaryMap->getInversed($expr); + if ($inversedBinaryOp) { + return new $inversedBinaryOp($expr->left, $expr->right); + } + } + + if ($expr instanceof Expr\BooleanNot) { + return $expr->expr; + } + + return new Expr\BooleanNot($expr); + } + + private function resolveInversedNodeClass(BinaryOp $binaryOp): ?string + { + $inversedNodeClass = $this->assignAndBinaryMap->getInversed($binaryOp); + if ($inversedNodeClass !== null) { + return $inversedNodeClass; + } + + if ($binaryOp instanceof BooleanOr) { + return BooleanAnd::class; + } + + return null; + } } diff --git a/src/PhpParser/Node/Manipulator/IfManipulator.php b/src/PhpParser/Node/Manipulator/IfManipulator.php index e9f8f8b3b1c..fdb82562c72 100644 --- a/src/PhpParser/Node/Manipulator/IfManipulator.php +++ b/src/PhpParser/Node/Manipulator/IfManipulator.php @@ -123,6 +123,15 @@ final class IfManipulator return null; } + public function isIfWithOnlyStmtIf(If_ $if): bool + { + if (! $this->isIfWithoutElseAndElseIfs($if)) { + return false; + } + + return $this->hasOnlyStmtOfType($if, If_::class); + } + public function isEarlyElse(If_ $if): bool { if (! $this->isAlwaysAllowedType((array) $if->stmts, self::ALLOWED_BREAKING_NODE_TYPES)) { @@ -138,6 +147,39 @@ final class IfManipulator return $if->else !== null; } + public function hasOnlyStmtOfType(If_ $if, string $desiredType): bool + { + if (count($if->stmts) !== 1) { + return false; + } + + return is_a($if->stmts[0], $desiredType); + } + + /** + * @return If_[] + */ + public function collectNestedIfsWithOnlyReturn(If_ $if): array + { + $ifs = []; + + $currentIf = $if; + while ($this->isIfWithOnlyStmtIf($currentIf)) { + $ifs[] = $currentIf; + + $currentIf = $currentIf->stmts[0]; + } + + if (! $this->hasOnlyStmtOfType($currentIf, Return_::class)) { + return []; + } + + // last node is with the return value + $ifs[] = $currentIf; + + return $ifs; + } + private function matchComparedAndReturnedNode(NotIdentical $notIdentical, Return_ $returnNode): ?Expr { if ($this->betterStandardPrinter->areNodesEqual($notIdentical->left, $returnNode->expr)) { @@ -205,4 +247,13 @@ final class IfManipulator return false; } + + private function isIfWithoutElseAndElseIfs(If_ $if): bool + { + if ($if->else !== null) { + return false; + } + + return ! (bool) $if->elseifs; + } } diff --git a/src/Rector/MethodCall/MethodCallToReturnRector.php b/src/Rector/MethodCall/MethodCallToReturnRector.php index aec56e4e0a2..affb693cee5 100644 --- a/src/Rector/MethodCall/MethodCallToReturnRector.php +++ b/src/Rector/MethodCall/MethodCallToReturnRector.php @@ -82,7 +82,7 @@ PHP } /** - * @param Node\Stmt\Expression $node + * @param Expression $node */ public function refactor(Node $node): ?Node {