From e5c94a20cf618a23b882329860a441a0d3eb4040 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Sat, 23 Apr 2022 06:00:40 +0700 Subject: [PATCH] [DeadCode] Handle skipped RemoveUnreachableStatementRector on MultiDirnameRector (#2129) * [DeadCode] Handle skipped RemoveUnreachableStatementRector on MultiDirnameRector * Fixed :tada: * Fixed :tada: * add fixture for has side effect not return early * final touch: first stmt is never removed * final touch: back start from - 2 as jump * really final touch: clean up * really final touch: comment * final touch: no - 1 means it already last, so skip early --- rector.php | 6 -- .../Fixture/side_effect_return_early.php.inc | 31 +++++++++ .../skip_side_effect_not_return_early.php.inc | 21 ++++++ .../Stmt/RemoveUnreachableStatementRector.php | 64 +++++++++++++++++-- 4 files changed, 111 insertions(+), 11 deletions(-) create mode 100644 rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/side_effect_return_early.php.inc create mode 100644 rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_side_effect_not_return_early.php.inc diff --git a/rector.php b/rector.php index 0d9c4719b8d..04aa04eb910 100644 --- a/rector.php +++ b/rector.php @@ -7,7 +7,6 @@ use Rector\CodingStyle\Rector\ClassMethod\ReturnArrayClassMethodToYieldRector; use Rector\CodingStyle\Rector\MethodCall\PreferThisOrSelfMethodCallRector; use Rector\CodingStyle\ValueObject\ReturnArrayClassMethodToYield; use Rector\Config\RectorConfig; -use Rector\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector; use Rector\Nette\Set\NetteSetList; use Rector\Php55\Rector\String_\StringClassNameToClassConstantRector; use Rector\Php81\Rector\Class_\MyCLabsClassToEnumRector; @@ -63,11 +62,6 @@ return static function (RectorConfig $rectorConfig): void { $rectorConfig->skip([ StringClassNameToClassConstantRector::class, - // phpstan false positive unreachable - RemoveUnreachableStatementRector::class => [ - __DIR__ . '/rules/Php70/Rector/FuncCall/MultiDirnameRector.php', - ], - FinalizeClassesWithoutChildrenRector::class => [ __DIR__ . '/rules/DowngradePhp74/Rector/Array_/DowngradeArraySpreadRector.php', ], diff --git a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/side_effect_return_early.php.inc b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/side_effect_return_early.php.inc new file mode 100644 index 00000000000..f8447e0b11e --- /dev/null +++ b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/side_effect_return_early.php.inc @@ -0,0 +1,31 @@ +sideEffect(); + return 5; + + $removeMe = 10; + } +} + +?> +----- +sideEffect(); + return 5; + } +} + +?> \ No newline at end of file diff --git a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_side_effect_not_return_early.php.inc b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_side_effect_not_return_early.php.inc new file mode 100644 index 00000000000..6bc8f7ae393 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_side_effect_not_return_early.php.inc @@ -0,0 +1,21 @@ +value = 1; + + if ($this->sideEffect()) { + + } + + if ($this->value === 1) { + return 'a'; + } + + return 'b'; + } +} diff --git a/rules/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector.php b/rules/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector.php index f41f1bc33b9..3c38d1431ad 100644 --- a/rules/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector.php +++ b/rules/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Rector\DeadCode\Rector\Stmt; use PhpParser\Node; +use PhpParser\Node\Expr\Exit_; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\FunctionLike; @@ -14,7 +15,10 @@ use PhpParser\Node\Stmt\Expression; use PhpParser\Node\Stmt\Foreach_; use PhpParser\Node\Stmt\If_; use PhpParser\Node\Stmt\Nop; +use PhpParser\Node\Stmt\Return_; +use PhpParser\Node\Stmt\Throw_; use Rector\Core\Rector\AbstractRector; +use Rector\DeadCode\SideEffect\SideEffectNodeDetector; use Rector\NodeTypeResolver\Node\AttributeKey; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; @@ -26,6 +30,10 @@ use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; */ final class RemoveUnreachableStatementRector extends AbstractRector { + public function __construct(private readonly SideEffectNodeDetector $sideEffectNodeDetector) + { + } + public function getRuleDefinition(): RuleDefinition { return new RuleDefinition('Remove unreachable statements', [ @@ -71,7 +79,7 @@ CODE_SAMPLE $stmts = $node->stmts; $isPassedTheUnreachable = false; - $hasChanged = false; + $toBeRemovedKeys = []; foreach ($stmts as $key => $stmt) { if ($this->shouldSkipNode($stmt)) { @@ -91,15 +99,61 @@ CODE_SAMPLE continue; } - unset($stmts[$key]); - $hasChanged = true; + $toBeRemovedKeys[] = $key; } - if (! $hasChanged) { + if ($toBeRemovedKeys === []) { return null; } - $node->stmts = $stmts; + $start = reset($toBeRemovedKeys); + if (! isset($stmts[$start - 1])) { + return null; + } + + $previousFirstUnreachable = $stmts[$start - 1]; + if (in_array($previousFirstUnreachable::class, [Throw_::class, Return_::class, Exit_::class], true)) { + return $this->processCleanUpUnreachabelStmts($node, $toBeRemovedKeys); + } + + // check previous side effect can check against start jump key - 2 + // as previously already checked as reachable part + if (! $this->hasPreviousSideEffect($start - 2, $stmts)) { + return $this->processCleanUpUnreachabelStmts($node, $toBeRemovedKeys); + } + + return null; + } + + /** + * @param Stmt[] $stmts + */ + private function hasPreviousSideEffect(int $start, array $stmts): bool + { + for ($key = $start; $key > 0; --$key) { + $previousStmt = $stmts[$key]; + $hasSideEffect = (bool) $this->betterNodeFinder->findFirst( + $previousStmt, + fn (Node $node): bool => $this->sideEffectNodeDetector->detectCallExpr($node) + ); + + if ($hasSideEffect) { + return true; + } + } + + return false; + } + + /** + * @param int[] $toBeRemovedKeys + */ + private function processCleanUpUnreachabelStmts(Foreach_|FunctionLike|Else_|If_ $node, array $toBeRemovedKeys): Node + { + foreach ($toBeRemovedKeys as $toBeRemovedKey) { + unset($node->stmts[$toBeRemovedKey]); + } + return $node; }