From 4d986dd23c9f9c64e0d63d45301bd0a4643d26f6 Mon Sep 17 00:00:00 2001 From: Jeroen Smit Date: Wed, 15 Jan 2020 21:13:29 +0100 Subject: [PATCH] Added more logic for if statement splitting --- docs/AllRectorsOverview.md | 2 +- .../src/Rector/If_/RemoveAlwaysElseRector.php | 54 +++++++---- .../Fixture/elseif.php.inc | 44 +++++++++ .../Fixture/empty_if.php.inc | 17 ++++ .../Fixture/exit.php.inc | 34 +++++++ .../Fixture/no_break.php.inc | 16 ++++ .../Fixture/process_empty_return_last.php.inc | 35 +++++++- .../Fixture/simple_if.php.inc | 15 ++++ .../Fixture/skip_not_only_else.php.inc | 11 --- .../Fixture/throw.php.inc | 34 +++++++ .../Node/Manipulator/IfManipulator.php | 90 ------------------- 11 files changed, 227 insertions(+), 125 deletions(-) create mode 100644 packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/elseif.php.inc create mode 100644 packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/empty_if.php.inc create mode 100644 packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/exit.php.inc create mode 100644 packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/no_break.php.inc create mode 100644 packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/simple_if.php.inc create mode 100644 packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/throw.php.inc diff --git a/docs/AllRectorsOverview.md b/docs/AllRectorsOverview.md index 294121d3265..499dd2f2b33 100644 --- a/docs/AllRectorsOverview.md +++ b/docs/AllRectorsOverview.md @@ -7148,7 +7148,7 @@ Finalize every class constant that is used only locally - class: `Rector\SOLID\Rector\If_\RemoveAlwaysElseRector` -Remove if for last else, if previous values were throw +Split if statement, when if condition always break execution flow ```diff class SomeClass diff --git a/packages/SOLID/src/Rector/If_/RemoveAlwaysElseRector.php b/packages/SOLID/src/Rector/If_/RemoveAlwaysElseRector.php index a46d636d482..36a9d090062 100644 --- a/packages/SOLID/src/Rector/If_/RemoveAlwaysElseRector.php +++ b/packages/SOLID/src/Rector/If_/RemoveAlwaysElseRector.php @@ -5,30 +5,25 @@ declare(strict_types=1); namespace Rector\SOLID\Rector\If_; use PhpParser\Node; +use PhpParser\Node\Expr\Exit_; +use PhpParser\Node\Stmt\Continue_; +use PhpParser\Node\Stmt\ElseIf_; +use PhpParser\Node\Stmt\Expression; use PhpParser\Node\Stmt\If_; -use Rector\PhpParser\Node\Manipulator\IfManipulator; +use PhpParser\Node\Stmt\Return_; +use PhpParser\Node\Stmt\Throw_; use Rector\Rector\AbstractRector; use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; /** - * @see \Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElseRector\RemoveAlwaysElseRectorTest + * @see \Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\SplitIfsRectorTest */ final class RemoveAlwaysElseRector extends AbstractRector { - /** - * @var IfManipulator - */ - private $ifManipulator; - - public function __construct(IfManipulator $ifManipulator) - { - $this->ifManipulator = $ifManipulator; - } - public function getDefinition(): RectorDefinition { - return new RectorDefinition('Remove if for last else, if previous values were throw', [ + return new RectorDefinition('Split if statement, when if condition always break execution flow', [ new CodeSample( <<<'PHP' class SomeClass @@ -75,16 +70,37 @@ PHP */ public function refactor(Node $node): ?Node { - if (! $this->ifManipulator->isEarlyElse($node)) { + if ($this->lastStatementBreaksFlow($node)) { return null; } - - foreach ($node->else->stmts as $elseStmt) { - $this->addNodeAfterNode($elseStmt, $node); + if ($node->elseifs !== []) { + $newNode = new If_($node->cond); + $newNode->stmts = $node->stmts; + $this->addNodeBeforeNode($newNode, $node); + /** @var ElseIf_ $firstElseIf */ + $firstElseIf = array_shift($node->elseifs); + $node->cond = $firstElseIf->cond; + $node->stmts = $firstElseIf->stmts; + return $node; } - $node->else = null; + if ($node->else !== null) { + foreach ($node->else->stmts as $stmt) { + $this->addNodeAfterNode($stmt, $node); + } + $node->else = null; + return $node; + } - return $node; + return null; + } + + protected function lastStatementBreaksFlow(Node $node): bool + { + $lastStmt = end($node->stmts); + return ! ($lastStmt instanceof Return_ + || $lastStmt instanceof Throw_ + || $lastStmt instanceof Continue_ + || ($lastStmt instanceof Expression && $lastStmt->expr instanceof Exit_)); } } diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/elseif.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/elseif.php.inc new file mode 100644 index 00000000000..ac977db976c --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/elseif.php.inc @@ -0,0 +1,44 @@ + +----- + diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/empty_if.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/empty_if.php.inc new file mode 100644 index 00000000000..dbbb353727d --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/empty_if.php.inc @@ -0,0 +1,17 @@ + diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/exit.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/exit.php.inc new file mode 100644 index 00000000000..4f9621658e0 --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/exit.php.inc @@ -0,0 +1,34 @@ + +----- + diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/no_break.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/no_break.php.inc new file mode 100644 index 00000000000..a8518dbfd4b --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/no_break.php.inc @@ -0,0 +1,16 @@ + diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/process_empty_return_last.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/process_empty_return_last.php.inc index 5620b0fe833..9a61ab51102 100644 --- a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/process_empty_return_last.php.inc +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/process_empty_return_last.php.inc @@ -4,11 +4,24 @@ namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Fixture; class ProcessEmptyReturnLast { - public function runAgain($value) + public function firstRun($value) { if ($value) { return 5; - } elseif ($value- 1) { + } elseif ($value - 1) { + $value = 55; + return 10; + } else { + return; + } + } + + public function secondRun($value) + { + if ($value) { + return 5; + } + if ($value - 1) { $value = 55; return 10; } else { @@ -25,11 +38,25 @@ namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Fixture; class ProcessEmptyReturnLast { - public function runAgain($value) + public function firstRun($value) { if ($value) { return 5; - } elseif ($value- 1) { + } + if ($value - 1) { + $value = 55; + return 10; + } else { + return; + } + } + + public function secondRun($value) + { + if ($value) { + return 5; + } + if ($value - 1) { $value = 55; return 10; } diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/simple_if.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/simple_if.php.inc new file mode 100644 index 00000000000..98ba7fcafe7 --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/simple_if.php.inc @@ -0,0 +1,15 @@ + diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/skip_not_only_else.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/skip_not_only_else.php.inc index df33aa5f2a2..e40da9b32e5 100644 --- a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/skip_not_only_else.php.inc +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/skip_not_only_else.php.inc @@ -4,17 +4,6 @@ namespace Rector\SOLID\Tests\Rector\If_\RemoveAlwaysElse\Fixture; class SkipNotOnlyElse { - public function run($value) - { - if ($value) { - return 5; - } elseif ($value- 1) { - $value = 55; - return; - } else { - return 10; - } - } public function runAgainAndAgain($value) { diff --git a/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/throw.php.inc b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/throw.php.inc new file mode 100644 index 00000000000..9b83a399d79 --- /dev/null +++ b/packages/SOLID/tests/Rector/If_/RemoveAlwaysElseRector/Fixture/throw.php.inc @@ -0,0 +1,34 @@ + +----- + diff --git a/src/PhpParser/Node/Manipulator/IfManipulator.php b/src/PhpParser/Node/Manipulator/IfManipulator.php index c37653bfe11..90508f957cb 100644 --- a/src/PhpParser/Node/Manipulator/IfManipulator.php +++ b/src/PhpParser/Node/Manipulator/IfManipulator.php @@ -11,30 +11,13 @@ use PhpParser\Node\Expr\BinaryOp\Identical; use PhpParser\Node\Expr\BinaryOp\NotIdentical; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\Variable; -use PhpParser\Node\Stmt\Continue_; -use PhpParser\Node\Stmt\Do_; -use PhpParser\Node\Stmt\Else_; use PhpParser\Node\Stmt\If_; use PhpParser\Node\Stmt\Return_; -use PhpParser\Node\Stmt\Throw_; -use PhpParser\Node\Stmt\While_; -use PhpParser\NodeTraverser; use Rector\PhpParser\Node\Resolver\NameResolver; -use Rector\PhpParser\NodeTraverser\CallableNodeTraverser; use Rector\PhpParser\Printer\BetterStandardPrinter; final class IfManipulator { - /** - * @var string[] - */ - private const ALLOWED_BREAKING_NODE_TYPES = [Return_::class, Throw_::class, Continue_::class]; - - /** - * @var string[] - */ - private const SCOPE_CHANGING_NODE_TYPES = [Do_::class, While_::class, If_::class, Else_::class]; - /** * @var BetterStandardPrinter */ @@ -45,11 +28,6 @@ final class IfManipulator */ private $constFetchManipulator; - /** - * @var CallableNodeTraverser - */ - private $callableNodeTraverser; - /** * @var StmtsManipulator */ @@ -63,13 +41,11 @@ final class IfManipulator public function __construct( BetterStandardPrinter $betterStandardPrinter, ConstFetchManipulator $constFetchManipulator, - CallableNodeTraverser $callableNodeTraverser, StmtsManipulator $stmtsManipulator, NameResolver $nameResolver ) { $this->betterStandardPrinter = $betterStandardPrinter; $this->constFetchManipulator = $constFetchManipulator; - $this->callableNodeTraverser = $callableNodeTraverser; $this->stmtsManipulator = $stmtsManipulator; $this->nameResolver = $nameResolver; } @@ -150,21 +126,6 @@ final class IfManipulator return $this->hasOnlyStmtOfType($if, If_::class); } - public function isEarlyElse(If_ $if): bool - { - if (! $this->isAlwaysAllowedType((array) $if->stmts, self::ALLOWED_BREAKING_NODE_TYPES)) { - return false; - } - - foreach ($if->elseifs as $elseif) { - if (! $this->isAlwaysAllowedType((array) $elseif->stmts, self::ALLOWED_BREAKING_NODE_TYPES)) { - return false; - } - } - - return $if->else !== null; - } - public function hasOnlyStmtOfType(If_ $if, string $desiredType): bool { if (count($if->stmts) !== 1) { @@ -283,57 +244,6 @@ final class IfManipulator return null; } - /** - * @param Node[] $stmts - * @param string[] $allowedTypes - */ - private function isAlwaysAllowedType(array $stmts, array $allowedTypes): bool - { - $isAlwaysReturnValue = false; - - $this->callableNodeTraverser->traverseNodesWithCallable($stmts, function (Node $node) use ( - &$isAlwaysReturnValue, - $allowedTypes - ) { - if ($this->isScopeChangingNode($node)) { - $isAlwaysReturnValue = false; - - return NodeTraverser::STOP_TRAVERSAL; - } - - foreach ($allowedTypes as $allowedType) { - if (is_a($node, $allowedType, true)) { - if ($allowedType === Return_::class) { - if ($node->expr === null) { - $isAlwaysReturnValue = false; - - return NodeTraverser::STOP_TRAVERSAL; - } - } - - $isAlwaysReturnValue = true; - } - } - - return null; - }); - - return $isAlwaysReturnValue; - } - - private function isScopeChangingNode(Node $node): bool - { - foreach (self::SCOPE_CHANGING_NODE_TYPES as $scopeChangingNode) { - if (! is_a($node, $scopeChangingNode, true)) { - continue; - } - - return true; - } - - return false; - } - private function isIfWithoutElseAndElseIfs(If_ $if): bool { if ($if->else !== null) {