[SOLID] Add ChangeIfElseValueAssignToEarlyReturnRector

This commit is contained in:
TomasVotruba 2020-01-05 22:19:26 +01:00
parent 3159da6712
commit a38d61f00c
14 changed files with 529 additions and 25 deletions

View File

@ -9,13 +9,17 @@ Development of Rector is made possible thanks to these awesome backers! Would yo
Check out all the tiers - higher ones include additional goodies like placing the logo of your company in Rector's README or creating custom set for your needs.
- Jan Votruba
- Sebastian Schreiber
- 11com7
- Alexander Schnitzler
- Arnaud TIERANT
- Attila Fulop
- Jakob Oberhummer
- Jan Kuchař
- Jan Votruba
- Jan Mikes
- Arnaud TIERANT
- Ruud Boon
- thechris
- Toby
**Thank you for making this happen.**
@ -25,5 +29,6 @@ Check out all the tiers - higher ones include additional goodies like placing th
- Musement S.p.a.
- Sebastian Schreiber
- John Linhart
<!-- source: https://www.patreon.com/manageRewardsList -->

View File

@ -0,0 +1,127 @@
<?php
declare(strict_types=1);
namespace Rector\SOLID\Rector\If_;
use PhpParser\Node;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Stmt\If_;
use PhpParser\Node\Stmt\Return_;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\PhpParser\Node\Manipulator\IfManipulator;
use Rector\PhpParser\Node\Manipulator\StmtsManipulator;
use Rector\Rector\AbstractRector;
use Rector\RectorDefinition\CodeSample;
use Rector\RectorDefinition\RectorDefinition;
/**
* @see https://engineering.helpscout.com/reducing-complexity-with-guard-clauses-in-php-and-javascript-74600fd865c7
*
* @see \Rector\SOLID\Tests\Rector\If_\ChangeIfElseValueAssignToEarlyReturnRector\ChangeIfElseValueAssignToEarlyReturnRectorTest
*/
final class ChangeIfElseValueAssignToEarlyReturnRector extends AbstractRector
{
/**
* @var IfManipulator
*/
private $ifManipulator;
/**
* @var StmtsManipulator
*/
private $stmtsManipulator;
public function __construct(IfManipulator $ifManipulator, StmtsManipulator $stmtsManipulator)
{
$this->ifManipulator = $ifManipulator;
$this->stmtsManipulator = $stmtsManipulator;
}
public function getDefinition(): RectorDefinition
{
return new RectorDefinition('Change if/else value to early return', [
new CodeSample(
<<<'PHP'
class SomeClass
{
public function run()
{
if ($this->hasDocBlock($tokens, $index)) {
$docToken = $tokens[$this->getDocBlockIndex($tokens, $index)];
} else {
$docToken = null;
}
return $docToken;
}
}
PHP
,
<<<'PHP'
class SomeClass
{
public function run()
{
if ($this->hasDocBlock($tokens, $index)) {
return $tokens[$this->getDocBlockIndex($tokens, $index)];
}
return null;
}
}
PHP
),
]);
}
/**
* @return string[]
*/
public function getNodeTypes(): array
{
return [If_::class];
}
/**
* @param If_ $node
*/
public function refactor(Node $node): ?Node
{
$nextNode = $node->getAttribute(AttributeKey::NEXT_NODE);
if (! $nextNode instanceof Return_) {
return null;
}
if ($nextNode->expr === null) {
return null;
}
if (! $this->ifManipulator->isIfAndElseWithSameVariableAssignAsLastStmts($node, $nextNode->expr)) {
return null;
}
/** @var Assign $assign */
$assign = $this->stmtsManipulator->getUnwrappedLastStmt($node->stmts);
$lastIfStmtKey = array_key_last($node->stmts);
$node->stmts[$lastIfStmtKey] = new Return_($assign->expr);
/** @var Assign $assign */
$assign = $this->stmtsManipulator->getUnwrappedLastStmt($node->else->stmts);
$lastElseStmtKey = array_key_last($node->else->stmts);
$elseStmts = $node->else->stmts;
$elseStmts[$lastElseStmtKey] = new Return_($assign->expr);
$node->else = null;
foreach ($elseStmts as $elseStmt) {
$this->addNodeAfterNode($elseStmt, $node);
}
$this->removeNode($nextNode);
return $node;
}
}

View File

@ -5,10 +5,11 @@ declare(strict_types=1);
namespace Rector\SOLID\Rector\If_;
use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\BinaryOp;
use PhpParser\Node\Expr\BooleanNot;
use PhpParser\Node\Stmt\If_;
use PhpParser\Node\Stmt\Return_;
use Rector\Exception\NotImplementedException;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\PhpParser\Node\Manipulator\BinaryOpManipulator;
use Rector\PhpParser\Node\Manipulator\IfManipulator;
@ -123,20 +124,50 @@ PHP
if ($nestedIfsWithOnlyReturnCount === $key + 1) {
$this->addNodeAfterNode($nestedIfWithOnlyReturn, $if);
} else {
if (! $nestedIfWithOnlyReturn->cond instanceof BinaryOp) {
throw new NotImplementedException(__METHOD__);
}
$inversedCondition = $this->binaryOpManipulator->inverseCondition($nestedIfWithOnlyReturn->cond);
if ($inversedCondition === null) {
throw new NotImplementedException(__METHOD__);
}
$nestedIfWithOnlyReturn->cond = $inversedCondition;
$nestedIfWithOnlyReturn->stmts = [clone $nextReturn];
$this->addNodeAfterNode($nestedIfWithOnlyReturn, $if);
$this->addStandaloneIfsWithReturn($nestedIfWithOnlyReturn, $if, $nextReturn);
}
}
}
private function createInvertedCondition(Expr $expr): Expr
{
// inverse condition
if ($expr instanceof BinaryOp) {
$inversedCondition = $this->binaryOpManipulator->invertCondition($expr);
if ($inversedCondition === null) {
return new BooleanNot($expr);
}
return $inversedCondition;
}
return new BooleanNot($expr);
}
private function addStandaloneIfsWithReturn(If_ $nestedIfWithOnlyReturn, If_ $if, Return_ $return): void
{
$return = clone $return;
$invertedCondition = $this->createInvertedCondition($nestedIfWithOnlyReturn->cond);
// special case
if ($invertedCondition instanceof BooleanNot) {
if ($invertedCondition->expr instanceof BinaryOp\BooleanAnd) {
$booleanNotPartIf = new If_(new BooleanNot($invertedCondition->expr->left));
$booleanNotPartIf->stmts = [clone $return];
$this->addNodeAfterNode($booleanNotPartIf, $if);
$booleanNotPartIf = new If_(new BooleanNot($invertedCondition->expr->right));
$booleanNotPartIf->stmts = [clone $return];
$this->addNodeAfterNode($booleanNotPartIf, $if);
return;
}
}
$nestedIfWithOnlyReturn->cond = $invertedCondition;
$nestedIfWithOnlyReturn->stmts = [clone $return];
$this->addNodeAfterNode($nestedIfWithOnlyReturn, $if);
}
}

View File

@ -0,0 +1,30 @@
<?php
declare(strict_types=1);
namespace Rector\SOLID\Tests\Rector\If_\ChangeIfElseValueAssignToEarlyReturnRector;
use Iterator;
use Rector\SOLID\Rector\If_\ChangeIfElseValueAssignToEarlyReturnRector;
use Rector\Testing\PHPUnit\AbstractRectorTestCase;
final class ChangeIfElseValueAssignToEarlyReturnRectorTest extends AbstractRectorTestCase
{
/**
* @dataProvider provideDataForTest()
*/
public function test(string $file): void
{
$this->doTestFile($file);
}
public function provideDataForTest(): Iterator
{
return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
}
protected function getRectorClass(): string
{
return ChangeIfElseValueAssignToEarlyReturnRector::class;
}
}

View File

@ -0,0 +1,37 @@
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeIfElseValueAssignToEarlyReturnRector\Fixture;
class SomeClass
{
public function run()
{
if ($this->hasDocBlock($tokens, $index)) {
$docToken = $tokens[$this->getDocBlockIndex($tokens, $index)];
} else {
$docToken = null;
}
return $docToken;
}
}
?>
-----
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeIfElseValueAssignToEarlyReturnRector\Fixture;
class SomeClass
{
public function run()
{
if ($this->hasDocBlock($tokens, $index)) {
return $tokens[$this->getDocBlockIndex($tokens, $index)];
}
return null;
}
}
?>

View File

@ -0,0 +1,49 @@
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeNestedIfsToEarlyReturnRector\Fixture;
use Rector\NodeTypeResolver\Node\AttributeKey;
class BooleanAnd
{
private function isPreviousExpressionVisuallySimilar(Expression $previousExpression, Node $previousNode): bool
{
$prePreviousExpression = $previousExpression->getAttribute(AttributeKey::PREVIOUS_STATEMENT);
if ($prePreviousExpression instanceof Expression && $prePreviousExpression->expr instanceof AssignOp) {
if ($this->areNodesEqual($prePreviousExpression->expr->var, $previousNode->var)) {
return true;
}
}
return false;
}
}
?>
-----
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeNestedIfsToEarlyReturnRector\Fixture;
use Rector\NodeTypeResolver\Node\AttributeKey;
class BooleanAnd
{
private function isPreviousExpressionVisuallySimilar(Expression $previousExpression, Node $previousNode): bool
{
$prePreviousExpression = $previousExpression->getAttribute(AttributeKey::PREVIOUS_STATEMENT);
if (!$prePreviousExpression instanceof Expression) {
return false;
}
if (!$prePreviousExpression->expr instanceof AssignOp) {
return false;
}
if ($this->areNodesEqual($prePreviousExpression->expr->var, $previousNode->var)) {
return true;
}
return false;
}
}
?>

View File

@ -0,0 +1,44 @@
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeNestedIfsToEarlyReturnRector\Fixture;
class HavingElseIfs
{
public function run()
{
if ($value === 5) {
if ($value2 === 10) {
return 'yes';
} elseif ($value === 555) {
return 'maybe';
}
}
return 'no';
}
}
?>
-----
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeNestedIfsToEarlyReturnRector\Fixture;
class HavingElseIfs
{
public function run()
{
if ($value !== 5) {
return 'no';
}
if ($value2 === 10) {
return 'yes';
} elseif ($value === 555) {
return 'maybe';
}
return 'no';
}
}
?>

View File

@ -0,0 +1,86 @@
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeNestedIfsToEarlyReturnRector\Fixture;
use PhpParser\Node\Stmt\If_;
class RectorSample
{
public function run(If_ $node)
{
// $a < $b
if ($node->cond instanceof Smaller) {
// $a < $b ? -1
if ($this->getValue($node->if) === -1) {
// $a < $b ? -1 : ( ? : )
if ($node->else instanceof Ternary) {
// $a < $b ? -1 : ( > ? : )
if ($node->else->cond instanceof BinaryOp\Greater) {
// $a < $b ? -1 : ($a > $b ? : )
if ($this->areNodesEqual($node->cond->left, $node->else->cond->left)) {
if ($this->areNodesEqual($node->cond->right, $node->else->cond->right)) {
// $a < $b ? -1 : ($a > $b ? 1 : 0)
if ($this->getValue($node->else->if) === 1) {
if ($this->getValue($node->else->else) === 0) {
return new Spaceship($node->cond->left, $node->cond->right);
}
}
}
}
}
}
}
}
return null;
}
}
?>
-----
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeNestedIfsToEarlyReturnRector\Fixture;
use PhpParser\Node\Stmt\If_;
class RectorSample
{
public function run(If_ $node)
{
// $a < $b
if (!$node->cond instanceof Smaller) {
return null;
}
// $a < $b ? -1
if ($this->getValue($node->if) !== -1) {
return null;
}
// $a < $b ? -1 : ( ? : )
if (!$node->else instanceof Ternary) {
return null;
}
// $a < $b ? -1 : ( > ? : )
if (!$node->else->cond instanceof BinaryOp\Greater) {
return null;
}
// $a < $b ? -1 : ($a > $b ? : )
if (!$this->areNodesEqual($node->cond->left, $node->else->cond->left)) {
return null;
}
if (!$this->areNodesEqual($node->cond->right, $node->else->cond->right)) {
return null;
}
// $a < $b ? -1 : ($a > $b ? 1 : 0)
if ($this->getValue($node->else->if) !== 1) {
return null;
}
if ($this->getValue($node->else->else) === 0) {
return new Spaceship($node->cond->left, $node->cond->right);
}
return null;
}
}
?>

View File

@ -0,0 +1,17 @@
<?php
namespace Rector\SOLID\Tests\Rector\If_\ChangeNestedIfsToEarlyReturnRector\Fixture;
class SkipSingleIf
{
public function isParameterUsedMethod(Param $param, ClassMethod $classMethod): bool
{
$compactFuncCalls = $this->betterNodeFinder->find((array) $classMethod->stmts, function (Node $node): bool {
if (! $node instanceof FuncCall) {
return false;
}
return $this->nameResolver->isName($node, 'compact');
});
}
}

View File

@ -1,5 +1,6 @@
services:
Rector\DeadCode\Rector\Class_\RemoveUnusedClassesRector: null
Rector\SOLID\Rector\If_\ChangeNestedIfsToEarlyReturnRector: null
# Rector\SOLID\Rector\If_\ChangeIfElseValueAssignToEarlyReturnRector: null
imports:
- { resource: "create-rector.yaml", ignore_errors: true }

View File

@ -77,7 +77,7 @@ final class BinaryOpManipulator
return new $inversedNodeClass($firstInversedNode, $secondInversedNode);
}
public function inverseCondition(BinaryOp $binaryOp): ?BinaryOp
public function invertCondition(BinaryOp $binaryOp): ?BinaryOp
{
// no nesting
if ($binaryOp->left instanceof BooleanOr) {

View File

@ -6,8 +6,10 @@ namespace Rector\PhpParser\Node\Manipulator;
use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\BinaryOp\Identical;
use PhpParser\Node\Expr\BinaryOp\NotIdentical;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Stmt\Continue_;
use PhpParser\Node\Stmt\Do_;
use PhpParser\Node\Stmt\Else_;
@ -46,14 +48,21 @@ final class IfManipulator
*/
private $callableNodeTraverser;
/**
* @var StmtsManipulator
*/
private $stmtsManipulator;
public function __construct(
BetterStandardPrinter $betterStandardPrinter,
ConstFetchManipulator $constFetchManipulator,
CallableNodeTraverser $callableNodeTraverser
CallableNodeTraverser $callableNodeTraverser,
StmtsManipulator $stmtsManipulator
) {
$this->betterStandardPrinter = $betterStandardPrinter;
$this->constFetchManipulator = $constFetchManipulator;
$this->callableNodeTraverser = $callableNodeTraverser;
$this->stmtsManipulator = $stmtsManipulator;
}
/**
@ -170,6 +179,10 @@ final class IfManipulator
$currentIf = $currentIf->stmts[0];
}
if ($ifs === []) {
return [];
}
if (! $this->hasOnlyStmtOfType($currentIf, Return_::class)) {
return [];
}
@ -180,6 +193,46 @@ final class IfManipulator
return $ifs;
}
public function isIfWithElse(If_ $if): bool
{
if ($if->else === null) {
return false;
}
return ! (bool) $if->elseifs;
}
public function isIfAndElseWithSameVariableAssignAsLastStmts(If_ $if, Expr $desiredExpr): bool
{
if (! $this->isIfWithElse($if)) {
return false;
}
$lastIfStmt = $this->stmtsManipulator->getUnwrappedLastStmt($if->stmts);
if (! $lastIfStmt instanceof Assign) {
return false;
}
$lastElseStmt = $this->stmtsManipulator->getUnwrappedLastStmt($if->else->stmts);
if (! $lastElseStmt instanceof Assign) {
return false;
}
if (! $lastIfStmt->var instanceof Variable) {
return false;
}
if (! $this->betterStandardPrinter->areNodesEqual($lastIfStmt->var, $lastElseStmt->var)) {
return false;
}
if (! $this->betterStandardPrinter->areNodesEqual($desiredExpr, $lastElseStmt->var)) {
return false;
}
return true;
}
private function matchComparedAndReturnedNode(NotIdentical $notIdentical, Return_ $returnNode): ?Expr
{
if ($this->betterStandardPrinter->areNodesEqual($notIdentical->left, $returnNode->expr)) {
@ -188,10 +241,11 @@ final class IfManipulator
}
}
if ($this->betterStandardPrinter->areNodesEqual($notIdentical->right, $returnNode->expr)) {
if ($this->constFetchManipulator->isNull($notIdentical->left)) {
return $notIdentical->right;
}
if (! $this->betterStandardPrinter->areNodesEqual($notIdentical->right, $returnNode->expr)) {
return null;
}
if ($this->constFetchManipulator->isNull($notIdentical->left)) {
return $notIdentical->right;
}
return null;

View File

@ -0,0 +1,23 @@
<?php
declare(strict_types=1);
namespace Rector\PhpParser\Node\Manipulator;
use PhpParser\Node;
use PhpParser\Node\Stmt\Expression;
final class StmtsManipulator
{
public function getUnwrappedLastStmt(array $stmts): ?Node
{
$lastStmtKey = array_key_last($stmts);
$lastStmt = $stmts[$lastStmtKey];
if ($lastStmt instanceof Expression) {
return $lastStmt->expr;
}
return $lastStmt;
}
}

View File

@ -235,7 +235,7 @@ final class CreateRectorCommand extends Command
}
$setConfigContent = trim($setConfigContent) . sprintf(
'%s%s: ~%s',
'%s%s: null%s',
PHP_EOL,
$this->indentFourSpaces($rectorFqnName),
PHP_EOL