diff --git a/packages/ReadWrite/NodeAnalyzer/ReadWritePropertyAnalyzer.php b/packages/ReadWrite/NodeAnalyzer/ReadWritePropertyAnalyzer.php index 6736d3c4cc2..329b027bcc4 100644 --- a/packages/ReadWrite/NodeAnalyzer/ReadWritePropertyAnalyzer.php +++ b/packages/ReadWrite/NodeAnalyzer/ReadWritePropertyAnalyzer.php @@ -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; } diff --git a/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/skip_using_coalesce_assign_operator.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/skip_using_coalesce_assign_operator.php.inc new file mode 100644 index 00000000000..8b46483e440 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/skip_using_coalesce_assign_operator.php.inc @@ -0,0 +1,20 @@ +someService = $someService; + } + + public function get(string $key) + { + return $this->types[$key] ??= $this->someService->resolve(); + } +} diff --git a/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/skip_using_coalesce_assign_operator2.php.inc b/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/skip_using_coalesce_assign_operator2.php.inc new file mode 100644 index 00000000000..80d7a23d804 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector/Fixture/skip_using_coalesce_assign_operator2.php.inc @@ -0,0 +1,20 @@ +someService = $someService; + } + + public function get(string $key, string $key2) + { + return $this->types[$key][$key2] ??= $this->someService->resolve(); + } +} diff --git a/rules/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector.php b/rules/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector.php index d3e557dd3fd..2af8f56cfdd 100644 --- a/rules/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector.php +++ b/rules/DeadCode/Rector/Property/RemoveUnusedPrivatePropertyRector.php @@ -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)) { @@ -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 diff --git a/rules/Removing/NodeManipulator/ComplexNodeRemover.php b/rules/Removing/NodeManipulator/ComplexNodeRemover.php index b8559c4c70c..6307ad66d8a 100644 --- a/rules/Removing/NodeManipulator/ComplexNodeRemover.php +++ b/rules/Removing/NodeManipulator/ComplexNodeRemover.php @@ -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 @@ -32,7 +34,8 @@ 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 ) { } @@ -40,18 +43,15 @@ 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; @@ -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; } @@ -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; } /** diff --git a/src/NodeAnalyzer/PropertyFetchAnalyzer.php b/src/NodeAnalyzer/PropertyFetchAnalyzer.php index c45b997257a..89eec091903 100644 --- a/src/NodeAnalyzer/PropertyFetchAnalyzer.php +++ b/src/NodeAnalyzer/PropertyFetchAnalyzer.php @@ -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 { @@ -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 ) { } @@ -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; + } ); }