Merge pull request #1288 from rectorphp/get-and-set-fix

[MagicDisclosure] Fix property assign magic call [closes #1190]

src/Console/Command/ProcessCommand.php

@@ -18,7 +18,6 @@ use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Symplify\PackageBuilder\Console\Command\CommandNaming;
-use Symplify\PackageBuilder\Parameter\ParameterProvider;
 
 final class ProcessCommand extends AbstractCommand
 {
@@ -70,8 +69,11 @@ final class ProcessCommand extends AbstractCommand
     /**
      * @var string[]
      */
-    private $fileExtensions;
+    private $fileExtensions = [];
 
+    /**
+     * @param string[] $fileExtensions
+     */
     public function __construct(
         SymfonyStyle $symfonyStyle,
         FilesFinder $phpFilesFinder,

src/PhpParser/Node/Manipulator/ClassManipulator.php

@@ -240,6 +240,25 @@ final class ClassManipulator
         });
     }
 
+    public function hasPropertyFetchAsProperty(Class_ $class, Node\Expr\PropertyFetch $propertyFetch): bool
+    {
+        if (! $this->nameResolver->isName($propertyFetch->var, 'this')) {
+            return false;
+        }
+
+        foreach ((array) $class->stmts as $classStmt) {
+            if (! $classStmt instanceof Property) {
+                continue;
+            }
+
+            if ($this->nameResolver->areNamesEqual($classStmt->props[0], $propertyFetch)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     private function tryInsertBeforeFirstMethod(Class_ $classNode, Stmt $stmt): bool
     {
         foreach ($classNode->stmts as $key => $classElementNode) {

src/PhpParser/Node/Manipulator/PropertyFetchManipulator.php

@@ -4,7 +4,6 @@ namespace Rector\PhpParser\Node\Manipulator;
 
 use PhpParser\Node;
 use PhpParser\Node\Expr\PropertyFetch;
-use PhpParser\Node\Expr\Variable;
 use PHPStan\Analyser\Scope;
 use PHPStan\Broker\Broker;
 use PHPStan\Type\ObjectType;
@@ -34,20 +33,36 @@ final class PropertyFetchManipulator
      */
     private $nameResolver;
 
-    public function __construct(NodeTypeResolver $nodeTypeResolver, Broker $broker, NameResolver $nameResolver)
-    {
+    /**
+     * @var ClassManipulator
+     */
+    private $classManipulator;
+
+    public function __construct(
+        NodeTypeResolver $nodeTypeResolver,
+        Broker $broker,
+        NameResolver $nameResolver,
+        ClassManipulator $classManipulator
+    ) {
         $this->nodeTypeResolver = $nodeTypeResolver;
         $this->broker = $broker;
         $this->nameResolver = $nameResolver;
+        $this->classManipulator = $classManipulator;
     }
 
     public function isPropertyToSelf(PropertyFetch $propertyFetch): bool
     {
-        if (! $propertyFetch->var instanceof Variable) {
+        if (! $this->nameResolver->isName($propertyFetch->var, 'this')) {
             return false;
         }
 
-        return $propertyFetch->var->name === 'this';
+        /** @var Node\Stmt\Class_|null $class */
+        $class = $propertyFetch->getAttribute(Attribute::CLASS_NODE);
+        if ($class === null) {
+            return false;
+        }
+
+        return $this->classManipulator->hasPropertyFetchAsProperty($class, $propertyFetch);
     }
 
     public function isMagicOnType(Node $node, string $type): bool

src/PhpParser/Node/Resolver/NameResolver.php

@@ -192,4 +192,9 @@ final class NameResolver
 
         return (string) $node->name;
     }
+
+    public function areNamesEqual(Node $firstNode, Node $secondNode): bool
+    {
+        return $this->resolve($firstNode) === $this->resolve($secondNode);
+    }
 }

src/Rector/MagicDisclosure/GetAndSetToMethodCallRector.php

@@ -7,7 +7,7 @@ use PhpParser\Node\Expr\Assign;
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\PropertyFetch;
 use PhpParser\Node\Expr\Variable;
-use PhpParser\Node\Stmt\If_;
+use Rector\NodeTypeResolver\Node\Attribute;
 use Rector\PhpParser\Node\Manipulator\PropertyFetchManipulator;
 use Rector\Rector\AbstractRector;
 use Rector\RectorDefinition\ConfiguredCodeSample;
@@ -83,58 +83,23 @@ CODE_SAMPLE
      */
     public function getNodeTypes(): array
     {
-        return [Assign::class, If_::class];
+        return [Assign::class, PropertyFetch::class];
     }
 
     /**
-     * @param Assign|If_ $node
+     * @param Assign|PropertyFetch $node
      */
     public function refactor(Node $node): ?Node
     {
         if ($node instanceof Assign) {
-            if ($node->expr instanceof PropertyFetch) {
-                return $this->processMagicGet($node);
-            }
-
             if ($node->var instanceof PropertyFetch) {
                 return $this->processMagicSet($node);
             }
+
+            return null;
         }
 
-        if ($node instanceof If_ && $node->cond instanceof PropertyFetch) {
-            return $this->processMagicGetOnIf($node);
-        }
-
-        return null;
-    }
-
-    private function processMagicGet(Assign $assign): ?Node
-    {
-        /** @var PropertyFetch $propertyFetchNode */
-        $propertyFetchNode = $assign->expr;
-
-        foreach ($this->typeToMethodCalls as $type => $transformation) {
-            if (! $this->isType($propertyFetchNode, $type)) {
-                continue;
-            }
-
-            if ($this->propertyFetchManipulator->isPropertyToSelf($propertyFetchNode)) {
-                continue;
-            }
-
-            if (! $this->propertyFetchManipulator->isMagicOnType($propertyFetchNode, $type)) {
-                continue;
-            }
-
-            $assign->expr = $this->createMethodCallNodeFromPropertyFetchNode(
-                $propertyFetchNode,
-                $transformation['get']
-            );
-
-            return $assign;
-        }
-
-        return null;
+        return $this->processPropertyFetch($node);
     }
 
     private function processMagicSet(Assign $assign): ?Node
@@ -143,15 +108,7 @@ CODE_SAMPLE
         $propertyFetchNode = $assign->var;
 
         foreach ($this->typeToMethodCalls as $type => $transformation) {
-            if (! $this->isType($propertyFetchNode, $type)) {
-                continue;
-            }
-
-            if ($this->propertyFetchManipulator->isPropertyToSelf($propertyFetchNode)) {
-                continue;
-            }
-
-            if (! $this->propertyFetchManipulator->isMagicOnType($propertyFetchNode, $type)) {
+            if ($this->shouldSkipPropertyFetch($propertyFetchNode, $type)) {
                 continue;
             }
 
@@ -165,31 +122,6 @@ CODE_SAMPLE
         return null;
     }
 
-    private function processMagicGetOnIf(If_ $if): ?Node
-    {
-        /** @var PropertyFetch $propertyFetchNode */
-        $propertyFetchNode = $if->cond;
-
-        foreach ($this->typeToMethodCalls as $type => $transformation) {
-            if (! $this->isType($propertyFetchNode, $type)) {
-                continue;
-            }
-
-            if (! $this->propertyFetchManipulator->isMagicOnType($propertyFetchNode, $type)) {
-                continue;
-            }
-
-            $if->cond = $this->createMethodCallNodeFromPropertyFetchNode(
-                $propertyFetchNode,
-                $transformation['get']
-            );
-
-            return $if;
-        }
-
-        return null;
-    }
-
     private function createMethodCallNodeFromPropertyFetchNode(
         PropertyFetch $propertyFetch,
         string $method
@@ -210,4 +142,40 @@ CODE_SAMPLE
 
         return $this->createMethodCall($variableNode, $method, [$this->getName($propertyFetch), $node]);
     }
+
+    private function shouldSkipPropertyFetch(PropertyFetch $propertyFetch, string $type): bool
+    {
+        if (! $this->isType($propertyFetch, $type)) {
+            return true;
+        }
+
+        if (! $this->propertyFetchManipulator->isMagicOnType($propertyFetch, $type)) {
+            return true;
+        }
+
+        // $this->value = $value
+        return $this->propertyFetchManipulator->isPropertyToSelf($propertyFetch);
+    }
+
+    private function processPropertyFetch(PropertyFetch $propertyFetch): ?MethodCall
+    {
+        foreach ($this->typeToMethodCalls as $type => $transformation) {
+            if ($this->shouldSkipPropertyFetch($propertyFetch, $type)) {
+                continue;
+            }
+
+            // setter, skip
+            $parentNode = $propertyFetch->getAttribute(Attribute::PARENT_NODE);
+
+            if ($parentNode instanceof Assign) {
+                if ($parentNode->var === $propertyFetch) {
+                    continue;
+                }
+            }
+
+            return $this->createMethodCallNodeFromPropertyFetchNode($propertyFetch, $transformation['get']);
+        }
+
+        return null;
+    }
 }

src/Rector/NameResolverTrait.php

@@ -31,7 +31,7 @@ trait NameResolverTrait
 
     public function areNamesEqual(Node $firstNode, Node $secondNode): bool
     {
-        return $this->getName($firstNode) === $this->getName($secondNode);
+        return $this->nameResolver->areNamesEqual($firstNode, $secondNode);
     }
 
     public function isNameInsensitive(Node $node, string $name): bool

tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/klarka.php.inc

@@ -0,0 +1,55 @@
+<?php
+
+namespace Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Fixture;
+
+use Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Source\Klarka;
+
+class KlarkaExtended extends Klarka
+{
+    private $existingProperty;
+
+    public function run()
+    {
+        if ($this->leafBreadcrumbCategory) {
+            $category = $this->leafBreadcrumbCategory;
+        }
+
+        while ($this->leafLet !== 5) {
+        }
+
+        while ($this->existingProperty !== 5) {
+        }
+        while ($this->existingProperty) {
+        }
+    }
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Fixture;
+
+use Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Source\Klarka;
+
+class KlarkaExtended extends Klarka
+{
+    private $existingProperty;
+
+    public function run()
+    {
+        if ($this->get('leafBreadcrumbCategory')) {
+            $category = $this->get('leafBreadcrumbCategory');
+        }
+
+        while ($this->get('leafLet') !== 5) {
+        }
+
+        while ($this->existingProperty !== 5) {
+        }
+        while ($this->existingProperty) {
+        }
+    }
+}
+
+?>

tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/GetAndSetToMethodCallRectorTest.php

@@ -4,19 +4,19 @@ namespace Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector;
 
 use Rector\Rector\MagicDisclosure\GetAndSetToMethodCallRector;
 use Rector\Testing\PHPUnit\AbstractRectorTestCase;
+use Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Source\Klarka;
 use Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Source\SomeContainer;
 
 final class GetAndSetToMethodCallRectorTest extends AbstractRectorTestCase
 {
     public function test(): void
     {
-        $this->doTestFiles(
-            [
-                __DIR__ . '/Fixture/fixture.php.inc',
-                __DIR__ . '/Fixture/fixture2.php.inc',
-                __DIR__ . '/Fixture/fixture3.php.inc',
-            ]
-        );
+        $this->doTestFiles([
+            __DIR__ . '/Fixture/get.php.inc',
+            __DIR__ . '/Fixture/fixture2.php.inc',
+            __DIR__ . '/Fixture/fixture3.php.inc',
+            __DIR__ . '/Fixture/klarka.php.inc',
+        ]);
     }
 
     protected function getRectorClass(): string
@@ -38,6 +38,9 @@ final class GetAndSetToMethodCallRectorTest extends AbstractRectorTestCase
                 'get' => 'getService',
                 'set' => 'addService',
             ],
+            Klarka::class => [
+                'get' => 'get',
+            ],
         ];
     }
 }

tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Source/Klarka.php

@@ -0,0 +1,8 @@
+<?php declare(strict_types=1);
+
+namespace Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Source;
+
+class Klarka
+{
+
+}