Refactor SymplifyUselessVariableRector to avoid using PREVIOUS_NODE, use current scope instead (#2045)

This commit is contained in:
Tomas Votruba 2022-04-10 20:12:34 +02:00 committed by GitHub
parent b9a912bb5d
commit a2ee46d16e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
23 changed files with 250 additions and 276 deletions

View File

@ -1496,7 +1496,7 @@ Simplify tautology ternary to value
Removes useless variable assigns
- class: [`Rector\CodeQuality\Rector\Return_\SimplifyUselessVariableRector`](../rules/CodeQuality/Rector/Return_/SimplifyUselessVariableRector.php)
- class: [`Rector\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector`](../rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php)
```diff
function () {

View File

@ -42,6 +42,7 @@ use Rector\CodeQuality\Rector\FuncCall\SimplifyStrposLowerRector;
use Rector\CodeQuality\Rector\FuncCall\SingleInArrayToCompareRector;
use Rector\CodeQuality\Rector\FuncCall\UnwrapSprintfOneArgumentRector;
use Rector\CodeQuality\Rector\FunctionLike\RemoveAlwaysTrueConditionSetInConstructorRector;
use Rector\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector;
use Rector\CodeQuality\Rector\Identical\BooleanNotIdenticalToNotIdenticalRector;
use Rector\CodeQuality\Rector\Identical\FlipTypeControlToUseExclusiveTypeRector;
use Rector\CodeQuality\Rector\Identical\GetClassToInstanceOfRector;
@ -65,7 +66,6 @@ use Rector\CodeQuality\Rector\LogicalAnd\LogicalToBooleanRector;
use Rector\CodeQuality\Rector\New_\NewStaticToNewSelfRector;
use Rector\CodeQuality\Rector\NotEqual\CommonNotEqualRector;
use Rector\CodeQuality\Rector\PropertyFetch\ExplicitMethodCallOverMagicGetSetRector;
use Rector\CodeQuality\Rector\Return_\SimplifyUselessVariableRector;
use Rector\CodeQuality\Rector\Switch_\SingularSwitchToIfRector;
use Rector\CodeQuality\Rector\Ternary\ArrayKeyExistsTernaryThenValueToCoalescingRector;
use Rector\CodeQuality\Rector\Ternary\SimplifyDuplicatedTernaryRector;

View File

@ -2,7 +2,7 @@
declare(strict_types=1);
use Rector\CodeQuality\Rector\Return_\SimplifyUselessVariableRector;
use Rector\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector;
use Rector\DeadCode\Rector\Array_\RemoveDuplicatedArrayKeyRector;
use Rector\DeadCode\Rector\Assign\RemoveDoubleAssignRector;
use Rector\DeadCode\Rector\Assign\RemoveUnusedVariableAssignRector;

View File

@ -570,3 +570,6 @@ parameters:
-
message: '#Call to static method Webmozart\\Assert\\Assert\:\:allString\(\) with array<string> will always evaluate to true#'
path: packages/Config/RectorConfig.php
# foreaching all stmts
- '#Cognitive complexity for "Rector\\CodeQuality\\Rector\\FunctionLike\\SimplifyUselessVariableRector\:\:refactor\(\)" is 1\d+, keep it under 10#'

View File

@ -0,0 +1,47 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function () {
$b += 1;
return $b;
};
function () {
$e /= 1;
return $e;
};
function () {
$f **= 1;
return $f;
};
function () {
$m .= 1;
return $m;
};
?>
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function () {
return $b + 1;
};
function () {
return $e / 1;
};
function () {
return $f ** 1;
};
function () {
return $m . 1;
};
?>

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function ClosureUseNOTByReference()
{
@ -21,7 +21,7 @@ function ClosureUseNOTByReference()
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function ClosureUseNOTByReference()
{

View File

@ -0,0 +1,88 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function () {
$a = true;
return $a;
};
function sameVariableInDifferentScope()
{
$n = array_map(function () {
return $n + 1;
}, []);
return $n;
}
function moreVariableOneWithoutAssigment() {
$o++;
$o = 10;
return $o;
}
function assigmentAsFunctionParametr() {
doSomething($p = 0);
return $p;
}
function assigmentAfterAssignment() {
doSomething($qq = $q = 0);
return $q;
}
function () {
$a = 1;
$b = 1;
$c = [
$b-- => $a++,
--$b => ++$a,
];
return $c;
};
?>
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function () {
return true;
};
function sameVariableInDifferentScope()
{
return array_map(function () {
return $n + 1;
}, []);
}
function moreVariableOneWithoutAssigment() {
$o++;
return 10;
}
function assigmentAsFunctionParametr() {
doSomething($p = 0);
return $p;
}
function assigmentAfterAssignment() {
doSomething($qq = $q = 0);
return $q;
}
function () {
$a = 1;
$b = 1;
return [
$b-- => $a++,
--$b => ++$a,
];
};
?>

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function simplifyUselessVariable3()
{
@ -24,7 +24,7 @@ function simplifyUselessVariable3()
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function simplifyUselessVariable3()
{

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function simplifyUselessVariable4()
{
@ -17,7 +17,7 @@ function simplifyUselessVariable4()
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function simplifyUselessVariable4()
{

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
class KeepVarDoc
{

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
// covers https://github.com/Sylius/Sylius/pull/9902/files#r231437165
function simplifyUselessVariable5()

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
function SkipClosureUseByReference()
{
@ -15,4 +15,4 @@ function SkipClosureUseByReference()
}
return false;
}
}

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
class SkipFixture2
{

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
$conn = new \stdClass;

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
class Foobar {
function & test()

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
class SkipStaticVariable
{

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
use DateTimeImmutable;

View File

@ -1,6 +1,6 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
use Exception;
@ -24,7 +24,7 @@ final class SpecialClassNameWithAnonymousClass
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\Fixture;
use Exception;

View File

@ -2,7 +2,7 @@
declare(strict_types=1);
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector;
namespace Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector;
use Iterator;
use Rector\Testing\PHPUnit\AbstractRectorTestCase;

View File

@ -2,7 +2,7 @@
declare(strict_types=1);
use Rector\CodeQuality\Rector\Return_\SimplifyUselessVariableRector;
use Rector\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
return static function (ContainerConfigurator $containerConfigurator): void {

View File

@ -1,196 +0,0 @@
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
function () {
$a = true;
return $a;
};
function () {
$b += 1;
return $b;
};
function () {
$c -= 1;
return $c;
};
function () {
$d *= 1;
return $d;
};
function () {
$e /= 1;
return $e;
};
function () {
$f **= 1;
return $f;
};
function () {
$g %= 1;
return $g;
};
function () {
$h &= 1;
return $h;
};
function () {
$i |= 1;
return $i;
};
function () {
$j ^= 1;
return $j;
};
function () {
$k <<= 1;
return $k;
};
function () {
$l >>= 1;
return $l;
};
function () {
$m .= 1;
return $m;
};
function sameVariableInDifferentScope()
{
$n = array_map(function () {
return $n + 1;
}, []);
return $n;
}
function moreVariableOneWithoutAssigment() {
$o++;
$o = 10;
return $o;
}
function assigmentAsFunctionParametr() {
doSomething($p = 0);
return $p;
}
function assigmentAfterAssignment() {
doSomething($qq = $q = 0);
return $q;
}
function () {
$a = 1;
$b = 1;
$c = [
$b-- => $a++,
--$b => ++$a,
];
return $c;
};
?>
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\Fixture;
function () {
return true;
};
function () {
return $b + 1;
};
function () {
return $c - 1;
};
function () {
return $d * 1;
};
function () {
return $e / 1;
};
function () {
return $f ** 1;
};
function () {
return $g % 1;
};
function () {
return $h & 1;
};
function () {
return $i | 1;
};
function () {
return $j ^ 1;
};
function () {
return $k << 1;
};
function () {
return $l >> 1;
};
function () {
return $m . 1;
};
function sameVariableInDifferentScope()
{
return array_map(function () {
return $n + 1;
}, []);
}
function moreVariableOneWithoutAssigment() {
$o++;
return 10;
}
function assigmentAsFunctionParametr() {
doSomething($p = 0);
return $p;
}
function assigmentAfterAssignment() {
doSomething($qq = $q = 0);
return $q;
}
function () {
$a = 1;
$b = 1;
return [
$b-- => $a++,
--$b => ++$a,
];
};
?>

View File

@ -0,0 +1,27 @@
<?php
declare(strict_types=1);
namespace Rector\CodeQuality\NodeAnalyzer;
use PhpParser\Node\FunctionLike;
use PhpParser\Node\Stmt\Return_;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
final class ReturnAnalyzer
{
public function __construct(
private readonly BetterNodeFinder $betterNodeFinder
) {
}
public function hasByRefReturn(Return_ $return): bool
{
$parentFunctionLike = $this->betterNodeFinder->findParentType($return, FunctionLike::class);
if ($parentFunctionLike instanceof FunctionLike) {
return $parentFunctionLike->returnsByRef();
}
return false;
}
}

View File

@ -2,17 +2,18 @@
declare(strict_types=1);
namespace Rector\CodeQuality\Rector\Return_;
namespace Rector\CodeQuality\Rector\FunctionLike;
use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\AssignOp;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\FunctionLike;
use PhpParser\Node\Stmt;
use PhpParser\Node\Stmt\Expression;
use PhpParser\Node\Stmt\Return_;
use PHPStan\Type\MixedType;
use Rector\CodeQuality\NodeAnalyzer\ReturnAnalyzer;
use Rector\Core\NodeAnalyzer\CallAnalyzer;
use Rector\Core\NodeAnalyzer\VariableAnalyzer;
use Rector\Core\PhpParser\Node\AssignAndBinaryMap;
@ -23,14 +24,15 @@ use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
/**
* @see Based on https://github.com/slevomat/coding-standard/blob/master/SlevomatCodingStandard/Sniffs/Variables/UselessVariableSniff.php
* @see \Rector\Tests\CodeQuality\Rector\Return_\SimplifyUselessVariableRector\SimplifyUselessVariableRectorTest
* @see \Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\SimplifyUselessVariableRectorTest
*/
final class SimplifyUselessVariableRector extends AbstractRector
{
public function __construct(
private readonly AssignAndBinaryMap $assignAndBinaryMap,
private readonly VariableAnalyzer $variableAnalyzer,
private readonly CallAnalyzer $callAnalyzer
private readonly CallAnalyzer $callAnalyzer,
private readonly ReturnAnalyzer $returnAnalyzer,
) {
}
@ -59,101 +61,104 @@ CODE_SAMPLE
*/
public function getNodeTypes(): array
{
return [Return_::class];
return [FunctionLike::class];
}
/**
* @param Return_ $node
* @param FunctionLike $node
*/
public function refactor(Node $node): ?Node
{
if ($this->shouldSkip($node)) {
$stmts = $node->getStmts();
if ($stmts === null) {
return null;
}
/** @var Expression $previousExpression */
$previousExpression = $node->getAttribute(AttributeKey::PREVIOUS_NODE);
/** @var Assign|AssignOp $previousNode */
$previousNode = $previousExpression->expr;
$previousVariableNode = $previousNode->var;
$previousStmt = null;
$hasChanged = false;
if ($this->hasSomeComment($previousVariableNode)) {
return null;
}
if ($previousNode instanceof Assign) {
if ($this->isReturnWithVarAnnotation($node)) {
return null;
foreach ($stmts as $stmt) {
if ($previousStmt === null || ! $stmt instanceof Return_) {
$previousStmt = $stmt;
continue;
}
$node->expr = $previousNode->expr;
}
if ($previousNode instanceof AssignOp) {
$binaryClass = $this->assignAndBinaryMap->getAlternative($previousNode);
if ($binaryClass === null) {
return null;
if ($this->shouldSkipStmt($stmt, $previousStmt)) {
$previousStmt = $stmt;
continue;
}
$node->expr = new $binaryClass($previousNode->var, $previousNode->expr);
/** @var Expression $previousStmt */
/** @var Assign|AssignOp $previousNode */
$previousNode = $previousStmt->expr;
/** @var Return_ $stmt */
if ($previousNode instanceof Assign) {
if ($this->isReturnWithVarAnnotation($stmt)) {
continue;
}
$stmt->expr = $previousNode->expr;
} else {
$binaryClass = $this->assignAndBinaryMap->getAlternative($previousNode);
if ($binaryClass === null) {
continue;
}
$stmt->expr = new $binaryClass($previousNode->var, $previousNode->expr);
}
$this->removeNode($previousStmt);
$hasChanged = true;
$previousStmt = $stmt;
}
$this->removeNode($previousNode);
if ($hasChanged) {
return $node;
}
return $node;
return null;
}
private function hasByRefReturn(Return_ $return): bool
private function shouldSkipStmt(Return_ $return, Stmt $previousStmt): bool
{
$node = $return;
while ($node = $node->getAttribute(AttributeKey::PARENT_NODE)) {
if ($node instanceof FunctionLike) {
return $node->returnsByRef();
}
if (! $node instanceof Node) {
break;
}
if ($this->hasSomeComment($previousStmt)) {
return true;
}
return false;
}
private function shouldSkip(Return_ $return): bool
{
if (! $return->expr instanceof Variable) {
return true;
}
if ($this->hasByRefReturn($return)) {
if ($this->returnAnalyzer->hasByRefReturn($return)) {
return true;
}
/** @var Variable $variableNode */
$variableNode = $return->expr;
/** @var Variable $variable */
$variable = $return->expr;
$previousExpression = $return->getAttribute(AttributeKey::PREVIOUS_NODE);
if (! $previousExpression instanceof Expression) {
if (! $previousStmt instanceof Expression) {
return true;
}
// is variable part of single assign
$previousNode = $previousExpression->expr;
$previousNode = $previousStmt->expr;
if (! $previousNode instanceof AssignOp && ! $previousNode instanceof Assign) {
return true;
}
// is the same variable
if (! $this->nodeComparator->areNodesEqual($previousNode->var, $variableNode)) {
if (! $this->nodeComparator->areNodesEqual($previousNode->var, $variable)) {
return true;
}
if ($this->isPreviousExpressionVisuallySimilar($previousExpression, $previousNode)) {
if ($this->isPreviousExpressionVisuallySimilar($previousStmt, $previousNode)) {
return true;
}
if ($this->variableAnalyzer->isStaticOrGlobal($variableNode)) {
if ($this->variableAnalyzer->isStaticOrGlobal($variable)) {
return true;
}
@ -161,16 +166,16 @@ CODE_SAMPLE
return true;
}
return $this->variableAnalyzer->isUsedByReference($variableNode);
return $this->variableAnalyzer->isUsedByReference($variable);
}
private function hasSomeComment(Expr $expr): bool
private function hasSomeComment(Stmt $stmt): bool
{
if ($expr->getComments() !== []) {
if ($stmt->getComments() !== []) {
return true;
}
return $expr->getDocComment() !== null;
return $stmt->getDocComment() !== null;
}
private function isReturnWithVarAnnotation(Return_ $return): bool