diff --git a/build/target-repository/docs/rector_rules_overview.md b/build/target-repository/docs/rector_rules_overview.md index 84fc575d3f4..8e7305bb0f0 100644 --- a/build/target-repository/docs/rector_rules_overview.md +++ b/build/target-repository/docs/rector_rules_overview.md @@ -1,4 +1,4 @@ -# 398 Rules Overview +# 399 Rules Overview
@@ -6,7 +6,7 @@ - [Arguments](#arguments) (5) -- [CodeQuality](#codequality) (75) +- [CodeQuality](#codequality) (76) - [CodingStyle](#codingstyle) (36) @@ -1478,6 +1478,25 @@ Simplify tautology ternary to value
+### SimplifyUselessLastVariableAssignRector + +Removes the latest useless variable assigns before a variable will return. + +- class: [`Rector\CodeQuality\Rector\FunctionLike\SimplifyUselessLastVariableAssignRector`](../rules/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector.php) + +```diff + function ($b) { +- $a = true; + if ($b === 1) { + return $b; + } +- return $a; ++ return true; + }; +``` + +
+ ### SimplifyUselessVariableRector Removes useless variable assigns diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/assign_ops.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/assign_ops.php.inc new file mode 100644 index 00000000000..ddb966e197b --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/assign_ops.php.inc @@ -0,0 +1,47 @@ + +----- + diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/empty_array.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/empty_array.php.inc new file mode 100644 index 00000000000..2f7e1da145e --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/empty_array.php.inc @@ -0,0 +1,36 @@ + +----- + diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/fixture.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/fixture.php.inc new file mode 100644 index 00000000000..a8db14c64d8 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/fixture.php.inc @@ -0,0 +1,91 @@ + $a++, + --$b => ++$a, + ]; + return $c; +}; + +?> +----- + $a++, + --$b => ++$a, + ]; + return $c; +}; + +?> diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/fixture2.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/fixture2.php.inc new file mode 100644 index 00000000000..aa51b2a4709 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/fixture2.php.inc @@ -0,0 +1,413 @@ +$parameter = 'this'; + } + +} + +$noErrorToo = null; +function ($noError = 'noError') use ($noErrorToo) { + +}; + +$used = true; + +function foo($foo) { + return preg_replace_callback('~~', function ($matches) { + return $matches[0]; + }, $foo); +} + +return $used; + +function ($values) { + $foo = ''; + + foreach ($values as $value) { + echo $foo . $value; + } +}; + +function () { + for ($i = 0; $i < 10; $i++) { + } +}; + +function ($values) { + foreach ($values as $value) { + $foo = 'foo' . $value; + } + echo $foo; +}; + +function ($values) { + list($a, $b) = $values; + return $a + $b; +}; + +function ($values) { + [$c, $d] = $values; + return $c * $d; +}; + +function ($values) { + $current = 'current'; + $next = 'next'; + + while ($next) { + if ($current) { + + } + + $current = false; + + if (true) { + foreach ($values as $value) { + $next = $value; + } + } + + do { + $previous = 'previous'; + } while ($previous); + } +}; + +function (&$parameter) { + $parameter = 'value-by-reference'; +}; + +function () use (&$inheritedVariable) { + $inheritedVariable = 'value-by-reference'; +}; + +function ($interval) { + $j = 0; + for ($i = $j; $i < 10; $i += $interval) { + } +}; + +function () { + static $static = false; + if ($static) { + return; + } + + $static = true; +}; + +function () { + $a = 'a'; + $b = 'b'; + + $this->compact; + + return compact('a', "b"); +}; + +function () { + $a = ''; + echo "$a"; +}; + +function () { + $a = ''; + echo "${a}"; +}; + +function () { + $a = ''; + echo "$a()"; +}; + +function () { + $a = ''; + echo <<a, $this->b) = [$a, $b]; + } + +} + +function () { + $i = 0; + while ($i++ <= 10) { + } +}; + +function () { + $i = 0; + do { + } while (++$i <= 10); +}; + +function () { + $i = 10; + while ($i-- > 0) { + } +}; + +function () { + $i = 10; + do { + } while (--$i > 0); +}; + +function ($data) { + $i = 0; + $c = ''; + foreach ($data as $c) { + $c = $i++; + } + echo $c; +}; + +function ($values) { + $expectedKey = 0; + + foreach ($values as $key => $value) { + if ($key !== $expectedKey++) { + return $value; + } + } + + return null; +}; + +?> +----- +$parameter = 'this'; + } + +} + +$noErrorToo = null; +function ($noError = 'noError') use ($noErrorToo) { + +}; + +function foo($foo) { + return preg_replace_callback('~~', function ($matches) { + return $matches[0]; + }, $foo); +} + +return true; + +function ($values) { + $foo = ''; + + foreach ($values as $value) { + echo $foo . $value; + } +}; + +function () { + for ($i = 0; $i < 10; $i++) { + } +}; + +function ($values) { + foreach ($values as $value) { + $foo = 'foo' . $value; + } + echo $foo; +}; + +function ($values) { + list($a, $b) = $values; + return $a + $b; +}; + +function ($values) { + [$c, $d] = $values; + return $c * $d; +}; + +function ($values) { + $current = 'current'; + $next = 'next'; + + while ($next) { + if ($current) { + + } + + $current = false; + + if (true) { + foreach ($values as $value) { + $next = $value; + } + } + + do { + $previous = 'previous'; + } while ($previous); + } +}; + +function (&$parameter) { + $parameter = 'value-by-reference'; +}; + +function () use (&$inheritedVariable) { + $inheritedVariable = 'value-by-reference'; +}; + +function ($interval) { + $j = 0; + for ($i = $j; $i < 10; $i += $interval) { + } +}; + +function () { + static $static = false; + if ($static) { + return; + } + + $static = true; +}; + +function () { + $a = 'a'; + $b = 'b'; + + $this->compact; + + return compact('a', "b"); +}; + +function () { + $a = ''; + echo "$a"; +}; + +function () { + $a = ''; + echo "${a}"; +}; + +function () { + $a = ''; + echo "$a()"; +}; + +function () { + $a = ''; + echo <<a, $this->b) = [$a, $b]; + } + +} + +function () { + $i = 0; + while ($i++ <= 10) { + } +}; + +function () { + $i = 0; + do { + } while (++$i <= 10); +}; + +function () { + $i = 10; + while ($i-- > 0) { + } +}; + +function () { + $i = 10; + do { + } while (--$i > 0); +}; + +function ($data) { + $i = 0; + $c = ''; + foreach ($data as $c) { + $c = $i++; + } + echo $c; +}; + +function ($values) { + $expectedKey = 0; + + foreach ($values as $key => $value) { + if ($key !== $expectedKey++) { + return $value; + } + } + + return null; +}; + +?> diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/nested_ifs.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/nested_ifs.php.inc new file mode 100644 index 00000000000..b1943e60bd7 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/nested_ifs.php.inc @@ -0,0 +1,52 @@ + +----- + diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_call_by_ref.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_call_by_ref.php.inc new file mode 100644 index 00000000000..08495b3d9d3 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_call_by_ref.php.inc @@ -0,0 +1,13 @@ + $value, + 'bar', + 'baz' => [ + 'qux', + ], + ]; + + if (\rand(0, 1)) { + echo ''; + } + + return $content; + } +} + +?> \ No newline at end of file diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_global_variable.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_global_variable.php.inc new file mode 100644 index 00000000000..529da4a3997 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_global_variable.php.inc @@ -0,0 +1,13 @@ +run($skipOnMethodCall); + return $content; + } +} diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_on_nested_return.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_on_nested_return.php.inc new file mode 100644 index 00000000000..c67ce6daeaf --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_on_nested_return.php.inc @@ -0,0 +1,14 @@ +getValue(); + + /** @var string $name */ + return $name; + } + + private function getValue() + { + return 'name'; + } +} diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_return_by_ref.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_return_by_ref.php.inc new file mode 100644 index 00000000000..9c57428626d --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/skip_return_by_ref.php.inc @@ -0,0 +1,25 @@ + 'foo', + 'bar', + 'baz' => [ + 'qux', + ], + ]; + + if (\rand(0, 1)) { + echo ''; + } + + return $content; + } +} + +?> +----- + 'foo', + 'bar', + 'baz' => [ + 'qux', + ], + ]; + } +} + +?> diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/variable_variable.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/variable_variable.php.inc new file mode 100644 index 00000000000..a493299deae --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/Fixture/variable_variable.php.inc @@ -0,0 +1,32 @@ + +----- + diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/SimplifyUselessLastVariableAssignRectorTest.php b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/SimplifyUselessLastVariableAssignRectorTest.php new file mode 100644 index 00000000000..bf0987382b1 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/SimplifyUselessLastVariableAssignRectorTest.php @@ -0,0 +1,32 @@ +doTestFile($filePath); + } + + /** + * @return Iterator> + */ + public function provideData(): Iterator + { + return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/config/configured_rule.php b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/config/configured_rule.php new file mode 100644 index 00000000000..3d5d3bdccc8 --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector/config/configured_rule.php @@ -0,0 +1,10 @@ +rule(SimplifyUselessLastVariableAssignRector::class); +}; diff --git a/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector.php b/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector.php new file mode 100644 index 00000000000..be117bae5a4 --- /dev/null +++ b/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessLastVariableAssignRector.php @@ -0,0 +1,293 @@ +> + */ + public function getNodeTypes(): array + { + return [StmtsAwareInterface::class]; + } + + /** + * @param StmtsAwareInterface $node + */ + public function refactor(Node $node): ?Node + { + $stmts = $node->stmts; + if ($stmts === null) { + return null; + } + + foreach ($stmts as $stmt) { + if (! $stmt instanceof Return_) { + continue; + } + + if ($this->shouldSkip($stmt)) { + return null; + } + + $assignStmt = $this->getLatestVariableAssignment($stmts, $stmt); + if (! $assignStmt instanceof Expression) { + return null; + } + + if ($this->shouldSkipOnAssignStmt($assignStmt)) { + return null; + } + + if ($this->isAssigmentUseless($stmts, $assignStmt, $stmt)) { + return null; + } + + $assign = $assignStmt->expr; + if (! $assign instanceof Assign && ! $assign instanceof AssignOp) { + return null; + } + + $this->removeNode($assignStmt); + return $this->processSimplifyUselessVariable($node, $stmt, $assign); + } + + return null; + } + + private function shouldSkip(Return_ $return): bool + { + $variable = $return->expr; + if (! $variable instanceof Variable) { + return true; + } + + if ($this->returnAnalyzer->hasByRefReturn($return)) { + return true; + } + + if ($this->variableAnalyzer->isStaticOrGlobal($variable)) { + return true; + } + + if ($this->variableAnalyzer->isUsedByReference($variable)) { + return true; + } + + $phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($return); + return ! $phpDocInfo->getVarType() instanceof MixedType; + } + + /** + * @param Stmt[] $stmts + */ + private function getLatestVariableAssignment(array $stmts, Return_ $return): ?Expression + { + $returnVariable = $return->expr; + if (! $returnVariable instanceof Variable) { + return null; + } + + //Search for the latest variable assigment + foreach (\array_reverse($stmts) as $stmt) { + if (! $stmt instanceof Expression) { + continue; + } + + $assignNode = $stmt->expr; + if (! $assignNode instanceof Assign && ! $assignNode instanceof AssignOp) { + continue; + } + + $currentVariableNode = $assignNode->var; + if (! $currentVariableNode instanceof Variable) { + continue; + } + + if ($this->nodeNameResolver->areNamesEqual($returnVariable, $currentVariableNode)) { + return $stmt; + } + } + + return null; + } + + private function shouldSkipOnAssignStmt(Expression $assignStmt): bool + { + if ($this->hasSomeComment($assignStmt)) { + return true; + } + + $assign = $assignStmt->expr; + if (! $assign instanceof Assign && ! $assign instanceof AssignOp) { + return true; + } + + $variable = $assign->var; + if (! $variable instanceof Variable) { + return true; + } + + $value = $assign->expr; + if ($this->exprAnalyzer->isDynamicExpr($value)) { + return true; + } + + if ($value instanceof Array_ && $this->arrayManipulator->isDynamicArray($value)) { + return true; + } + + return false; + } + + private function hasSomeComment(Expression $stmt): bool + { + if ($stmt->getComments() !== []) { + return true; + } + + return $stmt->getDocComment() !== null; + } + + /** + * @param Stmt[] $stmts + */ + private function isAssigmentUseless(array $stmts, Expression $assignStmt, Return_ $return): bool + { + $assign = $assignStmt->expr; + if (! $assign instanceof Assign && ! $assign instanceof AssignOp) { + return false; + } + + $variable = $assign->var; + if (! $variable instanceof Variable) { + return false; + } + + $nodesInRange = $this->getNodesInRange($stmts, $assignStmt, $return); + if ($nodesInRange === null) { + return false; + } + + //Find the variable usage + $variableUsageNodes = $this->betterNodeFinder->find( + $nodesInRange, + fn (Node $node): bool => $this->exprUsedInNodeAnalyzer->isUsed($node, $variable) + ); + + //Should be exactly used 2 times (assignment + return) + return count($variableUsageNodes) !== 2; + } + + /** + * @param Stmt[] $stmts + * @return Stmt[]|null + */ + private function getNodesInRange(array $stmts, Expression $start, Return_ $end): ?array + { + $resultStmts = []; + $wasStarted = false; + + foreach ($stmts as $stmt) { + if ($stmt === $start) { + $wasStarted = true; + } + + if ($wasStarted) { + $resultStmts[] = $stmt; + } + + if ($stmt === $end) { + return $resultStmts; + } + } + + if ($wasStarted) { + // This should not happen, if you land here check your given parameter + return null; + } + + return $resultStmts; + } + + private function processSimplifyUselessVariable( + StmtsAwareInterface $stmtsAware, + Return_ $return, + Assign|AssignOp $assign, + ): ?StmtsAwareInterface { + if (! $assign instanceof Assign) { + $binaryClass = $this->assignAndBinaryMap->getAlternative($assign); + if ($binaryClass === null) { + return null; + } + + $return->expr = new $binaryClass($assign->var, $assign->expr); + } else { + $return->expr = $assign->expr; + } + + return $stmtsAware; + } +} diff --git a/src/NodeAnalyzer/VariableAnalyzer.php b/src/NodeAnalyzer/VariableAnalyzer.php index 9fbac4b65e9..5fc6ddb1b97 100644 --- a/src/NodeAnalyzer/VariableAnalyzer.php +++ b/src/NodeAnalyzer/VariableAnalyzer.php @@ -5,6 +5,7 @@ namespace Rector\Core\NodeAnalyzer; use PhpParser\Node; +use PhpParser\Node\Expr\AssignRef; use PhpParser\Node\Expr\ClosureUse; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Param; @@ -73,11 +74,11 @@ public function isUsedByReference(Variable $variable): bool } $parentNode = $subNode->getAttribute(AttributeKey::PARENT_NODE); - if (! $parentNode instanceof ClosureUse) { - return false; + if ($parentNode instanceof ClosureUse) { + return $parentNode->byRef; } - return $parentNode->byRef; + return $parentNode instanceof AssignRef; }); }