From 220038d215291309f5815962cde39920ec7b4756 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 2 Apr 2019 23:02:20 +0200 Subject: [PATCH] improve GetAndSetToMethodCallRector --- docs/AllRectorsOverview.md | 2 +- docs/NodesOverview.md | 2 +- .../Node/Manipulator/ClassManipulator.php | 19 +++ .../Manipulator/PropertyFetchManipulator.php | 25 +++- src/PhpParser/Node/Resolver/NameResolver.php | 5 + .../GetAndSetToMethodCallRector.php | 114 +++++++----------- src/Rector/NameResolverTrait.php | 2 +- .../Fixture/{fixture.php.inc => get.php.inc} | 0 .../Fixture/klarka.php.inc | 20 +++ .../GetAndSetToMethodCallRectorTest.php | 2 +- 10 files changed, 111 insertions(+), 80 deletions(-) rename tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/{fixture.php.inc => get.php.inc} (100%) diff --git a/docs/AllRectorsOverview.md b/docs/AllRectorsOverview.md index c86df48da67..940c48fb04e 100644 --- a/docs/AllRectorsOverview.md +++ b/docs/AllRectorsOverview.md @@ -2714,7 +2714,7 @@ Changes rand, srand and getrandmax by new md_* alternatives. - class: `Rector\Php\Rector\FuncCall\PregReplaceEModifierRector` -The /e modifier is no longer supported, use preg_replace_callback instead +The /e modifier is no longer supported, use preg_replace_callback instead ```diff class SomeClass diff --git a/docs/NodesOverview.md b/docs/NodesOverview.md index 81aef55e937..134b5d8a865 100644 --- a/docs/NodesOverview.md +++ b/docs/NodesOverview.md @@ -910,7 +910,7 @@ if (true) { ```php ?> -feelfeel diff --git a/src/PhpParser/Node/Manipulator/ClassManipulator.php b/src/PhpParser/Node/Manipulator/ClassManipulator.php index 9d8afd05d1d..fa28bab056b 100644 --- a/src/PhpParser/Node/Manipulator/ClassManipulator.php +++ b/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) { diff --git a/src/PhpParser/Node/Manipulator/PropertyFetchManipulator.php b/src/PhpParser/Node/Manipulator/PropertyFetchManipulator.php index 91e76615f46..a235f033b0b 100644 --- a/src/PhpParser/Node/Manipulator/PropertyFetchManipulator.php +++ b/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 diff --git a/src/PhpParser/Node/Resolver/NameResolver.php b/src/PhpParser/Node/Resolver/NameResolver.php index ac536df6caa..ecdcb44e06c 100644 --- a/src/PhpParser/Node/Resolver/NameResolver.php +++ b/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); + } } diff --git a/src/Rector/MagicDisclosure/GetAndSetToMethodCallRector.php b/src/Rector/MagicDisclosure/GetAndSetToMethodCallRector.php index aa3eece6e8b..9f71763b65e 100644 --- a/src/Rector/MagicDisclosure/GetAndSetToMethodCallRector.php +++ b/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,54 +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->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 @@ -139,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; } @@ -161,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 @@ -206,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; + } } diff --git a/src/Rector/NameResolverTrait.php b/src/Rector/NameResolverTrait.php index c7c7f8a2899..7b550c1e71d 100644 --- a/src/Rector/NameResolverTrait.php +++ b/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 diff --git a/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/fixture.php.inc b/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/get.php.inc similarity index 100% rename from tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/fixture.php.inc rename to tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/get.php.inc diff --git a/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/klarka.php.inc b/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/klarka.php.inc index 0980e65f20c..5966001f2fc 100644 --- a/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/klarka.php.inc +++ b/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/Fixture/klarka.php.inc @@ -6,11 +6,21 @@ use Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Source\Klark 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) { + } } } @@ -24,11 +34,21 @@ use Rector\Tests\Rector\MagicDisclosure\GetAndSetToMethodCallRector\Source\Klark 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) { + } } } diff --git a/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/GetAndSetToMethodCallRectorTest.php b/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/GetAndSetToMethodCallRectorTest.php index 52eac943298..a19d2821a1e 100644 --- a/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/GetAndSetToMethodCallRectorTest.php +++ b/tests/Rector/MagicDisclosure/GetAndSetToMethodCallRector/GetAndSetToMethodCallRectorTest.php @@ -12,7 +12,7 @@ final class GetAndSetToMethodCallRectorTest extends AbstractRectorTestCase public function test(): void { $this->doTestFiles([ - __DIR__ . '/Fixture/fixture.php.inc', + __DIR__ . '/Fixture/get.php.inc', __DIR__ . '/Fixture/fixture2.php.inc', __DIR__ . '/Fixture/fixture3.php.inc', __DIR__ . '/Fixture/klarka.php.inc',