Decouple SimplifyIfNotNullReturnRector logic to IfMaintainer

This commit is contained in:
Tomas Votruba 2019-01-10 13:55:50 +01:00
parent 544e59540b
commit 30bbfeb60e
4 changed files with 99 additions and 58 deletions

View File

@ -3,18 +3,26 @@
namespace Rector\CodeQuality\Rector\If_;
use PhpParser\Node;
use PhpParser\Node\Expr\BinaryOp;
use PhpParser\Node\Expr\BinaryOp\Identical;
use PhpParser\Node\Expr\BinaryOp\NotIdentical;
use PhpParser\Node\Stmt\If_;
use PhpParser\Node\Stmt\Return_;
use Rector\NodeTypeResolver\Node\Attribute;
use Rector\PhpParser\Node\Maintainer\IfMaintainer;
use Rector\Rector\AbstractRector;
use Rector\RectorDefinition\CodeSample;
use Rector\RectorDefinition\RectorDefinition;
final class SimplifyIfNotNullReturnRector extends AbstractRector
{
/**
* @var IfMaintainer
*/
private $ifMaintainer;
public function __construct(IfMaintainer $ifMaintainer)
{
$this->ifMaintainer = $ifMaintainer;
}
public function getDefinition(): RectorDefinition
{
return new RectorDefinition('Changes redundant null check to instant return', [
@ -49,45 +57,38 @@ CODE_SAMPLE
*/
public function refactor(Node $node): ?Node
{
if (count($node->stmts) !== 1) {
return null;
}
$comparedNode = $this->ifMaintainer->matchIfNotNullReturnValue($node);
if ($comparedNode) {
$insideIfNode = $node->stmts[0];
$insideIfNode = $node->stmts[0];
if (! $insideIfNode instanceof Return_) {
return null;
}
$nextNode = $node->getAttribute(Attribute::NEXT_NODE);
if (! $nextNode instanceof Return_) {
return null;
}
$nextNode = $node->getAttribute(Attribute::NEXT_NODE);
if (! $nextNode instanceof Return_) {
return null;
}
if (! $this->isNull($nextNode->expr)) {
return null;
}
if ($node->cond instanceof NotIdentical && $this->areCrossNodesEquals($node->cond, $nextNode, $insideIfNode)) {
$this->removeNode($nextNode);
return $insideIfNode;
}
if ($node->cond instanceof Identical && $this->areCrossNodesEquals($node->cond, $nextNode, $insideIfNode)) {
$comparedNode = $this->ifMaintainer->matchIfValueReturnValue($node);
if ($comparedNode) {
$nextNode = $node->getAttribute(Attribute::NEXT_NODE);
if (! $nextNode instanceof Return_) {
return null;
}
if (! $this->areNodesEqual($comparedNode, $nextNode->expr)) {
return null;
}
$this->removeNode($nextNode);
return clone $nextNode;
}
return null;
}
private function areCrossNodesEquals(BinaryOp $condNode, Return_ $thirdNode, Return_ $fourthNode): bool
{
if ($this->areNodesEqual($condNode->left, $thirdNode->expr) && $this->areNodesEqual(
$condNode->right,
$fourthNode->expr
)) {
return true;
}
return $this->areNodesEqual($condNode->right, $thirdNode->expr) && $this->areNodesEqual(
$condNode->left,
$fourthNode->expr
);
}
}

View File

@ -0,0 +1,26 @@
<?php
namespace Rector\CodeQuality\Tests\Rector\If_\SimplifyIfNotNullReturnRector\Fixture;
class Skip
{
public function run()
{
$newNode = 'something';
if ($newNode === null) {
return null;
}
return 5;
}
public function runAgain()
{
$newNode = 'something';
if ($newNode !== null) {
return $newNode;
}
return 'another';
}
}

View File

@ -9,7 +9,11 @@ final class SimplifyIfNotNullReturnRectorTest extends AbstractRectorTestCase
{
public function test(): void
{
$this->doTestFiles([__DIR__ . '/Fixture/fixture.php.inc', __DIR__ . '/Fixture/fixture2.php.inc']);
$this->doTestFiles([
__DIR__ . '/Fixture/fixture.php.inc',
__DIR__ . '/Fixture/fixture2.php.inc',
__DIR__ . '/Fixture/skip.php.inc',
]);
}
public function getRectorClass(): string

View File

@ -2,6 +2,7 @@
namespace Rector\PhpParser\Node\Maintainer;
use PhpParser\Node;
use PhpParser\Node\Expr\BinaryOp\Identical;
use PhpParser\Node\Expr\BinaryOp\NotIdentical;
use PhpParser\Node\Stmt\If_;
@ -14,13 +15,16 @@ final class IfMaintainer
* @var BetterStandardPrinter
*/
private $betterStandardPrinter;
/**
* @var ConstFetchMaintainer
*/
private $constFetchMaintainer;
public function __construct(BetterStandardPrinter $betterStandardPrinter, ConstFetchMaintainer $constFetchMaintainer)
{
public function __construct(
BetterStandardPrinter $betterStandardPrinter,
ConstFetchMaintainer $constFetchMaintainer
) {
$this->betterStandardPrinter = $betterStandardPrinter;
$this->constFetchMaintainer = $constFetchMaintainer;
}
@ -28,78 +32,84 @@ final class IfMaintainer
/**
* Matches:
*
* if ($value !== null) {
* if (<$value> !== null) {
* return $value;
* }
*/
public function isIfNotNullReturnValue(If_ $ifNode): bool
public function matchIfNotNullReturnValue(If_ $ifNode): ?Node
{
if (count($ifNode->stmts) !== 1) {
return false;
return null;
}
$insideIfNode = $ifNode->stmts[0];
if (! $insideIfNode instanceof Return_) {
return false;
return null;
}
/** @var Return_ $returnNode */
$returnNode = $insideIfNode;
if (! $ifNode->cond instanceof NotIdentical) {
return false;
return null;
}
if ($this->betterStandardPrinter->areNodesEqual($ifNode->cond->left, $returnNode->expr)) {
return $this->constFetchMaintainer->isNull($ifNode->cond->right);
}
if ($this->betterStandardPrinter->areNodesEqual($ifNode->cond->right, $returnNode->expr)) {
return $this->constFetchMaintainer->isNull($ifNode->cond->left);
}
return false;
return $this->matchComparedAndReturnedNode($ifNode->cond, $returnNode);
}
/**
* Matches:
*
* if ($value === null) {
* if (<$value> === null) {
* return null;
* }
*
* if ($value === 53;) {
* if (<$value> === 53;) {
* return 53;
* }
*/
public function isIfValueReturnValue(If_ $ifNode): bool
public function matchIfValueReturnValue(If_ $ifNode): ?Node
{
if (count($ifNode->stmts) !== 1) {
return false;
return null;
}
$insideIfNode = $ifNode->stmts[0];
if (! $insideIfNode instanceof Return_) {
return false;
return null;
}
/** @var Return_ $returnNode */
$returnNode = $insideIfNode;
if (! $ifNode->cond instanceof Identical) {
return false;
return null;
}
if ($this->betterStandardPrinter->areNodesEqual($ifNode->cond->left, $returnNode->expr)) {
return true;
return $ifNode->cond->right;
}
if ($this->betterStandardPrinter->areNodesEqual($ifNode->cond->right, $returnNode->expr)) {
return true;
return $ifNode->cond->left;
}
return false;
return null;
}
private function matchComparedAndReturnedNode(NotIdentical $notIdenticalNode, Return_ $returnNode): ?Node
{
if ($this->betterStandardPrinter->areNodesEqual($notIdenticalNode->left, $returnNode->expr)) {
if ($this->constFetchMaintainer->isNull($notIdenticalNode->right)) {
return $notIdenticalNode->right;
}
}
if ($this->betterStandardPrinter->areNodesEqual($notIdenticalNode->right, $returnNode->expr)) {
if ($this->constFetchMaintainer->isNull($notIdenticalNode->left)) {
return $notIdenticalNode->left;
}
}
return null;
}
}