Renamer

[BladeMF]

V1 of the rewrite. Currently only VariableRenamer is implemented.

Various points

Some classes have 2 in the name.

This is temporary so the previous version of the rename keeps working as I am using it :-)

Workflow

Since in LSP a rename is always preceded by a prepareRename request which makes all the validation, I've decided to require this flow from the Renamer2 users. So renamers might collect data on prepare which they might use on rename. I expect that when move class is invoked, it will be on some other workspace event (rather than rename), so there should be used the two calls, one after the other Renamer2::prepareRename and Renamer2::rename.

VariableRenamer::locationsToTextEdits

This will probably be moved out of there but currently I can't figure out how to deal with the specifics of class renaming and class moving, i.e.:

• When renaming a class, for each document containing references the use statements need to be searched.
• When renaming a class, and a QualifiedName is found in a reference, only the last part of the name (containing the class name) must be renamed (including for the use statatements)
• When moving a class, this changes part of the namespace of the class, so whenever a QualifiedName is found some middle part of it is changed (as opposed to the last part in the case of class renames).

So let's get there and decide then.

NodeUtils2

This is changed now to return an array of names/tokens since it can be ambiguous which name are you seeking, e.g. the DefinitionLocator might return this location:

class MyClass
{
    <>public SomeType $someprop1, SomeType2 $someprop2;

There you might want the qualified names SomeType and SomeType2, and the names $someprop1 and $someprop2. The old version of NodeUtils required a the name compare to all the found names, which I decided was a dumb approach.

[BladeMF]

The classes that are included in the rewrite, are: interface Renamer2 class Rename class RenameLocationGroup class RenameLocationsProvider class NodeUtils2 class VariableRenamer implements Renamer2 class RenameResult and the corresponding tests.

[dantleech]

With regards to returning an array of tokens from NodeUtils - I would probably not have this class handling all the cases. For this PR it would probably be enough to have a Variable token resolver or similar, which can be generalised or not later on.

[BladeMF]

NodeUtils2 just returns all the possible names for a location. Returning an array leaves the logic with the caller - 1) get me all the names, 2) let's see which name I am interested in, 3) rename it. Some positions have just one possible name, e.g. $varia<>ble1 and some have more than one, e.g. function F(<>Type1 $arg1).

[dantleech]

NodeUtils2 just returns all the possible names for a location

yes - all the possible names. but we are only concerned with variables at the moment. i can understand the thinking, but I think that this class provides for 3 distinct operations: variable rename, member rename and class-like _moving_ (there is no rename, there is only move). each of which may present their own challenges and require different logic.

[BladeMF]

Rebased on master.

[BladeMF]

NodeUtils2 just returns all the possible names for a location

yes - all the possible names. but we are only concerned with variables at the moment. i can understand the thinking, but I think that this class provides for 3 distinct operations: variable rename, member rename and class-like _moving_ (there is no rename, there is only move). each of which may present their own challenges and require different logic.

I think I get your point and you may be right. Although I still disagree with combining class rename and namespace rename (which is move) as they are nothing like each other.

[dantleech]

Although I still disagree with combining class rename and namespace rename (which is move) as they are nothing like each other.

If you rename a project class (only the short-name), it is, in the common case, a file rename operation or the code no longer works.

I don't know how the client works with regards to rename, but at least in the current phpactor code you rename a class by changing the FQN or the file path. So I would imagine the API accepting an old FQN and a new FQN (as with the current API). This would also allow us to use the same code with the CLI and legacy RPC handlers.

In fact I wonder if we shouldn't simply update the class-mover library to be compatible with the index :thinking: it could then be used for the LS.

In anycase this is why I'd rather we think about this later.

[BladeMF]

NodeUtils2 just returns all the possible names for a location

yes - all the possible names. but we are only concerned with variables at the moment. i can understand the thinking, but I think that this class provides for 3 distinct operations: variable rename, member rename and class-like _moving_ (there is no rename, there is only move). each of which may present their own challenges and require different logic.

I think I get your point. Although I still disagree with combining class rename and namespace rename (which is move) as they are nothing like each other.

Although I still disagree with combining class rename and namespace rename (which is move) as they are nothing like each other.

If you rename a project class (only the short-name), it is, in the common case, a file rename operation or the code no longer works.

I don't know how the client works with regards to rename, but at least in the current phpactor code you rename a class by changing the FQN or the file path. So I would imagine the API accepting an old FQN and a new FQN (as with the current API). This would also allow us to use the same code with the CLI and legacy RPC handlers.

In fact I wonder if we shouldn't simply update the class-mover library to be compatible with the index 🤔 it could then be used for the LS.

In anycase this is why I'd rather we think about this later.

I agree they are both FQN changes. Currently you can rename a class by just standing on any of the references or the definition and press F2. It is the same experience as the other renames. I would suppose we can still calculate a new FQN and then call some common code somewhere. Hmm, lets see:

If you rename just the class, you:

• Find all the references + definition
• Change definition name
• Rename file if names match
• For each file with references:
  • Search use statements and rename the last part
  • Search references and rename if current name is not aliased. (Probably can skip this if class is found in use statements with an alias)
  • If a FQN is found in the references, rename only the last part.

If you rename class' namespace, you:

• Find all the references + definition
• Change the definition namespace
• Move file if old namespace and current calculated namespace are the same
• For each file with references:
  • Search use statements and change them as needed.
  • Search references for only FQNs and rename them.

What do you think?

[BladeMF]

MemberRenamer is also done. The code turned out to be identical to VariableRenamer except for getValidNode and getNameTokens.

[dantleech]

What do you think about this:

• RenameHandler: LSP Rename Handler
• Renamer: Interface for renaming
• TolerantRenamer: Implementation for the Renamer:

public function rename($document, $offset, $newName): LocationGroups
{
     $node = $this->parser->parse($document)->getDescendantnodeAtPosition($offset);
     $renamer = $this->renamers->renamerForNode($node);
     $locationGroups = $renamer->rename($node, $newName);
     return $locationGroups;
}

• TolerantRenameHandler: Interface for Tolerant rename handlers (public function supports($node): bool; public function rename(Node $node, $newName))
• TolerantHandler/VariableRenamer: The variable renamer.

This would remove the duplication from the renamers.

But if you decide to try this, please open a new PR. In general I think this looks really good, but it's very hard to review when we haven't decided on the essential architecture.

[BladeMF]

I thought we agreed on your proposal from last time. I don't mind they turned out to be similar. I actually quite like getting rid of the if/else in NodeUtils. I actually like the current architecture, I just changed it slightly as the prepareRename call needs to return a range. That is why there is no supports method.

Also the class renamer/mover will probably be nothing like these two. I am beginning to like your idea to do renamers that are usable for both RPC and LSP.

[weeman1337]

Hey @BladeMF @dantleech as I am desperately waiting for the rename support :D I am ready helping out here. Could you give me a summary of the open points. Following the conversations I didn't completely get it.

[dantleech]

@weeman1337 this is mostly now working in master - there is support for variables, class names and members. I am currently working on the ability to react on file movements (and so update class references etc)