Skip to content

Commit

Permalink
[DeadCode] Skip using coealesce assign operator on return on RemoveUn…
Browse files Browse the repository at this point in the history
…usedPrivatePropertyRector (#2476)

Co-authored-by: GitHub Action <action@github.com>
  • Loading branch information
samsonasik and actions-user committed Jun 11, 2022
1 parent a8dae7f commit 9679ed6
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 28 deletions.
8 changes: 4 additions & 4 deletions packages/ReadWrite/NodeAnalyzer/ReadWritePropertyAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@ public function isRead(PropertyFetch | StaticPropertyFetch $node): bool
return true;
}

if ($parent instanceof ArrayDimFetch && $parent->dim === $node && $this->isNotInsideIssetUnset($parent)) {
return $this->isArrayDimFetchRead($parent);
}

if (! $parent instanceof ArrayDimFetch) {
return ! $this->assignManipulator->isLeftPartOfAssign($node);
}

if ($parent->dim === $node && $this->isNotInsideIssetUnset($parent)) {
return $this->isArrayDimFetchRead($parent);
}

if ($this->assignManipulator->isLeftPartOfAssign($parent)) {
return false;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;

final class SkipUsingCoalesceAssignOperator
{
private SomeService $someService;

private array $types = [];

public function __construct(SomeService $someService)
{
$this->someService = $someService;
}

public function get(string $key)
{
return $this->types[$key] ??= $this->someService->resolve();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace Rector\Tests\DeadCode\Rector\Property\RemoveUnusedPrivatePropertyRector\Fixture;

final class SkipUsingCoalesceAssignOperator2
{
private SomeService $someService;

private array $types = [];

public function __construct(SomeService $someService)
{
$this->someService = $someService;
}

public function get(string $key, string $key2)
{
return $this->types[$key][$key2] ??= $this->someService->resolve();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public function getNodeTypes(): array
*/
public function refactor(Node $node): ?Node
{
$hasChanged = false;
$hasRemoved = false;

foreach ($node->getProperties() as $property) {
if ($this->shouldSkipProperty($property)) {
Expand All @@ -94,12 +94,20 @@ public function refactor(Node $node): ?Node
continue;
}

$this->complexNodeRemover->removePropertyAndUsages($node, $property, $this->removeAssignSideEffect);
// use different variable to avoid re-assign back $hasRemoved to false
// when already asssigned to true
$isRemoved = $this->complexNodeRemover->removePropertyAndUsages(
$node,
$property,
$this->removeAssignSideEffect
);

$hasChanged = true;
if ($isRemoved) {
$hasRemoved = true;
}
}

return $hasChanged ? $node : null;
return $hasRemoved ? $node : null;
}

private function shouldSkipProperty(Property $property): bool
Expand Down
43 changes: 25 additions & 18 deletions rules/Removing/NodeManipulator/ComplexNodeRemover.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
use PhpParser\Node\Stmt\Expression;
use PhpParser\Node\Stmt\Property;
use Rector\Core\NodeAnalyzer\PropertyFetchAnalyzer;
use Rector\Core\PhpParser\Comparing\NodeComparator;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
use Rector\Core\ValueObject\MethodName;
use Rector\DeadCode\SideEffect\SideEffectNodeDetector;
use Rector\NodeNameResolver\NodeNameResolver;
use Rector\NodeRemoval\NodeRemover;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Symplify\Astral\NodeTraverser\SimpleCallableNodeTraverser;

final class ComplexNodeRemover
Expand All @@ -32,26 +34,24 @@ public function __construct(
private readonly NodeRemover $nodeRemover,
private readonly SideEffectNodeDetector $sideEffectNodeDetector,
private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser,
private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer
private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer,
private readonly NodeComparator $nodeComparator
) {
}

public function removePropertyAndUsages(
Class_ $class,
Property $property,
bool $removeAssignSideEffect
): void {
): bool {
$propertyName = $this->nodeNameResolver->getName($property);

$hasSideEffect = false;
$isPartOfAnotherAssign = false;
$totalPropertyFetch = $this->propertyFetchAnalyzer->countLocalPropertyFetchName($class, $propertyName);

$this->simpleCallableNodeTraverser->traverseNodesWithCallable($class->stmts, function (Node $node) use (
$removeAssignSideEffect,
$propertyName,
&$hasSideEffect,
&$isPartOfAnotherAssign
) {
&$totalPropertyFetch
): ?Node {
// here should be checked all expr like stmts that can hold assign, e.f. if, foreach etc. etc.
if (! $node instanceof Expression) {
return null;
Expand All @@ -68,7 +68,11 @@ public function removePropertyAndUsages(

// skip double assigns
if ($assign->expr instanceof Assign) {
$isPartOfAnotherAssign = true;
return null;
}

$originalNode = $assign->getAttribute(AttributeKey::ORIGINAL_NODE);
if (! $this->nodeComparator->areNodesEqual($originalNode, $assign)) {
return null;
}

Expand All @@ -77,32 +81,35 @@ public function removePropertyAndUsages(
return null;
}

$currentTotalPropertyFetch = $totalPropertyFetch;
foreach ($propertyFetches as $propertyFetch) {
if ($this->nodeNameResolver->isName($propertyFetch->name, $propertyName)) {
if (! $removeAssignSideEffect && $this->sideEffectNodeDetector->detect($assign->expr)) {
$hasSideEffect = true;
return null;
}

$this->nodeRemover->removeNode($node);
--$totalPropertyFetch;
}
}

if ($totalPropertyFetch < $currentTotalPropertyFetch) {
$this->nodeRemover->removeNode($node);
return $node;
}

return null;
});

// do not remove anyhting in case of side-effect
if ($hasSideEffect) {
return;
}

if ($isPartOfAnotherAssign) {
return;
// not all property fetch with name removed
if ($totalPropertyFetch > 0) {
return false;
}

$this->removeConstructorDependency($class, $propertyName);

$this->nodeRemover->removeNode($property);

return true;
}

/**
Expand Down
53 changes: 51 additions & 2 deletions src/NodeAnalyzer/PropertyFetchAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
use Rector\Core\PhpParser\Node\BetterNodeFinder;
use Rector\Core\ValueObject\MethodName;
use Rector\NodeNameResolver\NodeNameResolver;
use Symplify\Astral\NodeTraverser\SimpleCallableNodeTraverser;

final class PropertyFetchAnalyzer
{
Expand All @@ -34,7 +35,8 @@ final class PropertyFetchAnalyzer
public function __construct(
private readonly NodeNameResolver $nodeNameResolver,
private readonly BetterNodeFinder $betterNodeFinder,
private readonly AstResolver $astResolver
private readonly AstResolver $astResolver,
private readonly SimpleCallableNodeTraverser $simpleCallableNodeTraverser
) {
}

Expand Down Expand Up @@ -72,11 +74,58 @@ public function isLocalPropertyFetchName(Node $node, string $desiredPropertyName
return $this->nodeNameResolver->isName($node->name, $desiredPropertyName);
}

public function countLocalPropertyFetchName(ClassLike $classLike, string $propertyName): int
{
$total = 0;

$this->simpleCallableNodeTraverser->traverseNodesWithCallable($classLike->stmts, function (Node $subNode) use (
$classLike,
$propertyName,
&$total
): ?Node {
if (! $this->isLocalPropertyFetchName($subNode, $propertyName)) {
return null;
}

$parentClassLike = $this->betterNodeFinder->findParentType($subNode, ClassLike::class);

// property fetch in Trait cannot get parent ClassLike
if (! $parentClassLike instanceof ClassLike) {
++$total;
}

if ($parentClassLike === $classLike) {
++$total;
}

return $subNode;
});

return $total;
}

public function containsLocalPropertyFetchName(Node $node, string $propertyName): bool
{
$classLike = $node instanceof ClassLike
? $node
: $this->betterNodeFinder->findParentType($node, ClassLike::class);

return (bool) $this->betterNodeFinder->findFirst(
$node,
fn (Node $node): bool => $this->isLocalPropertyFetchName($node, $propertyName)
function (Node $node) use ($classLike, $propertyName): bool {
if (! $this->isLocalPropertyFetchName($node, $propertyName)) {
return false;
}

$parentClassLike = $this->betterNodeFinder->findParentType($node, ClassLike::class);

// property fetch in Trait cannot get parent ClassLike
if (! $parentClassLike instanceof ClassLike) {
return true;
}

return $parentClassLike === $classLike;
}
);
}

Expand Down

0 comments on commit 9679ed6

Please sign in to comment.