Rename provider should support to 'resolve' rename location (to expand, invalidate, etc)

[ghost]

• VSCode Version: 1.2.0
• OS Version: Windows 10

I’m having trouble with getting VS Code to do a rename operation correctly. When I put the caret on the word I want to rename and I hit F2, I see this:

However, if I do the same operation (F2) on a simple string, I get just the contents I expect:

[aeschli]

It look like rename is using the css word rule to determine the word at the location. This doesn't work well with structured content inside string on which this rename wants to work.
Maybe the rename support should be able to tell what range is modified?

[ghost]

It makes sense to me that if there is a selection, the rename operation should apply to the entire selection. If there isn't a selection, then the rename should apply to the word that the caret is currently inside of.
An even better option (in my opinion) would be to add a new provided named RenameRangeProvider that could be registered to tell VS Code what range to rename given the current selection.

[jrieken]

@daschult I what language do you see this? As in many other places we rely on the word definition each language can contribute. Under the hood rename is a positional operation and doesn't mind what the current word is - it's only to populate the UI

[ghost]

This happens in a JSON file.

[jrieken]

Closing as we only have the word definition so expand the a selection into something more meaningful. This doesn't affect how the rename feature works.

[aeschli]

@jrieken I think we should reconsider. Marking this 'as designed' means we don't support rename for embedded languages.
The initial word is important for the user to understand what is renamed. In this example showing the wrong word will completely confuse the user and the user will never use the refactoring. The word rule of the host language can not be used here. What if the rename provider can override the word rule? Maybe provide range for a offset. Normally there's just one rename provider for a position. Not sure if we support multiple rename providers for the same positions, I guess we would need to pick one (or what happens if there are two and both make changes to the same documents and even locations?)

[jrieken]

we don't support rename for embedded languages.

Why? The editor always uses the word rule of the corresponding mode, so I don't see why we would not support that? Actually, we did support it already with the 'old world'

[jrieken]

reopen since @aeschli is very passionate about this

[aeschli]

Also the old world would not support that scenario. The JSON mode can not (and should not) know what's the language inside the string literal. Maybe one day we will have a mechanism where we we can express embedded languages ranges.
But in this case, we don't need the full embedded language story. The current providers do to a great job of letting the Azure extension add all the supports they need. If they could also tell us what they intend to rename, then we can properly initialize the rename box.

[DanTup]

I'd also love to see some improvements here. I can't merge our rename support currently because of incompatibilities with how Code does rename and how our language server does; it results in very unexpected behaviour.

In Dart, let's say we have this:

import "dart:html";

If you put the cursor on import Code will give a popup with the word import filled in. Firstly, it's not valid for us to rename this but secondly, the language service accepts a rename here as the user wanting to add an alias; so if you append a 2, the code gets changed to:

import "dart:html" as import2;

There are a bunch of other places where our language service has additional functionality like this that makes the implementation against Code look super buggy and stops me from shipping it.

It'll be hard to convince the Dart team to remove this functionality (it is useful, adding an alias to something that doesn't yet have one, for example) but there's no good way for us to fix this without starting to parse the Dart ourselves and then just skip the rename if it's something that doesn't match the cursor exactly (though it'll still leave weird behaviour where the user is allowed to enter a new value over "import"!).

Some ideas:

1. Allow the provider to state whether the value under the cursor is valid for rename
2. Allow the provider to provide the text to be pre-populated for the rename

In the case of import "dart:html"; with cursor in import we can pre-populate with empty string, but in import "dart:html" as foo; also with cursor on import we could pre-populate with foo. These would need changes to the language service, but they feel like better changes all around.

Both of these could be rolled into the same call, as long as signalling "rename not valid here" and "pre-populate with an empty string" are not considered the same.

[DanTup]

May I also request that if this is implemented such that we can return the text to pre-fill in the box (or reject the request to rename) that it supports returning as a Promise? (I suspect you'd do it this way anyway, but I can only get this value asynchronously from the language service).

[jrieken]

Hm, I understand

[jrieken]

This is what we are shipping with as stable API (last chance for feedback):

	export interface RenameProvider {

		/**
		 * Provide an edit that describes changes that have to be made to one
		 * or many resources to rename a symbol to a different name.
		 *
		 * @param document The document in which the command was invoked.
		 * @param position The position at which the command was invoked.
		 * @param newName The new name of the symbol. If the given name is not valid, the provider must return a rejected promise.
		 * @param token A cancellation token.
		 * @return A workspace edit or a thenable that resolves to such. The lack of a result can be
		 * signaled by returning `undefined` or `null`.
		 */
		provideRenameEdits(document: TextDocument, position: Position, newName: string, token: CancellationToken): ProviderResult<WorkspaceEdit>;

		/**
		 * Optional function for resolving and validating a position *before* running rename. The result can
		 * be a range or a range and a placeholder text. The placeholder text should be the identifier of the symbol 
		 * which is being renamed - when omitted the text in the returned range is used.
		 *
		 * @param document The document in which rename will be invoked.
		 * @param position The position at which rename will be invoked.
		 * @param token A cancellation token.
		 * @return The range or range and placeholder text of the identifier that is to be renamed. The lack of a result can signaled by returning `undefined` or `null`.
		 */
		resolveRenameLocation?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<Range | { range: Range, placeholder: string }>;
	}

[DanTup]

This looks perfect to me - did I miss that this was already in insiders?

[nehashri]

The API's look good. Any idea when these requests will be supported through the language server protocol?

[jrieken]

@dbaeumer will know

[Krzysztof-Cieslak]

LGTM 👍

[dbaeumer]

I opened microsoft/language-server-protocol#455. As always a PR helps to speed this up.

[DanTup]

@jrieken Is there a way to call resolveRenameLocation from tests?

[DanTup]

@jrieken Also, in today's insiders I can't make this work - my resolveRenameLocation never seems to be invoked. The definition appears in vscode.d.ts and I'm targetted VS Code 1.23. Do I need to do anything special to make this work?

[jrieken]

oh! there actually is something wrong! great catch

[DanTup]

@jrieken Cool; it didn't seem like there wasn't much I could've done wrong. I did scan through the code, but obviously not that bit! I'll try again after next insiders build, cheers!

Btw, any comments on the testing? Currently I use vscode.executeDocumentRenameProvider for testing the rename, but can't see how I could test the resolve is working (I'd like to have tests to ensure I'm mapping to expected locations and getting expected placeholders).

[DanTup]

@jrieken I just updated Insiders now, and this still doesn't seem to hit my resolve method; can't figure out what I'm doing wrong? Hitting F2 just opened the box as it used to with the "closest word" pre-filled, and if I press enter calls my provideRename method. Breakpoint in resolveRenameLocation wasn't hit.

My packages.json references VS Code 1.23 and I've nuked both node_modules and out and done npm install and rebuilt. I can see resolveRenameLocation in the vscode.d.ts definition.

Any ideas?

[jrieken]

Yeah, we made a last-minute change and decided to call it prepareRename

[DanTup]

@jrieken Aha, awesome - I didn't check in vscode.d.ts after I re-fetched it! Works great now 👍! However, it doesn't seem like I can call it from tests? I've opened #48840 about it.