[DX] Remove poorly designed NodeToReplacePostRector, return changed nodes directly in the current rule (#2229)

This commit is contained in:
Tomas Votruba 2022-05-05 00:16:25 +02:00 committed by GitHub
parent 767e7692e1
commit 2d16736414
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 158 additions and 301 deletions

View File

@ -1,51 +0,0 @@
<?php
declare(strict_types=1);
namespace Rector\NodeRemoval;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Stmt;
use PhpParser\Node\Stmt\Expression;
use Rector\ChangesReporting\Collector\RectorChangeCollector;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
use Rector\DeadCode\NodeManipulator\LivingCodeManipulator;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\PostRector\Collector\NodesToReplaceCollector;
final class AssignRemover
{
public function __construct(
private readonly NodesToReplaceCollector $nodesToReplaceCollector,
private readonly RectorChangeCollector $rectorChangeCollector,
private readonly NodeRemover $nodeRemover,
private readonly LivingCodeManipulator $livingCodeManipulator,
private readonly BetterNodeFinder $betterNodeFinder
) {
}
public function removeAssignNode(Assign $assign): void
{
$currentStatement = $this->betterNodeFinder->resolveCurrentStatement($assign);
if (! $currentStatement instanceof Stmt) {
return;
}
$this->livingCodeManipulator->addLivingCodeBeforeNode($assign->var, $currentStatement);
/** @var Assign $assign */
$parent = $assign->getAttribute(AttributeKey::PARENT_NODE);
if ($parent instanceof Expression) {
$this->nodeRemover->removeNode($assign);
return;
}
$this->nodesToReplaceCollector->addReplaceNodeWithAnotherNode($assign, $assign->expr);
$this->rectorChangeCollector->notifyNodeFileInfo($assign->expr);
if ($parent instanceof Assign) {
$this->removeAssignNode($parent);
}
}
}

View File

@ -1,37 +0,0 @@
<?php
declare(strict_types=1);
namespace Rector\PostRector\Collector;
use PhpParser\Node;
use Rector\PostRector\Contract\Collector\NodeCollectorInterface;
/**
* @deprecated Resolve in the rules itself instead of hacking around collector.
*/
final class NodesToReplaceCollector implements NodeCollectorInterface
{
/**
* @var array<array{Node, Node}>
*/
private array $nodesToReplace = [];
public function addReplaceNodeWithAnotherNode(Node $node, Node $replaceWith): void
{
$this->nodesToReplace[] = [$node, $replaceWith];
}
public function isActive(): bool
{
return $this->nodesToReplace !== [];
}
/**
* @return array<array{Node, Node}>
*/
public function getNodes(): array
{
return $this->nodesToReplace;
}
}

View File

@ -1,63 +0,0 @@
<?php
declare(strict_types=1);
namespace Rector\PostRector\Rector;
use PhpParser\Node;
use Rector\PostRector\Collector\NodesToReplaceCollector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
final class NodeToReplacePostRector extends AbstractPostRector
{
public function __construct(
private readonly NodesToReplaceCollector $nodesToReplaceCollector
) {
}
public function getPriority(): int
{
return 1100;
}
public function leaveNode(Node $node): ?Node
{
foreach ($this->nodesToReplaceCollector->getNodes() as [$nodeToFind, $replacement]) {
if ($node === $nodeToFind) {
return $replacement;
}
}
return null;
}
public function getRuleDefinition(): RuleDefinition
{
return new RuleDefinition(
'Replaces nodes on weird positions',
[
new CodeSample(
<<<'CODE_SAMPLE'
class SomeClass
{
public function run($value)
{
return 1;
}
}
CODE_SAMPLE
,
<<<'CODE_SAMPLE'
class SomeClass
{
public function run($value)
{
return $value;
}
}
CODE_SAMPLE
), ]
);
}
}

View File

@ -7,6 +7,7 @@ namespace Rector\ReadWrite\NodeAnalyzer;
use PhpParser\Node;
use PhpParser\Node\Arg;
use PhpParser\Node\Expr\ArrayDimFetch;
use PhpParser\Node\Expr\AssignOp;
use PhpParser\Node\Expr\FuncCall;
use PhpParser\Node\Expr\Isset_;
use PhpParser\Node\Expr\PostDec;
@ -41,6 +42,26 @@ final class ReadWritePropertyAnalyzer
throw new ShouldNotHappenException();
}
if ($parent instanceof PostInc) {
return true;
}
if ($parent instanceof PreInc) {
return true;
}
if ($parent instanceof PostDec) {
return true;
}
if ($parent instanceof PreDec) {
return true;
}
if ($parent instanceof AssignOp) {
return true;
}
$parent = $this->unwrapPostPreIncDec($parent);
if ($parent instanceof Arg) {

View File

@ -89,7 +89,6 @@ parameters:
- rules/Php70/EregToPcreTransformer.php
- packages/NodeTypeResolver/NodeTypeResolver.php
- rules/Renaming/NodeManipulator/ClassRenamer.php
- rules/DogFood/Rector/Closure/UpgradeRectorConfigRector.php
- "#^Cognitive complexity for \"Rector\\\\Php70\\\\EregToPcreTransformer\\:\\:(.*?)\" is (.*?), keep it under 10$#"
- '#Cognitive complexity for "Rector\\Core\\PhpParser\\Node\\Value\\ValueResolver\:\:getValue\(\)" is \d+, keep it under 10#'
@ -150,8 +149,6 @@ parameters:
- '#Method Rector\\CodeQuality\\Rector\\Foreach_\\SimplifyForeachToCoalescingRector\:\:matchReturnOrAssignNode\(\) should return PhpParser\\Node\\Expr\\Assign\|PhpParser\\Node\\Stmt\\Return_\|null but returns PhpParser\\Node\|null#'
- '#Instanceof between PhpParser\\Node\\Stmt and Rector\\Core\\PhpParser\\Node\\CustomNode\\FileWithoutNamespace will always evaluate to false#'
-
message: '#Unreachable statement \- code above always terminates#'
paths:
@ -184,15 +181,12 @@ parameters:
paths:
- src/Rector/AbstractRector.php
- '#Method Rector\\Core\\PhpParser\\Node\\BetterNodeFinder\:\:findParentType\(\) should return T of PhpParser\\Node\|null but returns class\-string<T of PhpParser\\Node\>\|T of PhpParser\\Node#'
- '#Property Rector\\Core\\PhpParser\\Node\\AssignAndBinaryMap\:\:\$binaryOpToAssignClasses \(array<class\-string<PhpParser\\Node\\Expr\\BinaryOp\>, class\-string<PhpParser\\Node\\Expr\\BinaryOp\>\>\) does not accept array#'
-
message: '#Function "property_exists\(\)" cannot be used/left in the code#'
paths:
# on PhpParser Nodes
- packages/NodeTypeResolver/NodeVisitor/StatementNodeVisitor.php
- packages/NodeNameResolver/NodeNameResolver.php
- packages/NodeNameResolver/NodeNameResolver/ClassNameResolver.php
- packages/NodeTypeResolver/PHPStan/Scope/PHPStanNodeScopeResolver.php
@ -342,6 +336,7 @@ parameters:
message: '#Class cognitive complexity is \d+, keep it under 50#'
paths:
- src/PhpParser/Node/BetterNodeFinder.php
- rules/Removing/NodeManipulator/ComplexNodeRemover.php
-
message: '#Parameter \#2 \$length of function str_split expects int<1, max\>, int given#'
@ -579,28 +574,19 @@ parameters:
# move later in BC break release
- '#Class "(Rector\\DeadCode\\Rector\\(.*?)\\(RemoveUnreachableStatementRector|RemoveDoubleAssignRector))" has invalid namespace category "(.*?)"\. Pick one of\: "(.*?)"#'
# by child generics, this property is always available
- '#Access to an undefined property PhpParser\\Node\\FunctionLike\|PhpParser\\Node\\Stmt\\Else_\|PhpParser\\Node\\Stmt\\Foreach_\|PhpParser\\Node\\Stmt\\If_\:\:\$stmts#'
- '#Access to an undefined property PhpParser\\Node\\FunctionLike\|PhpParser\\Node\\Stmt\\Foreach_\|PhpParser\\Node\\Stmt\\If_\|PhpParser\\Node\\Stmt\\Namespace_\|Rector\\Core\\PhpParser\\Node\\CustomNode\\FileWithoutNamespace\:\:\$stmts#'
# stmts refactoring
- '#Cognitive complexity for "Rector\\DeadCode\\Rector\\Assign\\RemoveDoubleAssignRector\:\:refactor\(\)" is \d+, keep it under 10#'
# todo add custom stmts aware interface
-
message: '#Access to an undefined property PhpParser\\Node\:\:\$stmts#'
path: src/PhpParser/Node/BetterNodeFinder.php
# empty parent construct
- '#Rector\\Core\\PhpParser\\NodeTraverser\\FileWithoutNamespaceNodeTraverser\:\:__construct\(\) does not call parent constructor from PhpParser\\NodeTraverser#'
# resolve later to collector
-
message: '#Class cognitive complexity is \d+, keep it under 50#'
path: rules/TypeDeclaration/PHPStan/ObjectTypeSpecifier.php
- '#Cognitive complexity for "Rector\\TypeDeclaration\\PHPStan\\ObjectTypeSpecifier\:\:matchShortenedObjectType\(\)" is \d+, keep it under 10#'
- '#Cognitive complexity for "Rector\\TypeDeclaration\\PHPStan\\ObjectTypeSpecifier\:\:narrowToFullyQualifiedOrAliasedObjectType\(\)" is \d+, keep it under 10#'
- '#Class "Rector\\CodeQuality\\Rector\\ClassMethod\\InlineArrayReturnAssignRector" has invalid namespace category "ClassMethod"\. Pick one of\: "NodeTypeGroup"#'
- '#Parameter \#1 \$node (.*?) of method Rector\\(.*?)\:\:refactorWithScope\(\) should be contravariant with parameter \$node \(PhpParser\\Node\) of method Rector\\Core\\Contract\\Rector\\ScopeAwarePhpRectorInterface\:\:refactorWithScope\(\)#'
- '#Return type \(array\|null\) of method Rector\\CodeQuality\\Rector\\For_\\ForRepeatedCountToOwnVariableRector\:\:refactorWithScope\(\) should be covariant with return type \(array<PhpParser\\Node>\|PhpParser\\Node\|null\) of method Rector\\Core\\Contract\\Rector\\ScopeAwarePhpRectorInterface\:\:refactorWithScope\(\)#'
# cleanup after merge of assing remover refactoring
- '#Cognitive complexity for "Rector\\Removing\\NodeManipulator\\ComplexNodeRemover\:\:removePropertyAndUsages\(\)" is \d+, keep it under 10#'
- '#Class "Rector\\DeadCode\\Rector\\Property\\RemoveUnusedPrivatePropertyRector" has invalid namespace category "Property"\. Pick one of\: "Class_"#'
- '#Cognitive complexity for "Rector\\ReadWrite\\NodeAnalyzer\\ReadWritePropertyAnalyzer\:\:isRead\(\)" is 14, keep it under 10#'

View File

@ -2,7 +2,7 @@
namespace Rector\Tests\CodeQuality\Rector\Expression\InlineIfToExplicitIfRector\Fixture;
class AssignOp
final class InlineAssignOp
{
public function run()
{
@ -17,7 +17,7 @@ class AssignOp
namespace Rector\Tests\CodeQuality\Rector\Expression\InlineIfToExplicitIfRector\Fixture;
class AssignOp
final class InlineAssignOp
{
public function run()
{

View File

@ -1,37 +0,0 @@
<?php
namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
final class SomeFunctionCall
{
private $prop;
public function doThing()
{
$this->prop = foo();
$this->prop[bar()] = foo();
return $this->prop[bar()] = foo();
}
}
?>
-----
<?php
namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
final class SomeFunctionCall
{
public function doThing()
{
bar();
bar();
return foo();
}
}
?>

View File

@ -1,40 +0,0 @@
<?php
namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
final class WriteOnlyDimFetchUse
{
/**
* @var string
*/
private $key;
public function __construct(string $key)
{
$this->key = $key;
}
public function buildData(): array
{
$data[$this->key] = 10000;
}
}
?>
-----
<?php
namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;
final class WriteOnlyDimFetchUse
{
public function __construct()
{
}
public function buildData(): array
{
}
}
?>

View File

@ -7,6 +7,5 @@ use Rector\Tests\Transform\Rector\New_\NewToConstructorInjectionRector\Source\Du
use Rector\Transform\Rector\New_\NewToConstructorInjectionRector;
return static function (RectorConfig $rectorConfig): void {
$rectorConfig
->ruleWithConfiguration(NewToConstructorInjectionRector::class, [DummyValidator::class]);
$rectorConfig->ruleWithConfiguration(NewToConstructorInjectionRector::class, [DummyValidator::class]);
};

View File

@ -29,27 +29,16 @@ use PhpParser\Node\Expr\UnaryMinus;
use PhpParser\Node\Expr\UnaryPlus;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Scalar;
use PhpParser\Node\Stmt\Expression;
use PHPStan\Type\ObjectType;
use Rector\NodeTypeResolver\NodeTypeResolver;
use Rector\PostRector\Collector\NodesToAddCollector;
final class LivingCodeManipulator
{
public function __construct(
private readonly NodesToAddCollector $nodesToAddCollector,
private readonly NodeTypeResolver $nodeTypeResolver
) {
}
public function addLivingCodeBeforeNode(Expr $expr, Node $addBeforeThisNode): void
{
$livinExprs = $this->keepLivingCodeFromExpr($expr);
foreach ($livinExprs as $livinExpr) {
$this->nodesToAddCollector->addNodeBeforeNode(new Expression($livinExpr), $addBeforeThisNode);
}
}
/**
* @return Expr[]|mixed[]
*/

View File

@ -36,6 +36,9 @@ final class RemoveUnusedPrivatePropertyRector extends AbstractRector implements
) {
}
/**
* @param mixed[] $configuration
*/
public function configure(array $configuration): void
{
$this->removeAssignSideEffect = $configuration[self::REMOVE_ASSIGN_SIDE_EFFECT] ?? (bool) current(
@ -72,25 +75,35 @@ CODE_SAMPLE
*/
public function getNodeTypes(): array
{
return [Property::class];
return [Class_::class];
}
/**
* @param Property $node
* @param Class_ $node
*/
public function refactor(Node $node): ?Node
{
if ($this->shouldSkipProperty($node)) {
return null;
$hasChanged = false;
foreach ($node->getProperties() as $property) {
if ($this->shouldSkipProperty($property)) {
continue;
}
if ($this->propertyManipulator->isPropertyUsedInReadContext($property)) {
continue;
}
$this->complexNodeRemover->removePropertyAndUsages($node, $property, $this->removeAssignSideEffect);
$hasChanged = true;
}
if ($this->propertyManipulator->isPropertyUsedInReadContext($node)) {
return null;
if ($hasChanged) {
return $node;
}
$this->complexNodeRemover->removePropertyAndUsages($node, $this->removeAssignSideEffect);
return $node;
return null;
}
private function shouldSkipProperty(Property $property): bool
@ -99,11 +112,6 @@ CODE_SAMPLE
return true;
}
if (! $property->isPrivate()) {
return true;
}
$class = $this->betterNodeFinder->findParentType($property, Class_::class);
return ! $class instanceof Class_;
return ! $property->isPrivate();
}
}

View File

@ -240,7 +240,10 @@ CODE_SAMPLE
// Assign the value to the variable, and replace the element with the variable
$newVariable = new Variable($variableName);
$this->nodesToAddCollector->addNodeBeforeNode(new Assign($newVariable, $arrayItem->value), $array);
$newVariableAssign = new Assign($newVariable, $arrayItem->value);
$this->nodesToAddCollector->addNodeBeforeNode($newVariableAssign, $array);
return $newVariable;
}

View File

@ -5,6 +5,8 @@ declare(strict_types=1);
namespace Rector\Removing\NodeManipulator;
use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\ArrayDimFetch;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\PropertyFetch;
use PhpParser\Node\Expr\StaticPropertyFetch;
@ -13,6 +15,7 @@ use PhpParser\Node\Param;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\ClassLike;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\Node\Stmt\Expression;
use PhpParser\Node\Stmt\Property;
use Rector\Core\PhpParser\Comparing\NodeComparator;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
@ -20,32 +23,87 @@ use Rector\Core\PhpParser\NodeFinder\PropertyFetchFinder;
use Rector\Core\ValueObject\MethodName;
use Rector\DeadCode\SideEffect\SideEffectNodeDetector;
use Rector\NodeNameResolver\NodeNameResolver;
use Rector\NodeRemoval\AssignRemover;
use Rector\NodeRemoval\NodeRemover;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Removing\NodeAnalyzer\ForbiddenPropertyRemovalAnalyzer;
use Symplify\Astral\NodeTraverser\SimpleCallableNodeTraverser;
final class ComplexNodeRemover
{
public function __construct(
private readonly AssignRemover $assignRemover,
private readonly PropertyFetchFinder $propertyFetchFinder,
private readonly NodeNameResolver $nodeNameResolver,
private readonly BetterNodeFinder $betterNodeFinder,
private readonly NodeRemover $nodeRemover,
private readonly NodeComparator $nodeComparator,
private readonly ForbiddenPropertyRemovalAnalyzer $forbiddenPropertyRemovalAnalyzer,
private readonly SideEffectNodeDetector $sideEffectNodeDetector
private readonly SideEffectNodeDetector $sideEffectNodeDetector,
private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser,
) {
}
public function removePropertyAndUsages(Property $property, bool $removeAssignSideEffect = true): void
{
public function removePropertyAndUsages(
Class_ $class,
Property $property,
bool $removeAssignSideEffect
): void {
$propertyName = $this->nodeNameResolver->getName($property);
$hasSideEffect = false;
$this->simpleCallableNodeTraverser->traverseNodesWithCallable($class->stmts, function (Node $node) use (
$removeAssignSideEffect,
$propertyName,
&$hasSideEffect
) {
// here should be checked all expr like stmts that can hold assign, e.f. if, foreach etc. etc.
if ($node instanceof Expression) {
$nodeExpr = $node->expr;
// remove direct assigns
if ($nodeExpr instanceof Assign) {
$assign = $nodeExpr;
// skip double assigns
if ($assign->expr instanceof Assign) {
return null;
}
$propertyFetches = $this->resolvePropertyFetchFromDimFetch($assign->var);
if ($propertyFetches === []) {
return null;
}
foreach ($propertyFetches as $propertyFetch) {
if (! $this->nodeNameResolver->isName($propertyFetch->var, 'this')) {
continue;
}
if ($this->nodeNameResolver->isName($propertyFetch->name, $propertyName)) {
if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) {
$hasSideEffect = true;
return null;
}
$this->nodeRemover->removeNode($node);
}
}
}
}
return null;
});
// do not remove anyhting in case of side-effect
if ($hasSideEffect) {
return;
}
$propertyFetches = $this->propertyFetchFinder->findPrivatePropertyFetches($property);
$assigns = [];
foreach ($propertyFetches as $propertyFetch) {
$assign = $this->resolveAssign($propertyFetch);
$assign = $this->resolvePropertyFetchAssign($propertyFetch);
if (! $assign instanceof Assign) {
return;
}
@ -54,10 +112,6 @@ final class ComplexNodeRemover
return;
}
if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) {
return;
}
$assigns[] = $assign;
}
@ -105,12 +159,11 @@ final class ComplexNodeRemover
{
foreach ($assigns as $assign) {
// remove assigns
$this->assignRemover->removeAssignNode($assign);
$this->removeConstructorDependency($assign);
}
}
private function resolveAssign(PropertyFetch | StaticPropertyFetch $expr): ?Assign
private function resolvePropertyFetchAssign(PropertyFetch | StaticPropertyFetch $expr): ?Assign
{
$assign = $expr->getAttribute(AttributeKey::PARENT_NODE);
@ -234,4 +287,27 @@ final class ComplexNodeRemover
$expressionVariable = $node->getAttribute(AttributeKey::PARENT_NODE);
return ! $expressionVariable instanceof Assign;
}
/**
* @return PropertyFetch[]
*/
private function resolvePropertyFetchFromDimFetch(Expr $expr): array
{
// unwrap array dim fetch, till we get to parent too caller node
$propertyFetches = [];
while ($expr instanceof ArrayDimFetch) {
if ($expr->dim instanceof PropertyFetch) {
$propertyFetches[] = $expr->dim;
}
$expr = $expr->var;
}
if ($expr instanceof PropertyFetch) {
$propertyFetches[] = $expr;
}
return $propertyFetches;
}
}

View File

@ -10,12 +10,12 @@ use PhpParser\Node\Expr\MethodCall;
use PhpParser\Node\Expr\New_;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\Expression;
use PHPStan\Type\ObjectType;
use Rector\Core\Contract\Rector\ConfigurableRectorInterface;
use Rector\Core\Rector\AbstractRector;
use Rector\Naming\Naming\PropertyNaming;
use Rector\Naming\ValueObject\ExpectedName;
use Rector\NodeRemoval\AssignRemover;
use Rector\PostRector\Collector\PropertyToAddCollector;
use Rector\PostRector\ValueObject\PropertyMetadata;
use Rector\Transform\NodeFactory\PropertyFetchFactory;
@ -37,7 +37,6 @@ final class NewToConstructorInjectionRector extends AbstractRector implements Co
private readonly PropertyFetchFactory $propertyFetchFactory,
private readonly PropertyNaming $propertyNaming,
private readonly PropertyToAddCollector $propertyToAddCollector,
private readonly AssignRemover $assignRemover
) {
}
@ -86,11 +85,11 @@ CODE_SAMPLE
*/
public function getNodeTypes(): array
{
return [New_::class, Assign::class, MethodCall::class];
return [New_::class, Assign::class, MethodCall::class, Expression::class];
}
/**
* @param New_|Assign|MethodCall $node
* @param New_|Assign|MethodCall|Expression $node
*/
public function refactor(Node $node): ?Node
{
@ -98,8 +97,11 @@ CODE_SAMPLE
return $this->refactorMethodCall($node);
}
if ($node instanceof Assign) {
$this->refactorAssign($node);
if ($node instanceof Expression) {
$nodeExpr = $node->expr;
if ($nodeExpr instanceof Assign) {
$this->refactorAssignExpression($node, $nodeExpr);
}
}
if ($node instanceof New_) {
@ -144,18 +146,19 @@ CODE_SAMPLE
return null;
}
private function refactorAssign(Assign $assign): void
private function refactorAssignExpression(Expression $expression, Assign $assign): void
{
if (! $assign->expr instanceof New_) {
$currentAssign = $assign->expr instanceof Assign ? $assign->expr : $assign;
if (! $currentAssign->expr instanceof New_) {
return;
}
foreach ($this->constructorInjectionObjectTypes as $constructorInjectionObjectType) {
if (! $this->isObjectType($assign->expr, $constructorInjectionObjectType)) {
if (! $this->isObjectType($currentAssign->expr, $constructorInjectionObjectType)) {
continue;
}
$this->assignRemover->removeAssignNode($assign);
$this->removeNode($expression);
}
}