From ee1f60682763b73140f1ecd40109a1fc5233d358 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 12 May 2019 18:26:23 +0200 Subject: [PATCH] [CodeQuality] Add ForToForeachRector --- composer.json | 1 + config/level/code-quality/code-quality.yaml | 1 + .../src/Rector/For_/ForToForeachRector.php | 307 ++++++++++++++++++ .../Fixture/fixture.php.inc | 51 +++ .../Fixture/for_with_count.php.inc | 29 ++ .../Fixture/for_with_switched_compare.php.inc | 29 ++ .../ForToForeachRectorTest.php | 23 ++ .../src/Command/DumpNodesCommand.php | 3 +- .../Configuration/ConfigurationFactory.php | 4 +- .../NodeTypeResolver/src/NodeTypeResolver.php | 3 +- .../ClosureToArrowFunctionRectorTest.php | 2 +- ...raySpreadInsteadOfArrayMergeRectorTest.php | 2 +- phpstan.neon | 2 + rector.yaml | 3 +- 14 files changed, 454 insertions(+), 6 deletions(-) create mode 100644 packages/CodeQuality/src/Rector/For_/ForToForeachRector.php create mode 100644 packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/fixture.php.inc create mode 100644 packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_count.php.inc create mode 100644 packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_switched_compare.php.inc create mode 100644 packages/CodeQuality/tests/Rector/For_/ForToForeachRector/ForToForeachRectorTest.php diff --git a/composer.json b/composer.json index 9677be760644..e9e9c2a44598 100644 --- a/composer.json +++ b/composer.json @@ -7,6 +7,7 @@ "require": { "php": "^7.1", "composer/xdebug-handler": "^1.3", + "doctrine/inflector": "^1.3", "jean85/pretty-package-versions": "^1.2", "jetbrains/phpstorm-stubs": "^2019.1", "nette/robot-loader": "^3.1", diff --git a/config/level/code-quality/code-quality.yaml b/config/level/code-quality/code-quality.yaml index c4bbab1ea67f..8eb115aec3fb 100644 --- a/config/level/code-quality/code-quality.yaml +++ b/config/level/code-quality/code-quality.yaml @@ -33,3 +33,4 @@ services: Rector\CodeQuality\Rector\Identical\BooleanNotIdenticalToNotIdenticalRector: ~ Rector\CodeQuality\Rector\Array_\CallableThisArrayToAnonymousFunctionRector: ~ Rector\CodeQuality\Rector\LogicalAnd\AndAssignsToSeparateLinesRector: ~ + Rector\CodeQuality\Rector\For_\ForToForeachRector: ~ diff --git a/packages/CodeQuality/src/Rector/For_/ForToForeachRector.php b/packages/CodeQuality/src/Rector/For_/ForToForeachRector.php new file mode 100644 index 000000000000..db7c9709edc1 --- /dev/null +++ b/packages/CodeQuality/src/Rector/For_/ForToForeachRector.php @@ -0,0 +1,307 @@ +callableNodeTraverser = $callableNodeTraverser; + } + + public function getDefinition(): RectorDefinition + { + return new RectorDefinition('Change for() to foreach() where useful', [ + new CodeSample( + <<<'CODE_SAMPLE' +class SomeClass +{ + public function run($tokens) + { + for ($i = 0, $c = count($tokens); $i < $c; ++$i) { + if ($tokens[$i][0] === T_STRING && $tokens[$i][1] === 'fn') { + $previousNonSpaceToken = $this->getPreviousNonSpaceToken($tokens, $i); + if ($previousNonSpaceToken !== null && $previousNonSpaceToken[0] === T_OBJECT_OPERATOR) { + continue; + } + $tokens[$i][0] = self::T_FN; + } + } + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +class SomeClass +{ + public function run($tokens) + { + foreach ($tokens as $i => $token) { + if ($token[0] === T_STRING && $token[1] === 'fn') { + $previousNonSpaceToken = $this->getPreviousNonSpaceToken($tokens, $i); + if ($previousNonSpaceToken !== null && $previousNonSpaceToken[0] === T_OBJECT_OPERATOR) { + continue; + } + $tokens[$i][0] = self::T_FN; + } + } + } +} +CODE_SAMPLE + ), + ]); + } + + /** + * @return string[] + */ + public function getNodeTypes(): array + { + return [For_::class]; + } + + /** + * @param For_ $node + */ + public function refactor(Node $node): ?Node + { + $this->reset(); + + $this->matchInit((array) $node->init); + + if (! $this->isConditionMatch((array) $node->cond)) { + return null; + } + + if (! $this->isLoopMatch((array) $node->loop)) { + return null; + } + + if ($this->iteratedExpr === null || $this->keyValueName === null) { + return null; + } + + $iteratedVariable = $this->getName($this->iteratedExpr); + if ($iteratedVariable === null) { + return null; + } + + $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); + + $this->useForeachVariableInStmts($foreach->valueVar, $foreach->stmts); + + return $foreach; + } + + /** + * @param Expr[] $initExprs + */ + 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->expr instanceof FuncCall && $this->isName($initExpr->expr, 'count')) { + $this->countValueName = $this->getName($initExpr->var); + $this->iteratedExpr = $initExpr->expr->args[0]->value; + } + } + } + } + + /** + * @param Expr[] $condExprs + */ + private function isConditionMatch(array $condExprs): bool + { + if (count($condExprs) !== 1) { + return false; + } + + if ($this->keyValueName === null) { + return false; + } + + if ($this->countValueName !== null) { + return $this->isSmallerOrGreater($condExprs, $this->keyValueName, $this->countValueName); + } + + // count($values) + if ($condExprs[0]->right instanceof FuncCall) { + if ($this->isName($condExprs[0]->right, 'count')) { + /** @var FuncCall $countFuncCall */ + $countFuncCall = $condExprs[0]->right; + $this->iteratedExpr = $countFuncCall->args[0]->value; + + return true; + } + } + + return false; + } + + /** + * @param Expr[] $loopExprs + * $param + */ + private function isLoopMatch(array $loopExprs): bool + { + if (count($loopExprs) !== 1) { + return false; + } + + if ($this->keyValueName === null) { + return false; + } + + if ($loopExprs[0] instanceof PreInc || $loopExprs[0] instanceof PostInc) { + return $this->isName($loopExprs[0]->var, $this->keyValueName); + } + + return false; + } + + private function reset(): void + { + $this->keyValueName = null; + $this->countValueName = null; + $this->iteratedExpr = null; + } + + /** + * @param Expr[] $condExprs + */ + private function isSmallerOrGreater(array $condExprs, string $keyValueName, string $countValueName): bool + { + // $i < $count + if ($condExprs[0] instanceof Smaller) { + if (! $this->isName($condExprs[0]->left, $keyValueName)) { + return false; + } + + return $this->isName($condExprs[0]->right, $countValueName); + } + + // $i > $count + if ($condExprs[0] instanceof Greater) { + if (! $this->isName($condExprs[0]->left, $countValueName)) { + return false; + } + + return $this->isName($condExprs[0]->right, $keyValueName); + } + + return false; + } + + /** + * @param Node\Stmt[] $stmts + */ + private function useForeachVariableInStmts(Expr $expr, array $stmts): void + { + if ($this->keyValueName === null) { + throw new ShouldNotHappenException(); + } + + $this->callableNodeTraverser->traverseNodesWithCallable($stmts, function (Node $node) use ($expr): ?Expr { + if (! $node instanceof ArrayDimFetch) { + return null; + } + + if ($this->areNodesEqual($node->var, $expr)) { + return null; + } + + $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); + + if ($this->isPartOfAssign($parentNode)) { + return null; + } + + if (! $node->dim instanceof Variable) { + return null; + } + + if (! $this->isName($node->dim, $this->keyValueName)) { + return null; + } + + return $expr; + }); + } + + private function isPartOfAssign(?Node $node): bool + { + if ($node === null) { + return false; + } + + $previousNode = $node; + $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); + + while ($parentNode !== null && ! $parentNode instanceof Node\Stmt\Expression) { + if ($parentNode instanceof Assign) { + if ($this->areNodesEqual($parentNode->var, $previousNode)) { + return true; + } + } + + $previousNode = $parentNode; + $parentNode = $parentNode->getAttribute(AttributeKey::PARENT_NODE); + } + + return false; + } +} diff --git a/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/fixture.php.inc b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/fixture.php.inc new file mode 100644 index 000000000000..6ebfe153417a --- /dev/null +++ b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/fixture.php.inc @@ -0,0 +1,51 @@ +getPreviousNonSpaceToken($tokens, $i); + if ($previousNonSpaceToken !== null && $previousNonSpaceToken[0] === T_OBJECT_OPERATOR) { + continue; + } + $tokens[$i][0] = self::T_FN; + $tokens[$i][0][1] = self::T_FN; + + $tokens[$i][0][1][4] = self::T_FN; + $value = $tokens[$i][0][1][4]; + } + } + } +} + +?> +----- + $token) { + if ($token[0] === T_STRING && $token[1] === 'fn') { + $previousNonSpaceToken = $this->getPreviousNonSpaceToken($tokens, $i); + if ($previousNonSpaceToken !== null && $previousNonSpaceToken[0] === T_OBJECT_OPERATOR) { + continue; + } + $tokens[$i][0] = self::T_FN; + $tokens[$i][0][1] = self::T_FN; + + $tokens[$i][0][1][4] = self::T_FN; + $value = $token[0][1][4]; + } + } + } +} + +?> diff --git a/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_count.php.inc b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_count.php.inc new file mode 100644 index 000000000000..db203c15fb60 --- /dev/null +++ b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_count.php.inc @@ -0,0 +1,29 @@ + +----- + $token) { + } + } +} + +?> diff --git a/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_switched_compare.php.inc b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_switched_compare.php.inc new file mode 100644 index 000000000000..dc4350dbdf56 --- /dev/null +++ b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/Fixture/for_with_switched_compare.php.inc @@ -0,0 +1,29 @@ + $i; $i++) { + } + } +} + +?> +----- + $token) { + } + } +} + +?> diff --git a/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/ForToForeachRectorTest.php b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/ForToForeachRectorTest.php new file mode 100644 index 000000000000..235bd933461c --- /dev/null +++ b/packages/CodeQuality/tests/Rector/For_/ForToForeachRector/ForToForeachRectorTest.php @@ -0,0 +1,23 @@ +doTestFiles([ + __DIR__ . '/Fixture/fixture.php.inc', + __DIR__ . '/Fixture/for_with_count.php.inc', + __DIR__ . '/Fixture/for_with_switched_compare.php.inc', + ]); + } + + protected function getRectorClass(): string + { + return ForToForeachRector::class; + } +} diff --git a/packages/ContributorTools/src/Command/DumpNodesCommand.php b/packages/ContributorTools/src/Command/DumpNodesCommand.php index dc973101eee7..90643fc0c3e1 100644 --- a/packages/ContributorTools/src/Command/DumpNodesCommand.php +++ b/packages/ContributorTools/src/Command/DumpNodesCommand.php @@ -8,6 +8,7 @@ use PhpParser\Node\Const_; use PhpParser\Node\Expr\ArrayDimFetch; use PhpParser\Node\Expr\ArrayItem; +use PhpParser\Node\Expr\ArrowFunction; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\AssignOp; use PhpParser\Node\Expr\AssignRef; @@ -177,7 +178,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int if ($contructorReflection->getNumberOfRequiredParameters() === 0) { $node = $nodeClassReflection->newInstance(); // special case - if ($node instanceof Node\Expr\ArrowFunction) { + if ($node instanceof ArrowFunction) { $node->expr = new LNumber(1); } diff --git a/packages/ContributorTools/src/Configuration/ConfigurationFactory.php b/packages/ContributorTools/src/Configuration/ConfigurationFactory.php index f0fc3772ddff..d97a46c088a3 100644 --- a/packages/ContributorTools/src/Configuration/ConfigurationFactory.php +++ b/packages/ContributorTools/src/Configuration/ConfigurationFactory.php @@ -135,9 +135,10 @@ private function resolveLevelConfig(string $level): ?string return null; } + $fileLevel = sprintf('#^%s(\.yaml)?$#', $level); $finder = Finder::create()->files() ->in($this->levelsDirectory) - ->name(sprintf('#%s(\.yaml)?$#', $level)); + ->name($fileLevel); /** @var SplFileInfo[] $fileInfos */ $fileInfos = iterator_to_array($finder->getIterator()); @@ -153,6 +154,7 @@ private function resolveLevelConfig(string $level): ?string /** @var SplFileInfo $foundLevelConfigFileInfo */ $foundLevelConfigFileInfo = array_pop($fileInfos); + return $foundLevelConfigFileInfo->getRealPath(); } diff --git a/packages/NodeTypeResolver/src/NodeTypeResolver.php b/packages/NodeTypeResolver/src/NodeTypeResolver.php index 918c8f5a4b79..a5b138920bdd 100644 --- a/packages/NodeTypeResolver/src/NodeTypeResolver.php +++ b/packages/NodeTypeResolver/src/NodeTypeResolver.php @@ -14,6 +14,7 @@ use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\StaticCall; +use PhpParser\Node\Scalar\String_; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassConst; use PhpParser\Node\Stmt\ClassMethod; @@ -285,7 +286,7 @@ public function isArrayType(Node $node): bool public function getNodeStaticType(Node $node): ?Type { - if ($node instanceof Node\Scalar\String_) { + if ($node instanceof String_) { return new ConstantStringType($node->value); } diff --git a/packages/Php/tests/Rector/Closure/ClosureToArrowFunctionRector/ClosureToArrowFunctionRectorTest.php b/packages/Php/tests/Rector/Closure/ClosureToArrowFunctionRector/ClosureToArrowFunctionRectorTest.php index ce6b9e740f60..f1136e8ba39b 100644 --- a/packages/Php/tests/Rector/Closure/ClosureToArrowFunctionRector/ClosureToArrowFunctionRectorTest.php +++ b/packages/Php/tests/Rector/Closure/ClosureToArrowFunctionRector/ClosureToArrowFunctionRectorTest.php @@ -10,7 +10,7 @@ final class ClosureToArrowFunctionRectorTest extends AbstractRectorTestCase public function test(): void { if (! class_exists('PhpParser\Node\Expr\ArrowFunction')) { - $this->markTestSkipped('Wait on php-parser release'); + $this->markTestSkipped('Waits on php-parser release'); } $this->doTestFiles([ diff --git a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php index ff999e0fd9e8..9e6deac30312 100644 --- a/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php +++ b/packages/Php/tests/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/ArraySpreadInsteadOfArrayMergeRectorTest.php @@ -10,7 +10,7 @@ final class ArraySpreadInsteadOfArrayMergeRectorTest extends AbstractRectorTestC public function test(): void { if (! class_exists('PhpParser\Node\Expr\ArrowFunction')) { - $this->markTestSkipped('Wait on php-parser release'); + $this->markTestSkipped('Waits on php-parser release'); } $this->doTestFiles([ diff --git a/phpstan.neon b/phpstan.neon index 54183aa035b9..63f5b78f0a73 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -166,3 +166,5 @@ parameters: - '#Parameter \#6 \$subNodeName of method PhpParser\\PrettyPrinterAbstract\:\:pArray\(\) expects string, int\|null given#' - '#Parameter \#7 \$fixup of method PhpParser\\PrettyPrinterAbstract\:\:pArray\(\) expects int\|null, string\|null given#' - '#Class PhpParser\\Node\\Expr\\ArrayItem constructor invoked with 5 parameters, 1\-4 required#' + + - '#Parameter \#2 \$name of method Rector\\Rector\\AbstractRector\:\:isName\(\) expects string, string\|null given#' diff --git a/rector.yaml b/rector.yaml index 6eec4b922f25..4a89e6c83371 100644 --- a/rector.yaml +++ b/rector.yaml @@ -16,4 +16,5 @@ parameters: services: Rector\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector: ~ -# Rector\TypeDeclaration\Rector\Closure\AddClosureReturnTypeRector: ~ + +# Rector\CodeQuality\Rector\For_\ForToForeachRector: ~