[SOLID] Add MoveVariableDeclarationNearReferenceRector (#4410)

* Create test cases

* Create rector

* Add closure to possible parent scopes

* Register in SOLID set

* Document the feature

* fix adding variable in closure

* move MoveVariableDeclarationNearReferenceRector to Assign category, based on getNodeTypes()

* ci: drop phpunit matcher, not much helpful

* ignore for now, many cases to cover

* prepare release

Co-authored-by: Anna Filina <afilina@gmail.com>
This commit is contained in:
Tomas Votruba 2020-10-30 14:25:24 +01:00 committed by GitHub
parent 49b7635b27
commit b302e493dd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
19 changed files with 422 additions and 24 deletions

View File

@ -20,17 +20,16 @@ jobs:
name: PHP ${{ matrix.php }} tests
steps:
- uses: actions/checkout@v2
- uses: shivammathur/setup-php@v1
with:
php-version: ${{ matrix.php }}
coverage: none # disable xdebug, pcov
# report phpunit errors into files-changed PR tab
# https://github.com/actions/toolkit/blob/master/docs/problem-matchers.md
- name: Setup Problem Matchers for PHPUnit
run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
- run: composer update --prefer-lowest --no-progress --ansi
if: "matrix.dependencies == 'lowest'"
- run: composer install --no-progress --ansi
if: "matrix.dependencies != 'lowest'"
- run: vendor/bin/phpunit

View File

@ -3,6 +3,7 @@
declare(strict_types=1);
use Rector\CodingStyle\Rector\MethodCall\UseMessageVariableForSprintfInSymfonyStyleRector;
use Rector\SOLID\Rector\Assign\MoveVariableDeclarationNearReferenceRector;
use Rector\SOLID\Rector\Class_\ChangeReadOnlyVariableWithDefaultValueToConstantRector;
use Rector\SOLID\Rector\Class_\FinalizeClassesWithoutChildrenRector;
use Rector\SOLID\Rector\Class_\MakeUnusedClassesWithChildrenAbstractRector;
@ -23,4 +24,7 @@ return static function (ContainerConfigurator $containerConfigurator): void {
$services->set(RepeatedLiteralToClassConstantRector::class);
$services->set(RemoveAlwaysElseRector::class);
$services->set(UseMessageVariableForSprintfInSymfonyStyleRector::class);
// this one requires to cover edge cases
// $services->set(MoveVariableDeclarationNearReferenceRector::class);
};

View File

@ -1,4 +1,4 @@
# All 597 Rectors Overview
# All 598 Rectors Overview
- [Projects](#projects)
---
@ -64,7 +64,7 @@
- [RemovingStatic](#removingstatic) (6)
- [Renaming](#renaming) (10)
- [Restoration](#restoration) (9)
- [SOLID](#solid) (13)
- [SOLID](#solid) (14)
- [Sensio](#sensio) (3)
- [StrictCodeQuality](#strictcodequality) (1)
- [Symfony](#symfony) (34)
@ -14227,6 +14227,23 @@ Classes that have no children nor are used, should have abstract
<br><br>
### `MoveVariableDeclarationNearReferenceRector`
- class: [`Rector\SOLID\Rector\Assign\MoveVariableDeclarationNearReferenceRector`](/rules/solid/src/Rector/Assign/MoveVariableDeclarationNearReferenceRector.php)
- [test fixtures](/rules/solid/tests/Rector/Assign/MoveVariableDeclarationNearReferenceRector/Fixture)
Move variable declaration near its reference
```diff
-$var = 1;
if ($condition === null) {
+ $var = 1;
return $var;
}
```
<br><br>
### `MultiParentingToAbstractDependencyRector`
- class: [`Rector\SOLID\Rector\Class_\MultiParentingToAbstractDependencyRector`](/rules/solid/src/Rector/Class_/MultiParentingToAbstractDependencyRector.php)

View File

@ -5,6 +5,7 @@ declare(strict_types=1);
namespace Rector\NodeNestingScope;
use PhpParser\Node;
use PhpParser\Node\Expr\Closure;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\Node\Stmt\Function_;
@ -14,7 +15,7 @@ use Rector\NodeTypeResolver\Node\AttributeKey;
final class ParentScopeFinder
{
/**
* @return ClassMethod|Function_|Class_|Namespace_|null
* @return ClassMethod|Function_|Class_|Namespace_|Closure|null
*/
public function find(Node $node): ?Node
{

View File

@ -49,5 +49,6 @@ final class ControlStructure
Case_::class,
Match_::class,
Switch_::class,
Foreach_::class,
];
}

View File

@ -8,7 +8,6 @@ use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Stmt;
use PhpParser\Node\Stmt\Expression;
use PhpParser\Node\Stmt\If_;
use Rector\Core\Exception\ShouldNotHappenException;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
use Rector\NodeTypeResolver\Node\AttributeKey;
@ -103,12 +102,7 @@ final class NodesToAddCollector implements NodeCollectorInterface
private function resolveNearestExpressionPosition(Node $node): string
{
if ($node instanceof Expression) {
return spl_object_hash($node);
}
// special case for "If_"
if ($node instanceof If_) {
if ($node instanceof Expression || $node instanceof Stmt) {
return spl_object_hash($node);
}

View File

@ -359,11 +359,6 @@ parameters:
- '#Property PhpParser\\Node\\Stmt\\Foreach_\:\:\$valueVar \(PhpParser\\Node\\Expr\) does not accept PhpParser\\Node\\Expr\|null#'
- '#Access to an undefined property PHPStan\\PhpDocParser\\Ast\\PhpDoc\\PhpDocTagValueNode\:\:\$type#'
# local type
-
message: '#Method call "isObjectType\(\)" argument on position 1 cannot use "\:\:class" reference#'
path: 'packages/dynamic-type-analysis/src/Rector/StaticCall/RemoveArgumentTypeProbeRector.php'
# only local use
-
message: '#Class "Rector\\RectorGenerator\\Rector\\Closure\\AddNewServiceToSymfonyPhpConfigRector" is missing @see annotation with test case class reference#'
@ -528,8 +523,6 @@ parameters:
# 3rd party code
- packages/doctrine-annotation-generated/src/DataCollector/ResolvedConstantStaticCollector.php
# on purpose
- packages/dynamic-type-analysis/src/Probe/TypeStaticProbe.php
- packages/dynamic-type-analysis/tests/ProbeStorage/StaticInMemoryProbeStorage.php
- src/Testing/PHPUnit/AbstractGenericRectorTestCase.php # 251
-
@ -581,7 +574,6 @@ parameters:
- packages/attribute-aware-php-doc/src/AttributeAwareNodeFactory/Type/AttributeAwareUnionTypeNodeFactory.php # 44
- packages/better-php-doc-parser/src/PhpDocNodeFactory/PHPUnitDataProviderDocNodeFactory.php # 52
- packages/better-php-doc-parser/src/PhpDocNodeFactory/ParamPhpDocNodeFactory.php # 81
- packages/dynamic-type-analysis/src/Probe/TypeStaticProbe.php # 82
- packages/node-type-resolver/src/FileSystem/CurrentFileInfoProvider.php # 35
- packages/phpstan-static-type-mapper/src/TypeMapper/ObjectTypeMapper.php # 123
- packages/phpstan-static-type-mapper/src/TypeMapper/ObjectWithoutClassTypeMapper.php # 72
@ -684,3 +676,10 @@ parameters:
-
message: '#new <class\> is limited to 3 "new <class\>\(new <class\>\)\)" nesting to each other\. You have \d+ nesting#'
path: config/set/*
- '#Cognitive complexity for "Rector\\CodeQuality\\Naming\\MethodCallToVariableNameResolver\:\:resolveVariableName\(\)" is 12, keep it under 9#'
# nodes/stmts
- '#Parameter \#1 \$nodes of method Rector\\Core\\PhpParser\\Node\\BetterNodeFinder\:\:findFirst\(\) expects array<PhpParser\\Node\>\|PhpParser\\Node, array<PhpParser\\Node\\Stmt\>\|null given#'
# finder type
- '#Method Rector\\SOLID\\Rector\\Assign\\MoveVariableDeclarationNearReferenceRector\:\:findFirstVariableUsageInScope\(\) should return PhpParser\\Node\\Expr\\Variable\|null but returns PhpParser\\Node\|null#'

View File

@ -0,0 +1,154 @@
<?php
declare(strict_types=1);
namespace Rector\SOLID\Rector\Assign;
use PhpParser\Node;
use PhpParser\Node\Expr\ArrayDimFetch;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\Closure;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\Node\Stmt\Function_;
use PhpParser\Node\Stmt\Namespace_;
use PHPStan\Type\TypeWithClassName;
use Rector\Core\Rector\AbstractRector;
use Rector\Core\RectorDefinition\CodeSample;
use Rector\Core\RectorDefinition\RectorDefinition;
use Rector\NodeNestingScope\ParentScopeFinder;
use Rector\NodeTypeResolver\Node\AttributeKey;
/**
* @see \Rector\SOLID\Tests\Rector\Assign\MoveVariableDeclarationNearReferenceRector\MoveVariableDeclarationNearReferenceRectorTest
*/
final class MoveVariableDeclarationNearReferenceRector extends AbstractRector
{
/**
* @var ParentScopeFinder
*/
private $parentScopeFinder;
public function __construct(ParentScopeFinder $parentScopeFinder)
{
$this->parentScopeFinder = $parentScopeFinder;
}
public function getDefinition(): RectorDefinition
{
return new RectorDefinition('Move variable declaration near its reference', [
new CodeSample(
<<<'CODE_SAMPLE'
$var = 1;
if ($condition === null) {
return $var;
}
CODE_SAMPLE
,
<<<'CODE_SAMPLE'
if ($condition === null) {
$var = 1;
return $var;
}
CODE_SAMPLE
),
]);
}
/**
* @return string[]
*/
public function getNodeTypes(): array
{
return [Assign::class];
}
/**
* @param Assign $node
*/
public function refactor(Node $node): ?Node
{
$assign = $node;
$variable = $node->var;
if (! $variable instanceof Variable) {
return null;
}
$variableType = $this->getStaticType($variable);
$parentScope = $this->parentScopeFinder->find($assign);
if ($parentScope === null) {
return null;
}
$firstUsedVariable = $this->findFirstVariableUsageInScope($variable, $assign, $parentScope);
if ($firstUsedVariable === null) {
return null;
}
if ($this->shouldSkipUsedVariable($firstUsedVariable)) {
return null;
}
$firstVariableUsageStatement = $firstUsedVariable->getAttribute(AttributeKey::CURRENT_STATEMENT);
$assignStatement = $assign->getAttribute(AttributeKey::CURRENT_STATEMENT);
$this->addNodeBeforeNode($assignStatement, $firstVariableUsageStatement);
$this->removeNode($assignStatement);
return $node;
}
/**
* Find the first node within the same method being a usage of the assigned variable,
* but not the original assignment itself.
*
* @param ClassMethod|Function_|Class_|Namespace_|Closure $parentScopeNode
*/
private function findFirstVariableUsageInScope(
Variable $desiredVariable,
Assign $assign,
Node $parentScopeNode
): ?Variable {
$desiredVariableName = $this->getName($desiredVariable);
/** @var Variable|null $foundVariable */
$foundVariable = $this->betterNodeFinder->findFirst(
(array) $parentScopeNode->stmts,
function (Node $node) use ($desiredVariableName, $assign): bool {
if (! $node instanceof Variable) {
return false;
}
if ($this->isVariableInOriginalAssign($node, $assign)) {
return false;
}
return $this->isName($node, $desiredVariableName);
}
);
return $foundVariable;
}
private function shouldSkipUsedVariable(Variable $variable): bool
{
$parent = $variable->getAttribute(AttributeKey::PARENT_NODE);
if ($parent instanceof ArrayDimFetch) {
return true;
}
$variableType = $this->getStaticType($variable);
// possibly service of value object, that changes inner state
return $variableType instanceof TypeWithClassName;
}
private function isVariableInOriginalAssign(Variable $variable, Assign $assign): bool
{
$parentNode = $variable->getAttribute(AttributeKey::PARENT_NODE);
return $parentNode === $assign;
}
}

View File

@ -0,0 +1,15 @@
<?php
namespace Rector\SOLID\Tests\Rector\Assign\MoveVariableDeclarationNearReferenceRector\Fixture;
class PropertyInMethod
{
private $var;
function myMethod()
{
$this->var = 1;
if (mktime() === false) {
return $this->var;
}
}
}

View File

@ -0,0 +1,19 @@
<?php
namespace Rector\SOLID\Tests\Rector\Assign\MoveVariableDeclarationNearReferenceRector\Fixture;
use Symplify\SmartFileSystem\SmartFileInfo;
class SkipInstantiation
{
/**
* @param SmartFileInfo[] $foundFiles
*/
function myMethod(array $foundFiles)
{
$foundFileNames = [];
foreach ($foundFiles as $foundFile) {
$foundFileNames[] = $foundFile->getFilename();
}
}
}

View File

@ -0,0 +1,19 @@
<?php
namespace Rector\SOLID\Tests\Rector\Assign\MoveVariableDeclarationNearReferenceRector\Fixture;
use PhpParser\PrettyPrinter\Standard;
final class SkipMethodCall
{
public function myMethod(array $nodes)
{
$standard = new Standard();
$printedContents = [];
foreach ($nodes as $node) {
$printedContent = $standard->prettyPrint([$node]);
$printedContents[] = $printedContent;
}
}
}

View File

@ -0,0 +1,27 @@
<?php
function variableInClosure()
{
$closure = function () {
$var = 1;
if (mktime() === false) {
return $var;
}
};
}
?>
-----
<?php
function variableInClosure()
{
$closure = function () {
if (mktime() === false) {
$var = 1;
return $var;
}
};
}
?>

View File

@ -0,0 +1,27 @@
<?php
function variableInClosureWithReturn()
{
return function () {
$var = 1;
if (mktime() === false) {
return $var;
}
};
}
?>
-----
<?php
function variableInClosureWithReturn()
{
return function () {
if (mktime() === false) {
$var = 1;
return $var;
}
};
}
?>

View File

@ -0,0 +1,23 @@
<?php
function variableInFunction()
{
$var = 1;
if (mktime() === false) {
return $var;
}
}
?>
-----
<?php
function variableInFunction()
{
if (mktime() === false) {
$var = 1;
return $var;
}
}
?>

View File

@ -0,0 +1,33 @@
<?php
namespace Rector\SOLID\Tests\Rector\Assign\MoveVariableDeclarationNearReferenceRector\Fixture;
class VariableInMethod
{
function myMethod()
{
$var = 1;
if (mktime() === false) {
return $var;
}
}
}
?>
-----
<?php
namespace Rector\SOLID\Tests\Rector\Assign\MoveVariableDeclarationNearReferenceRector\Fixture;
class VariableInMethod
{
function myMethod()
{
if (mktime() === false) {
$var = 1;
return $var;
}
}
}
?>

View File

@ -0,0 +1,25 @@
<?php
function variableWithMultipleUsages()
{
$var = 1;
if (mktime() === false) {
echo $var;
return $var;
}
}
?>
-----
<?php
function variableWithMultipleUsages()
{
if (mktime() === false) {
$var = 1;
echo $var;
return $var;
}
}
?>

View File

@ -0,0 +1,10 @@
<?php
function variableWithUsageInMultipleLevels()
{
$var = 1;
if (mktime() === false) {
return $var;
}
echo $var;
}

View File

@ -0,0 +1,31 @@
<?php
declare(strict_types=1);
namespace Rector\SOLID\Tests\Rector\Assign\MoveVariableDeclarationNearReferenceRector;
use Iterator;
use Rector\Core\Testing\PHPUnit\AbstractRectorTestCase;
use Rector\SOLID\Rector\Assign\MoveVariableDeclarationNearReferenceRector;
use Symplify\SmartFileSystem\SmartFileInfo;
final class MoveVariableDeclarationNearReferenceRectorTest extends AbstractRectorTestCase
{
/**
* @dataProvider provideData()
*/
public function test(SmartFileInfo $fileInfo): void
{
$this->doTestFileInfo($fileInfo);
}
public function provideData(): Iterator
{
return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture');
}
protected function getRectorClass(): string
{
return MoveVariableDeclarationNearReferenceRector::class;
}
}

View File

@ -4,9 +4,9 @@ sonar.projectKey=rectorphp_rector
# relative paths to source
# wildcards don't work :(
sonar.sources=compiler/src,src,rules/autodiscovery/src,rules/architecture/src,packages/attribute-aware-php-doc/src,packages/better-php-doc-parser/src,rules/cakephp/src,rules/code-quality/src,rules/coding-style/src,packages/console-differ/src,rules/dead-code/src,rules/doctrine/src,rules/doctrine-code-quality/src,packages/file-system-rector/src,rules/laravel/src,rules/legacy/src,rules/mysql-to-mysqli/src,rules/nette-tester-to-phpunit/src,rules/nette-to-symfony/src,rules/nette/src,packages/node-collector/src,packages/node-type-resolver/src,packages/node-name-resolver/src,rules/phpstan/src,packages/phpstan-static-type-mapper/src,rules/phpunit-symfony/src,rules/phpunit/src,rules/psr4/src,rules/php-spec-to-phpunit/src,rules/php52/src,rules/php53/src,rules/php54/src,rules/php55/src,rules/php56/src,rules/php70/src,rules/php71/src,rules/php72/src,rules/php73/src,rules/php74/src,rules/php80/src,rules/removing-static/src,rules/renaming/src,rules/restoration/src,rules/solid/src,rules/sensio/src,packages/static-type-mapper/src,rules/symfony-code-quality/src,rules/symfony-phpunit/src,rules/symfony/src,rules/twig/src,rules/type-declaration/src,packages/vendor-locker/src,packages/rector-generator/src,rules/strict-code-quality/src,packages/dynamic-type-analysis/src,rules/php-deglobalize/src,rules/phalcon/src,rules/doctrine-gedmo-to-knplabs/src,rules/polyfill/src,rules/generic/src
sonar.sources=compiler/src,src,rules/autodiscovery/src,rules/architecture/src,packages/attribute-aware-php-doc/src,packages/better-php-doc-parser/src,rules/cakephp/src,rules/code-quality/src,rules/coding-style/src,packages/console-differ/src,rules/dead-code/src,rules/doctrine/src,rules/doctrine-code-quality/src,packages/file-system-rector/src,rules/laravel/src,rules/legacy/src,rules/mysql-to-mysqli/src,rules/nette-tester-to-phpunit/src,rules/nette-to-symfony/src,rules/nette/src,packages/node-collector/src,packages/node-type-resolver/src,packages/node-name-resolver/src,rules/phpstan/src,packages/phpstan-static-type-mapper/src,rules/phpunit-symfony/src,rules/phpunit/src,rules/psr4/src,rules/php-spec-to-phpunit/src,rules/php52/src,rules/php53/src,rules/php54/src,rules/php55/src,rules/php56/src,rules/php70/src,rules/php71/src,rules/php72/src,rules/php73/src,rules/php74/src,rules/php80/src,rules/removing-static/src,rules/renaming/src,rules/restoration/src,rules/solid/src,rules/sensio/src,packages/static-type-mapper/src,rules/symfony-code-quality/src,rules/symfony-phpunit/src,rules/symfony/src,rules/twig/src,rules/type-declaration/src,packages/vendor-locker/src,packages/rector-generator/src,rules/strict-code-quality/src,rules/php-deglobalize/src,rules/phalcon/src,rules/doctrine-gedmo-to-knplabs/src,rules/polyfill/src,rules/generic/src
sonar.tests=tests,rules/autodiscovery/tests,rules/architecture/tests,packages/better-php-doc-parser/tests,rules/cakephp/tests,rules/code-quality/tests,rules/coding-style/tests,rules/dead-code/tests,rules/doctrine/tests,rules/doctrine-code-quality/tests,rules/laravel/tests,rules/legacy/tests,rules/mysql-to-mysqli/tests,rules/nette-tester-to-phpunit/tests,rules/nette-to-symfony/tests,rules/nette/tests,packages/node-type-resolver/tests,utils/phpstan-extensions/src,rules/phpstan/tests,rules/phpunit-symfony/tests,rules/phpunit/tests,rules/psr4/tests,rules/php-spec-to-phpunit/tests,rules/php52/tests,rules/php53/tests,rules/php54/tests,rules/php55/tests,rules/php56/tests,rules/php70/tests,rules/php71/tests,rules/php72/tests,rules/php73/tests,rules/php74/tests,rules/php80/tests,rules/removing-static/tests,rules/renaming/tests,rules/restoration/tests,rules/solid/tests,rules/sensio/tests,rules/symfony-code-quality/tests,rules/symfony-phpunit/tests,rules/symfony/tests,rules/twig/tests,rules/type-declaration/tests,rules/strict-code-quality/tests,packages/dynamic-type-analysis/tests,rules/php-deglobalize/tests,rules/phalcon/tests,utils/node-documentation-generator/src,utils/phpstan-attribute-type-syncer/src,utils/phpstan-static-type-mapper-checker/src,rules/doctrine-gedmo-to-knplabs/tests,rules/polyfill/tests,rules/generic/tests
sonar.tests=tests,rules/autodiscovery/tests,rules/architecture/tests,packages/better-php-doc-parser/tests,rules/cakephp/tests,rules/code-quality/tests,rules/coding-style/tests,rules/dead-code/tests,rules/doctrine/tests,rules/doctrine-code-quality/tests,rules/laravel/tests,rules/legacy/tests,rules/mysql-to-mysqli/tests,rules/nette-tester-to-phpunit/tests,rules/nette-to-symfony/tests,rules/nette/tests,packages/node-type-resolver/tests,utils/phpstan-extensions/src,rules/phpstan/tests,rules/phpunit-symfony/tests,rules/phpunit/tests,rules/psr4/tests,rules/php-spec-to-phpunit/tests,rules/php52/tests,rules/php53/tests,rules/php54/tests,rules/php55/tests,rules/php56/tests,rules/php70/tests,rules/php71/tests,rules/php72/tests,rules/php73/tests,rules/php74/tests,rules/php80/tests,rules/removing-static/tests,rules/renaming/tests,rules/restoration/tests,rules/solid/tests,rules/sensio/tests,rules/symfony-code-quality/tests,rules/symfony-phpunit/tests,rules/symfony/tests,rules/twig/tests,rules/type-declaration/tests,rules/strict-code-quality/tests,rules/php-deglobalize/tests,rules/phalcon/tests,utils/node-documentation-generator/src,utils/phpstan-attribute-type-syncer/src,utils/phpstan-static-type-mapper-checker/src,rules/doctrine-gedmo-to-knplabs/tests,rules/polyfill/tests,rules/generic/tests
# possibly manual operation needed :/
# https://docs.sonarqube.org/latest/project-administration/narrowing-the-focus/