From 21470736318dd5175c17b47e1f311cffcf202e57 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 30 Aug 2026 00:03:19 +0200 Subject: [PATCH] [dx] Deprecate SimplifyIfElseToTernaryRector The result depends on context and can worsen readability; extracting a method is a better fix when needed. Replace the body with a deprecation message, remove its tests, and unregister it from the if set. Claude-Session: https://claude.ai/code/session_012HQ19gVsT8wVkekuVqVXGx --- config/set/if.php | 2 - .../Fixture/fixture.php.inc | 31 ---- .../Fixture/keep.php.inc | 33 ---- .../Fixture/keep_nested_ternary.php.inc | 21 --- .../Fixture/mirror_comment.php.inc | 33 ---- .../Fixture/operator_precedence.php.inc | 37 ----- .../Fixture/skip_too_long.php.inc | 18 --- .../Fixture/skip_with_comment_inside.php.inc | 17 -- ...use_on_return_after_if_else_assign.php.inc | 35 ---- .../Fixture/with_assign.php.inc | 31 ---- .../SimplifyIfElseToTernaryRectorTest.php | 28 ---- .../config/configured_rule.php | 9 -- .../If_/SimplifyIfElseToTernaryRector.php | 150 +----------------- ...use_on_return_after_if_else_assign.php.inc | 41 ----- .../IfElseAssignReturnUsedTest.php | 28 ---- .../config/configured_rule.php | 10 -- .../Fixture/fixture.php.inc | 49 ------ .../KeepDoubleAssignParamTest.php | 28 ---- .../config/configured_rule.php | 10 -- .../Fixture/do_not_duplicated_expr.php.inc | 37 ----- .../SimplifyVariableIfElseTernaryTest.php | 28 ---- .../config/configured_rule.php | 17 -- 22 files changed, 8 insertions(+), 685 deletions(-) delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/fixture.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep_nested_ternary.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/mirror_comment.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/operator_precedence.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_too_long.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_with_comment_inside.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/use_on_return_after_if_else_assign.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/with_assign.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/SimplifyIfElseToTernaryRectorTest.php delete mode 100644 rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/config/configured_rule.php delete mode 100644 tests/Issues/IfElseAssignReturnUsed/Fixture/use_on_return_after_if_else_assign.php.inc delete mode 100644 tests/Issues/IfElseAssignReturnUsed/IfElseAssignReturnUsedTest.php delete mode 100644 tests/Issues/IfElseAssignReturnUsed/config/configured_rule.php delete mode 100644 tests/Issues/KeepDoubleAssignParam/Fixture/fixture.php.inc delete mode 100644 tests/Issues/KeepDoubleAssignParam/KeepDoubleAssignParamTest.php delete mode 100644 tests/Issues/KeepDoubleAssignParam/config/configured_rule.php delete mode 100644 tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc delete mode 100644 tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.php delete mode 100644 tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php diff --git a/config/set/if.php b/config/set/if.php index 2ee951f8120..0a0c9db1adf 100644 --- a/config/set/if.php +++ b/config/set/if.php @@ -4,13 +4,11 @@ use Rector\CodeQuality\Rector\If_\CombineIfRector; use Rector\CodeQuality\Rector\If_\ExplicitBoolCompareRector; -use Rector\CodeQuality\Rector\If_\SimplifyIfElseToTernaryRector; use Rector\Config\RectorConfig; return static function (RectorConfig $rectorConfig): void { $rectorConfig->rules([ ExplicitBoolCompareRector::class, CombineIfRector::class, - SimplifyIfElseToTernaryRector::class, ]); }; diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/fixture.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/fixture.php.inc deleted file mode 100644 index aaa38954dbe..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/fixture.php.inc +++ /dev/null @@ -1,31 +0,0 @@ -arrayBuilt[][$key] = true; - } else { - $this->arrayBuilt[][$key] = $value; - } - } -} - -?> ------ -arrayBuilt[][$key] = empty($value) ? true : $value; - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep.php.inc deleted file mode 100644 index 4a57ed9655a..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep.php.inc +++ /dev/null @@ -1,33 +0,0 @@ -arrayBuilt[][$key] = true; - } else { - $this->arrayBuilt[][$key2] = $value; - } - - if (empty($value)) { - $this->arrayBuilt[][$key] = true; - } elseif (!empty($value)) { - $this->arrayBuilt[][$key] = $value; - } - - if (empty($value)) { - $this->arrayBuilt[][$key] = true; - } elseif (!empty($value)) { - $this->arrayBuilt[][$key] = $value; - } - - if (empty($value)) { - $name = true; - } else { - $surname = $value; - } - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep_nested_ternary.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep_nested_ternary.php.inc deleted file mode 100644 index a7c82da8bbd..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/keep_nested_ternary.php.inc +++ /dev/null @@ -1,21 +0,0 @@ -typeAnalyzer->isPhpReservedType($type) ? $type : '\\' . $type; - } else { - $type = (string) $type; - } - - if ($type ? true : false) { - $type = 'Hou'; - } else { - $type = (string) $type; - } - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/mirror_comment.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/mirror_comment.php.inc deleted file mode 100644 index 4c7ef292159..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/mirror_comment.php.inc +++ /dev/null @@ -1,33 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/operator_precedence.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/operator_precedence.php.inc deleted file mode 100644 index 0aa82d52ea6..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/operator_precedence.php.inc +++ /dev/null @@ -1,37 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_too_long.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_too_long.php.inc deleted file mode 100644 index df4d5edebed..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_too_long.php.inc +++ /dev/null @@ -1,18 +0,0 @@ -nodeTypeResolver->isStringyType($staticCall->args[1]->value)) { - $name = $this->nameResolver->isName( - $staticCall, - 'contains' - ) ? 'assertStringContainsString' : 'assertStringNotContainsString'; - } else { - $name = $this->nameResolver->isName($staticCall, 'contains') ? 'assertContains' : 'assertNotContains'; - } - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_with_comment_inside.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_with_comment_inside.php.inc deleted file mode 100644 index 93458b1a3e2..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/skip_with_comment_inside.php.inc +++ /dev/null @@ -1,17 +0,0 @@ -toRawArray(); - } else { - $properties = (array) $data; - } - - return $properties; - } -} - -?> ------ -toRawArray() : (array) $data; - - return $properties; - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/with_assign.php.inc b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/with_assign.php.inc deleted file mode 100644 index 91e2cbf49c2..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/Fixture/with_assign.php.inc +++ /dev/null @@ -1,31 +0,0 @@ -methodX()) { - $this->out = $a; - } else { - $this->out = $this->methodY(); - } - } -} - -?> ------ -out = ($a = $this->methodX()) ? $a : $this->methodY(); - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/SimplifyIfElseToTernaryRectorTest.php b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/SimplifyIfElseToTernaryRectorTest.php deleted file mode 100644 index e488ebb2b31..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/SimplifyIfElseToTernaryRectorTest.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_/SimplifyIfElseToTernaryRector/config/configured_rule.php b/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/config/configured_rule.php deleted file mode 100644 index eba00b24ee1..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector/config/configured_rule.php +++ /dev/null @@ -1,9 +0,0 @@ -withRules([SimplifyIfElseToTernaryRector::class]); diff --git a/rules/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector.php b/rules/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector.php index 808ac5d7136..cd2292e7440 100644 --- a/rules/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector.php +++ b/rules/CodeQuality/Rector/If_/SimplifyIfElseToTernaryRector.php @@ -5,34 +5,18 @@ namespace Rector\CodeQuality\Rector\If_; use PhpParser\Node; -use PhpParser\Node\Expr; -use PhpParser\Node\Expr\Assign; -use PhpParser\Node\Expr\BinaryOp; -use PhpParser\Node\Expr\Ternary; -use PhpParser\Node\Stmt; -use PhpParser\Node\Stmt\Else_; -use PhpParser\Node\Stmt\Expression; use PhpParser\Node\Stmt\If_; -use Rector\NodeTypeResolver\Node\AttributeKey; -use Rector\PhpParser\Node\BetterNodeFinder; -use Rector\PhpParser\Printer\BetterStandardPrinter; +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_\SimplifyIfElseToTernaryRector\SimplifyIfElseToTernaryRectorTest + * @deprecated This rule is deprecated, as the result depends on context and can worsen readability. Extract a method instead if needed. */ -final class SimplifyIfElseToTernaryRector extends AbstractRector +final class SimplifyIfElseToTernaryRector extends AbstractRector implements DeprecatedInterface { - private const int LINE_LENGTH_LIMIT = 120; - - public function __construct( - private readonly BetterStandardPrinter $betterStandardPrinter, - private readonly BetterNodeFinder $betterNodeFinder - ) { - } - public function getRuleDefinition(): RuleDefinition { return new RuleDefinition( @@ -80,127 +64,9 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - if (! $node->else instanceof Else_) { - return null; - } - - if ($node->elseifs !== []) { - return null; - } - - $ifAssignVarExpr = $this->resolveOnlyStmtAssignVar($node->stmts); - if (! $ifAssignVarExpr instanceof Expr) { - return null; - } - - $elseAssignExpr = $this->resolveOnlyStmtAssignVar($node->else->stmts); - if (! $elseAssignExpr instanceof Expr) { - return null; - } - - if (! $this->nodeComparator->areNodesEqual($ifAssignVarExpr, $elseAssignExpr)) { - return null; - } - - $ternaryIfExpr = $this->resolveOnlyStmtAssignExpr($node->stmts); - $expr = $this->resolveOnlyStmtAssignExpr($node->else->stmts); - if (! $ternaryIfExpr instanceof Expr) { - return null; - } - - if (! $expr instanceof Expr) { - return null; - } - - // has nested ternary → skip, it's super hard to read - if ($this->haveNestedTernary([$node->cond, $ternaryIfExpr, $expr])) { - return null; - } - - $ternary = new Ternary($node->cond, $ternaryIfExpr, $expr); - $assign = new Assign($ifAssignVarExpr, $ternary); - - // do not create super long lines - if ($this->isNodeTooLong($assign)) { - return null; - } - - if ($ternary->cond instanceof BinaryOp || $ternary->cond instanceof Assign) { - $ternary->cond->setAttribute(AttributeKey::ORIGINAL_NODE, null); - } - - $expression = new Expression($assign); - $this->mirrorComments($expression, $node); - - return $expression; - } - - /** - * @param Stmt[] $stmts - */ - private function resolveOnlyStmtAssignVar(array $stmts): ?Expr - { - if (count($stmts) !== 1) { - return null; - } - - $stmt = $stmts[0]; - if (! $stmt instanceof Expression) { - return null; - } - - $stmtExpr = $stmt->expr; - if (! $stmtExpr instanceof Assign) { - return null; - } - - return $stmtExpr->var; - } - - /** - * @param Stmt[] $stmts - */ - private function resolveOnlyStmtAssignExpr(array $stmts): ?Expr - { - if (count($stmts) !== 1) { - return null; - } - - $stmt = $stmts[0]; - if (! $stmt instanceof Expression) { - return null; - } - - if ($stmt->getComments() !== []) { - return null; - } - - $stmtExpr = $stmt->expr; - if (! $stmtExpr instanceof Assign) { - return null; - } - - return $stmtExpr->expr; - } - - /** - * @param Node[] $nodes - */ - private function haveNestedTernary(array $nodes): bool - { - foreach ($nodes as $node) { - $ternary = $this->betterNodeFinder->findFirstInstanceOf($node, Ternary::class); - if ($ternary instanceof Ternary) { - return true; - } - } - - return false; - } - - private function isNodeTooLong(Assign $assign): bool - { - $assignContent = $this->betterStandardPrinter->print($assign); - return strlen($assignContent) > self::LINE_LENGTH_LIMIT; + throw new ShouldNotHappenException(sprintf( + '"%s" rule is deprecated, as the result depends on context and can worsen readability; extract a method instead if needed', + self::class + )); } } diff --git a/tests/Issues/IfElseAssignReturnUsed/Fixture/use_on_return_after_if_else_assign.php.inc b/tests/Issues/IfElseAssignReturnUsed/Fixture/use_on_return_after_if_else_assign.php.inc deleted file mode 100644 index 626a775465c..00000000000 --- a/tests/Issues/IfElseAssignReturnUsed/Fixture/use_on_return_after_if_else_assign.php.inc +++ /dev/null @@ -1,41 +0,0 @@ -toRawArray(); - } else { - $properties = (array) $data; - } - - return $properties; - } -} - -?> ------ -toRawArray(); - } - - return (array) $data; - } -} - -?> diff --git a/tests/Issues/IfElseAssignReturnUsed/IfElseAssignReturnUsedTest.php b/tests/Issues/IfElseAssignReturnUsed/IfElseAssignReturnUsedTest.php deleted file mode 100644 index d00bd1bbc48..00000000000 --- a/tests/Issues/IfElseAssignReturnUsed/IfElseAssignReturnUsedTest.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/tests/Issues/IfElseAssignReturnUsed/config/configured_rule.php b/tests/Issues/IfElseAssignReturnUsed/config/configured_rule.php deleted file mode 100644 index 89b4036772d..00000000000 --- a/tests/Issues/IfElseAssignReturnUsed/config/configured_rule.php +++ /dev/null @@ -1,10 +0,0 @@ -withRules([ChangeIfElseValueAssignToEarlyReturnRector::class, SimplifyIfElseToTernaryRector::class]); diff --git a/tests/Issues/KeepDoubleAssignParam/Fixture/fixture.php.inc b/tests/Issues/KeepDoubleAssignParam/Fixture/fixture.php.inc deleted file mode 100644 index 1e994546ab5..00000000000 --- a/tests/Issues/KeepDoubleAssignParam/Fixture/fixture.php.inc +++ /dev/null @@ -1,49 +0,0 @@ -items = [$input]; - } else { - $this->items = $input; - } - - $this->items = $this->getItems(); - } - - public function getItems() - { - return sort($this->items); - } -} - -?> ------ -items = ! \is_array($input) ? [$input] : $input; - - $this->items = $this->getItems(); - } - - public function getItems() - { - return sort($this->items); - } -} - -?> diff --git a/tests/Issues/KeepDoubleAssignParam/KeepDoubleAssignParamTest.php b/tests/Issues/KeepDoubleAssignParam/KeepDoubleAssignParamTest.php deleted file mode 100644 index 4cbe0f9c96e..00000000000 --- a/tests/Issues/KeepDoubleAssignParam/KeepDoubleAssignParamTest.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/tests/Issues/KeepDoubleAssignParam/config/configured_rule.php b/tests/Issues/KeepDoubleAssignParam/config/configured_rule.php deleted file mode 100644 index 0c0f164e715..00000000000 --- a/tests/Issues/KeepDoubleAssignParam/config/configured_rule.php +++ /dev/null @@ -1,10 +0,0 @@ -withRules([RemoveDoubleAssignRector::class, SimplifyIfElseToTernaryRector::class]); diff --git a/tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc b/tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc deleted file mode 100644 index 97b1eb8d3fa..00000000000 --- a/tests/Issues/SimplifyVariableIfElseTernary/Fixture/do_not_duplicated_expr.php.inc +++ /dev/null @@ -1,37 +0,0 @@ - 0) { - $baz = 'a'; - } else { - $baz = 'b'; - } - - return $baz; - } -} - -?> ------ - 0 ? 'a' : 'b'; - } -} - -?> diff --git a/tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.php b/tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.php deleted file mode 100644 index ed5c1539eab..00000000000 --- a/tests/Issues/SimplifyVariableIfElseTernary/SimplifyVariableIfElseTernaryTest.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/tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php b/tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php deleted file mode 100644 index a49cb9a6ad7..00000000000 --- a/tests/Issues/SimplifyVariableIfElseTernary/config/configured_rule.php +++ /dev/null @@ -1,17 +0,0 @@ -withRules( - [ - SimplifyIfElseToTernaryRector::class, - SimplifyUselessVariableRector::class, - CompleteDynamicPropertiesRector::class, - ] - );