From 80afcf6df7943a93fa82511b5fc94b57d9286e1d Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 30 Aug 2026 00:04:27 +0200 Subject: [PATCH] [dx] Deprecate CombineIfRector Merging nested ifs can create much less readable code and depends on context. Replace the body with a deprecation message, remove its tests, and unregister it from the if set. Skip the type-perfect narrowing of the generic CommentsMerger::keepComments(), which only appears because this rule stopped calling it. Claude-Session: https://claude.ai/code/session_012HQ19gVsT8wVkekuVqVXGx --- config/set/if.php | 6 +- phpstan.neon | 5 + .../CombineIfRector/CombineIfRectorTest.php | 28 ------ .../CombineIfRector/Fixture/docblock.php.inc | 51 ---------- .../CombineIfRector/Fixture/fixture.php.inc | 33 ------- .../property_fetch_in_condition.php.inc | 33 ------- .../property_fetch_in_condition2.php.inc | 33 ------- .../Fixture/skip_child_else.php.inc | 17 ---- .../Fixture/skip_child_elseif.php.inc | 17 ---- .../Fixture/skip_more_statements.php.inc | 18 ---- .../Fixture/skip_nested_type.php.inc | 29 ------ .../Fixture/skip_parent_else.php.inc | 17 ---- .../Fixture/skip_parent_elseif.php.inc | 17 ---- .../Fixture/with_assign.php.inc | 33 ------- ...with_negation_binaryop_previous_if.php.inc | 31 ------ .../config/configured_rule.php | 9 -- .../Rector/If_/CombineIfRector.php | 98 ++----------------- 17 files changed, 15 insertions(+), 460 deletions(-) delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/CombineIfRectorTest.php delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/docblock.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/fixture.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition2.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_child_else.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_child_elseif.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_more_statements.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_nested_type.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_parent_else.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_parent_elseif.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/with_assign.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/with_negation_binaryop_previous_if.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/CombineIfRector/config/configured_rule.php diff --git a/config/set/if.php b/config/set/if.php index 43cab14516a..2faf066c142 100644 --- a/config/set/if.php +++ b/config/set/if.php @@ -2,11 +2,9 @@ declare(strict_types=1); -use Rector\CodeQuality\Rector\If_\CombineIfRector; use Rector\Config\RectorConfig; +// note: all if rules were moved to code quality and coding style sets, or deprecated return static function (RectorConfig $rectorConfig): void { - $rectorConfig->rules([ - CombineIfRector::class, - ]); + $rectorConfig->rules([]); }; diff --git a/phpstan.neon b/phpstan.neon index 6923a5ea250..43d0fb1241f 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -65,6 +65,11 @@ parameters: constants: true ignoreErrors: + # keepComments() is a generic helper; do not narrow it after the deprecated CombineIfRector stopped calling it + - + identifier: typePerfect.narrowPublicClassMethodParamType + path: src/BetterPhpDocParser/Comment/CommentsMerger.php + # the deprecated set objects are still resolved internally, until every extension bonds its rules - message: '#deprecated (class|interface) Rector\\(Set|Bridge)\\#' diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/CombineIfRectorTest.php b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/CombineIfRectorTest.php deleted file mode 100644 index eee478ed119..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/CombineIfRectorTest.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/CodeQuality/Rector/If_/CombineIfRector/Fixture/docblock.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/docblock.php.inc deleted file mode 100644 index f48889279e1..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/docblock.php.inc +++ /dev/null @@ -1,51 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/fixture.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/fixture.php.inc deleted file mode 100644 index 36c4568c8bd..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/fixture.php.inc +++ /dev/null @@ -1,33 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition.php.inc deleted file mode 100644 index d71f39ff905..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition.php.inc +++ /dev/null @@ -1,33 +0,0 @@ -art->netzid > 0 && $artzo_list = $this->artzo_list) { - if ($artzo_list !== []) { - foreach ($artzo_list as $art) { - } - } - } - } -} -?> ------ -art->netzid > 0 && ($artzo_list = $this->artzo_list) && $artzo_list !== []) { - foreach ($artzo_list as $art) { - } - } - } -} -?> diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition2.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition2.php.inc deleted file mode 100644 index f7b54d84f66..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/property_fetch_in_condition2.php.inc +++ /dev/null @@ -1,33 +0,0 @@ -art->netzid > 0 && $artzo_list = $this->artzo_list) { - foreach ($artzo_list as $art) { - } - } - } - } -} -?> ------ -art->netzid > 0 && $artzo_list = $this->artzo_list)) { - foreach ($artzo_list as $art) { - } - } - } -} -?> diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_child_else.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_child_else.php.inc deleted file mode 100644 index 800101e9466..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_child_else.php.inc +++ /dev/null @@ -1,17 +0,0 @@ - diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_nested_type.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_nested_type.php.inc deleted file mode 100644 index 8c7ada706da..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_nested_type.php.inc +++ /dev/null @@ -1,29 +0,0 @@ -isAssign($expr)) { - /** @var Assign $expr */ - if ($expr->var) { - return true; - } - } - - return false; - } - - private function isAssign($expr) - { - if ($expr instanceof Assign) { - return true; - } - - return false; - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_parent_else.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_parent_else.php.inc deleted file mode 100644 index 0220ad6ee04..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/skip_parent_else.php.inc +++ /dev/null @@ -1,17 +0,0 @@ -getCond2Value()) === null) { - return 'foo'; - } - } - } -} - -?> ------ -getCond2Value()) === null) { - return 'foo'; - } - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/with_negation_binaryop_previous_if.php.inc b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/with_negation_binaryop_previous_if.php.inc deleted file mode 100644 index 920b5b7795a..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/Fixture/with_negation_binaryop_previous_if.php.inc +++ /dev/null @@ -1,31 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/config/configured_rule.php b/rules-tests/CodeQuality/Rector/If_/CombineIfRector/config/configured_rule.php deleted file mode 100644 index 0357f4362ca..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/CombineIfRector/config/configured_rule.php +++ /dev/null @@ -1,9 +0,0 @@ -withRules([CombineIfRector::class]); diff --git a/rules/CodeQuality/Rector/If_/CombineIfRector.php b/rules/CodeQuality/Rector/If_/CombineIfRector.php index b3e13d6f28c..6609a713e2c 100644 --- a/rules/CodeQuality/Rector/If_/CombineIfRector.php +++ b/rules/CodeQuality/Rector/If_/CombineIfRector.php @@ -5,31 +5,18 @@ namespace Rector\CodeQuality\Rector\If_; use PhpParser\Node; -use PhpParser\Node\Expr\BinaryOp; -use PhpParser\Node\Expr\BinaryOp\BooleanAnd; -use PhpParser\Node\Expr\BooleanNot; -use PhpParser\Node\Stmt\Else_; use PhpParser\Node\Stmt\If_; -use PHPStan\PhpDocParser\Ast\PhpDoc\VarTagValueNode; -use Rector\BetterPhpDocParser\Comment\CommentsMerger; -use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfo; -use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory; -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\CodeQuality\Rector\If_\CombineIfRector\CombineIfRectorTest + * @deprecated This rule is deprecated, as merging nested ifs can create much less readable code and depends on context. */ -final class CombineIfRector extends AbstractRector +final class CombineIfRector extends AbstractRector implements DeprecatedInterface { - public function __construct( - private readonly CommentsMerger $commentsMerger, - private readonly PhpDocInfoFactory $phpDocInfoFactory - ) { - } - public function getRuleDefinition(): RuleDefinition { return new RuleDefinition('Merge nested if statements', [ @@ -76,78 +63,9 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - if ($this->shouldSkip($node)) { - return null; - } - - /** @var If_ $subIf */ - $subIf = $node->stmts[0]; - - if ($this->hasVarTag($subIf)) { - return null; - } - - $node->cond->setAttribute(AttributeKey::ORIGINAL_NODE, null); - - $cond = $node->cond; - - while ($cond instanceof BinaryOp) { - if (! $cond->right instanceof BinaryOp) { - $cond->right->setAttribute(AttributeKey::ORIGINAL_NODE, null); - } - - $cond = $cond->right; - } - - if ($subIf->cond instanceof BinaryOp && ! $subIf->cond->left instanceof BinaryOp) { - $subIf->cond->left->setAttribute(AttributeKey::ORIGINAL_NODE, null); - } - - if ($node->cond instanceof BooleanNot && $node->cond->expr instanceof BinaryOp) { - $node->cond->expr->setAttribute(AttributeKey::ORIGINAL_NODE, null); - } - - $node->cond = new BooleanAnd($node->cond, $subIf->cond); - - $node->stmts = $subIf->stmts; - - $this->commentsMerger->keepComments($node, [$subIf]); - - return $node; - } - - private function shouldSkip(If_ $if): bool - { - if ($if->else instanceof Else_) { - return true; - } - - if (count($if->stmts) !== 1) { - return true; - } - - if ($if->elseifs !== []) { - return true; - } - - if (! $if->stmts[0] instanceof If_) { - return true; - } - - if ($if->stmts[0]->else instanceof Else_) { - return true; - } - - return (bool) $if->stmts[0]->elseifs; - } - - private function hasVarTag(If_ $if): bool - { - $subIfPhpDocInfo = $this->phpDocInfoFactory->createFromNode($if); - if (! $subIfPhpDocInfo instanceof PhpDocInfo) { - return false; - } - - return $subIfPhpDocInfo->getVarTagValueNode() instanceof VarTagValueNode; + throw new ShouldNotHappenException(sprintf( + '"%s" rule is deprecated, as merging nested ifs can create much less readable code and depends on context', + self::class + )); } }