Fix renaming attributed class members (#2627)

* Fix renaming attributed methods * Add separate test * Change visibility * Remove left `continue` * Clear the logic * Fix also renaming (not promoted) properties * Fix use explicit types * format * Add test for promoted properties * Fix for class constants * Support enum cases * Use `getFileContents()` for node * Only tokens * Simplify * Remove unused import * Simplify * Simplify * Simplify * Compare strictly * Reformat ReflectionMethod to version from master --------- Co-authored-by: __ <__@__>
phpactor · Apr 6, 2024 · 6478aa0 · 6478aa0
1 parent ef18b10
commit 6478aa0
Show file tree

Hide file tree

Showing 13 changed files with 221 additions and 5 deletions.
diff --git a/lib/Rename/Tests/Cases/member_renamer/constant_declaration_public/ClassOne.ph b/lib/Rename/Tests/Cases/member_renamer/constant_declaration_public/ClassOne.ph
@@ -1,6 +1,11 @@
 <?php
 
+use JetBrains\PhpStorm\Deprecated;
+
 class ClassOne
 {
     public const FOO = 'bar';
+
+    #[Deprecated]
+    public const MOO = 'bar', ZOO = 'bar';
 }
diff --git a/lib/Rename/Tests/Cases/member_renamer/constant_declaration_public/ClassTwo.ph b/lib/Rename/Tests/Cases/member_renamer/constant_declaration_public/ClassTwo.ph
@@ -6,4 +6,9 @@ class ClassTwo extends ClassOne
     {
         return parent::FOO;
     }
+
+    public function barzoo(): string
+    {
+        return parent::ZOO;
+    }
 }
diff --git a/lib/Rename/Tests/Cases/member_renamer/constant_declaration_public/test.ph b/lib/Rename/Tests/Cases/member_renamer/constant_declaration_public/test.ph
@@ -13,3 +13,7 @@ if (ClassOne::FOO !== 'bar') {
     echo 'expected "foobar" but didn\'t get it';
     exit(127);
 }
+if (ClassOne::ZOO !== 'bar') {
+    echo 'expected "foobar" but didn\'t get it';
+    exit(127);
+}
diff --git a/lib/Rename/Tests/Cases/member_renamer/enum_case_declaration_private/ClassOne.ph b/lib/Rename/Tests/Cases/member_renamer/enum_case_declaration_private/ClassOne.ph
@@ -1,7 +1,10 @@
 <?php
 
+use JetBrains\PhpStorm\Deprecated;
+
 enum ClassOne
 {
     case BAR;
+    #[Deprecated]
     case BAZ;
 }
diff --git a/lib/Rename/Tests/Cases/member_renamer/method_declaration/ClassOne.ph b/lib/Rename/Tests/Cases/member_renamer/method_declaration/ClassOne.ph
@@ -6,4 +6,10 @@ class ClassOne
     {
         return 'foobar';
     }
+
+    #[\ReturnTypeWillChange]
+    public function złom(): string
+    {
+        return 'f';
+    }
 }
diff --git a/lib/Rename/Tests/Cases/member_renamer/property_declaration_public/ClassOne.ph b/lib/Rename/Tests/Cases/member_renamer/property_declaration_public/ClassOne.ph
@@ -1,9 +1,14 @@
 <?php
 
+use JetBrains\PhpStorm\Deprecated;
+
 class ClassOne
 {
     public string $foobar;
 
+    #[Deprecated]
+    public string $found = '';
+
     public function __construct(string $foobar)
     {
         $this->foobar = $foobar;

diff --git a/lib/Rename/Tests/Cases/member_renamer/property_declaration_public/ClassTwo.ph b/lib/Rename/Tests/Cases/member_renamer/property_declaration_public/ClassTwo.ph
@@ -4,6 +4,8 @@ class ClassTwo extends ClassOne
 {
     public function barfoo(): string
     {
-        return $this->foobar;
+        $foobar = $this->foobar;
+
+        return $this->found;
     }
 }
diff --git a/lib/Rename/Tests/Cases/member_renamer/property_declaration_public/test.ph b/lib/Rename/Tests/Cases/member_renamer/property_declaration_public/test.ph
@@ -13,3 +13,7 @@ if (!$two->foobar === 'foobar') {
     echo 'expected "foobar" but didn\'t get it';
     exit(127);
 }
+if (!$two->found === 'foobar') {
+    echo 'expected "foobar" but didn\'t get it';
+    exit(127);
+}
diff --git a/lib/Rename/Tests/Cases/member_renamer/property_promoted_declaration_public/ClassOne.ph b/lib/Rename/Tests/Cases/member_renamer/property_promoted_declaration_public/ClassOne.ph
@@ -2,10 +2,15 @@
 
 namespace Test;
 
+use JetBrains\PhpStorm\Deprecated;
+
 class ClassOne
 {
-    public function __construct(public string $foobar)
-    {
+    public function __construct(
+        public string $foobar,
+        #[Deprecated]
+        private string $depOld,
+    ) {
     }
 
     public function bar(): string

diff --git a/lib/Rename/Tests/Cases/member_renamer/property_promoted_declaration_public/ClassTwo.ph b/lib/Rename/Tests/Cases/member_renamer/property_promoted_declaration_public/ClassTwo.ph
@@ -6,4 +6,9 @@ class ClassTwo extends Test\ClassOne
     {
         return $this->foobar;
     }
+
+    public function dep(): string
+    {
+        return $this->depOld;
+    }
 }
diff --git a/lib/Rename/Tests/Cases/member_renamer/property_promoted_declaration_public/test.ph b/lib/Rename/Tests/Cases/member_renamer/property_promoted_declaration_public/test.ph
@@ -3,8 +3,8 @@
 require __DIR__ . '/ClassOne.php';
 require __DIR__ . '/ClassTwo.php';
 
-$two = new ClassTwo('foobar');
-$one = new Test\ClassOne('foobar');
+$two = new ClassTwo('foobar', 'foobar');
+$one = new Test\ClassOne('foobar', 'foobar');
 if (!$two->barfoo() === 'foobar') {
     echo 'expected "foobar" but didn\'t get it';
     exit(127);
@@ -13,6 +13,10 @@ if (!$two->foobar === 'foobar') {
     echo 'expected "foobar" but didn\'t get it';
     exit(127);
 }
+if (!$two->depOld === 'foobar') {
+    echo 'expected "foobar" but didn\'t get it';
+    exit(127);
+}
 if (!$two->barfoo() === 'foobar') {
     echo 'expected "foobar" but didn\'t get it';
     exit(127);

diff --git a/lib/Rename/Tests/Integration/Adapter/ReferenceFinder/MemberRenamerTest.php b/lib/Rename/Tests/Integration/Adapter/ReferenceFinder/MemberRenamerTest.php
@@ -13,6 +13,7 @@
 use Phpactor\Rename\Tests\RenamerTestCase;
 use Phpactor\TextDocument\ByteOffset;
 use Phpactor\TextDocument\TextDocumentBuilder;
+use Phpactor\WorseReflection\Bridge\TolerantParser\Reflection\ReflectionPropertyAccess;
 use Phpactor\WorseReflection\Core\Reflection\ReflectionMethodCall;
 use Phpactor\WorseReflection\Reflector;
 
@@ -67,6 +68,24 @@ function (Reflector $reflector): void {
             }
         ];
 
+        yield 'attributed method declaration' => [
+            'member_renamer/method_declaration',
+            function (Reflector $reflector, Renamer $renamer): Generator {
+                $reflection = $reflector->reflectClass('ClassOne');
+                $method = $reflection->methods()->get('złom');
+
+                return $renamer->rename(
+                    $reflection->sourceCode(),
+                    $method->nameRange()->start(),
+                    'scrap'
+                );
+            },
+            function (Reflector $reflector): void {
+                $reflection = $reflector->reflectClass('ClassOne');
+                self::assertTrue($reflection->methods()->has('scrap'));
+            }
+        ];
+
         yield 'method reference' => [
             'member_renamer/method_declaration',
             function (Reflector $reflector, Renamer $renamer): Generator {
@@ -158,6 +177,35 @@ function (Reflector $reflector): void {
             }
         ];
 
+        yield 'attributed property declaration public' => [
+            'member_renamer/property_declaration_public',
+            function (Reflector $reflector, Renamer $renamer): Generator {
+                $reflection = $reflector->reflectClass('ClassOne');
+                $property = $reflection->properties()->get('found');
+
+                return $renamer->rename(
+                    $reflection->sourceCode(),
+                    $property->nameRange()->start(),
+                    'results'
+                );
+            },
+            function (Reflector $reflector): void {
+                $propertyAccesses = [...$reflector->navigate(
+                    TextDocumentBuilder::fromUri($this->workspace()->path('project/ClassTwo.php'))->build()
+                )->propertyAccesses()];
+                $property = end($propertyAccesses);
+                self::assertInstanceOf(ReflectionPropertyAccess::class, $property);
+                self::assertEquals('results', $property->name());
+
+                $propertyAccesses = [...$reflector->navigate(
+                    TextDocumentBuilder::fromUri($this->workspace()->path('project/test.php'))->build()
+                )->propertyAccesses()->getIterator()];
+                $property = end($propertyAccesses);
+                self::assertInstanceOf(ReflectionPropertyAccess::class, $property);
+                self::assertEquals('results', $property->name());
+            }
+        ];
+
         yield 'property declaration generic' => [
             'member_renamer/property_declaration_public_generic',
             function (Reflector $reflector, Renamer $renamer): Generator {
@@ -210,6 +258,34 @@ function (Reflector $reflector): void {
             }
         ];
 
+        yield 'attributed promoted property declaration public' => [
+            'member_renamer/property_promoted_declaration_public',
+            function (Reflector $reflector, Renamer $renamer): Generator {
+                $reflection = $reflector->reflectClass('Test\ClassOne');
+                $property = $reflection->properties()->get('depOld');
+
+                return $renamer->rename(
+                    $reflection->sourceCode(),
+                    $property->nameRange()->start(),
+                    'depNew'
+                );
+            },
+            function (Reflector $reflector): void {
+                $propertyAccesses = [...$reflector->navigate(
+                    TextDocumentBuilder::fromUri($this->workspace()->path('project/ClassTwo.php'))->build()
+                )->propertyAccesses()];
+                $property = end($propertyAccesses);
+                self::assertInstanceOf(ReflectionPropertyAccess::class, $property);
+                self::assertEquals('depNew', $property->name());
+                $propertyAccesses = [...$reflector->navigate(
+                    TextDocumentBuilder::fromUri($this->workspace()->path('project/test.php'))->build()
+                )->propertyAccesses()];
+                $property = end($propertyAccesses);
+                self::assertInstanceOf(ReflectionPropertyAccess::class, $property);
+                self::assertEquals('depNew', $property->name());
+            }
+        ];
+
         yield 'property declaration public does not rename other members' => [
             'member_renamer/property_declaration_public_does_not_rename_others',
             function (Reflector $reflector, Renamer $renamer): Generator {
@@ -293,6 +369,23 @@ function (Reflector $reflector): void {
                 self::assertTrue($reflection->constants()->has('newName'));
             }
         ];
+        yield 'attributed constant declaration public' => [
+            'member_renamer/constant_declaration_public',
+            function (Reflector $reflector, Renamer $renamer): Generator {
+                $reflection = $reflector->reflectClass('ClassOne');
+                $constant = $reflection->constants()->get('ZOO');
+
+                return $renamer->rename(
+                    $reflection->sourceCode(),
+                    $constant->nameRange()->start(),
+                    'ZŁOM'
+                );
+            },
+            function (Reflector $reflector): void {
+                $reflection = $reflector->reflectClass('ClassOne');
+                self::assertTrue($reflection->constants()->has('ZŁOM'));
+            }
+        ];
     }
 
     /**
@@ -317,6 +410,24 @@ function (Reflector $reflector): void {
                 self::assertTrue($reflection->cases()->has('newName'));
             }
         ];
+
+        yield 'enum attributed case declaration' => [
+            'member_renamer/enum_case_declaration_private',
+            function (Reflector $reflector, Renamer $renamer): Generator {
+                $reflection = $reflector->reflectEnum('ClassOne');
+                $enum = $reflection->cases()->get('BAZ');
+
+                return $renamer->rename(
+                    $reflection->sourceCode(),
+                    $enum->nameRange()->start(),
+                    'newName'
+                );
+            },
+            function (Reflector $reflector): void {
+                $reflection = $reflector->reflectEnum('ClassOne');
+                self::assertTrue($reflection->cases()->has('newName'));
+            }
+        ];
     }
 
     /**

diff --git a/lib/WorseReflection/Bridge/TolerantParser/Reflection/AbstractReflectionClassMember.php b/lib/WorseReflection/Bridge/TolerantParser/Reflection/AbstractReflectionClassMember.php
@@ -2,6 +2,9 @@
 
 namespace Phpactor\WorseReflection\Bridge\TolerantParser\Reflection;
 
+use Microsoft\PhpParser\Node\AttributeGroup;
+use Microsoft\PhpParser\Token;
+use Phpactor\TextDocument\ByteOffsetRange;
 use Phpactor\WorseReflection\Core\ClassName;
 use Phpactor\WorseReflection\Core\Deprecation;
 use Phpactor\WorseReflection\Core\Reflection\ReflectionMember;
@@ -82,5 +85,59 @@ public function deprecation(): Deprecation
         return $this->docblock()->deprecation();
     }
 
+    public function position(): ByteOffsetRange
+    {
+        if (null === $this->node()->getFirstChildNode(AttributeGroup::class)) {
+            return parent::position();
+        }
+
+        $tokenKind = match ($this->memberType()) {
+            ReflectionMember::TYPE_PROPERTY => TokenKind::VariableName,
+            ReflectionMember::TYPE_METHOD => TokenKind::FunctionKeyword,
+            ReflectionMember::TYPE_CONSTANT => TokenKind::ConstKeyword,
+            ReflectionMember::TYPE_CASE => TokenKind::CaseKeyword,
+        };
+
+        $name = $this->findDescendantNamedToken($tokenKind);
+
+        if (null === $name) {
+            return parent::position();
+        }
+
+        return ByteOffsetRange::fromInts(
+            $name->getStartPosition(),
+            $this->node()->getEndPosition()
+        );
+    }
+
     abstract protected function serviceLocator(): ServiceLocator;
+
+    private function findDescendantNamedToken(int $tokenBeforeKind): ?Token
+    {
+        $found = false;
+
+        foreach ($this->node()->getDescendantTokens() as $token) {
+            if (true === $found) {
+                if ($tokenBeforeKind !== TokenKind::ConstKeyword) {
+                    return $token->kind === TokenKind::Name ? $token : null;
+                }
+
+                if ($token->kind === TokenKind::Name && $token->getText($this->node()->getFileContents()) === $this->name()) {
+                    return $token;
+                }
+            }
+
+            if ($token->kind !== $tokenBeforeKind) {
+                continue;
+            }
+
+            if ($tokenBeforeKind === TokenKind::VariableName) {
+                return $token;
+            }
+
+            $found = true;
+        }
+
+        return null;
+    }
 }