Rename LSP support #21

BladeMF · 2021-01-08T18:52:56Z

I am creating this PR so we can discuss the code. It's not going to get any better if I wait longer :-)

The code works by using Find all references to find everything that can be renamed.

Rename type. If type has the same name as the file, rename the file.
Rename local variable.
Rename method.

dantleech

Great that you've achieved what you have so far.

In general I think this is looking good, I want to suggest a different architetcture though:

Model\Rename.php // main class
Model\Renamer.php // interface
Model\Renamer\VariableRenamer.php
Model\Renamer\ClassLikeRenamer.php
Model\Renamer\MethodRenamer.php

This could be implemented by the Rename service having a collection of Rename instances, resolving the appropriate renamer for the node:

$this->resolveRenamer($node)->rename(...);

private function resolveRenamer(Node $node): Renamer
{
    foreach ($this->renamers as $renamer) {
        if (!$renamer->supports($node)) {
            continue;
        }
        return $renamer;
    }

    throw new CouldNotFindRenamer(sprintf('Could not find renamer for node "%s"', get_class($node));
}

We can start with only implementing VariableRenamer: this will allow us to merge and build a solid base for implementing the remaining functionality - some of which is not trivial (e.g. class renaming aka. class moving)

some other comments:

The public API for the Rename service should accept the Phpactor value objects (Phpactor\...\TextDocument etc) ...
... as the Renamer class may be split into it's own "package" or otherwise extracted so that it can be used by the CLI and the legacy RPC implementation at a later date.

Some of the coding-standard is not what I'd expect - not sure if the CS fixer is working or not.

.gitignore

dantleech · 2021-01-09T10:45:16Z

composer.json

@@ -62,6 +62,8 @@
            "Phpactor\\Extension\\LanguageServer\\Tests\\": "tests/LanguageServer/",
            "Phpactor\\Extension\\LanguageServerCompletion\\Tests\\": "tests/LanguageServerCompletion/",
            "Phpactor\\Extension\\LanguageServerReferenceFinder\\Tests\\": "tests/LanguageServerReferenceFinder/",
+            "Phpactor\\Extension\\LanguageServerRename\\Tests\\": "tests/LanguageServerRename/",
+            "Phpactor\\Extension\\LanguageServerSelectionRange\\Tests\\": "tests/LanguageServerSelectionRange/",


added in error?

Not really, otherwise "Fix namespace..." didn't work properly.

I needed that and composer install (supposedly to update the autoloader) for it to work. The other namespace I added I think you've missed because tests have weird namespace (note the missing starting letter of Selection):

namespace Phpactor\Extension\LanguageServer\Tests\electionRange\Unit\Model;

dantleech · 2021-01-09T10:46:32Z

lib/LanguageServerHover/Handler/HoverHandler.php

@@ -176,7 +176,7 @@ private function renderMember(SymbolContext $symbolContext): string
    private function renderFunction(SymbolContext $symbolContext): string
    {
        $name = $symbolContext->symbol()->name();
-        $function = $this->reflector->reflectFunction($name);
+        $function = $this->reflector->reflectFunction((string)$symbolContext->name());


no need for a type cast. the WR API can accept either Name or string

lib/LanguageServerRename/LanguageServerRenameExtension.php

lib/LanguageServerRename/Model/Renamer.php

dantleech · 2021-01-09T11:12:17Z

lib/LanguageServerRename/Model/Renamer.php

+        $this->nodeUtils = $nodeUtils;
+    }
+
+    public function prepareRename(TextDocumentItem $textDocument, Position $position): ?Range


public APIs should accept Phpactor documents, in this case the text document and range from the Phpactor phpactor/text-document library. This would enable us to use this service outside of the LS (CLI / RPC)

Yep, for the next version.

lib/LanguageServerRename/Model/Renamer.php

dantleech · 2021-01-09T11:18:14Z

lib/LanguageServerRename/Model/Renamer.php

+            /** @var Location $location */
+            $node = $rootNode->getDescendantNodeAtPosition($location->offset()->toInt());
+
+            if(($r = $this->renameFileIfNeeded($node, $documentUri, $oldName, $newName)) !== null){


would be happy if we removed the file-rename functionality for now, this is a more complex topic I think. Let's concentrate on a smaller set of features.

What do you mean, 'more complex'? Specifically renaming when renaming the class itself is pretty straight-forward - either they have the same name or they don't (unless I am missing something). The other way around is the complicated bit, I think - when the class file is being moved..

I mean it will spiral out of control probably. If we focus on a single feature then we can be happy with it faster and merge it, then see about other features.

I am inclined to agree, although I'd be happy to hear the scenario you imagine.

Off the top of my head the class moving stuff:

Rename is moving.

Moving is moving a file - finding out where that file should live depending on the namespace (Phpactor already has a solution for this problem)

The current Phpactor implementation moves the file in GiT too (via. the source code filesystem)

Dealing with class / trait / blah / aliases is always fun.

The current RPC handler for move will also move any related test files.

There will always be more complexity that will take you by surprise.

For the member (constant,property,method) renaming etc. it may be simpler, but the logic still differs from local variables considerably (no need to worry about scope etc).

In fact, member renaming may even be the easiest thing, but there again there are subtle differences between the members (as with the $)

dantleech · 2021-01-09T11:18:53Z

lib/LanguageServerRename/Model/Renamer.php

+            }
+        }
+
+        return [ $edits, $rename ];


if we remove renmae file then this becomes returns only TextEdits

That is going to be reworked completely (we'd be returning RenameResults) so no need to clear that now, I think.

BladeMF · 2021-01-09T13:35:27Z

Thank you for your notes and ideas! I appreciate your time with this.

Just so I am sure I understand your idea correctly, let me try to explain how it currently works and then we can get to me understanding your idea.

How it currently works

Use the current DefinitionLocator to locate the definition using the current offset
Use the current ReferenceFinder to locate all the references at the current offset
Collect all the locations and group them by document
For each document, for each location:
4.1. Check if file needs renaming (are we renaming a type with the same name as the file?)
4.2. Get the precise range for each edit
Create a WorkspaceEdit object.

Your idea

While I agree that class moving should be somewhere in there, I have trouble seeing how we can use this architecture while using DefinitionLocator, ReferenceFinder because they currently implement the task of splitting between class, members variables and methods and variables.

Can you please elaborate more on how the above logic will go into three classes as it does not cite classes or members or variables with the exception of when deciding whether to rename the file. Do you want to reimplement the same reference finding logic in VariableRenamer, ClassRenamer and MethodRenamer or am I misunderstanding you?

-- Extract renamer as a separate service -- Remove Handler's dependency on Tolerant Parser -- Renamer refactoring

-- fix failing test

weeman1337 · 2021-04-18T09:47:29Z

This one seems to be replaced by #24 ? Could it then be closed?

dantleech · 2021-04-18T09:58:08Z

yes, this has been extracted and implemented in other PRs :)

dantleech reviewed Jan 9, 2021

View reviewed changes

BladeMF force-pushed the try-actions branch 2 times, most recently from 9260045 to 3bd622c Compare January 12, 2021 20:05

dantleech mentioned this pull request Jan 18, 2021

Language Server Refactoring Progress phpactor/phpactor#1184

Closed

Vladislav Kosev and others added 25 commits February 7, 2021 14:32

Fix WorkspaceIndex not reindexing when document changes.

6e83a5e

Fix coding standards.

23bb5b5

Initial rename extension

077112b

Rudimentary prepareRename check

8be011a

Renamer

91ee80e

Fix class constant name detection

e037c44

Variable rename

4726315

Reflect on fully qualified function name

db69877

Rename refactoring

6a9a7bc

Coding standards

1131887

Renamer tests

a199bd7

More tests

23c41c9

Renamer testing

b168628

Refactor rename

64341f4

Rename file for class/interfaces/traits renames

8aaf30c

--

7cceeb8

CS fix

c5411a0

-- fix rename property bug

b3a1cb9

Fix WorkspaceIndex not reindexing when document changes.

32fdbd1

Fix coding standards.

b32cb69

Initial rename extension

4f319a7

Rudimentary prepareRename check

52647d8

Renamer

1a6e5b4

Fix class constant name detection

48909c1

Variable rename

0d2cb18

BladeMF added 17 commits February 7, 2021 14:34

Rename refactoring

f395950

Coding standards

7d5a382

Renamer tests

5f0b519

More tests

363f55b

Renamer testing

a1d6d86

Refactor rename

baf0805

Rename file for class/interfaces/traits renames

024f694

Rename use clauses and aliases.

c02e38a

CS

6a35b0b

Comment

d440f1c

Fix CS

86a716b

-- Remove cast

e79425a

-- Extract renamer as a separate service -- Remove Handler's dependency on Tolerant Parser -- Renamer refactoring

-- fix CS

077916f

-- fix failing test

Fix renaming foreach key and value

4f619db

Fix CS

5d6c619

Fix renaming variables inside list deconstruction expressions

3d05edb

Fix CS

09d1a72

BladeMF force-pushed the try-actions branch from 52e7000 to 09d1a72 Compare February 7, 2021 12:35

--

d593188

dantleech closed this Apr 18, 2021

Rename LSP support #21

Rename LSP support #21

Uh oh!

Conversation

BladeMF commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dantleech left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech Jan 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BladeMF commented Jan 9, 2021

How it currently works

Your idea

Uh oh!

weeman1337 commented Apr 18, 2021

Uh oh!

dantleech commented Apr 18, 2021

Uh oh!

Uh oh!

BladeMF commented Jan 8, 2021 •

edited

Loading

dantleech left a comment •

edited

Loading

dantleech Jan 10, 2021 •

edited

Loading