Skip to content

Commit a680258

Browse files
dantleechdantleech
authored
Optimise private property and method renaming (phpactor#2672)
Use static reflection to find private members to rename instead of using the indexer.
1 parent 1b89592 commit a680258

File tree

13 files changed

+289
-2
lines changed

13 files changed

+289
-2
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ Features:
99

1010
Improvements:
1111

12+
- Do not use indexer when renaming private properties/methods #2672 @dantleech
1213
- Fix contextual completion in constructor agrument position #2504
1314
- Basic support for `array_reduce` stub #2576
1415
- Support variadics in contextual completion #2603

‎lib/Extension/LanguageServerRename/LanguageServerRenameWorseExtension.php

+10
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Phpactor\Rename\Adapter\ReferenceFinder\ClassMover\ClassRenamer;
1616
use Phpactor\Rename\Adapter\ReferenceFinder\MemberRenamer;
1717
use Phpactor\Rename\Adapter\ReferenceFinder\VariableRenamer;
18+
use Phpactor\Rename\Adapter\WorseReflection\WorseReflectionMemberRenamer;
1819
use Phpactor\Rename\Model\FileRenamer;
1920
use Phpactor\Rename\Model\FileRenamer\LoggingFileRenamer;
2021
use Phpactor\Extension\LanguageServer\LanguageServerExtension;
@@ -29,6 +30,7 @@
2930
use Phpactor\ReferenceFinder\ReferenceFinder;
3031
use Phpactor\TextDocument\TextDocumentLocator;
3132
use Phpactor\WorseReferenceFinder\TolerantVariableReferenceFinder;
33+
use Phpactor\WorseReflection\Reflector;
3234

3335
class LanguageServerRenameWorseExtension implements Extension
3436
{
@@ -50,6 +52,14 @@ public function load(ContainerBuilder $container): void
5052
LanguageServerRenameExtension::TAG_RENAMER => []
5153
]);
5254

55+
$container->register(WorseReflectionMemberRenamer::class, function (Container $container) {
56+
return new WorseReflectionMemberRenamer(
57+
$container->expect(WorseReflectionExtension::SERVICE_REFLECTOR, Reflector::class),
58+
);
59+
}, [
60+
LanguageServerRenameExtension::TAG_RENAMER => []
61+
]);
62+
5363
$container->register(MemberRenamer::class, function (Container $container) {
5464
return new MemberRenamer(
5565
$container->get(DefinitionAndReferenceFinder::class),

‎lib/Rename/Adapter/WorseReflection/WorseReflectionMemberRenamer.php

+109
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
<?php
2+
3+
namespace Phpactor\Rename\Adapter\WorseReflection;
4+
5+
use Generator;
6+
use Phpactor\Rename\Model\LocatedTextEdit;
7+
use Phpactor\Rename\Model\Renamer;
8+
use Phpactor\TextDocument\ByteOffset;
9+
use Phpactor\TextDocument\ByteOffsetRange;
10+
use Phpactor\TextDocument\TextDocument;
11+
use Phpactor\TextDocument\TextEdit as PhpactorTextEdit;
12+
use Phpactor\WorseReflection\Core\Inference\Symbol;
13+
use Phpactor\WorseReflection\Core\Reflection\ReflectionMember;
14+
use Phpactor\WorseReflection\Core\Type\ClassLikeType;
15+
use Phpactor\WorseReflection\Reflector;
16+
17+
class WorseReflectionMemberRenamer implements Renamer
18+
{
19+
public function __construct(
20+
private Reflector $reflector,
21+
) {
22+
}
23+
24+
public function getRenameRange(TextDocument $textDocument, ByteOffset $offset): ?ByteOffsetRange
25+
{
26+
$member = $this->resolveMember($textDocument, $offset);
27+
28+
if (null === $member) {
29+
return null;
30+
}
31+
32+
return $member->nameRange();
33+
}
34+
35+
public function rename(TextDocument $textDocument, ByteOffset $offset, string $newName): Generator
36+
{
37+
$member = $this->resolveMember($textDocument, $offset);
38+
39+
if (null === $member) {
40+
return;
41+
}
42+
43+
$uri = $member->class()->sourceCode()->uri();
44+
45+
if (null === $uri) {
46+
return;
47+
}
48+
49+
$rangeStart = $member->nameRange()->start();
50+
yield new LocatedTextEdit(
51+
$uri,
52+
PhpactorTextEdit::create(
53+
$rangeStart,
54+
$member->nameRange()->length(),
55+
$newName,
56+
)
57+
);
58+
59+
$accesses = match ($member->memberType()) {
60+
ReflectionMember::TYPE_METHOD => $this->reflector->navigate($textDocument)->methodCalls(),
61+
ReflectionMember::TYPE_PROPERTY => $this->reflector->navigate($textDocument)->propertyAccesses(),
62+
default => [],
63+
};
64+
65+
foreach ($accesses as $access) {
66+
if ($access->name() !== $member->name()) {
67+
continue;
68+
}
69+
yield new LocatedTextEdit(
70+
$uri,
71+
PhpactorTextEdit::create($access->nameRange()->start(), $access->nameRange()->length(), $newName)
72+
);
73+
}
74+
}
75+
76+
private function resolveMember(TextDocument $textDocument, ByteOffset $offset): ?ReflectionMember
77+
{
78+
$context = $this->reflector->reflectOffset($textDocument, $offset)->nodeContext();
79+
80+
$symbolType = $context->symbol()->symbolType();
81+
if (!in_array($symbolType, [
82+
Symbol::METHOD,
83+
Symbol::PROPERTY,
84+
Symbol::VARIABLE, // promoted properties 🙃
85+
])) {
86+
return null;
87+
}
88+
89+
$containerType = $context->containerType();
90+
91+
if (!$containerType instanceof ClassLikeType) {
92+
return null;
93+
}
94+
95+
$class = $this->reflector->reflectClassLike($containerType->name());
96+
97+
$memberType = $symbolType === Symbol::VARIABLE ? 'property' : $symbolType;
98+
$members = $class->members()->byMemberType($memberType)->byName($context->symbol()->name());
99+
100+
foreach ($members as $member) {
101+
if (!$member->visibility()->isPrivate()) {
102+
return null;
103+
}
104+
return $member;
105+
}
106+
107+
return null;
108+
}
109+
}

‎lib/Rename/Tests/Adapter/WorseReflection/WorseReflectionMemberRenamerTest.php

+85
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
<?php
2+
3+
namespace Phpactor\Rename\Tests\Adapter\WorseReflection;
4+
5+
use Generator;
6+
use Phpactor\Rename\Adapter\WorseReflection\WorseReflectionMemberRenamer;
7+
use Phpactor\Rename\Model\Renamer;
8+
use Phpactor\Rename\Tests\RenamerTestCase;
9+
use Phpactor\WorseReflection\Reflector;
10+
11+
class WorseReflectionMemberRenamerTest extends RenamerTestCase
12+
{
13+
/**
14+
* @return Generator<string,array<mixed>>
15+
*/
16+
public function provideRename(): Generator
17+
{
18+
yield from $this->renames();
19+
}
20+
21+
protected function createRenamer(): Renamer
22+
{
23+
return new WorseReflectionMemberRenamer(
24+
$this->reflector,
25+
);
26+
}
27+
28+
/**
29+
* @return Generator<string,array<mixed>>
30+
*/
31+
private function renames(): Generator
32+
{
33+
yield 'private method declaration' => [
34+
'member_renamer/method_declaration_private',
35+
function (Reflector $reflector, Renamer $renamer): Generator {
36+
$reflection = $reflector->reflectClass('Foo\ClassOne');
37+
$method = $reflection->methods()->get('foobar');
38+
39+
return $renamer->rename(
40+
$reflection->sourceCode(),
41+
$method->nameRange()->start(),
42+
'newName'
43+
);
44+
},
45+
function (Reflector $reflector): void {
46+
$reflection = $reflector->reflectClass('Foo\ClassOne');
47+
self::assertTrue($reflection->methods()->has('newName'));
48+
}
49+
];
50+
yield 'private property declaration' => [
51+
'member_renamer/property_declaration_private',
52+
function (Reflector $reflector, Renamer $renamer): Generator {
53+
$reflection = $reflector->reflectClass('ClassOne');
54+
$method = $reflection->properties()->get('foobar');
55+
56+
return $renamer->rename(
57+
$reflection->sourceCode(),
58+
$method->nameRange()->start(),
59+
'newName'
60+
);
61+
},
62+
function (Reflector $reflector): void {
63+
$reflection = $reflector->reflectClass('ClassOne');
64+
self::assertTrue($reflection->properties()->has('newName'));
65+
}
66+
];
67+
yield 'private promoted property declaration' => [
68+
'member_renamer/property_promoted_private',
69+
function (Reflector $reflector, Renamer $renamer): Generator {
70+
$reflection = $reflector->reflectClass('ClassOne');
71+
$method = $reflection->properties()->get('foobar');
72+
73+
return $renamer->rename(
74+
$reflection->sourceCode(),
75+
$method->nameRange()->start(),
76+
'newName'
77+
);
78+
},
79+
function (Reflector $reflector): void {
80+
$reflection = $reflector->reflectClass('ClassOne');
81+
self::assertTrue($reflection->properties()->has('newName'));
82+
}
83+
];
84+
}
85+
}

‎lib/Rename/Tests/Cases/member_renamer/constant_declaration_private/ClassTwo.ph

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<?php
2+
3+
class ClassTwo
4+
{
5+
public function hello(): string
6+
{
7+
return (new ClassOne())->foobar();
8+
}
9+
}

‎lib/Rename/Tests/Cases/member_renamer/method_declaration_private/ClassOne.ph

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<?php
2+
3+
namespace Foo;
4+
5+
class ClassOne
6+
{
7+
private function foobar(): string
8+
{
9+
return 'foobar';
10+
}
11+
12+
public function hello(): string
13+
{
14+
return $this->foobar();
15+
}
16+
}

‎lib/Rename/Tests/Cases/member_renamer/method_declaration_private/test.ph

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
3+
require __DIR__ . '/ClassOne.php';
4+
5+
$one = new Foo\ClassOne();
6+
if (!$one->hello() === 'foobar') {
7+
echo 'expected "foobar" but didn\'t get it';
8+
exit(127);
9+
}
10+

‎lib/Rename/Tests/Cases/member_renamer/property_promoted_private/ClassOne.ph

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
class ClassOne
4+
{
5+
private string $barfoo;
6+
7+
public function __construct(
8+
private string $foobar,
9+
) {
10+
$this->barfoo = 'barfoo';
11+
}
12+
13+
public function bar(): string
14+
{
15+
return $this->foobar;
16+
}
17+
18+
public function foo(): string
19+
{
20+
return $this->barfoo;
21+
}
22+
}

‎lib/Rename/Tests/Cases/member_renamer/property_promoted_private/test.ph

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?php
2+
3+
require __DIR__ . '/ClassOne.php';
4+
5+
$two = new ClassOne('foobar', 'foobar');
6+
7+
// ensure that the method call will still work
8+
// after the renaming
9+
if (!$two->bar() === 'foobar') {
10+
echo 'expected "foobar" but didn\'t get it';
11+
exit(127);
12+
}
13+
14+
// this method call ensure that we did not replace
15+
// all property names
16+
if (!$two->foo() === 'barfoo') {
17+
echo 'expected "foobar" but didn\'t get it';
18+
exit(127);
19+
}

‎lib/Rename/Tests/RenamerTestCase.php

+1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ abstract class RenamerTestCase extends TestCase
2929
protected function setUp(): void
3030
{
3131
$this->workspace()->reset();
32+
$this->workspace()->mkdir('project');
3233
$this->reflector = ReflectorBuilder::create()
3334
->addLocator(new BruteForceSourceLocator(ReflectorBuilder::create()->build(), $this->workspace()->path('project')))
3435
->build();

‎lib/TextDocument/ByteOffsetRange.php

+5
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,11 @@ public function start(): ByteOffset
3131
return $this->start;
3232
}
3333

34+
public function length(): int
35+
{
36+
return $this->end->toInt() - $this->start->toInt();
37+
}
38+
3439
public function end(): ByteOffset
3540
{
3641
return $this->end;

‎lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionPromotedProperty.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public function name(): string
6868
public function nameRange(): ByteOffsetRange
6969
{
7070
return ByteOffsetRange::fromInts(
71-
$this->parameter->variableName->getStartPosition(),
71+
$this->parameter->variableName->getStartPosition() + 1, // return the range after the `$`
7272
$this->parameter->variableName->getEndPosition(),
7373
);
7474
}

‎lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionProperty.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public function name(): string
6565
public function nameRange(): ByteOffsetRange
6666
{
6767
return ByteOffsetRange::fromInts(
68-
$this->variable->getStartPosition(),
68+
$this->variable->getStartPosition() + 1, // do not return the $
6969
$this->variable->getEndPosition(),
7070
);
7171
}

0 commit comments

Comments
 (0)