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).

[nehashri]

I also like the idea of the language server letting VSCode know what text to be pre-filled in the rename box based on the position of when the rename was invoked. It would also help if we could accept or reject a rename request based on the position. Right now I am finding it hard to let VSCode know when and what to rename. Any estimate on when such feature will be implemented in VSCode?

[jrieken]

from @DanTup in https://github.com/Microsoft/vscode/issues/45384

There's a new RenameProvider API coming to allow us to resolve the initial value in the rename box. It'd be good if we could also provide a validation method, so the box can go red if the user types characters that are not valid here in our language (similar to the window.showInputBox).

VS and Roslyn are doing something similar as they resolve some conflicts automagically and/or present errors and warnings to users while typing. I wonder if we can do this all in one call or need a separate API method...

[DanTup]

@jrieken

I wonder if we can do this all in one call or need a separate API method...

Do you mean bundling it with the request to actually get the edits? I think the validation would be significantly cheaper than the actual refactor (which might span many files), so asking for full edits on every key press might be excessive?

[jrieken]

Do you mean bundling it with the request to actually get the edits

No, I mean to use the new function this issue will introduce. Today, it used to determine the range that is being renamed, so before but not while showing the input widget. However, we could make it such that it constantly checks. E.g the first invocation during a rename session will be empty and a provider can fill in the range to be renamed or provide an error, e.g. when renaming a keyword etc. Subsequent request will send the new name (as typed) and the provider can validate that

[DanTup]

Ah, so you mean to add a new argument to resolveInitialRenameValue to pass the current value back in? If so, can you pass the whole previous RenameInitialValue and not just the text, so we can skip re-calculating the range and just do the validation (for me the validation is likely client-side but for the range I need to go over to the server)?

[jrieken]

Yeah, something along those lines

[jrieken]

So, today's proposed api is the following

export class RenameContext {
  range: Range;
  newName?: string;
  constructor(range: Range, newName?: string);
}

resolveRenameContext?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameContext>;

and an idea would be to generalise that into some form of validation-function. Maybe like so

validateRenameContext?(document: TextDocument, context: RenameContext, token: CancellationToken): ProviderResult<RenameContext>;

and the flow would be the following (assuming the provider implements this method)

1. rename is being initiated
2. the editor provides a default context (using the word and word range at the position)
3. the provider is being called and has a chance to update and validate the context
4. as long as the context is valid and the user is typing we repeat step 3

Challenges are that the provider might need to know that it is being called for the first time so that it can resolve the range and/or block the range, e.g. users 'renames' a keyword. For that we could consider to start with an empty context. For subsequent invocations the newName should be validated, e.g. we call the provider repeatedly with the given range and the updated newName. Inserting a new, invalid name should be a different class of error than invoking rename in the wrong position because we expect users to recover from that.

@DanTup, @aeschli, @Krzysztof-Cieslak Thoughts?

[DanTup]

the editor provides a default context (using the word and word range at the position)

We need to ensure that we can always control the first thing populated in the box and it's not guessed by Code, as in some cases what Code would pick is incorrect. For ex:

import "mything.dart" as a;

In the dart language server, invoking rename on the whole line will rename a so we'd want it to be pre-populated with a that even if the user pressed F2 while the cursor was in the filename.

In the validation, we also need to be able to report a string back to the user about why it's invalid. For example it might be because they've typed invalid characters, but it might also be that the name they're typed is already in use.

It's not clear from the API you posted how validation errors would be reported?

[Krzysztof-Cieslak]

We need to ensure that we can always control the first thing populated in the box and it's not guessed by Code, as in some cases what Code would pick is incorrect

I do :+1: this requirement - providing some initial context is the main use case for us at the moment (and it's reflected by current implementation, I guess).

I think some kind of generalization would be OKish but we need to know if it's initial call of the function or some next call. Maybe context could be RenameContext | undefined for example? (with undefined for first call). On the other hand I believe it would lead to something like:

if context == undefined then
   doSomething
else
  doSomethingTotallyDifferent

which kinda suggest that we try to put 2 unrelated things into one function.

What about keeping those 2 things separated? Keeping function to provide initial context as is right now and adding new API for validation (and I agree with @DanTup - if we add any validation API for renaming there must be some way to return error message and give feedback to user, not just silent failure)?

[DanTup]

If we get an undefined context on the first call, we wouldn't have the range which we need to figure out the initial text to populate with?

I do think keeping them separate might be easiest - a resolve that lets us provide the initial text and then a validate that lets us validate and return an error message. Trying to put them into one I think will make the API more confusing to implement (also, if separate they could individually be optional which might make sense?).

[jrieken]

In the dart language server, invoking rename on the whole line will rename a so we'd want it to be pre-populated with a that even if the user pressed F2 while the cursor was in the filename.

I'd argue that rename should only work on empty selections and not jump to next best fit - when resolving a range from a position we will definitely check that the original position is contained in the returned range.

Yeah, maybe we should keep resolving and validation separate for now despite the overlap in them. Another idea (VS with Roslyn does that) is to call provideRenameEdits while the user is typing and reuse the validation from there...

[DanTup]

I'd argue that rename should only work on empty selections and not jump to next best fit - when resolving a range from a position we will definitely check that the original position is contained in the returned range.

It's not jumping to the next best fit, but doing the thing that it believes is logical/most useful. For example if I hit F2 on the word class of class Danny it's highly likely that I want to rename the class and not try to change the keyword. What's the reason for wanting to block this? (it comes in really handy, for ex. you can F2 on an import that doesn't have an alias, and the rename gives it an alias, while updating all of the existing code that calls it to use it).

[jrieken]

What's the reason for wanting to block this?

Because it might lead to confusion, think of a provider that returns a range many line below/above the current position.

can F2 on an import that doesn't have an alias, and the rename gives it an alias, while updating all of the existing code that calls it to use it

Can you elaborate on that? Is this a code-action use case? What do I rename when there no alias yet?

[DanTup]

Because it might lead to confusion, think of a provider that returns a range many line below/above the current position.

But surely that can only mean one of two things:

1. The provider has better context and is providing a feature (as in our example)
2. The provider is buggy or implemented to do something confusing

I don't think you should be trying to work around 2 because if the provider is buggy they could give you a valid range but then return nonsense edits anyway - the provider has to be responsible for their own bugs and you shouldn't block functionality in an attempt to stop them (or if you do, at least gives us a flag to override it).

Can you elaborate on that? Is this a code-action use case? What do I rename when there no alias yet?

The difference between "renaming alias a to b" and "adding an alias as b` is fairly minor IMO. It would be a shame to have a different workflow (making a lightbulb always appear and then showing our own input box to get the name). Here's a more specific example:

import "dart:io" show a, b, c, d, e, f;

That list of things I'm showing is getting quite long, so I decide I just want to change this to:

import "dart:io" as io;

The dart analyzer allows me to hit rename on the string "dart:io" and tell it to rename to "io" and it would automatically add as io.

Forcing the returned range to span where the cursor is means we can't provide functionality like this. Yes, we could provide another command to do it, but it seems like an arbitrary restriction providing no real benefit.

Similarly, if this was supported and you hit rename in the string class Danny but with the cursor in class but the textbox appeared with Danny pre-filled, it would absolutely not be confusing to the user - they would know exactly what will happen when they change it and hit enter. It's a convenience with (IMO) no drawbacks.

[jrieken]

I understand the cla|ss Foo use-case and we can think about allowing to stay on the same line but jumping around isn't the right thing to do and also not the right thing to ask providers to implement - we provide the current position for a reason.

The scenario you describe is a prime sample for a code actions and you cannot expect us to optimise rename for that scenario. You can expect us to improve code actions, e.g. code actions that require input, code actions that are triggered via the Hint-severity etc.

[DanTup]

but jumping around isn't the right thing to do

I'm not asking to support "jumping around" - my cases are likely all on the same line too.

and also not the right thing to ask providers to implement

I'm not suggested providers are asked to do anything like this. I'm just asking that a provider can return a range that doesn't include position if they deem it a good feature.

The scenario you describe is a prime sample for a code actions and you cannot expect us to optimise rename for that scenario

I'm not asking you to optimise for it, I'm just asking that you don't block it with a rule that I don't think is adding any benefit. There are ways around it (we could just expand our range to include the whole line, since it's us doing the edits anyway), it'd just be nice if we didn't have to fight the API unless there's a good reason.

You can expect us to improve code actions, e.g. code actions that require input, code actions that are triggered via the Hint-severity etc.

I'm just trying to avoid working against my language service. It provides this (IMO, very nice) functionality that beyond what's expected with renames but I can't expose it to my users (and may have to do work to detect when I can't do it). I don't understand the advantage to disallowing it :(

[Krzysztof-Cieslak]

Because it might lead to confusion, think of a provider that returns a range many line below/above the current position.

How this is different than any other existing API that can do something "silly" or unexpected? I can write completion provider that inserts "HERE BE DRAGONS" string in random place in the file additionally to normal completion. I can write rename provider that do random changes instead of "correct" renaming. Every API that you expose can be "abused" and lead to confusing behavior - and I don't think it's a problem of editor or API, but particular extension.

Anyway, in case of F# extension, we need providing initial context to provide range that expands around range suggested by editor (https://github.com/Microsoft/vscode/issues/32762) so situation is bit different than the one @DanTup describe, also, as far as I remember for current implementation range in context is used just to get text that's put into the rename box - not to move rename box itself.

[jrieken]

I can write completion provider that inserts "HERE BE DRAGONS" string in random place in the file additionally to normal completion

Good example because we actually check that the main edit of a completion item happens on the same line. Sure, extensions can always do whatever they want but not always with API-blessings.

[Krzysztof-Cieslak]

Good example because we actually check that the main edit of a completion item happens on the same line

But you can provide additional edits that can do anything ;-)

Anyway, I do understand why you want to limit functionality. And I do agree that jumping rename box can lead to confusing behavior. However I think the case that @DanTup describes seems plausible and potentially useful/possible to implemented for different extensions. Maybe limiting this to same line would be good middle ground?

[DanTup]

And I do agree that jumping rename box can lead to confusing behavio

To be clear, I wasn't asking for the rename box to move at all. I just want to be able to rename something did not span the cursor position. If required, I'll just sent a fake edit for what's at the cursor position and then put the real rename into the other edits. It just feels like a really strange restriction (unless ofc, it was entirely because of moving the input box - which I don't want).

[jrieken]

Maybe limiting this to same line would be good middle ground?

Yeah, that's something I am OK with. The flow will be that the user hits F2, we make the resolve-request, and then open the widget. So we shouldn't be too far away from the cursor, e.g. no horizontal scrolling required.

[jrieken]

I have branched off the validation of a rename context, e.g new name, for now. This is what @aeschli and I have agreed on

    export interface RenameContext {
        range?: Range;
        newName?: string;
        message?: string;
    }

    export interface RenameProvider2 extends RenameProvider {

        /**
         * Optional function for resolving and validating a position at which rename is
         * being carried out.
         *
         * @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 A `RenameContext` with more information. The lack of a result can signaled by returning `undefined` or `null`.
         */
        resolveRenameLocation?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<RenameContext>;

    }

[DanTup]

I'm assuming that range will be used for placement of the input box and newName is the default text to put in it, but what's message used for?

[jrieken]

When message is set rename will be blocked, a sample would be "Cannot rename this element". The range property determines the box position and preselection which again newName overwrites

[jrieken]

There is a push to keep this more simple, so that a rejected-promise is replacing the message-property and then there a double of newName being useful, e.g. the rename-input-box should be populated with the value in range. That would reduce the proposal to something like this:

resolveRenameLocation?(document: TextDocument, position: Position, token: CancellationToken): ProviderResult<Range>;

[DanTup]

@jrieken I don't think that'll work for me with the current language server API - when I send a rename, the range I get back is for more than just the token being renamed (eg. in the case of import "myfile.dart" as alias; a rename on alias gets back the whole import directive).

If you want a simple value, being able to provide the original text is more important to me than the range (in the cases where the item to be renamed is not directly where the rename is invoked, it's always close enough - leaving the input box where it was invoked then renaming something a few characters down the line seems fine to me). I don't know if that's the same for others though.

[jrieken]

@DanTup I understand your scenario but what you are trying to achieve it's not really rename but a code action. As said, I am happy to help on that too. For instance the showInputBox-api can be enhanced to show an input box in the context of an editor.

So, back to rename. What would be the use-case for suggesting a name that's different than the selected text? Are there language services that suggest meaningful names, iff so which?

[DanTup]

I understand your scenario but what you are trying to achieve it's not really rename but a code action

I'm not sure if we're talking about the same thing. I'm talking about the following code:

import "myfile.dart" as myprefix;

I am invoking rename on myprefix. That seems like a rename action, not a code action? The problem is my language server returns the whole line as its range (along with the text myprefix). From this information, I can't reliably give you the range of the text.

What's the advantage to us giving a range and you looking the text up in the document versus us just giving you the text?

What would be the use-case for suggesting a name that's different than the selected text?

How about if I hit F2 on the word class from class Danny? Why shouldn't that perform a rename of the class? It's almost certainly what the user intended.

Are there language services that suggest meaningful names, iff so which?

I don't know; but I think it'd be confusing if they did and it was pre-filled. Imagine you have class named fred but your language server is of the opinion it should be named Fred.. it'd look a bit weird if you hit F2 and it was pre-filled with that (to the point where it might feel like a bug).

[nehashri]

@jrieken I quite agree with @DanTup. The language server should be able to provide the text to be populated in the rename box. We in Gauge too are looking at a rename provider that would allow us to provide the text to be populated in the input box.

Here is my use case. In Gauge we let users write their code in Markdown. While using markdown, some of these steps can span multiple lines. An example piece of code is as such.

# Specification
* A sample step with some params
    |col 1| col2|
    |one  | two |

Invoking rename on line * A sample step with some params should rename the lines 2, 3 and 4. We understand that showing a box spanning three lines can be quite confusing. So, we would like to show the signature of the code instead. In Gauge, the lines 2, 3 and 4 can be denoted by the text A sample step with some params <table>. Showing such a text in the input box would be more intuitive to the users, as the user can be sure as to what they are trying to rename.

[jrieken]

I am invoking rename on myprefix. That seems like a rename action, not a code action? The problem is my language server returns the whole line as its range (along with the text myprefix). From this information, I can't reliably give you the range of the text.

Yeah, that looks like rename. However, if the language server returns the whole statement and not the identifier to be renamed. why use it? It seems that the information is just not suitable and that you would be happy with the current default.

How about if I hit F2 on the word class from class Danny? Why shouldn't that perform a rename of the class? It's almost certainly what the user intended.

The proposed API can do that, but the range must be the identifier that's to be renamed not more.

it'd look a bit weird if you hit F2 and it was pre-filled with that (to the point where it might feel like a bug).

Yeah, we see it like that as well and therefore have concluded the simple api (map a position to an identifier-range) is enough.

[jrieken]

So, we would like to show the signature of the code instead. In Gauge, the lines 2, 3 and 4 can be denoted by the text A sample step with some params <table>. Showing such a text in the input box would be more intuitive to the users

@nehashri In your same what are the identifier that are to be renamed? The arguments one and two. What is the function name? Why do want to show a signature when renaming a symbol?

[DanTup]

However, if the language server returns the whole statement and not the identifier to be renamed. why use it? It seems that the information is just not suitable and that you would be happy with the current default.

Because it relies on Code's default always being correct and since it doesn't understand the semantics of the language it's likely to result in edge cases and bugs being raised. I already have to reject a bunch of renames and display "This rename is not yet supported" to my user when I detect bad renames happening (for ex. if the user hits rename on class of class Danny), though admittedly I don't know if there are any cases that I could strongly argue are valid (I think hitting rename on class should work, but I'm biased because my language server handles this :-))

How about if I hit F2 on the word class from class Danny? Why shouldn't that perform a rename of the class? It's almost certainly what the user intended.

The proposed API can do that, but the range must be the identifier that's to be renamed not more.

I can certainly request that my language server adds a property, but they also have a huge backlog and it's likely to be a long time before I was able to use it. Since you're adding a new API anyway, it seems that just letting my specify the text is relatively minor and would allow my users to get much better renames much quicker. I'm happy to do whatever work I need to in my extension, but it'll be a shame if users have to wait even longer - it's already been a very long time since this was first request and it's a bit weird that I have to explain these rename issues to my users :(

Other than a marginally simpler API, what is the advantage of not allowing us to provide the text? There seem to be many drawbacks. Providing the text seems far more useful than providing the range so if you only want one item in the API, why can't it be that?

[jrieken]

Other than a marginally simpler API, what is the advantage of not allowing us to provide the text?

The range does more than defining the text. It also positions the input widget, defines its width and the area in which editor selection changes don't cancel rename.

I understand your problem but I cannot tailor the API onto something that seems like a limitation on your side. Given class Danny and a rename on class, I'd expect that the returned range contains just Danny. However, if it contains more then more is being used for the aforementioned things (position, default text, auto-hide-area) which isn't nice but also not a blocker.

[DanTup]

I understand that; but it's not like it's a completely unreasonable or totally-specific-to-my-language-server change; it seems like a nice feature to let the language server provide the text and a relatively small tweak. Without it, Dart and Flutter users will continue to have a worse experience in VS Code than IntelliJ and Android Studio :-(

but also not a blocker

I don't understand the last line. It seems blocking if we can't pre-fill the box with something that will work? Putting class Danny into the box would not be a valid rename.

[nehashri]

@jrieken In Gauge each line/step in Markdown is mapped to a method signature. In the above example, the lines

* A sample step with some params
    |col 1| col2|
    |one  | two |

is similar to a function call with parameters. The signature of the function being A sample step with some params <table>. The parameter <table> gets the value

|col 1| col2|
|one  | two |

(essentially a table in Markdown) In this case rename would span more than one line. Hence, showing the signature would be more useful.

Also, let me give you another example where showing the signature is more useful to us. In Gauge, we can make a function call with parameters. ex:-

* Number of vowels in "apple" is "2".

In the above example is a function call to the function with signature Number of words in <word> is <numberOfVowels>.. There are two parameters in the function, <word> and <numberOfVowels>. In the function call, they are "apple" and "2" respectively. Unlike other programing languages, since we deal with natural text, there is no specific place where the parameters can appear. Hence, we have to show the whole line for rename. The user must be allowed to rename the function name, but not the parameters.

There are many more situations where showing the function signature has proven to be more intuitive the users. We would like to allow for a similar experience to the users in VSCode. So, we would like to show the function signature instead of the line.

[jrieken]

The user must be allowed to rename the function name, but not the parameters.

So, the name of this function is the whole signature? So, Number of words in <word> is <numberOfVowels>. and when hitting rename anywhere in that string you want populate the rename input box with that whole string?

[nehashri]

@jrieken Yes, I would like to populate the rename box with the whole string i.e., Number of words in <word> is <numberOfVowels>.

[jrieken]

Ok - that should work with the proposal. Your provider would then return the range of the statement and the box would be populated with the text in that range.

[nehashri]

@jrieken Sorry, if I misunderstood your statement yesterday. You mean that only the range will be provided. To be correct, my range would have the text Number of vowels in "apple" is "2". but I would like the input box to be populated with Number of words in <word> is <numberOfVowels>.. So, when you say that it would work with the proposal, what do you mean?