From be554a4a53ac8c1b4c9bf68563b1b479a38f8122 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 25 Feb 2021 15:41:43 +0700 Subject: [PATCH] [EarlyReturn] Add ReturnBinaryOrToEarlyReturnRector (#5661) Co-authored-by: kaizen-ci --- .../ReturnBinaryAndToEarlyReturnRector.php | 65 +++---- .../ReturnBinaryOrToEarlyReturnRector.php | 160 ++++++++++++++++++ .../Fixture/comment.php.inc | 24 +-- .../dont_change_already_casted.php.inc | 12 +- .../dont_change_already_return_typed.php.inc | 50 ++++++ .../Fixture/fixture.php.inc | 10 +- .../Fixture/identical.php.inc | 10 +- .../Fixture/last_return_bool.php.inc | 18 +- .../Fixture/multiple_binary_and.php.inc | 14 +- .../Fixture/not_object_call_in_last.php.inc | 30 ++++ .../Fixture/skip_not_object_call.php.inc | 13 ++ .../Fixture/skip_or_in_next.php.inc | 4 +- .../Fixture/some_not_identical.php.inc | 10 +- .../Fixture/truthy_negation.php.inc | 10 +- .../Fixture/comment.php.inc | 44 +++++ .../dont_change_already_casted.php.inc | 30 ++++ .../dont_change_already_return_typed.php.inc | 50 ++++++ .../Fixture/fixture.php.inc | 30 ++++ .../Fixture/identical.php.inc | 30 ++++ .../Fixture/last_return_bool.php.inc | 53 ++++++ .../Fixture/multiple_binary_or.php.inc | 36 ++++ .../Fixture/not_object_call_in_last.php.inc | 30 ++++ .../Fixture/skip_and_in_next.php.inc | 13 ++ .../Fixture/skip_not_object_call.php.inc | 13 ++ .../Fixture/some_not_identical.php.inc | 30 ++++ .../Fixture/truthy_negation.php.inc | 30 ++++ .../ReturnBinaryOrToEarlyReturnRectorTest.php | 31 ++++ src/NodeAnalyzer/CallAnalyzer.php | 47 +++++ src/PhpParser/Node/AssignAndBinaryMap.php | 29 ++++ 29 files changed, 828 insertions(+), 98 deletions(-) create mode 100644 rules/early-return/src/Rector/Return_/ReturnBinaryOrToEarlyReturnRector.php create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_not_object_call.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/comment.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/fixture.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/identical.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/last_return_bool.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/multiple_binary_or.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_and_in_next.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_not_object_call.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/some_not_identical.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/truthy_negation.php.inc create mode 100644 rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/ReturnBinaryOrToEarlyReturnRectorTest.php create mode 100644 src/NodeAnalyzer/CallAnalyzer.php diff --git a/rules/early-return/src/Rector/Return_/ReturnBinaryAndToEarlyReturnRector.php b/rules/early-return/src/Rector/Return_/ReturnBinaryAndToEarlyReturnRector.php index e29d145f920..7ed2c6c42ce 100644 --- a/rules/early-return/src/Rector/Return_/ReturnBinaryAndToEarlyReturnRector.php +++ b/rules/early-return/src/Rector/Return_/ReturnBinaryAndToEarlyReturnRector.php @@ -7,15 +7,12 @@ namespace Rector\EarlyReturn\Rector\Return_; use PhpParser\Node; use PhpParser\Node\Expr; use PhpParser\Node\Expr\BinaryOp\BooleanAnd; -use PhpParser\Node\Expr\BooleanNot; -use PhpParser\Node\Expr\Cast\Bool_; use PhpParser\Node\Stmt\If_; use PhpParser\Node\Stmt\Return_; -use PHPStan\Analyser\Scope; -use PHPStan\Type\BooleanType; +use Rector\Core\NodeAnalyzer\CallAnalyzer; use Rector\Core\NodeManipulator\IfManipulator; +use Rector\Core\PhpParser\Node\AssignAndBinaryMap; use Rector\Core\Rector\AbstractRector; -use Rector\NodeTypeResolver\Node\AttributeKey; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; @@ -29,21 +26,33 @@ final class ReturnBinaryAndToEarlyReturnRector extends AbstractRector */ private $ifManipulator; - public function __construct(IfManipulator $ifManipulator) + /** + * @var AssignAndBinaryMap + */ + private $assignAndBinaryMap; + + /** + * @var CallAnalyzer + */ + private $callAnalyzer; + + public function __construct(IfManipulator $ifManipulator, AssignAndBinaryMap $assignAndBinaryMap, CallAnalyzer $callAnalyzer) { $this->ifManipulator = $ifManipulator; + $this->assignAndBinaryMap = $assignAndBinaryMap; + $this->callAnalyzer = $callAnalyzer; } public function getRuleDefinition(): RuleDefinition { - return new RuleDefinition('Changes Single return of && && to early returns', [ + return new RuleDefinition('Changes Single return of && to early returns', [ new CodeSample( <<<'CODE_SAMPLE' class SomeClass { - public function accept($something, $somethingelse) + public function accept() { - return $something && $somethingelse; + return $this->something() && $this->somethingelse(); } } CODE_SAMPLE @@ -52,12 +61,12 @@ CODE_SAMPLE <<<'CODE_SAMPLE' class SomeClass { - public function accept($something, $somethingelse) + public function accept() { - if (!$something) { + if (!$this->something()) { return false; } - return (bool) $somethingelse; + return (bool) $this->somethingelse(); } } CODE_SAMPLE @@ -85,15 +94,16 @@ CODE_SAMPLE $left = $node->expr->left; $ifNegations = $this->createMultipleIfsNegation($left, $node, []); - foreach ($ifNegations as $key => $ifNegation) { - if ($key === 0) { - $this->mirrorComments($ifNegation, $node); + $this->mirrorComments($ifNegations[0], $node); + foreach ($ifNegations as $ifNegation) { + if (! $this->callAnalyzer->isObjectCall($ifNegation->cond)) { + return null; } $this->addNodeBeforeNode($ifNegation, $node); } - $lastReturnExpr = $this->getLastReturnExpr($node->expr->right); + $lastReturnExpr = $this->assignAndBinaryMap->getTruthyExpr($node->expr->right); $this->addNodeBeforeNode(new Return_($lastReturnExpr), $node); $this->removeNode($node); @@ -120,29 +130,6 @@ CODE_SAMPLE ]; } - private function getLastReturnExpr(Expr $expr): Expr - { - if ($expr instanceof Bool_) { - return $expr; - } - - if ($expr instanceof BooleanNot) { - return $expr; - } - - $scope = $expr->getAttribute(AttributeKey::SCOPE); - if (! $scope instanceof Scope) { - return new Bool_($expr); - } - - $type = $scope->getType($expr); - if ($type instanceof BooleanType) { - return $expr; - } - - return new Bool_($expr); - } - /** * @param If_[] $ifNegations * @return If_[] diff --git a/rules/early-return/src/Rector/Return_/ReturnBinaryOrToEarlyReturnRector.php b/rules/early-return/src/Rector/Return_/ReturnBinaryOrToEarlyReturnRector.php new file mode 100644 index 00000000000..040bc1f0ade --- /dev/null +++ b/rules/early-return/src/Rector/Return_/ReturnBinaryOrToEarlyReturnRector.php @@ -0,0 +1,160 @@ +ifManipulator = $ifManipulator; + $this->assignAndBinaryMap = $assignAndBinaryMap; + $this->callAnalyzer = $callAnalyzer; + } + + public function getRuleDefinition(): RuleDefinition + { + return new RuleDefinition('Changes Single return of || to early returns', [ + new CodeSample( + <<<'CODE_SAMPLE' +class SomeClass +{ + public function accept() + { + return $this->something() || $this->somethingElse(); + } +} +CODE_SAMPLE + + , + <<<'CODE_SAMPLE' +class SomeClass +{ + public function accept() + { + if ($this->something()) { + return true; + } + return (bool) $this->somethingElse(); + } +} +CODE_SAMPLE + ), + ]); + } + + /** + * @return string[] + */ + public function getNodeTypes(): array + { + return [Return_::class]; + } + + /** + * @param Return_ $node + */ + public function refactor(Node $node): ?Node + { + if (! $node->expr instanceof BooleanOr) { + return null; + } + + $left = $node->expr->left; + $ifs = $this->createMultipleIfs($left, $node, []); + + if ($ifs === []) { + return null; + } + + $this->mirrorComments($ifs[0], $node); + foreach ($ifs as $if) { + if (! $this->callAnalyzer->isObjectCall($if->cond)) { + return null; + } + + $this->addNodeBeforeNode($if, $node); + } + + $lastReturnExpr = $this->assignAndBinaryMap->getTruthyExpr($node->expr->right); + $this->addNodeBeforeNode(new Return_($lastReturnExpr), $node); + $this->removeNode($node); + + return $node; + } + + /** + * @param If_[] $ifs + * @return If_[] + */ + private function createMultipleIfs(Expr $expr, Return_ $return, array $ifs): array + { + while ($expr instanceof BooleanOr) { + $ifs = array_merge($ifs, $this->collectLeftBooleanOrToIfs($expr, $return, $ifs)); + $ifs[] = $this->ifManipulator->createIfExpr( + $expr->right, + new Return_($this->nodeFactory->createTrue()) + ); + + $expr = $expr->right; + if ($expr instanceof BooleanAnd) { + return []; + } + if (! $expr instanceof BooleanOr) { + continue; + } + return []; + } + + return $ifs + [$this->ifManipulator->createIfExpr($expr, new Return_($this->nodeFactory->createTrue()))]; + } + + /** + * @param If_[] $ifs + * @return If_[] + */ + private function collectLeftBooleanOrToIfs(BooleanOr $BooleanOr, Return_ $return, array $ifs): array + { + $left = $BooleanOr->left; + if (! $left instanceof BooleanOr) { + return [$this->ifManipulator->createIfExpr($left, new Return_($this->nodeFactory->createTrue()))]; + } + + return $this->createMultipleIfs($left, $return, $ifs); + } +} diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/comment.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/comment.php.inc index 936e135af08..d0cd41df70e 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/comment.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/comment.php.inc @@ -4,16 +4,16 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class Comment { - public function accept($something, $somethingelse, $anotherelse, $last) + public function accept() { // a comment - if ($a) { + if (rand(0, 1)) { // another comment - return $something && $somethingelse && $anotherelse && $last; + return $this->something() && $this->somethingElse(); } // another next comment - return $a; + return 1; } } @@ -25,25 +25,19 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class Comment { - public function accept($something, $somethingelse, $anotherelse, $last) + public function accept() { // a comment - if ($a) { + if (rand(0, 1)) { // another comment - if (!$something) { + if (!$this->something()) { return false; } - if (!$somethingelse) { - return false; - } - if (!$anotherelse) { - return false; - } - return (bool) $last; + return (bool) $this->somethingElse(); } // another next comment - return $a; + return 1; } } diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc index f8181b73dfa..11c0a036d21 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc @@ -4,9 +4,9 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class DontChangeAlreadyCasted { - public function accept($something, $somethingelse) + public function accept() { - return $something && (bool) $somethingelse; + return $this->something() && (bool) $this->somethingelse(); } } @@ -18,13 +18,13 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class DontChangeAlreadyCasted { - public function accept($something, $somethingelse) + public function accept() { - if (!$something) { + if (!$this->something()) { return false; } - return (bool) $somethingelse; + return (bool) $this->somethingelse(); } } -?> +?> \ No newline at end of file diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc new file mode 100644 index 00000000000..c15a00624df --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc @@ -0,0 +1,50 @@ +something() && $this->somethingElse(); + } + + private function something(): bool + { + return true; + } + + private function somethingElse(): bool + { + return true; + } +} + +?> +----- +something()) { + return false; + } + return $this->somethingElse(); + } + + private function something(): bool + { + return true; + } + + private function somethingElse(): bool + { + return true; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/fixture.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/fixture.php.inc index 61aff6ea22a..8166d660031 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/fixture.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/fixture.php.inc @@ -4,9 +4,9 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class Fixture { - public function accept($something, $somethingelse) + public function accept() { - return $something && $somethingelse; + return $this->something() && $this->somethingElse(); } } @@ -18,12 +18,12 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class Fixture { - public function accept($something, $somethingelse) + public function accept() { - if (!$something) { + if (!$this->something()) { return false; } - return (bool) $somethingelse; + return (bool) $this->somethingElse(); } } diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/identical.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/identical.php.inc index bba2c518aac..3f9c44a61d3 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/identical.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/identical.php.inc @@ -4,9 +4,9 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class Identical { - public function accept($something, $somethingelse) + public function accept() { - return $something === 1 && !$somethingelse; + return $this->something() === 1 && !$this->somethingElse(); } } @@ -18,12 +18,12 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class Identical { - public function accept($something, $somethingelse) + public function accept() { - if ($something !== 1) { + if ($this->something() !== 1) { return false; } - return !$somethingelse; + return !$this->somethingElse(); } } diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/last_return_bool.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/last_return_bool.php.inc index 193d2736655..4c35616a36e 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/last_return_bool.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/last_return_bool.php.inc @@ -4,14 +4,14 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class LastReturnBool { - public function accept($something) + public function accept() { - return $something && $this->getSomethingElse(); + return $this->something() && $this->getSomethingElse(); } - public function accept2($something, $somethingelse) + public function accept2() { - return $something && $somethingelse === 'something else'; + return $this->something() && $this->somethingelse() === 'something else'; } private function getSomethingElse(): bool @@ -28,20 +28,20 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class LastReturnBool { - public function accept($something) + public function accept() { - if (!$something) { + if (!$this->something()) { return false; } return $this->getSomethingElse(); } - public function accept2($something, $somethingelse) + public function accept2() { - if (!$something) { + if (!$this->something()) { return false; } - return $somethingelse === 'something else'; + return $this->somethingelse() === 'something else'; } private function getSomethingElse(): bool diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/multiple_binary_and.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/multiple_binary_and.php.inc index 16302e13f96..6dabe019e36 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/multiple_binary_and.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/multiple_binary_and.php.inc @@ -4,9 +4,9 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class MultipleBinaryAnd { - public function accept($something, $somethingelse, $anotherelse, $last) + public function accept() { - return $something && $somethingelse && $anotherelse && $last; + return $this->something() && $this->somethingelse() && $this->anotherelse() && $this->last(); } } @@ -18,18 +18,18 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class MultipleBinaryAnd { - public function accept($something, $somethingelse, $anotherelse, $last) + public function accept() { - if (!$something) { + if (!$this->something()) { return false; } - if (!$somethingelse) { + if (!$this->somethingelse()) { return false; } - if (!$anotherelse) { + if (!$this->anotherelse()) { return false; } - return (bool) $last; + return (bool) $this->last(); } } diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc new file mode 100644 index 00000000000..2879dcac3cd --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc @@ -0,0 +1,30 @@ +something() && true; + } +} + +?> +----- +something()) { + return false; + } + return true; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_not_object_call.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_not_object_call.php.inc new file mode 100644 index 00000000000..473e08fb470 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_not_object_call.php.inc @@ -0,0 +1,13 @@ +something || $this->somethingelse; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_or_in_next.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_or_in_next.php.inc index bab4174cf20..2b5af79e8d3 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_or_in_next.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/skip_or_in_next.php.inc @@ -4,9 +4,9 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class SkipOrInNext { - public function accept($something, $somethingelse, $anotherelse, $last) + public function accept() { - return $something && $somethingelse || $anotherelse && $last; + return $this->something() && $this->somethingelse() || $this->anotherelse() && $this->last(); } } diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/some_not_identical.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/some_not_identical.php.inc index 2c69d79f187..54a867b020a 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/some_not_identical.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/some_not_identical.php.inc @@ -4,9 +4,9 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe final class SomeNotIdentical { - public function accept($something, $somethingelse) + public function accept() { - return $something !== 1 && !$somethingelse; + return $this->something() !== 1 && !$this->somethingelse(); } } @@ -18,12 +18,12 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe final class SomeNotIdentical { - public function accept($something, $somethingelse) + public function accept() { - if ($something === 1) { + if ($this->something() === 1) { return false; } - return !$somethingelse; + return !$this->somethingelse(); } } diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/truthy_negation.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/truthy_negation.php.inc index 9874242ad58..358f27027c1 100644 --- a/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/truthy_negation.php.inc +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryAndToEarlyReturnRector/Fixture/truthy_negation.php.inc @@ -4,9 +4,9 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class TruthyNegation { - public function accept($something, $somethingelse) + public function accept() { - return !$something && !$somethingelse; + return !$this->something() && !$this->somethingelse(); } } @@ -18,12 +18,12 @@ namespace Rector\EarlyReturn\Tests\Rector\Return_\ReturnBinaryAndToEarlyReturnRe class TruthyNegation { - public function accept($something, $somethingelse) + public function accept() { - if ($something) { + if ($this->something()) { return false; } - return !$somethingelse; + return !$this->somethingelse(); } } diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/comment.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/comment.php.inc new file mode 100644 index 00000000000..3c90253e8d4 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/comment.php.inc @@ -0,0 +1,44 @@ +something() || $this->somethingElse(); + } + + // another next comment + return 1; + } +} + +?> +----- +something()) { + return true; + } + return (bool) $this->somethingElse(); + } + + // another next comment + return 1; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc new file mode 100644 index 00000000000..3e5b684a520 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_casted.php.inc @@ -0,0 +1,30 @@ +something() || (bool) $this->somethingelse(); + } +} + +?> +----- +something()) { + return true; + } + return (bool) $this->somethingelse(); + } +} + +?> \ No newline at end of file diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc new file mode 100644 index 00000000000..edae7bef6a3 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/dont_change_already_return_typed.php.inc @@ -0,0 +1,50 @@ +something() || $this->somethingElse(); + } + + private function something(): bool + { + return true; + } + + private function somethingElse(): bool + { + return true; + } +} + +?> +----- +something()) { + return true; + } + return $this->somethingElse(); + } + + private function something(): bool + { + return true; + } + + private function somethingElse(): bool + { + return true; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/fixture.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/fixture.php.inc new file mode 100644 index 00000000000..99ee3dfb723 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/fixture.php.inc @@ -0,0 +1,30 @@ +something() || $this->somethingElse(); + } +} + +?> +----- +something()) { + return true; + } + return (bool) $this->somethingElse(); + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/identical.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/identical.php.inc new file mode 100644 index 00000000000..1a112b9a112 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/identical.php.inc @@ -0,0 +1,30 @@ +something() === 1 || !$this->somethingElse(); + } +} + +?> +----- +something() === 1) { + return true; + } + return !$this->somethingElse(); + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/last_return_bool.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/last_return_bool.php.inc new file mode 100644 index 00000000000..ddd502d5e8b --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/last_return_bool.php.inc @@ -0,0 +1,53 @@ +something() || $this->getSomethingElse(); + } + + public function accept2() + { + return $this->something() || $this->somethingelse() === 'something else'; + } + + private function getSomethingElse(): bool + { + return true; + } +} + +?> +----- +something()) { + return true; + } + return $this->getSomethingElse(); + } + + public function accept2() + { + if ($this->something()) { + return true; + } + return $this->somethingelse() === 'something else'; + } + + private function getSomethingElse(): bool + { + return true; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/multiple_binary_or.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/multiple_binary_or.php.inc new file mode 100644 index 00000000000..0f1d5298218 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/multiple_binary_or.php.inc @@ -0,0 +1,36 @@ +something() || $this->somethingelse() || $this->anotherelse() || $this->last(); + } +} + +?> +----- +something()) { + return true; + } + if ($this->somethingelse()) { + return true; + } + if ($this->anotherelse()) { + return true; + } + return (bool) $this->last(); + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc new file mode 100644 index 00000000000..5ae08f9e935 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/not_object_call_in_last.php.inc @@ -0,0 +1,30 @@ +something() || true; + } +} + +?> +----- +something()) { + return true; + } + return true; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_and_in_next.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_and_in_next.php.inc new file mode 100644 index 00000000000..896babad69a --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_and_in_next.php.inc @@ -0,0 +1,13 @@ +something() || $this->somethingelse() && $this->anotherelse() || $this->last(); + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_not_object_call.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_not_object_call.php.inc new file mode 100644 index 00000000000..2e9d5577034 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/skip_not_object_call.php.inc @@ -0,0 +1,13 @@ +something || $this->somethingelse; + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/some_not_identical.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/some_not_identical.php.inc new file mode 100644 index 00000000000..22646f10946 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/some_not_identical.php.inc @@ -0,0 +1,30 @@ +something() !== 1 || !$this->somethingelse(); + } +} + +?> +----- +something() !== 1) { + return true; + } + return !$this->somethingelse(); + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/truthy_negation.php.inc b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/truthy_negation.php.inc new file mode 100644 index 00000000000..df40e4e8634 --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/Fixture/truthy_negation.php.inc @@ -0,0 +1,30 @@ +something() || !$this->somethingelse(); + } +} + +?> +----- +something()) { + return true; + } + return !$this->somethingelse(); + } +} + +?> diff --git a/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/ReturnBinaryOrToEarlyReturnRectorTest.php b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/ReturnBinaryOrToEarlyReturnRectorTest.php new file mode 100644 index 00000000000..6a23280024a --- /dev/null +++ b/rules/early-return/tests/Rector/Return_/ReturnBinaryOrToEarlyReturnRector/ReturnBinaryOrToEarlyReturnRectorTest.php @@ -0,0 +1,31 @@ +doTestFileInfo($fileInfo); + } + + public function provideData(): Iterator + { + return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + protected function getRectorClass(): string + { + return ReturnBinaryOrToEarlyReturnRector::class; + } +} diff --git a/src/NodeAnalyzer/CallAnalyzer.php b/src/NodeAnalyzer/CallAnalyzer.php new file mode 100644 index 00000000000..f25660150cd --- /dev/null +++ b/src/NodeAnalyzer/CallAnalyzer.php @@ -0,0 +1,47 @@ +> + */ + private const OBJECT_CALLS = [ + MethodCall::class, + NullsafeMethodCall::class, + StaticCall::class + ]; + + public function isObjectCall(Node $node): bool + { + if ($node instanceof BooleanNot) { + $node = $node->expr; + } + + if ($node instanceof BinaryOp) { + $isObjectCallLeft = $this->isObjectCall($node->left); + $isObjectCallRight = $this->isObjectCall($node->right); + + return $isObjectCallLeft || $isObjectCallRight; + } + + foreach (self::OBJECT_CALLS as $objectCall) { + if (is_a($node, $objectCall, true)) { + return true; + } + } + + return false; + } +} diff --git a/src/PhpParser/Node/AssignAndBinaryMap.php b/src/PhpParser/Node/AssignAndBinaryMap.php index 64163b1a3ef..c449816a090 100644 --- a/src/PhpParser/Node/AssignAndBinaryMap.php +++ b/src/PhpParser/Node/AssignAndBinaryMap.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Rector\Core\PhpParser\Node; use PhpParser\Node; +use PhpParser\Node\Expr; use PhpParser\Node\Expr\AssignOp; use PhpParser\Node\Expr\AssignOp\BitwiseAnd as AssignBitwiseAnd; use PhpParser\Node\Expr\AssignOp\BitwiseOr as AssignBitwiseOr; @@ -39,6 +40,11 @@ use PhpParser\Node\Expr\BinaryOp\ShiftLeft; use PhpParser\Node\Expr\BinaryOp\ShiftRight; use PhpParser\Node\Expr\BinaryOp\Smaller; use PhpParser\Node\Expr\BinaryOp\SmallerOrEqual; +use PhpParser\Node\Expr\BooleanNot; +use PhpParser\Node\Expr\Cast\Bool_; +use PHPStan\Analyser\Scope; +use PHPStan\Type\BooleanType; +use Rector\NodeTypeResolver\Node\AttributeKey; final class AssignAndBinaryMap { @@ -104,4 +110,27 @@ final class AssignAndBinaryMap $nodeClass = get_class($binaryOp); return self::BINARY_OP_TO_INVERSE_CLASSES[$nodeClass] ?? null; } + + public function getTruthyExpr(Expr $expr): Expr + { + if ($expr instanceof Bool_) { + return $expr; + } + + if ($expr instanceof BooleanNot) { + return $expr; + } + + $scope = $expr->getAttribute(AttributeKey::SCOPE); + if (! $scope instanceof Scope) { + return new Bool_($expr); + } + + $type = $scope->getType($expr); + if ($type instanceof BooleanType) { + return $expr; + } + + return new Bool_($expr); + } }