Skip to content

Commit

Permalink
Make PreparedValueToEarlyReturnRector use StmtsAwareInterface (#3772)
Browse files Browse the repository at this point in the history
  • Loading branch information
@TomasVotruba
TomasVotruba committed May 8, 2023
1 parent 1e052f5 commit 9a2fe21
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 151 deletions.
2 changes: 1 addition & 1 deletion config/set/code-quality.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
use Rector\CodeQuality\Rector\LogicalAnd\LogicalToBooleanRector;
use Rector\CodeQuality\Rector\New_\NewStaticToNewSelfRector;
use Rector\CodeQuality\Rector\NotEqual\CommonNotEqualRector;
use Rector\CodeQuality\Rector\NullsafeMethodCall\CleanupUnneededNullsafeOperatorRector;
use Rector\CodeQuality\Rector\PropertyFetch\ExplicitMethodCallOverMagicGetSetRector;
use Rector\CodeQuality\Rector\Switch_\SingularSwitchToIfRector;
use Rector\CodeQuality\Rector\Switch_\SwitchTrueToIfRector;
Expand All @@ -81,7 +82,6 @@
use Rector\CodingStyle\Rector\ClassMethod\FuncGetArgsToVariadicParamRector;
use Rector\CodingStyle\Rector\FuncCall\CallUserFuncToMethodCallRector;
use Rector\CodingStyle\Rector\FuncCall\CountArrayToEmptyArrayComparisonRector;
use Rector\CodeQuality\Rector\NullsafeMethodCall\CleanupUnneededNullsafeOperatorRector;
use Rector\Config\RectorConfig;
use Rector\Php52\Rector\Property\VarToPublicPropertyRector;
use Rector\Php71\Rector\FuncCall\RemoveExtraParametersRector;
Expand Down
11 changes: 8 additions & 3 deletions phpstan.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ parameters:
- '#Property PhpParser\\Node\\Stmt\\Foreach_\:\:\$valueVar \(PhpParser\\Node\\Expr\) does not accept PhpParser\\Node\\Expr\|null#'

# is nested expr
- '#Access to an undefined property PhpParser\\Node\\Expr\:\:\$expr#'
-
message: '#Access to an undefined property PhpParser\\Node\\Expr\:\:\$expr#'
path: rules/DeadCode/NodeManipulator/LivingCodeManipulator.php

# 3rd party package
-
Expand Down Expand Up @@ -131,9 +133,8 @@ parameters:
# known types
- '#Call to an undefined method PHPStan\\Type\\ConstantType\:\:getValue\(\)#'
- '#Parameter \#1 \$stmts of method Rector\\EarlyReturn\\Rector\\Return_\\PreparedValueToEarlyReturnRector\:\:collectIfs\(\) expects array<PhpParser\\Node\\Stmt\\If_\>, array<PhpParser\\Node\\Stmt\> given#'
- '#Access to an undefined property PhpParser\\Node\\FunctionLike\|PhpParser\\Node\\Stmt\\If_\:\:\$stmts#'

- '#Cognitive complexity for "Rector\\CodeQuality\\Rector\\Isset_\\IssetOnPropertyObjectToPropertyExistsRector\:\:refactor\(\)" is \d+, keep it under 10#'
# - '#Cognitive complexity for "Rector\\CodeQuality\\Rector\\Isset_\\IssetOnPropertyObjectToPropertyExistsRector\:\:refactor\(\)" is \d+, keep it under 10#'

- '#(.*?) class\-string, string given#'

Expand Down Expand Up @@ -784,3 +785,7 @@ parameters:
- rules/Naming/Rector/Assign/RenameVariableToMatchMethodCallReturnTypeRector.php
- rules/Naming/Rector/ClassMethod/RenameVariableToMatchNewTypeRector.php
- rules/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector.php

# next to resolve
- '#Cognitive complexity for "Rector\\CodeQuality\\Rector\\Isset_\\IssetOnPropertyObjectToPropertyExistsRector\:\:refactor\(\)" is 13, keep it under 10#'
- '#Cognitive complexity for "Rector\\EarlyReturn\\Rector\\Return_\\PreparedValueToEarlyReturnRector\:\:refactor\(\)" is 11, keep it under 10#'
253 changes: 108 additions & 145 deletions rules/EarlyReturn/Rector/Return_/PreparedValueToEarlyReturnRector.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@
use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\FunctionLike;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Stmt;
use PhpParser\Node\Stmt\Expression;
use PhpParser\Node\Stmt\If_;
use PhpParser\Node\Stmt\Return_;
use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
use Rector\Core\NodeManipulator\IfManipulator;
use Rector\Core\Rector\AbstractRector;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\EarlyReturn\ValueObject\BareSingleAssignIf;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

Expand All @@ -38,11 +40,11 @@ public function run()
{
$var = null;
if (rand(0,1)) {
if (rand(0, 1)) {
$var = 1;
}
if (rand(0,1)) {
if (rand(0, 1)) {
$var = 2;
}
Expand All @@ -57,11 +59,11 @@ class SomeClass
{
public function run()
{
if (rand(0,1)) {
if (rand(0, 1)) {
return 1;
}
if (rand(0,1)) {
if (rand(0, 1)) {
return 2;
}
Expand All @@ -78,193 +80,154 @@ public function run()
*/
public function getNodeTypes(): array
{
return [Return_::class];
return [StmtsAwareInterface::class];
}

/**
* @param Return_ $node
* @param StmtsAwareInterface $node
*/
public function refactor(Node $node): ?Node
public function refactor(Node $node)
{
$ifsBefore = $this->getIfsBefore($node);
/** @var BareSingleAssignIf[] $bareSingleAssignIfs */
$bareSingleAssignIfs = [];

if ($this->shouldSkip($ifsBefore, $node->expr)) {
return null;
}

if ($this->isAssignVarUsedInIfCond($ifsBefore, $node->expr)) {
return null;
}
$initialAssign = null;
$initialAssignPosition = null;

/** @var Expr $returnExpr */
$returnExpr = $node->expr;
foreach ((array) $node->stmts as $key => $stmt) {
$bareSingleAssignIf = $this->matchBareSingleAssignIf($stmt);

/** @var Expression $previousFirstExpression */
$previousFirstExpression = $this->getPreviousIfLinearEquals($ifsBefore[0], $returnExpr);
if ($bareSingleAssignIf instanceof BareSingleAssignIf) {
$bareSingleAssignIfs[] = $bareSingleAssignIf;
continue;
}

/** @var Assign $previousAssign */
$previousAssign = $previousFirstExpression->expr;
if ($stmt instanceof Expression && $stmt->expr instanceof Assign) {
$initialAssign = $stmt->expr;
$initialAssignPosition = $key;
}

if ($this->isPreviousVarUsedInAssignExpr($ifsBefore, $previousAssign->var)) {
return null;
}
if (! $stmt instanceof Return_) {
continue;
}

foreach ($ifsBefore as $ifBefore) {
/** @var Expression $expressionIf */
$expressionIf = $ifBefore->stmts[0];
/** @var Assign $assignIf */
$assignIf = $expressionIf->expr;
$return = $stmt;

$ifBefore->stmts[0] = new Return_($assignIf->expr);
}
// match exact variable
if (! $return->expr instanceof Variable) {
return null;
}

/** @var Assign $assignPrevious */
$assignPrevious = $previousFirstExpression->expr;
$node->expr = $assignPrevious->expr;
$this->removeNode($previousFirstExpression);
if (! is_int($initialAssignPosition)) {
return null;
}

return $node;
}
if (! $initialAssign instanceof Assign) {
return null;
}

/**
* @param If_[] $ifsBefore
*/
private function isAssignVarUsedInIfCond(array $ifsBefore, ?Expr $expr): bool
{
foreach ($ifsBefore as $ifBefore) {
$isUsedInIfCond = (bool) $this->betterNodeFinder->findFirst(
$ifBefore->cond,
fn (Node $node): bool => $this->nodeComparator->areNodesEqual($node, $expr)
);
if ($bareSingleAssignIfs === []) {
return null;
}

if ($isUsedInIfCond) {
return true;
if (! $this->isVariableSharedInAssignIfsAndReturn($bareSingleAssignIfs, $return->expr, $initialAssign)) {
return null;
}

return $this->refactorToDirectReturns(
$node,
$initialAssignPosition,
$bareSingleAssignIfs,
$initialAssign,
$return
);
}

return false;
return null;
}

/**
* @param If_[] $ifsBefore
* @param BareSingleAssignIf[] $bareSingleAssignIfs
*/
private function isPreviousVarUsedInAssignExpr(array $ifsBefore, Expr $expr): bool
{
foreach ($ifsBefore as $ifBefore) {
/** @var Expression $expression */
$expression = $ifBefore->stmts[0];
/** @var Assign $assign */
$assign = $expression->expr;

$isUsedInAssignExpr = (bool) $this->betterNodeFinder->findFirst(
$assign->expr,
fn (Node $node): bool => $this->nodeComparator->areNodesEqual($node, $expr)
private function isVariableSharedInAssignIfsAndReturn(
array $bareSingleAssignIfs,
Expr $returnedExpr,
Assign $initialAssign
): bool {
if (! $this->nodeComparator->areNodesEqual($returnedExpr, $initialAssign->var)) {
return false;
}

foreach ($bareSingleAssignIfs as $bareSingleAssignIf) {
$assign = $bareSingleAssignIf->getAssign();

$isVariableUsed = (bool) $this->betterNodeFinder->findFirst(
[$bareSingleAssignIf->getIfCondExpr(), $assign->expr],
fn (Node $node): bool => $this->nodeComparator->areNodesEqual($node, $returnedExpr)
);

if ($isUsedInAssignExpr) {
return true;
if ($isVariableUsed) {
return false;
}
}

return false;
}

/**
* @param If_[] $ifsBefore
*/
private function shouldSkip(array $ifsBefore, ?Expr $returnExpr): bool
{
if ($ifsBefore === []) {
return true;
if (! $this->nodeComparator->areNodesEqual($assign->var, $returnedExpr)) {
return false;
}
}

return ! (bool) $this->getPreviousIfLinearEquals($ifsBefore[0], $returnExpr);
return true;
}

private function getPreviousIfLinearEquals(?Node $node, ?Expr $expr): ?Expression
private function matchBareSingleAssignIf(Stmt $stmt): ?BareSingleAssignIf
{
if (! $node instanceof Node) {
if (! $stmt instanceof If_) {
return null;
}

if (! $expr instanceof Expr) {
// is exactly single stmt
if (count($stmt->stmts) !== 1) {
return null;
}

$previous = $node->getAttribute(AttributeKey::PREVIOUS_NODE);
if (! $previous instanceof Expression) {
return $this->getPreviousIfLinearEquals($previous, $expr);
}

if (! $previous->expr instanceof Assign) {
$onlyStmt = $stmt->stmts[0];
if (! $onlyStmt instanceof Expression) {
return null;
}

if ($this->nodeComparator->areNodesEqual($previous->expr->var, $expr)) {
return $previous;
}

return null;
}

/**
* @return If_[]
*/
private function getIfsBefore(Return_ $return): array
{
$parentNode = $return->getAttribute(AttributeKey::PARENT_NODE);
if (! $parentNode instanceof FunctionLike && ! $parentNode instanceof If_) {
return [];
}

if ($parentNode->stmts === []) {
return [];
$expression = $onlyStmt;
if (! $expression->expr instanceof Assign) {
return null;
}

$firstItemPosition = array_key_last($parentNode->stmts);
if ($parentNode->stmts[$firstItemPosition] !== $return) {
return [];
if (! $this->ifManipulator->isIfWithoutElseAndElseIfs($stmt)) {
return null;
}

return $this->collectIfs($parentNode->stmts, $return);
return new BareSingleAssignIf($stmt, $expression->expr);
}

/**
* @param If_[] $stmts
* @return If_[]
* @param BareSingleAssignIf[] $bareSingleAssignIfs
*/
private function collectIfs(array $stmts, Return_ $return): array
{
/** @va If_[] $ifs */
$ifs = $this->betterNodeFinder->findInstanceOf($stmts, If_::class);

/** Skip entirely if found skipped ifs */
foreach ($ifs as $if) {
/** @var If_ $if */
if (! $this->ifManipulator->isIfWithoutElseAndElseIfs($if)) {
return [];
}

$stmts = $if->stmts;
if (count($stmts) !== 1) {
return [];
}

$expression = $stmts[0];
if (! $expression instanceof Expression) {
return [];
}

if (! $expression->expr instanceof Assign) {
return [];
}

$assign = $expression->expr;
if (! $this->nodeComparator->areNodesEqual($assign->var, $return->expr)) {
return [];
}
}

return $ifs;
private function refactorToDirectReturns(
StmtsAwareInterface $stmtsAware,
int $initialAssignPosition,
array $bareSingleAssignIfs,
Assign $initialAssign,
Return_ $return
): StmtsAwareInterface {
// 1. remove initial assign
unset($stmtsAware->stmts[$initialAssignPosition]);

// 2. make ifs early return
foreach ($bareSingleAssignIfs as $bareSingleAssignIf) {
$if = $bareSingleAssignIf->getIf();
$if->stmts[0] = new Return_($bareSingleAssignIf->getAssign()->expr);
}

// 3. make return default value
$return->expr = $initialAssign->expr;

return $stmtsAware;
}
}
Loading

0 comments on commit 9a2fe21

Please sign in to comment.