From a00c9754022f4229f1e07c88e27eade90470a1cc Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 21 May 2023 16:42:31 +0200 Subject: [PATCH 1/2] Remove NEXT node dependency in ChangeAndIfToEarlyReturnRector, narrow to only closed scope function likes --- packages/NodeNestingScope/ContextAnalyzer.php | 36 ---------- phpstan.neon | 1 + .../assign_in_loop_with_continue.php.inc | 59 +++++++++++++++++ .../Fixture/non_first_level_if.php.inc | 39 ----------- .../Fixture/return_parent_next.php.inc | 65 ------------------- ...ssign_in_loop_with_indirect_return.php.inc | 27 -------- .../If_/ChangeAndIfToEarlyReturnRector.php | 39 +++++------ .../NodeResolver/SwitchExprsResolver.php | 10 ++- 8 files changed, 84 insertions(+), 192 deletions(-) create mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/assign_in_loop_with_continue.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/non_first_level_if.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/return_parent_next.php.inc delete mode 100644 rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/skip_assign_in_loop_with_indirect_return.php.inc diff --git a/packages/NodeNestingScope/ContextAnalyzer.php b/packages/NodeNestingScope/ContextAnalyzer.php index 1f2c70ab173..1513cb5c8db 100644 --- a/packages/NodeNestingScope/ContextAnalyzer.php +++ b/packages/NodeNestingScope/ContextAnalyzer.php @@ -5,18 +5,12 @@ namespace Rector\NodeNestingScope; use PhpParser\Node; -use PhpParser\Node\Expr; -use PhpParser\Node\Expr\Assign; use PhpParser\Node\FunctionLike; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\If_; -use PhpParser\Node\Stmt\Return_; use PhpParser\Node\Stmt\Switch_; -use PHPStan\Type\ObjectType; use Rector\Core\PhpParser\Node\BetterNodeFinder; use Rector\NodeNestingScope\ValueObject\ControlStructure; -use Rector\NodeTypeResolver\Node\AttributeKey; -use Rector\NodeTypeResolver\NodeTypeResolver; final class ContextAnalyzer { @@ -28,7 +22,6 @@ final class ContextAnalyzer public function __construct( private readonly BetterNodeFinder $betterNodeFinder, - private readonly NodeTypeResolver $nodeTypeResolver, ) { } @@ -71,33 +64,4 @@ public function isInIf(Node $node): bool return $previousNode instanceof If_; } - - public function hasAssignWithIndirectReturn(Node $node, If_ $if): bool - { - foreach (ControlStructure::LOOP_NODES as $loopNode) { - $loopObjectType = new ObjectType($loopNode); - $parentType = $this->nodeTypeResolver->getType($node); - - $superType = $parentType->isSuperTypeOf($loopObjectType); - if (! $superType->yes()) { - continue; - } - - $nextNode = $node->getAttribute(AttributeKey::NEXT_NODE); - if ($nextNode instanceof Node) { - if ($nextNode instanceof Return_ && ! $nextNode->expr instanceof Expr) { - continue; - } - - $hasAssign = (bool) $this->betterNodeFinder->findFirstInstanceOf($if->stmts, Assign::class); - if (! $hasAssign) { - continue; - } - - return true; - } - } - - return false; - } } diff --git a/phpstan.neon b/phpstan.neon index 928d5ab580a..5387c969fcc 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -766,3 +766,4 @@ parameters: - '#Anonymous variable in a `\$stmtsAware\->stmts\[\$key\]\->\.\.\.\(\)` method call can lead to false dead methods\. Make sure the variable type is known#' - '#Cognitive complexity for "Rector\\TypeDeclaration\\Rector\\ClassMethod\\BoolReturnTypeFromStrictScalarReturnsRector\:\:hasOnlyBoolScalarReturnExprs\(\)" is 11, keep it under 10#' + - '#Access to an undefined property PhpParser\\Node\\Stmt\\ClassLike\|PhpParser\\Node\\Stmt\\Declare_\|Rector\\Core\\Contract\\PhpParser\\Node\\StmtsAwareInterface\:\:\$stmts#' diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/assign_in_loop_with_continue.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/assign_in_loop_with_continue.php.inc new file mode 100644 index 00000000000..a42dabfeb22 --- /dev/null +++ b/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/assign_in_loop_with_continue.php.inc @@ -0,0 +1,59 @@ +specs as $key => $pdf) { + if ($this->shouldHaveArtwork($key) && !$this->hasArtwork($key)) { + $status = Release::ARTWORK_STATUS_INCOMPLETE; + } + } + + if ($this->artwork_status != $status) { + $this->artwork_status = $status; + $this->artwork_status_message = null; + } + + return $this; + } +} + +?> +----- +specs as $key => $pdf) { + if (!$this->shouldHaveArtwork($key)) { + continue; + } + if ($this->hasArtwork($key)) { + continue; + } + $status = Release::ARTWORK_STATUS_INCOMPLETE; + } + + if ($this->artwork_status != $status) { + $this->artwork_status = $status; + $this->artwork_status_message = null; + } + + return $this; + } +} + +?> diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/non_first_level_if.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/non_first_level_if.php.inc deleted file mode 100644 index bff8ac5b334..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/non_first_level_if.php.inc +++ /dev/null @@ -1,39 +0,0 @@ -wheelsCount() > 2) { - if ($car->hasWheels && $car->hasFuel) { - $this->canDrive = true; - } - } - } -} - -?> ------ -wheelsCount() > 2) { - if (!$car->hasWheels) { - return; - } - if (!$car->hasFuel) { - return; - } - $this->canDrive = true; - } - } -} - -?> diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/return_parent_next.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/return_parent_next.php.inc deleted file mode 100644 index c01c1fa1bb7..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/return_parent_next.php.inc +++ /dev/null @@ -1,65 +0,0 @@ -hasArg('widget') && $this->getArg('widget')) { - - } else { - if ($value && $this->hasArg('output') && 'id' != $this->getArg('output')) { - $value = rex_getUrl($value); - } - } - return $value; - } -} - -abstract class rex_var { - /** - * Returns the output. - * - * @return false|string - */ - abstract protected function getOutput(); -} -?> ------ -hasArg('widget') && $this->getArg('widget')) { - - } else { - if (!$value) { - return $value; - } - if (!$this->hasArg('output')) { - return $value; - } - if ('id' == $this->getArg('output')) { - return $value; - } - $value = rex_getUrl($value); - return $value; - } - return $value; - } -} - -abstract class rex_var { - /** - * Returns the output. - * - * @return false|string - */ - abstract protected function getOutput(); -} -?> diff --git a/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/skip_assign_in_loop_with_indirect_return.php.inc b/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/skip_assign_in_loop_with_indirect_return.php.inc deleted file mode 100644 index 945138078a2..00000000000 --- a/rules-tests/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector/Fixture/skip_assign_in_loop_with_indirect_return.php.inc +++ /dev/null @@ -1,27 +0,0 @@ -specs as $key => $pdf) { - if ($this->shouldHaveArtwork($key) && !$this->hasArtwork($key)) { - $status = Release::ARTWORK_STATUS_INCOMPLETE; - } - } - - if ($this->artwork_status != $status) { - $this->artwork_status = $status; - $this->artwork_status_message = null; - } - - return $this; - } -} - -?> diff --git a/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php b/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php index 98ce354ce81..367b74b906d 100644 --- a/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php +++ b/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php @@ -7,15 +7,21 @@ use PhpParser\Node; use PhpParser\Node\Expr; use PhpParser\Node\Expr\BinaryOp\BooleanAnd; +use PhpParser\Node\Expr\Closure; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\Break_; +use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Continue_; use PhpParser\Node\Stmt\Else_; use PhpParser\Node\Stmt\ElseIf_; +use PhpParser\Node\Stmt\Foreach_; +use PhpParser\Node\Stmt\Function_; use PhpParser\Node\Stmt\If_; +use PhpParser\Node\Stmt\Namespace_; use PhpParser\Node\Stmt\Return_; use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface; use Rector\Core\NodeManipulator\IfManipulator; +use Rector\Core\PhpParser\Node\CustomNode\FileWithoutNamespace; use Rector\Core\Rector\AbstractRector; use Rector\EarlyReturn\NodeAnalyzer\IfAndAnalyzer; use Rector\EarlyReturn\NodeAnalyzer\SimpleScalarAnalyzer; @@ -86,11 +92,18 @@ public function canDrive(Car $car) */ public function getNodeTypes(): array { - return [StmtsAwareInterface::class]; + return [ + ClassMethod::class, + Function_::class, + Foreach_::class, + Closure::class, + FileWithoutNamespace::class, + Namespace_::class, + ]; } /** - * @param StmtsAwareInterface $node + * @param Stmt\ClassMethod|Stmt\Function_|Stmt\Foreach_|Expr\Closure|FileWithoutNamespace|Stmt\Namespace_ $node */ public function refactor(Node $node): ?Node { @@ -229,10 +242,6 @@ private function shouldSkip(StmtsAwareInterface $stmtsAware, If_ $if, ?Stmt $nex return true; } - if ($this->isParentIfReturnsVoidOrParentIfHasNextNode($stmtsAware)) { - return true; - } - if ($this->isNestedIfInLoop($if, $stmtsAware)) { return true; } @@ -252,22 +261,6 @@ private function shouldSkip(StmtsAwareInterface $stmtsAware, If_ $if, ?Stmt $nex return ! $this->isLastIfOrBeforeLastReturn($if, $nexStmt); } - private function isParentIfReturnsVoidOrParentIfHasNextNode(StmtsAwareInterface $stmtsAware): bool - { - if (! $stmtsAware instanceof If_) { - $parent = $stmtsAware->getAttribute(AttributeKey::PARENT_NODE); - if ($parent instanceof If_) { - $node = $parent->getAttribute(AttributeKey::NEXT_NODE); - return ! $node instanceof Return_; - } - - return false; - } - - $nextNode = $stmtsAware->getAttribute(AttributeKey::NEXT_NODE); - return $nextNode instanceof Node; - } - private function isNestedIfInLoop(If_ $if, StmtsAwareInterface $stmtsAware): bool { if (! $this->contextAnalyzer->isInLoop($if)) { @@ -292,6 +285,6 @@ private function isLastIfOrBeforeLastReturn(If_ $if, ?Stmt $nextStmt): bool return $this->isLastIfOrBeforeLastReturn($parentNode, $nextStmt); } - return ! $this->contextAnalyzer->hasAssignWithIndirectReturn($parentNode, $if); + return true; } } diff --git a/rules/Php80/NodeResolver/SwitchExprsResolver.php b/rules/Php80/NodeResolver/SwitchExprsResolver.php index b31ba74f7b0..c2b170ed1aa 100644 --- a/rules/Php80/NodeResolver/SwitchExprsResolver.php +++ b/rules/Php80/NodeResolver/SwitchExprsResolver.php @@ -36,9 +36,15 @@ public function resolve(Switch_ $switch): array return []; } - if ($case->stmts === [] && $case->cond instanceof Expr) { - $collectionEmptyCasesCond[$key] = $case->cond; + if ($case->stmts !== []) { + continue; } + + if (!$case->cond instanceof Expr) { + continue; + } + + $collectionEmptyCasesCond[$key] = $case->cond; } foreach ($newSwitch->cases as $key => $case) { From 1e23baabe153f709bd6022d8a1f666d74948b655 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 21 May 2023 16:04:34 +0100 Subject: [PATCH 2/2] Update rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php Co-authored-by: Abdul Malik Ikhsan --- .../If_/ChangeAndIfToEarlyReturnRector.php | 20 ++----------------- .../NodeResolver/SwitchExprsResolver.php | 2 +- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php b/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php index 367b74b906d..0bc3fed3854 100644 --- a/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php +++ b/rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php @@ -12,14 +12,11 @@ use PhpParser\Node\Stmt\Break_; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Continue_; -use PhpParser\Node\Stmt\Else_; -use PhpParser\Node\Stmt\ElseIf_; use PhpParser\Node\Stmt\Foreach_; use PhpParser\Node\Stmt\Function_; use PhpParser\Node\Stmt\If_; use PhpParser\Node\Stmt\Namespace_; use PhpParser\Node\Stmt\Return_; -use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface; use Rector\Core\NodeManipulator\IfManipulator; use Rector\Core\PhpParser\Node\CustomNode\FileWithoutNamespace; use Rector\Core\Rector\AbstractRector; @@ -123,7 +120,7 @@ public function refactor(Node $node): ?Node $nextStmt = $stmts[$key + 1] ?? null; - if ($this->shouldSkip($node, $stmt, $nextStmt)) { + if ($this->shouldSkip($stmt, $nextStmt)) { $newStmts[] = $stmt; continue; } @@ -228,7 +225,7 @@ private function processReplaceIfs( return array_merge($result, [$ifNextReturnClone]); } - private function shouldSkip(StmtsAwareInterface $stmtsAware, If_ $if, ?Stmt $nexStmt): bool + private function shouldSkip(If_ $if, ?Stmt $nexStmt): bool { if (! $this->ifManipulator->isIfWithOnlyOneStmt($if)) { return true; @@ -242,10 +239,6 @@ private function shouldSkip(StmtsAwareInterface $stmtsAware, If_ $if, ?Stmt $nex return true; } - if ($this->isNestedIfInLoop($if, $stmtsAware)) { - return true; - } - // is simple return? skip it $onlyStmt = $if->stmts[0]; if ($onlyStmt instanceof Return_ && $onlyStmt->expr instanceof Expr && $this->simpleScalarAnalyzer->isSimpleScalar( @@ -261,15 +254,6 @@ private function shouldSkip(StmtsAwareInterface $stmtsAware, If_ $if, ?Stmt $nex return ! $this->isLastIfOrBeforeLastReturn($if, $nexStmt); } - private function isNestedIfInLoop(If_ $if, StmtsAwareInterface $stmtsAware): bool - { - if (! $this->contextAnalyzer->isInLoop($if)) { - return false; - } - - return $stmtsAware instanceof If_ || $stmtsAware instanceof Else_ || $stmtsAware instanceof ElseIf_; - } - private function isLastIfOrBeforeLastReturn(If_ $if, ?Stmt $nextStmt): bool { if ($nextStmt instanceof Node) { diff --git a/rules/Php80/NodeResolver/SwitchExprsResolver.php b/rules/Php80/NodeResolver/SwitchExprsResolver.php index c2b170ed1aa..2803be4aa5e 100644 --- a/rules/Php80/NodeResolver/SwitchExprsResolver.php +++ b/rules/Php80/NodeResolver/SwitchExprsResolver.php @@ -40,7 +40,7 @@ public function resolve(Switch_ $switch): array continue; } - if (!$case->cond instanceof Expr) { + if (! $case->cond instanceof Expr) { continue; }