Fix static private member renaming for phpactor#2672 (phpactor#2674)

dantleech · web-flow · commit 252939554ca5 · 2024-06-12T21:55:27.000+01:00
diff --git a/lib/Rename/Adapter/ReferenceFinder/VariableRenamer.php b/lib/Rename/Adapter/ReferenceFinder/VariableRenamer.php
@@ -4,6 +4,7 @@
 
 use Microsoft\PhpParser\Node;
 use Microsoft\PhpParser\Node\CatchClause;
+use Microsoft\PhpParser\Node\Expression\ScopedPropertyAccessExpression;
 use Microsoft\PhpParser\Node\Expression\Variable;
 use Microsoft\PhpParser\Node\Parameter;
 use Microsoft\PhpParser\Node\PropertyDeclaration;
@@ -14,13 +15,19 @@ class VariableRenamer extends AbstractReferenceRenamer
 {
     public function getRenameRangeForNode(Node $node): ?ByteOffsetRange
     {
+        // do not try and rename static property names
+        if ($node->parent instanceof ScopedPropertyAccessExpression) {
+            return null;
+        }
+
         if (
             $node instanceof Variable &&
             !$node->getFirstAncestor(PropertyDeclaration::class)
         ) {
             return $this->offsetRangeFromToken($node->name, true);
         }
 
+
         if ($node instanceof Parameter && $node->visibilityToken) {
             return null;
         }
diff --git a/lib/Rename/Tests/Adapter/WorseReflection/WorseReflectionMemberRenamerTest.php b/lib/Rename/Tests/Adapter/WorseReflection/WorseReflectionMemberRenamerTest.php
@@ -47,6 +47,23 @@ function (Reflector $reflector): void {
                 self::assertTrue($reflection->methods()->has('newName'));
             }
         ];
+        yield 'private static method declaration' => [
+            'member_renamer/method_declaration_static_private',
+            function (Reflector $reflector, Renamer $renamer): Generator {
+                $reflection = $reflector->reflectClass('ClassOne');
+                $method = $reflection->methods()->get('foobar');
+
+                return $renamer->rename(
+                    $reflection->sourceCode(),
+                    $method->nameRange()->start(),
+                    'newName'
+                );
+            },
+            function (Reflector $reflector): void {
+                $reflection = $reflector->reflectClass('ClassOne');
+                self::assertTrue($reflection->methods()->has('newName'));
+            }
+        ];
         yield 'private property declaration' => [
             'member_renamer/property_declaration_private',
             function (Reflector $reflector, Renamer $renamer): Generator {
@@ -64,6 +81,23 @@ function (Reflector $reflector): void {
                 self::assertTrue($reflection->properties()->has('newName'));
             }
         ];
+        yield 'private static property declaration' => [
+            'member_renamer/property_declaration_static_private',
+            function (Reflector $reflector, Renamer $renamer): Generator {
+                $reflection = $reflector->reflectClass('ClassOne');
+                $method = $reflection->properties()->get('foobar');
+
+                return $renamer->rename(
+                    $reflection->sourceCode(),
+                    $method->nameRange()->start(),
+                    'newName'
+                );
+            },
+            function (Reflector $reflector): void {
+                $reflection = $reflector->reflectClass('ClassOne');
+                self::assertTrue($reflection->properties()->has('newName'));
+            }
+        ];
         yield 'private promoted property declaration' => [
             'member_renamer/property_promoted_private',
             function (Reflector $reflector, Renamer $renamer): Generator {
diff --git a/lib/Rename/Tests/Cases/member_renamer/method_declaration_static_private/ClassOne.ph b/lib/Rename/Tests/Cases/member_renamer/method_declaration_static_private/ClassOne.ph
@@ -0,0 +1,14 @@
+<?php
+
+class ClassOne
+{
+
+    public function hello(): string
+    {
+        return self::foobar();
+    }
+    private static function foobar(): string
+    {
+        return 'foobar';
+    }
+}
diff --git a/lib/Rename/Tests/Cases/member_renamer/method_declaration_static_private/test.ph b/lib/Rename/Tests/Cases/member_renamer/method_declaration_static_private/test.ph
@@ -0,0 +1,10 @@
+<?php
+
+require __DIR__ . '/ClassOne.php';
+
+$one = new ClassOne();
+if (!$one->hello() === 'foobar') {
+    echo 'expected "foobar" but didn\'t get it';
+    exit(127);
+}
+
diff --git a/lib/Rename/Tests/Cases/member_renamer/property_declaration_static_private/ClassOne.ph b/lib/Rename/Tests/Cases/member_renamer/property_declaration_static_private/ClassOne.ph
@@ -0,0 +1,16 @@
+<?php
+
+class ClassOne
+{
+    private static string $foobar;
+
+    public function __construct(string $foobar)
+    {
+        self::$foobar = $foobar;
+    }
+
+    public function foobar(): string
+    {
+        return self::$foobar;
+    }
+}
diff --git a/lib/Rename/Tests/Cases/member_renamer/property_declaration_static_private/test.ph b/lib/Rename/Tests/Cases/member_renamer/property_declaration_static_private/test.ph
@@ -0,0 +1,9 @@
+<?php
+
+require __DIR__ . '/ClassOne.php';
+
+$two = new ClassOne('foobar');
+if (!$two->foobar() === 'foobar') {
+    echo 'expected "foobar" but didn\'t get it';
+    exit(127);
+}
diff --git a/lib/Rename/Tests/RenamerTestCase.php b/lib/Rename/Tests/RenamerTestCase.php
@@ -3,6 +3,7 @@
 namespace Phpactor\Rename\Tests;
 
 use Closure;
+use RuntimeException;
 use Generator;
 use PHPUnit\Framework\TestCase;
 use Phpactor\Indexer\Adapter\Worse\WorseRecordReferenceEnhancer;
@@ -50,6 +51,16 @@ protected function setUp(): void
     public function testRename(string $path, Closure $operation, Closure $assertion): void
     {
         $basePath = __DIR__ . '/Cases/' . $path;
+
+
+        if (!file_exists($basePath)) {
+            throw new RuntimeException(sprintf(
+                'Case path "%s" does not yet exist',
+                $basePath
+            ));
+        }
+
+
         foreach ((array)glob($basePath . '/**.ph') as $path) {
             $this->workspace()->put(
                 'project/' . ((string)substr((string)$path, strlen($basePath))) . 'p',
diff --git a/lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionNavigation.php b/lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionNavigation.php
@@ -18,17 +18,25 @@ public function __construct(private ServiceLocator $locator, private Node $node)
     }
 
     /**
-     * @return NavigatorElementCollection<ReflectionMethodCall>
+     * @return NavigatorElementCollection<AbstractReflectionMethodCall>
      */
     public function methodCalls(): NavigatorElementCollection
     {
         $calls = [];
         foreach ($this->node->getDescendantNodes() as $node) {
+            if ($node instanceof ScopedPropertyAccessExpression) {
+                if (!$node->parent instanceof CallExpression) {
+                    continue;
+                }
+                $calls[] = new ReflectionStaticMethodCall($this->locator, new Frame(), $node);
+                continue;
+            }
             if ($node instanceof MemberAccessExpression) {
                 if (!$node->parent instanceof CallExpression) {
                     continue;
                 }
                 $calls[] = new ReflectionMethodCall($this->locator, new Frame(), $node);
+                continue;
             }
         }
         return new NavigatorElementCollection($calls);
@@ -40,12 +48,16 @@ public function at(ByteOffset $offset): self
     }
 
     /**
-     * @return NavigatorElementCollection<ReflectionPropertyAccess>
+     * @return NavigatorElementCollection<ReflectionPropertyAccess|ReflectionStaticMemberAccess>
      */
     public function propertyAccesses(): NavigatorElementCollection
     {
         $elements = [];
         foreach ($this->node->getDescendantNodes() as $node) {
+            if ($node instanceof ScopedPropertyAccessExpression) {
+                $elements[] = new ReflectionStaticMemberAccess($this->locator, new Frame(), $node);
+                continue;
+            }
             if (!$node instanceof MemberAccessExpression) {
                 continue;
             }
@@ -55,6 +67,7 @@ public function propertyAccesses(): NavigatorElementCollection
 
             $elements[] = new ReflectionPropertyAccess($node);
         }
+
         return new NavigatorElementCollection($elements);
     }
 
diff --git a/lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionStaticMemberAccess.php b/lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionStaticMemberAccess.php
@@ -60,7 +60,7 @@ public function class(): ReflectionClassLike
 
     public function name(): string
     {
-        return NodeUtil::nameFromTokenOrNode($this->node, $this->node->memberName);
+        return ltrim(NodeUtil::nameFromTokenOrNode($this->node, $this->node->memberName), '$');
     }
 
     public function scope(): ReflectionScope
@@ -72,7 +72,7 @@ public function nameRange(): ByteOffsetRange
     {
         $memberName = $this->node->memberName;
         return ByteOffsetRange::fromInts(
-            $memberName->getStartPosition(),
+            $memberName->getStartPosition() + 1,
             $memberName->getEndPosition()
         );
     }
diff --git a/lib/WorseReflection/Core/NavigatorElementCollection.php b/lib/WorseReflection/Core/NavigatorElementCollection.php
@@ -9,7 +9,7 @@
 use Traversable;
 
 /**
- * @template T of object
+ * @template-covariant T of object
  * @implements IteratorAggregate<array-key,T>
  */
 class NavigatorElementCollection implements IteratorAggregate

Original file line number	Diff line number	Diff line change
`@@ -18,17 +18,25 @@ public function __construct(private ServiceLocator $locator, private Node $node)`
`18`	`18`	`}`
`19`	`19`
`20`	`20`	`/**`
`21`		`- * @return NavigatorElementCollection<ReflectionMethodCall>`
	`21`	`+ * @return NavigatorElementCollection<AbstractReflectionMethodCall>`
`22`	`22`	`*/`
`23`	`23`	`public function methodCalls(): NavigatorElementCollection`
`24`	`24`	`{`
`25`	`25`	`$calls = [];`
`26`	`26`	`foreach ($this->node->getDescendantNodes() as $node) {`
	`27`	`+ if ($node instanceof ScopedPropertyAccessExpression) {`
	`28`	`+ if (!$node->parent instanceof CallExpression) {`
	`29`	`+ continue;`
	`30`	`+ }`
	`31`	`+ $calls[] = new ReflectionStaticMethodCall($this->locator, new Frame(), $node);`
	`32`	`+ continue;`
	`33`	`+ }`
`27`	`34`	`if ($node instanceof MemberAccessExpression) {`
`28`	`35`	`if (!$node->parent instanceof CallExpression) {`
`29`	`36`	`continue;`
`30`	`37`	`}`
`31`	`38`	`$calls[] = new ReflectionMethodCall($this->locator, new Frame(), $node);`
	`39`	`+ continue;`
`32`	`40`	`}`
`33`	`41`	`}`
`34`	`42`	`return new NavigatorElementCollection($calls);`
`@@ -40,12 +48,16 @@ public function at(ByteOffset $offset): self`
`40`	`48`	`}`
`41`	`49`
`42`	`50`	`/**`
`43`		`- * @return NavigatorElementCollection<ReflectionPropertyAccess>`
	`51`	`+ * @return NavigatorElementCollection<ReflectionPropertyAccess\|ReflectionStaticMemberAccess>`
`44`	`52`	`*/`
`45`	`53`	`public function propertyAccesses(): NavigatorElementCollection`
`46`	`54`	`{`
`47`	`55`	`$elements = [];`
`48`	`56`	`foreach ($this->node->getDescendantNodes() as $node) {`
	`57`	`+ if ($node instanceof ScopedPropertyAccessExpression) {`
	`58`	`+ $elements[] = new ReflectionStaticMemberAccess($this->locator, new Frame(), $node);`
	`59`	`+ continue;`
	`60`	`+ }`
`49`	`61`	`if (!$node instanceof MemberAccessExpression) {`
`50`	`62`	`continue;`
`51`	`63`	`}`
`@@ -55,6 +67,7 @@ public function propertyAccesses(): NavigatorElementCollection`
`55`	`67`
`56`	`68`	`$elements[] = new ReflectionPropertyAccess($node);`
`57`	`69`	`}`
	`70`	`+`
`58`	`71`	`return new NavigatorElementCollection($elements);`
`59`	`72`	`}`
`60`	`73`
Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ public function class(): ReflectionClassLike`
`60`	`60`
`61`	`61`	`public function name(): string`
`62`	`62`	`{`
`63`		`- return NodeUtil::nameFromTokenOrNode($this->node, $this->node->memberName);`
	`63`	`+ return ltrim(NodeUtil::nameFromTokenOrNode($this->node, $this->node->memberName), '$');`
`64`	`64`	`}`
`65`	`65`
`66`	`66`	`public function scope(): ReflectionScope`
`@@ -72,7 +72,7 @@ public function nameRange(): ByteOffsetRange`
`72`	`72`	`{`
`73`	`73`	`$memberName = $this->node->memberName;`
`74`	`74`	`return ByteOffsetRange::fromInts(`
`75`		`- $memberName->getStartPosition(),`
	`75`	`+ $memberName->getStartPosition() + 1,`
`76`	`76`	`$memberName->getEndPosition()`
`77`	`77`	`);`
`78`	`78`	`}`