From 9ff0b6a07aac47e1c71516fb2da93fb3a9081e3e Mon Sep 17 00:00:00 2001 From: TomasVotruba Date: Sat, 29 Feb 2020 23:49:30 +0100 Subject: [PATCH 1/3] decopule isNodePartOfAssign() method --- .../src/Rector/For_/ForToForeachRector.php | 45 +++++++++---------- .../Node/Manipulator/AssignManipulator.php | 35 ++++++++++++++- 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/rules/code-quality/src/Rector/For_/ForToForeachRector.php b/rules/code-quality/src/Rector/For_/ForToForeachRector.php index 61c662245e78..c28488272dc0 100644 --- a/rules/code-quality/src/Rector/For_/ForToForeachRector.php +++ b/rules/code-quality/src/Rector/For_/ForToForeachRector.php @@ -16,10 +16,10 @@ use PhpParser\Node\Expr\PreInc; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Stmt; -use PhpParser\Node\Stmt\Expression; use PhpParser\Node\Stmt\For_; use PhpParser\Node\Stmt\Foreach_; use Rector\Core\Exception\ShouldNotHappenException; +use Rector\Core\PhpParser\Node\Manipulator\AssignManipulator; use Rector\Core\Rector\AbstractRector; use Rector\Core\RectorDefinition\CodeSample; use Rector\Core\RectorDefinition\RectorDefinition; @@ -45,6 +45,16 @@ final class ForToForeachRector extends AbstractRector */ private $iteratedExpr; + /** + * @var AssignManipulator + */ + private $assignManipulator; + + public function __construct(AssignManipulator $assignManipulator) + { + $this->assignManipulator = $assignManipulator; + } + public function getDefinition(): RectorDefinition { return new RectorDefinition('Change for() to foreach() where useful', [ @@ -151,15 +161,17 @@ private function reset(): void private function matchInit(array $initExprs): void { foreach ($initExprs as $initExpr) { - if ($initExpr instanceof Assign) { - if ($this->isValue($initExpr->expr, 0)) { - $this->keyValueName = $this->getName($initExpr->var); - } + if (! $initExpr instanceof Assign) { + continue; + } - if ($initExpr->expr instanceof FuncCall && $this->isName($initExpr->expr, 'count')) { - $this->countValueName = $this->getName($initExpr->var); - $this->iteratedExpr = $initExpr->expr->args[0]->value; - } + if ($this->isValue($initExpr->expr, 0)) { + $this->keyValueName = $this->getName($initExpr->var); + } + + if ($initExpr->expr instanceof FuncCall && $this->isName($initExpr->expr, 'count')) { + $this->countValueName = $this->getName($initExpr->var); + $this->iteratedExpr = $initExpr->expr->args[0]->value; } } } @@ -232,7 +244,6 @@ private function useForeachVariableInStmts(Expr $expr, array $stmts): void } $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); - if ($this->isPartOfAssign($parentNode)) { return null; } @@ -281,18 +292,6 @@ private function isPartOfAssign(?Node $node): bool return false; } - $previousNode = $node; - $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); - - while ($parentNode !== null && ! $parentNode instanceof Expression) { - if ($parentNode instanceof Assign && $this->areNodesEqual($parentNode->var, $previousNode)) { - return true; - } - - $previousNode = $parentNode; - $parentNode = $parentNode->getAttribute(AttributeKey::PARENT_NODE); - } - - return false; + return $this->assignManipulator->isNodePartOfAssign($node); } } diff --git a/src/PhpParser/Node/Manipulator/AssignManipulator.php b/src/PhpParser/Node/Manipulator/AssignManipulator.php index 06edfc9fd77f..067aeff35959 100644 --- a/src/PhpParser/Node/Manipulator/AssignManipulator.php +++ b/src/PhpParser/Node/Manipulator/AssignManipulator.php @@ -15,6 +15,8 @@ use PhpParser\Node\Expr\PostInc; use PhpParser\Node\Expr\PreDec; use PhpParser\Node\Expr\PreInc; +use PhpParser\Node\Stmt\Expression; +use Rector\Core\PhpParser\Printer\BetterStandardPrinter; use Rector\NodeNameResolver\NodeNameResolver; use Rector\NodeTypeResolver\Node\AttributeKey; @@ -41,10 +43,19 @@ final class AssignManipulator */ private $propertyFetchManipulator; - public function __construct(NodeNameResolver $nodeNameResolver, PropertyFetchManipulator $propertyFetchManipulator) - { + /** + * @var BetterStandardPrinter + */ + private $betterStandardPrinter; + + public function __construct( + NodeNameResolver $nodeNameResolver, + PropertyFetchManipulator $propertyFetchManipulator, + BetterStandardPrinter $betterStandardPrinter + ) { $this->nodeNameResolver = $nodeNameResolver; $this->propertyFetchManipulator = $propertyFetchManipulator; + $this->betterStandardPrinter = $betterStandardPrinter; } /** @@ -141,6 +152,26 @@ public function isNodeLeftPartOfAssign(Node $node): bool return false; } + public function isNodePartOfAssign(Node $node): bool + { + $previousNode = $node; + $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); + + while ($parentNode !== null && ! $parentNode instanceof Expression) { + if ($parentNode instanceof Assign && $this->betterStandardPrinter->areNodesEqual( + $parentNode->var, + $previousNode + )) { + return true; + } + + $previousNode = $parentNode; + $parentNode = $parentNode->getAttribute(AttributeKey::PARENT_NODE); + } + + return false; + } + private function isValueModifyingNode(Node $node): bool { foreach (self::MODIFYING_NODES as $modifyingNode) { From 83eca131e9e6a4749e6b4d2b91ff6d2f497c9141 Mon Sep 17 00:00:00 2001 From: TomasVotruba Date: Sat, 29 Feb 2020 23:54:40 +0100 Subject: [PATCH 2/3] improve ForToForeachRector --- .../src/Rector/For_/ForToForeachRector.php | 39 ++++++++++++------- .../Rector/HttpKernel/GetRequestRector.php | 17 +++++--- .../Node/Manipulator/AssignManipulator.php | 6 ++- 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/rules/code-quality/src/Rector/For_/ForToForeachRector.php b/rules/code-quality/src/Rector/For_/ForToForeachRector.php index c28488272dc0..bea39b489d55 100644 --- a/rules/code-quality/src/Rector/For_/ForToForeachRector.php +++ b/rules/code-quality/src/Rector/For_/ForToForeachRector.php @@ -133,15 +133,7 @@ public function refactor(Node $node): ?Node } $iteratedVariableSingle = Inflector::singularize($iteratedVariable); - - $foreach = new Foreach_($this->iteratedExpr, new Variable($iteratedVariableSingle)); - $foreach->stmts = $node->stmts; - - if ($this->keyValueName === null) { - return null; - } - - $foreach->keyVar = new Variable($this->keyValueName); + $foreach = $this->createForeach($node, $iteratedVariableSingle); $this->useForeachVariableInStmts($foreach->valueVar, $foreach->stmts); @@ -169,7 +161,7 @@ private function matchInit(array $initExprs): void $this->keyValueName = $this->getName($initExpr->var); } - if ($initExpr->expr instanceof FuncCall && $this->isName($initExpr->expr, 'count')) { + if ($this->isFuncCallName($initExpr->expr, 'count')) { $this->countValueName = $this->getName($initExpr->var); $this->iteratedExpr = $initExpr->expr->args[0]->value; } @@ -194,7 +186,7 @@ private function isConditionMatch(array $condExprs): bool } // count($values) - if ($condExprs[0]->right instanceof FuncCall && $this->isName($condExprs[0]->right, 'count')) { + if ($this->isFuncCallName($condExprs[0]->right, 'count')) { /** @var FuncCall $countFuncCall */ $countFuncCall = $condExprs[0]->right; $this->iteratedExpr = $countFuncCall->args[0]->value; @@ -244,7 +236,7 @@ private function useForeachVariableInStmts(Expr $expr, array $stmts): void } $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); - if ($this->isPartOfAssign($parentNode)) { + if ($this->assignManipulator->isNodePartOfAssign($parentNode)) { return null; } @@ -286,12 +278,29 @@ private function isSmallerOrGreater(array $condExprs, string $keyValueName, stri return false; } - private function isPartOfAssign(?Node $node): bool + private function isFuncCallName(Expr $expr, string $name): bool { - if ($node === null) { + if (! $expr instanceof FuncCall) { return false; } - return $this->assignManipulator->isNodePartOfAssign($node); + return $this->isName($expr, $name); + } + + private function createForeach(For_ $for, string $iteratedVariableName): Foreach_ + { + if ($this->iteratedExpr === null) { + throw new ShouldNotHappenException(); + } + + if ($this->keyValueName === null) { + throw new ShouldNotHappenException(); + } + + $foreach = new Foreach_($this->iteratedExpr, new Variable($iteratedVariableName)); + $foreach->stmts = $for->stmts; + $foreach->keyVar = new Variable($this->keyValueName); + + return $foreach; } } diff --git a/rules/symfony/src/Rector/HttpKernel/GetRequestRector.php b/rules/symfony/src/Rector/HttpKernel/GetRequestRector.php index 4b61d216a08d..c24782ac107f 100644 --- a/rules/symfony/src/Rector/HttpKernel/GetRequestRector.php +++ b/rules/symfony/src/Rector/HttpKernel/GetRequestRector.php @@ -139,7 +139,7 @@ private function isGetRequestInAction(Node $node): bool } // must be $this->getRequest() in controller - if (! $this->isName($node->var, 'this')) { + if (! $this->isThisVariable($node->var)) { return false; } @@ -220,15 +220,20 @@ private function containsGetRequestMethod(Node $node): bool return false; } - if (! $node->var instanceof Variable) { - return false; - } - - if (! $this->isName($node->var, 'this')) { + if (! $this->isThisVariable($node->var)) { return false; } return $this->isName($node->name, 'getRequest'); }); } + + private function isThisVariable(Node $node): bool + { + if (! $node instanceof Variable) { + return false; + } + + return $this->isName($node, 'this'); + } } diff --git a/src/PhpParser/Node/Manipulator/AssignManipulator.php b/src/PhpParser/Node/Manipulator/AssignManipulator.php index 067aeff35959..36fd6bf4f3c6 100644 --- a/src/PhpParser/Node/Manipulator/AssignManipulator.php +++ b/src/PhpParser/Node/Manipulator/AssignManipulator.php @@ -152,8 +152,12 @@ public function isNodeLeftPartOfAssign(Node $node): bool return false; } - public function isNodePartOfAssign(Node $node): bool + public function isNodePartOfAssign(?Node $node): bool { + if ($node === null) { + return false; + } + $previousNode = $node; $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); From fe86fc43655014ddbfff3a2b2abe2660dd024e65 Mon Sep 17 00:00:00 2001 From: TomasVotruba Date: Sun, 1 Mar 2020 00:06:45 +0100 Subject: [PATCH 3/3] make use of isFuncCallName() method --- .../Rector/Echo_/CakePHPTemplateLinkToTwigRector.php | 3 +-- .../BooleanAnd/SimplifyEmptyArrayCheckRector.php | 3 +-- .../src/Rector/For_/ForToForeachRector.php | 9 --------- .../src/Rector/Identical/SimplifyArraySearchRector.php | 2 +- .../src/Rector/If_/ExplicitBoolCompareRector.php | 3 +-- .../FuncCall/ParseStrWithResultArgumentRector.php | 2 +- .../FuncCall/ArraySpreadInsteadOfArrayMergeRector.php | 2 +- src/Rector/AbstractRector/NameResolverTrait.php | 10 ++++++++++ 8 files changed, 16 insertions(+), 18 deletions(-) diff --git a/rules/cakephp-to-symfony/src/Rector/Echo_/CakePHPTemplateLinkToTwigRector.php b/rules/cakephp-to-symfony/src/Rector/Echo_/CakePHPTemplateLinkToTwigRector.php index c83dac881aee..ecedc7467125 100644 --- a/rules/cakephp-to-symfony/src/Rector/Echo_/CakePHPTemplateLinkToTwigRector.php +++ b/rules/cakephp-to-symfony/src/Rector/Echo_/CakePHPTemplateLinkToTwigRector.php @@ -7,7 +7,6 @@ use Nette\Utils\Html; use PhpParser\Node; use PhpParser\Node\Expr; -use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Stmt\Echo_; @@ -81,7 +80,7 @@ public function refactor(Node $node): ?Node # e.g. |trans https://symfony.com/doc/current/translation/templates.html#using-twig-filters $labelFilters = []; - if ($label instanceof FuncCall && $this->isName($label, '__')) { + if ($this->isFuncCallName($label, '__')) { $labelFilters[] = 'trans'; $label = $label->args[0]->value; } diff --git a/rules/code-quality/src/Rector/BooleanAnd/SimplifyEmptyArrayCheckRector.php b/rules/code-quality/src/Rector/BooleanAnd/SimplifyEmptyArrayCheckRector.php index f8df04cf5293..bf51182f868f 100644 --- a/rules/code-quality/src/Rector/BooleanAnd/SimplifyEmptyArrayCheckRector.php +++ b/rules/code-quality/src/Rector/BooleanAnd/SimplifyEmptyArrayCheckRector.php @@ -9,7 +9,6 @@ use PhpParser\Node\Expr\BinaryOp\BooleanAnd; use PhpParser\Node\Expr\BinaryOp\Identical; use PhpParser\Node\Expr\Empty_; -use PhpParser\Node\Expr\FuncCall; use Rector\Core\PhpParser\Node\Manipulator\BinaryOpManipulator; use Rector\Core\Rector\AbstractRector; use Rector\Core\RectorDefinition\CodeSample; @@ -57,7 +56,7 @@ public function refactor(Node $node): ?Node $node, // is_array(...) function (Node $node): bool { - return $node instanceof FuncCall && $this->isName($node, 'is_array'); + return $this->isFuncCallName($node, 'is_array'); }, Empty_::class ); diff --git a/rules/code-quality/src/Rector/For_/ForToForeachRector.php b/rules/code-quality/src/Rector/For_/ForToForeachRector.php index bea39b489d55..d0d9be5c9a2d 100644 --- a/rules/code-quality/src/Rector/For_/ForToForeachRector.php +++ b/rules/code-quality/src/Rector/For_/ForToForeachRector.php @@ -278,15 +278,6 @@ private function isSmallerOrGreater(array $condExprs, string $keyValueName, stri return false; } - private function isFuncCallName(Expr $expr, string $name): bool - { - if (! $expr instanceof FuncCall) { - return false; - } - - return $this->isName($expr, $name); - } - private function createForeach(For_ $for, string $iteratedVariableName): Foreach_ { if ($this->iteratedExpr === null) { diff --git a/rules/code-quality/src/Rector/Identical/SimplifyArraySearchRector.php b/rules/code-quality/src/Rector/Identical/SimplifyArraySearchRector.php index 3fb9cb99f1e2..cbf1d459f1e1 100644 --- a/rules/code-quality/src/Rector/Identical/SimplifyArraySearchRector.php +++ b/rules/code-quality/src/Rector/Identical/SimplifyArraySearchRector.php @@ -64,7 +64,7 @@ public function refactor(Node $node): ?Node $matchedNodes = $this->binaryOpManipulator->matchFirstAndSecondConditionNode( $node, function (Node $node): bool { - return $node instanceof FuncCall && $this->isName($node, 'array_search'); + return $this->isFuncCallName($node, 'array_search'); }, function (Node $node): bool { return $this->isBool($node); diff --git a/rules/code-quality/src/Rector/If_/ExplicitBoolCompareRector.php b/rules/code-quality/src/Rector/If_/ExplicitBoolCompareRector.php index 4e46cc8feb8d..11dece117769 100644 --- a/rules/code-quality/src/Rector/If_/ExplicitBoolCompareRector.php +++ b/rules/code-quality/src/Rector/If_/ExplicitBoolCompareRector.php @@ -13,7 +13,6 @@ use PhpParser\Node\Expr\BinaryOp\NotIdentical; use PhpParser\Node\Expr\BooleanNot; use PhpParser\Node\Expr\Cast\Bool_; -use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\Ternary; use PhpParser\Node\Scalar\DNumber; use PhpParser\Node\Scalar\LNumber; @@ -113,7 +112,7 @@ public function refactor(Node $node): ?Node private function resolveNewConditionNode(Expr $expr, bool $isNegated): ?BinaryOp { // various cases - if ($expr instanceof FuncCall && $this->isName($expr, 'count')) { + if ($this->isFuncCallName($expr, 'count')) { return $this->resolveCount($isNegated, $expr); } diff --git a/rules/php-72/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php b/rules/php-72/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php index d5fe70cd9618..0c5e73d24435 100644 --- a/rules/php-72/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php +++ b/rules/php-72/src/Rector/FuncCall/ParseStrWithResultArgumentRector.php @@ -74,7 +74,7 @@ public function refactor(Node $node): ?Node } $this->traverseNodesWithCallable($nextExpression, function (Node $node) use ($resultVariable): ?Variable { - if ($node instanceof FuncCall && $this->isName($node, 'get_defined_vars')) { + if ($this->isFuncCallName($node, 'get_defined_vars')) { return $resultVariable; } diff --git a/rules/php-74/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php b/rules/php-74/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php index 6b267f6afbdb..248090379f49 100644 --- a/rules/php-74/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php +++ b/rules/php-74/src/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector.php @@ -173,7 +173,7 @@ private function createUnpackedArrayItem(Expr $expr): ArrayItem private function isIteratorToArrayFuncCall(Expr $expr): bool { - return $expr instanceof FuncCall && $this->isName($expr, 'iterator_to_array'); + return $this->isFuncCallName($expr, 'iterator_to_array'); } private function isConstantArrayTypeWithStringKeyType(Type $type): bool diff --git a/src/Rector/AbstractRector/NameResolverTrait.php b/src/Rector/AbstractRector/NameResolverTrait.php index 03d315d868bb..215d272a7fa5 100644 --- a/src/Rector/AbstractRector/NameResolverTrait.php +++ b/src/Rector/AbstractRector/NameResolverTrait.php @@ -5,6 +5,7 @@ namespace Rector\Core\Rector\AbstractRector; use PhpParser\Node; +use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Identifier; use PhpParser\Node\Name; use Rector\CodingStyle\Naming\ClassNaming; @@ -65,4 +66,13 @@ protected function getShortName($name): string { return $this->classNaming->getShortName($name); } + + protected function isFuncCallName(Node $node, string $name): bool + { + if (! $node instanceof FuncCall) { + return false; + } + + return $this->isName($node, $name); + } }