From cd991ab36e357e9ab59a77348e759528d89baa49 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Thu, 17 Sep 2026 23:37:59 +0200 Subject: [PATCH 1/2] [DeadCode] Add RemoveOverriddenAssignBeforeIfElseRector --- .../Fixture/override_in_both_branches.php.inc | 64 ++++++ .../skip_branch_without_override.php.inc | 18 ++ .../Fixture/skip_cond_uses_variable.php.inc | 18 ++ .../Fixture/skip_no_else.php.inc | 16 ++ .../Fixture/skip_partial_override.php.inc | 18 ++ .../Fixture/skip_read_in_override.php.inc | 18 ++ .../Fixture/skip_side_effect_initial.php.inc | 25 +++ ...OverriddenAssignBeforeIfElseRectorTest.php | 28 +++ .../config/configured_rule.php | 9 + ...moveOverriddenAssignBeforeIfElseRector.php | 212 ++++++++++++++++++ src/Config/Level/DeadCodeLevel.php | 2 + 11 files changed, 428 insertions(+) create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/override_in_both_branches.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_branch_without_override.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_cond_uses_variable.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_no_else.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_partial_override.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_read_in_override.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_side_effect_initial.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/RemoveOverriddenAssignBeforeIfElseRectorTest.php create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/config/configured_rule.php create mode 100644 rules/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector.php diff --git a/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/override_in_both_branches.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/override_in_both_branches.php.inc new file mode 100644 index 00000000000..7c6b169dd79 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/override_in_both_branches.php.inc @@ -0,0 +1,64 @@ +getDefaults(); + foreach ($settings as $key => $value) { + if (array_key_exists($key, $present)) { + unset($settings[$key]); + } + } + + $settings['enabled'] = 'yes'; + } else { + $settings = ['enabled' => 'no', 'id' => $id]; + } + + return $settings; + } + + private function getDefaults(): array + { + return []; + } +} + +?> +----- +getDefaults(); + foreach ($settings as $key => $value) { + if (array_key_exists($key, $present)) { + unset($settings[$key]); + } + } + + $settings['enabled'] = 'yes'; + } else { + $settings = ['enabled' => 'no', 'id' => $id]; + } + + return $settings; + } + + private function getDefaults(): array + { + return []; + } +} + +?> diff --git a/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_branch_without_override.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_branch_without_override.php.inc new file mode 100644 index 00000000000..c87db09e9a7 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_branch_without_override.php.inc @@ -0,0 +1,18 @@ +init(); + if ($value) { + $result = [1, 2, 3]; + } else { + $result = [4, 5, 6]; + } + + return $result; + } + + private function init(): array + { + echo 'side effect'; + + return []; + } +} diff --git a/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/RemoveOverriddenAssignBeforeIfElseRectorTest.php b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/RemoveOverriddenAssignBeforeIfElseRectorTest.php new file mode 100644 index 00000000000..3fe9d05da65 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/RemoveOverriddenAssignBeforeIfElseRectorTest.php @@ -0,0 +1,28 @@ +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/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/config/configured_rule.php b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/config/configured_rule.php new file mode 100644 index 00000000000..57711977f6e --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/config/configured_rule.php @@ -0,0 +1,9 @@ +withRules([RemoveOverriddenAssignBeforeIfElseRector::class]); diff --git a/rules/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector.php b/rules/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector.php new file mode 100644 index 00000000000..89aef926ee5 --- /dev/null +++ b/rules/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector.php @@ -0,0 +1,212 @@ +> + */ + public function getNodeTypes(): array + { + return NodeGroup::STMTS_AWARE; + } + + /** + * @param StmtsAware $node + * @return StmtsAware|null + */ + public function refactor(Node $node): ?Node + { + if ($node->stmts === null) { + return null; + } + + $hasChanged = false; + + foreach ($node->stmts as $key => $stmt) { + $variableName = $this->matchOverridableAssignVariableName($stmt); + if ($variableName === null) { + continue; + } + + $nextStmt = $node->stmts[$key + 1] ?? null; + if (! $nextStmt instanceof If_) { + continue; + } + + if (! $nextStmt->else instanceof Else_ || $nextStmt->elseifs !== []) { + continue; + } + + // the initial value is still read when the condition uses it + if ($this->isVariableUsedInNode($nextStmt->cond, $variableName)) { + continue; + } + + if (! $this->isOverriddenFirstInBranch($nextStmt->stmts, $variableName)) { + continue; + } + + if (! $this->isOverriddenFirstInBranch($nextStmt->else->stmts, $variableName)) { + continue; + } + + unset($node->stmts[$key]); + $hasChanged = true; + } + + if ($hasChanged) { + return $node; + } + + return null; + } + + private function matchOverridableAssignVariableName(Stmt $stmt): ?string + { + if (! $stmt instanceof Expression) { + return null; + } + + if (! $stmt->expr instanceof Assign) { + return null; + } + + $assign = $stmt->expr; + if (! $assign->var instanceof Variable) { + return null; + } + + // removing the assign must not drop a side effect + if ($this->sideEffectNodeDetector->detect($assign->expr)) { + return null; + } + + if ($this->variableAnalyzer->isStaticOrGlobal($assign->var)) { + return null; + } + + if ($this->variableAnalyzer->isUsedByReference($assign->var)) { + return null; + } + + $variableName = $this->getName($assign->var); + if (! is_string($variableName)) { + return null; + } + + if ($this->reservedKeywordAnalyzer->isNativeVariable($variableName)) { + return null; + } + + return $variableName; + } + + /** + * @param Stmt[] $stmts + */ + private function isOverriddenFirstInBranch(array $stmts, string $variableName): bool + { + foreach ($stmts as $stmt) { + if (! $this->isVariableUsedInNode($stmt, $variableName)) { + continue; + } + + // first statement touching the variable must fully override it + if (! $stmt instanceof Expression || ! $stmt->expr instanceof Assign) { + return false; + } + + $assign = $stmt->expr; + if (! $assign->var instanceof Variable || ! $this->isName($assign->var, $variableName)) { + return false; + } + + // e.g. $value = $value + 1 still reads the previous value + return ! $this->isVariableUsedInNode($assign->expr, $variableName); + } + + return false; + } + + private function isVariableUsedInNode(Node $node, string $variableName): bool + { + return (bool) $this->betterNodeFinder->findFirst( + $node, + fn (Node $subNode): bool => $subNode instanceof Variable && $this->isName($subNode, $variableName) + ); + } +} diff --git a/src/Config/Level/DeadCodeLevel.php b/src/Config/Level/DeadCodeLevel.php index 6a9a706dc04..4486a9f3b8a 100644 --- a/src/Config/Level/DeadCodeLevel.php +++ b/src/Config/Level/DeadCodeLevel.php @@ -55,6 +55,7 @@ use Rector\DeadCode\Rector\If_\RemoveAlwaysTrueIfConditionRector; use Rector\DeadCode\Rector\If_\RemoveDeadIfBlockRector; use Rector\DeadCode\Rector\If_\RemoveDeadInstanceOfRector; +use Rector\DeadCode\Rector\If_\RemoveOverriddenAssignBeforeIfElseRector; use Rector\DeadCode\Rector\If_\RemoveTypedPropertyDeadInstanceOfRector; use Rector\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector; use Rector\DeadCode\Rector\If_\SimplifyIfElseWithSameContentRector; @@ -108,6 +109,7 @@ final class DeadCodeLevel SimplifyMirrorAssignRector::class, RemoveDeadContinueRector::class, RemoveUnusedNonEmptyArrayBeforeForeachRector::class, + RemoveOverriddenAssignBeforeIfElseRector::class, RemoveNullPropertyInitializationRector::class, RemoveDefaultValueFromAssignedPropertyRector::class, RemoveUselessReturnExprInConstructRector::class, From bea1ef1103fea732d40e9cab3b7605378a40e63d Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Fri, 18 Sep 2026 00:04:06 +0200 Subject: [PATCH 2/2] guard compact/get_defined_vars/dynamic variable reads --- .../Fixture/skip_compact_reads_value.php.inc | 21 +++++++++++++++++++ ...moveOverriddenAssignBeforeIfElseRector.php | 18 ++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_compact_reads_value.php.inc diff --git a/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_compact_reads_value.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_compact_reads_value.php.inc new file mode 100644 index 00000000000..138200bde33 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveOverriddenAssignBeforeIfElseRector/Fixture/skip_compact_reads_value.php.inc @@ -0,0 +1,21 @@ +shouldSkipIf($nextStmt)) { + continue; + } + if (! $this->isOverriddenFirstInBranch($nextStmt->stmts, $variableName)) { continue; } @@ -202,6 +208,18 @@ private function isOverriddenFirstInBranch(array $stmts, string $variableName): return false; } + private function shouldSkipIf(If_ $if): bool + { + return (bool) $this->betterNodeFinder->findFirst($if, function (Node $subNode): bool { + if ($subNode instanceof FuncCall) { + return $this->isNames($subNode, ['compact', 'get_defined_vars', 'extract']); + } + + // dynamic variable access like $$name can read the value by name + return $subNode instanceof Variable && ! is_string($subNode->name); + }); + } + private function isVariableUsedInNode(Node $node, string $variableName): bool { return (bool) $this->betterNodeFinder->findFirst(