From 433f929cc314046e4f87fdba630ab4bd92830672 Mon Sep 17 00:00:00 2001 From: Ruud Kamphuis Date: Fri, 27 Dec 2019 16:46:52 +0100 Subject: [PATCH] Do not suggest typed property when defined in vendored parent When the property is defined in vendored code, it doesn't make sense to suggest typed properties on classes that extend from it. --- .../Rector/Property/TypedPropertyRector.php | 13 +++- .../parent_has_untyped_property.php.inc | 20 +++++ ...ent_of_parent_has_untyped_property.php.inc | 24 ++++++ .../TypedPropertyRector/Source/SomeParent.php | 18 +++++ .../TypedPropertyRectorTest.php | 1 + .../src/VendorLock/VendorLockResolver.php | 75 ++++++++++++++++++- 6 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_has_untyped_property.php.inc create mode 100644 packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_of_parent_has_untyped_property.php.inc create mode 100644 packages/Php74/tests/Rector/Property/TypedPropertyRector/Source/SomeParent.php diff --git a/packages/Php74/src/Rector/Property/TypedPropertyRector.php b/packages/Php74/src/Rector/Property/TypedPropertyRector.php index f1ffd842a957..dbc03697c434 100644 --- a/packages/Php74/src/Rector/Property/TypedPropertyRector.php +++ b/packages/Php74/src/Rector/Property/TypedPropertyRector.php @@ -11,6 +11,7 @@ use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; use Rector\TypeDeclaration\TypeInferer\PropertyTypeInferer; +use Rector\TypeDeclaration\VendorLock\VendorLockResolver; use Rector\ValueObject\PhpVersionFeature; /** @@ -25,9 +26,15 @@ final class TypedPropertyRector extends AbstractRector */ private $propertyTypeInferer; - public function __construct(PropertyTypeInferer $propertyTypeInferer) + /** + * @var VendorLockResolver + */ + private $vendorLockResolver; + + public function __construct(PropertyTypeInferer $propertyTypeInferer, VendorLockResolver $vendorLockResolver) { $this->propertyTypeInferer = $propertyTypeInferer; + $this->vendorLockResolver = $vendorLockResolver; } public function getDefinition(): RectorDefinition @@ -92,6 +99,10 @@ public function refactor(Node $node): ?Node return null; } + if ($this->vendorLockResolver->isPropertyChangeVendorLockedIn($node)) { + return null; + } + $node->type = $propertyTypeNode; return $node; diff --git a/packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_has_untyped_property.php.inc b/packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_has_untyped_property.php.inc new file mode 100644 index 000000000000..826e7f5841fb --- /dev/null +++ b/packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_has_untyped_property.php.inc @@ -0,0 +1,20 @@ + diff --git a/packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_of_parent_has_untyped_property.php.inc b/packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_of_parent_has_untyped_property.php.inc new file mode 100644 index 000000000000..8d23050a212f --- /dev/null +++ b/packages/Php74/tests/Rector/Property/TypedPropertyRector/Fixture/parent_of_parent_has_untyped_property.php.inc @@ -0,0 +1,24 @@ + diff --git a/packages/Php74/tests/Rector/Property/TypedPropertyRector/Source/SomeParent.php b/packages/Php74/tests/Rector/Property/TypedPropertyRector/Source/SomeParent.php new file mode 100644 index 000000000000..17cdd0b32989 --- /dev/null +++ b/packages/Php74/tests/Rector/Property/TypedPropertyRector/Source/SomeParent.php @@ -0,0 +1,18 @@ += 7.4 * @dataProvider provideDataForTest() */ public function test(string $file): void diff --git a/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php b/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php index 05d377e245da..85fe216ed888 100644 --- a/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php +++ b/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php @@ -4,9 +4,13 @@ namespace Rector\TypeDeclaration\VendorLock; +use PhpParser\Node; use PhpParser\Node\Stmt\Class_; +use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Interface_; +use PhpParser\Node\Stmt\Property; +use PhpParser\Node\Stmt\PropertyProperty; use Rector\NodeContainer\ParsedNodesByType; use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\PhpParser\Node\Manipulator\ClassManipulator; @@ -41,7 +45,12 @@ public function __construct( public function isParameterChangeVendorLockedIn(ClassMethod $classMethod, int $paramPosition): bool { - if (! $this->hasParentClassOrImplementsInterface($classMethod)) { + $classNode = $classMethod->getAttribute(AttributeKey::CLASS_NODE); + if ($classNode === null) { + return false; + } + + if (! $this->hasParentClassOrImplementsInterface($classNode)) { return false; } @@ -102,7 +111,12 @@ public function isParameterChangeVendorLockedIn(ClassMethod $classMethod, int $p public function isReturnChangeVendorLockedIn(ClassMethod $classMethod): bool { - if (! $this->hasParentClassOrImplementsInterface($classMethod)) { + $classNode = $classMethod->getAttribute(AttributeKey::CLASS_NODE); + if ($classNode === null) { + return false; + } + + if (! $this->hasParentClassOrImplementsInterface($classNode)) { return false; } @@ -160,13 +174,66 @@ public function isReturnChangeVendorLockedIn(ClassMethod $classMethod): bool return false; } - private function hasParentClassOrImplementsInterface(ClassMethod $classMethod): bool + public function isPropertyChangeVendorLockedIn(Property $property): bool { - $classNode = $classMethod->getAttribute(AttributeKey::CLASS_NODE); + $classNode = $property->getAttribute(AttributeKey::CLASS_NODE); if ($classNode === null) { return false; } + if (! $this->hasParentClassOrImplementsInterface($classNode)) { + return false; + } + + $propertyName = $this->nameResolver->getName($property); + + // @todo extract to some "inherited parent method" service + /** @var string|null $parentClassName */ + $parentClassName = $classNode->getAttribute(AttributeKey::PARENT_CLASS_NAME); + + if ($parentClassName !== null) { + $parentClassNode = $this->parsedNodesByType->findClass($parentClassName); + if ($parentClassNode !== null) { + $parentPropertyNode = $this->getProperty($parentClassNode, $propertyName); + // @todo validate type is conflicting + // parent class property in local scope → it's ok + if ($parentPropertyNode !== null) { + return $parentPropertyNode->type !== null; + } + + // if not, look for it's parent parent - @todo recursion + } + + if (property_exists($parentClassName, $propertyName)) { + // @todo validate type is conflicting + // parent class property in external scope → it's not ok + return true; + + // if not, look for it's parent parent - @todo recursion + } + } + + return false; + } + + // Until we have getProperty (https://github.com/nikic/PHP-Parser/pull/646) + private function getProperty(ClassLike $classLike, string $name) + { + $lowerName = strtolower($name); + foreach ($classLike->stmts as $stmt) { + if ($stmt instanceof Property) { + foreach ($stmt->props as $prop) { + if ($prop instanceof PropertyProperty && $lowerName === $prop->name->toLowerString()) { + return $stmt; + } + } + } + } + return null; + } + + private function hasParentClassOrImplementsInterface(Node $classNode): bool + { if ($classNode instanceof Class_ || $classNode instanceof Interface_) { if ($classNode->extends) { return true;