Skip to content

Commit

Permalink
Make SimplifyIfNullableReturnRector + ChangeNestedIfsToEarlyReturnRec…
Browse files Browse the repository at this point in the history 
…tor work without next/prev (#3794)
  • Loading branch information
@TomasVotruba
TomasVotruba committed May 11, 2023
1 parent 0e2b39c commit d8da061
Show file tree
Hide file tree
Showing 13 changed files with 110 additions and 97 deletions.
1 change: 1 addition & 0 deletions phpstan.neon
View file
Expand Up @@ -791,6 +791,7 @@ parameters:
- '#Cognitive complexity for "Rector\\EarlyReturn\\Rector\\Return_\\PreparedValueToEarlyReturnRector\:\:refactor\(\)" is 11, keep it under 10#'
- '#Cognitive complexity for "Rector\\CodeQuality\\Rector\\Foreach_\\SimplifyForeachToCoalescingRector\:\:refactor\(\)" is 16, keep it under 10#'
- '#Cognitive complexity for "Rector\\CodeQuality\\Rector\\Foreach_\\ForeachToInArrayRector\:\:refactor\(\)" is \d+, keep it under 10#'
- '#Cognitive complexity for "Rector\\CodeQuality\\Rector\\If_\\SimplifyIfNullableReturnRector\:\:refactor\(\)" is 15, keep it under 10#'

# resolve continually
- '#(Matching|Fetching) deprecated class constant (NEXT_NODE|PREVIOUS_NODE) of class Rector\\NodeTypeResolver\\Node\\AttributeKey#'
Expand Down
143 changes: 86 additions & 57 deletions rules/CodeQuality/Rector/If_/SimplifyIfNullableReturnRector.php
View file
Expand Up @@ -18,6 +18,7 @@
use PHPStan\Type\Type;
use PHPStan\Type\UnionType;
use Rector\CodeQuality\TypeResolver\AssignVariableTypeResolver;
use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
use Rector\Core\NodeManipulator\IfManipulator;
use Rector\Core\Rector\AbstractRector;
use Rector\DeadCode\PhpDoc\TagRemover\VarTagRemover;
Expand Down Expand Up @@ -76,74 +77,105 @@ public function run()
*/
public function getNodeTypes(): array
{
return [If_::class];
return [StmtsAwareInterface::class];
}

/**
* @param If_ $node
* @param StmtsAwareInterface $node
*/
public function refactor(Node $node): ?Node
{
if ($this->shouldSkip($node)) {
if ($node->stmts === null) {
return null;
}

/** @var BooleanNot|Instanceof_ $cond */
$cond = $node->cond;
/** @var Instanceof_ $instanceof */
$instanceof = $cond instanceof BooleanNot
? $cond->expr
: $cond;
$variable = $instanceof->expr;
$class = $instanceof->class;
foreach ($node->stmts as $key => $stmt) {
if (! $stmt instanceof Return_) {
continue;
}

if (! $class instanceof Name) {
return null;
}
$previousStmt = $node->stmts[$key - 1] ?? null;
if (! $previousStmt instanceof If_) {
continue;
}

/** @var Return_ $returnIfStmt */
$returnIfStmt = $node->stmts[0];
$if = $previousStmt;
if ($this->shouldSkip($if)) {
continue;
}

if ($this->isIfStmtReturnIncorrect($cond, $variable, $returnIfStmt)) {
return null;
}
/** @var BooleanNot|Instanceof_ $cond */
$cond = $if->cond;

$previous = $node->getAttribute(AttributeKey::PREVIOUS_NODE);
if (! $previous instanceof Expression) {
return null;
}
/** @var Instanceof_ $instanceof */
$instanceof = $cond instanceof BooleanNot ? $cond->expr : $cond;

$previousAssign = $previous->expr;
if (! $previousAssign instanceof Assign) {
return null;
}
// @todo allow property as well
$variable = $instanceof->expr;

if (! $this->nodeComparator->areNodesEqual($previousAssign->var, $variable)) {
return null;
}
$class = $instanceof->class;

/** @var Return_ $nextNode */
$nextNode = $node->getAttribute(AttributeKey::NEXT_NODE);
if ($this->isNextReturnIncorrect($cond, $variable, $nextNode)) {
return null;
}
if (! $class instanceof Name) {
continue;
}

$variableType = $this->assignVariableTypeResolver->resolve($previousAssign);
if (! $variableType instanceof UnionType) {
return null;
}
/** @var Return_ $returnIfStmt */
$returnIfStmt = $if->stmts[0];

if ($this->isIfStmtReturnIncorrect($cond, $variable, $returnIfStmt)) {
continue;
}

$className = $class->toString();
$types = $variableType->getTypes();
$previousPreviousStmt = $node->stmts[$key - 2] ?? null;
if (! $previousPreviousStmt instanceof Expression) {
continue;
}

return $this->processSimplifyNullableReturn(
$variableType,
$types,
$className,
$nextNode,
$previous,
$previousAssign->expr
);
if (! $previousPreviousStmt->expr instanceof Assign) {
continue;
}

$previousPreviousAssign = $previousPreviousStmt->expr;
if (! $this->nodeComparator->areNodesEqual($previousPreviousAssign->var, $variable)) {
continue;
}

if ($this->isNextReturnIncorrect($cond, $variable, $stmt)) {
continue;
}

$variableType = $this->assignVariableTypeResolver->resolve($previousPreviousAssign);
if (! $variableType instanceof UnionType) {
continue;
}

$className = $class->toString();
$types = $variableType->getTypes();

$directReturn = $this->processSimplifyNullableReturn(
$variableType,
$types,
$className,
$previousPreviousStmt,
$previousPreviousAssign->expr
);

if (! $directReturn instanceof Return_) {
continue;
}

// unset previous assign
unset($node->stmts[$key - 2]);

// unset previous if
unset($node->stmts[$key - 1]);

$node->stmts[$key] = $directReturn;

return $node;
}

return null;
}

private function isIfStmtReturnIncorrect(BooleanNot|Instanceof_ $expr, Expr $variable, Return_ $return): bool
Expand Down Expand Up @@ -179,7 +211,6 @@ private function processSimplifyNullableReturn(
UnionType $unionType,
array $types,
string $className,
Return_ $return,
Expression $expression,
Expr $expr
): ?Return_ {
Expand All @@ -188,18 +219,18 @@ private function processSimplifyNullableReturn(
}

if ($types[0] instanceof FullyQualifiedObjectType && $types[1] instanceof NullType && $className === $types[0]->getClassName()) {
return $this->removeAndReturn($return, $expression, $expr, $unionType);
return $this->createDirectReturn($expression, $expr, $unionType);
}

if ($types[0] instanceof NullType && $types[1] instanceof FullyQualifiedObjectType && $className === $types[1]->getClassName()) {
return $this->removeAndReturn($return, $expression, $expr, $unionType);
return $this->createDirectReturn($expression, $expr, $unionType);
}

if ($this->isNotTypedNullable($types, $className)) {
return null;
}

return $this->removeAndReturn($return, $expression, $expr, $unionType);
return $this->createDirectReturn($expression, $expr, $unionType);
}

/**
Expand All @@ -218,12 +249,10 @@ private function isNotTypedNullable(array $types, string $className): bool
return $className !== $types[0]->getClassName();
}

private function removeAndReturn(Return_ $return, Expression $expression, Expr $expr, UnionType $unionType): Return_
private function createDirectReturn(Expression $expression, Expr $expr, UnionType $unionType): Return_
{
$this->removeNode($return);
$this->removeNode($expression);

$exprReturn = new Return_($expr);

$this->varTagRemover->removeVarPhpTagValueNodeIfNotComment($expression, $unionType);
$this->mirrorComments($exprReturn, $expression);

Expand Down
1 change: 0 additions & 1 deletion rules/CodeQuality/TypeResolver/AssignVariableTypeResolver.php
View file
Expand Up @@ -19,7 +19,6 @@ public function __construct(
public function resolve(Assign $assign): Type
{
$exprType = $this->nodeTypeResolver->getType($assign->expr);

if ($exprType instanceof UnionType) {
return $exprType;
}
Expand Down
37 changes: 14 additions & 23 deletions rules/EarlyReturn/Rector/If_/ChangeNestedIfsToEarlyReturnRector.php
View file
Expand Up @@ -7,7 +7,6 @@
use PhpParser\Node;
use PhpParser\Node\Expr\BinaryOp\BooleanAnd;
use PhpParser\Node\Expr\BooleanNot;
use PhpParser\Node\Stmt;
use PhpParser\Node\Stmt\If_;
use PhpParser\Node\Stmt\Return_;
use Rector\Core\Contract\PhpParser\Node\StmtsAwareInterface;
Expand Down Expand Up @@ -80,39 +79,31 @@ public function getNodeTypes(): array
/**
* @param StmtsAwareInterface $node
*/
public function refactor(Node $node): ?Node
public function refactor(Node $node): ?StmtsAwareInterface
{
$stmts = $node->stmts;
if ($stmts === null) {
if ($node->stmts === null) {
return null;
}

/** @var Stmt[] $previousStmts[] */
$previousStmts = [];

foreach ($stmts as $key => $stmt) {
$nextStmt = $stmts[$key + 1] ?? null;
if (! $nextStmt instanceof Return_) {
if ($nextStmt instanceof Stmt) {
$previousStmts[] = $stmt;
}

foreach ($node->stmts as $key => $stmt) {
if (! $stmt instanceof If_) {
continue;
}

if (! $stmt instanceof If_) {
continue;
$nextStmt = $node->stmts[$key + 1] ?? null;
if (! $nextStmt instanceof Return_) {
return null;
}

$nestedIfsWithOnlyReturn = $this->ifManipulator->collectNestedIfsWithOnlyReturn($stmt);
if ($nestedIfsWithOnlyReturn === []) {
continue;
}

$node->stmts = array_merge(
$previousStmts,
$this->processNestedIfsWithOnlyReturn($nestedIfsWithOnlyReturn, $nextStmt)
);
$newStmts = $this->processNestedIfsWithOnlyReturn($nestedIfsWithOnlyReturn, $nextStmt);

// replace nested ifs with many separate ifs
array_splice($node->stmts, $key, 1, $newStmts);

return $node;
}
Expand All @@ -122,7 +113,7 @@ public function refactor(Node $node): ?Node

/**
* @param If_[] $nestedIfsWithOnlyReturn
* @return Stmt[]
* @return If_[]
*/
private function processNestedIfsWithOnlyReturn(array $nestedIfsWithOnlyReturn, Return_ $nextReturn): array
{
Expand All @@ -142,13 +133,13 @@ private function processNestedIfsWithOnlyReturn(array $nestedIfsWithOnlyReturn,
}
}

$newStmts[] = $nextReturn;
// $newStmts[] = $nextReturn;

return $newStmts;
}

/**
* @return Stmt[]
* @return If_[]
*/
private function createStandaloneIfsWithReturn(If_ $nestedIfWithOnlyReturn, Return_ $return): array
{
Expand Down
4 changes: 2 additions & 2 deletions rules/Php74/Rector/MethodCall/ChangeReflectionTypeToStringToGetNameRector.php
View file
Expand Up @@ -101,7 +101,7 @@ public function getNodeTypes(): array
public function refactorWithScope(Node $node, Scope $scope): ?Node
{
if ($node instanceof MethodCall) {
return $this->refactorMethodCall($node, $scope);
return $this->refactorMethodCall($node);
}

if ($node->expr instanceof MethodCall) {
Expand Down Expand Up @@ -145,7 +145,7 @@ private function isWithReflectionType(UnionType $unionType): bool
return false;
}

private function refactorMethodCall(MethodCall $methodCall, Scope $scope): ?Node
private function refactorMethodCall(MethodCall $methodCall): ?Node
{
$this->collectCallByVariable($methodCall);

Expand Down
2 changes: 1 addition & 1 deletion rules/Strict/Rector/AbstractFalsyScalarRuleFixerRector.php
View file
Expand Up @@ -11,7 +11,7 @@
/**
* @see \Rector\Tests\Strict\Rector\BooleanNot\BooleanInBooleanNotRuleFixerRector\BooleanInBooleanNotRuleFixerRectorTest
*
* @internal
* @internal
*/
abstract class AbstractFalsyScalarRuleFixerRector extends AbstractScopeAwareRector implements AllowEmptyConfigurableRectorInterface
{
Expand Down
1 change: 0 additions & 1 deletion rules/Strict/Rector/BooleanNot/BooleanInBooleanNotRuleFixerRector.php
View file
Expand Up @@ -8,7 +8,6 @@
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\BooleanNot;
use PHPStan\Analyser\Scope;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Strict\NodeFactory\ExactCompareFactory;
use Rector\Strict\Rector\AbstractFalsyScalarRuleFixerRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
Expand Down
2 changes: 0 additions & 2 deletions rules/Strict/Rector/ClassMethod/AddConstructorParentCallRector.php
View file
Expand Up @@ -12,10 +12,8 @@
use PHPStan\Analyser\Scope;
use Rector\Core\Enum\ObjectReference;
use Rector\Core\NodeManipulator\Dependency\DependencyClassMethodDecorator;
use Rector\Core\Rector\AbstractRector;
use Rector\Core\Rector\AbstractScopeAwareRector;
use Rector\Core\ValueObject\MethodName;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

Expand Down
1 change: 0 additions & 1 deletion rules/Strict/Rector/Empty_/DisallowedEmptyRuleFixerRector.php
View file
Expand Up @@ -10,7 +10,6 @@
use PhpParser\Node\Expr\BooleanNot;
use PhpParser\Node\Expr\Empty_;
use PHPStan\Analyser\Scope;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Strict\NodeFactory\ExactCompareFactory;
use Rector\Strict\Rector\AbstractFalsyScalarRuleFixerRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
Expand Down
1 change: 0 additions & 1 deletion rules/Strict/Rector/If_/BooleanInIfConditionRuleFixerRector.php
View file
Expand Up @@ -8,7 +8,6 @@
use PhpParser\Node\Expr;
use PhpParser\Node\Stmt\If_;
use PHPStan\Analyser\Scope;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Strict\NodeFactory\ExactCompareFactory;
use Rector\Strict\Rector\AbstractFalsyScalarRuleFixerRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
Expand Down
1 change: 0 additions & 1 deletion rules/Strict/Rector/Ternary/BooleanInTernaryOperatorRuleFixerRector.php
View file
Expand Up @@ -8,7 +8,6 @@
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\Ternary;
use PHPStan\Analyser\Scope;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Strict\NodeFactory\ExactCompareFactory;
use Rector\Strict\Rector\AbstractFalsyScalarRuleFixerRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
Expand Down
1 change: 0 additions & 1 deletion rules/Strict/Rector/Ternary/DisallowedShortTernaryRuleFixerRector.php
View file
Expand Up @@ -9,7 +9,6 @@
use PhpParser\Node\Expr\FuncCall;
use PhpParser\Node\Expr\Ternary;
use PHPStan\Analyser\Scope;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Strict\NodeFactory\ExactCompareFactory;
use Rector\Strict\Rector\AbstractFalsyScalarRuleFixerRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
Expand Down

0 comments on commit d8da061

Please sign in to comment.