Support 'textEdit.newText' in 'resolveSupport' of 'CompletionClientCapabilities'

[jdneo]

Proposal

Today, VS Code declares the resolveSupport of CompletionClientCapabilities as:

"resolveSupport": {
    "properties": [
        "documentation",
        "detail",
        "additionalTextEdits"
    ]
}

The textEdit is not included since the range of the textEdit is a key information to let the client do filtering correctly.

What if we limit the scope to textEdit.newText only? By doing this, the range is guaranteed to be available in the response of textDocument/completion. Only the content may have an update in completionItem/resolve.

Example

For example, when completing the constructor new String(byte[] bytes) in Java. If we support lazy-resolve the newText of TextEdit, we can first return

"textEdit": {
    "newText": "String()",
    "range": xxx
}

in textDocument/completion. And then give a more accurate content in completionItem/resolve:

"textEdit": {
    "newText": "String(${1:bytes})",
    "range": xxx
}

Even if the item is committed before the resolve result comes, the completion result is still acceptable.

I experimented and found that VS Code already supports update the newText of textEdit during resolve. If I change the content of it during completionItem/resolve, VS Code can apply the change correctly.

More background

The completion performance is always a key metric for a Language Server. Specific to Java Language Server, we found that the textEdit always takes a lot of time to calculate, mostly caused by the calculation of newText part (Because it requires to append parameter names. When it comes to constructor completion, the situation becomes like a disaster because the server needs to fetch source code from different types to get the parameter names of different constructors). This is how this idea came about.

[dbaeumer]

@jdneo actually this is not intended behavior in VS Code (I discussed this with @jrieken and he confirmed it). As stated in the vscode.d.ts changing a property like insertText is not something that is supported.

Due to that the LSP client is not able to support this either.

[jrieken]

@jdneo actually this is not intended behavior in VS Code

#183224 is up to enforce that behaviour

[jdneo]

Is there any reason to ban this capability? I know the behavior is not intended, but current VS Code supports it quite well.

[testforstephen]

Hi @jrieken I'm writing to express my concern about #183224 to disable textedit resolve in resolveCompletionItem. This will break the code completion feature for Java users, as Java extension relies on textedit lazy resolve for performance reasons (it can be 2X faster in some cases with delay resolving text edit). Code completion is a very important and widely used feature for VS Code Java users, and losing it will greatly affect the user experience and productivity. I understand that you have your reasons to disable textedit lazy resolve, but I hope you can reconsider this decision on #183224 or provide an alternative solution for us. Thank you for your understanding and collaboration.

[jrieken]

@jdneo @testforstephen Sorry, but I cannot follow. Resolving the primary text edit async cannot work reliably. Accepting a completion item in the UI (via the keyboard or mouse) is a synchronous operation and we synchronously apply the (primary) text edit. This never did (and technically cannot) wait for the completion item to be resolved - in fact it can be that we only call resolve after making the edit. This means fast typing/accepting of completions will not get the resolved item and its acceptance behaviour is more or less random (depends on the workload of the EH process and user interaction speed)

Note that I am only talking about that, e.g CompletionItem.insertText (in LSP lingo I believe it is textEdit) and not about additionalTextEdits. Please clarify and help me understand how this works today, my only guess is that today you get "saved" by this condition, e.g unresolved items insert their label, not the actual edit

[jdneo]

Please clarify and help me understand how this works today.

The Example section in the issue description shows how the response looks like. We return the textEdit in textDocument/completion, and update the newText of textEdit in completionItem/resolve.

This means fast typing/accepting of completions will not get the resolved item and its acceptance behaviour is more or less random

I agree that with a very fast typing speed, there will be a chance that the edit has not been resolved and get applied. To be honest, my own experience on that is, the possibility is very low. At least for myself, I haven't experienced that in my memory.

I think this is kind of tradeoff between the perf and accuracy. What about introducing a setting to control the behavior (allow/disallow resolving edit) and leave the choice to users?

I really hope VS Code team can reconsider it, if this capability is disabled, it will have a very huge impact on completion experience for Java language.

[testforstephen]

Share more background:

We will always return an insertText in the initial completion response. It's the textEdit property we want to lazy resolve in completion/resolve request. For example, for a “foreach” completion, we return a basic template for (${1:iterable_type} ${2:iterable_element} : ${3:iterable}) {\n\t$TM_SELECTED_TEXT${0}\n} as insertText for the initial completion response, but in the completion/resolve stage, we fill a textEdit with inferred iterable arguments, such as for (${1:String} ${2:string} : ${3:args}) {\n\t$TM_SELECTED_TEXT${0}\n}. The accurate textEdit calculation is more expensive and requires more context resolution. If we put it in the initial completion response, it would slow down the completion responsiveness significantly.

We understand that VS Code cannot guarantee that the completion/resolve request is executed before applying the completion item, but we think that’s an acceptable tradeoff between the perf and accuracy. Java extension has used lazy resolving textEdit in completion feature for years and we haven’t received negative feedback from users. Before we find a better alternative solution for completion performance (e.g. support incremental completion request), can we maintain the behavior as before for now?

@jrieken WDYT?

[jrieken]

Understood, I have pushed #183426 to give us some breathing room here. However, this will need to change.

To be honest, my own experience on that is, the possibility is very low. At least for myself, I haven't experienced that in my memory.

Sounds like "Works for me™️". I am your experience is going to be different when using a slower computer, having more extensions doing stuff in the EH process, working on codespaces with high latency etc pp. Doing things like this is an invitation for "randomness" and nothing great.

Before we find a better alternative solution for completion performance (e.g. support incremental completion request), can we maintain the behavior as before for now?

In a way the API already has these alternatives, e.g the CompletionItem#command property could be used for this but I would advise against it tbh. The better alternative is additionalTextEdits which already exists but is spec to not overlap with the primary edit. I am open to relaxing that restriction - it only exists to make the implementation simpler and I am open to suggestions/PRs (code pointer)

[testforstephen]

@jrieken thanks for revisiting the issue. additionalTextEdits looks like an interesting alternative we can look further. However, I still have some questions about the latest behavior. I have left a comment #183426 (comment) in the revert PR, where I want to confirm if the textEdit property added by completion/resolve request will be ignored. thanks.

[jdneo]

@jrieken Another restrictionof additionalTextEdit in current spec is that the insertTextFormat doesn't apply to it. Does VS Code open to relax that as well?

A usage example is that we can use additionalTextEdit to delay resolve the snippet completion like foreach:

for (${1:String} ${2:string} : ${3:args}) {
    $TM_SELECTED_TEXT${0}
}

[jrieken]

Does VS Code open to relax that as well?

Likely no, esp. not when the main edit is already a snippet and not when additional edits come late. Snippet insertion always requires user interaction, e.g the cursor jumps to placeholder etc pp. This technically cannot be done for two snippets at the same time and UX-wise this cannot be done at a "random" point in time

[jrieken]

With #184994 we are collecting telemetry about how often items are inserted before resolve is done. We will also log how often asynchronous additional edits can successfully be applied. Once we have the data we can make informed decisions.

[jrieken]

Some data over the last two weeks. This is always about inserting a completion item before has been resolved. This means:

• async computed command is ignored
• async changed insertText doesn't happen
• additional text edits are inserted on a best-effort basis

The columns mean the following

• Sessions (%) - A session corresponds to a VSCode window, so this is a good measure of how many users are affected by unresolved item
• AvgResolve (ms) - The average duration of resolve calls. This obviously highly depends on what resolve does, e.g some extensions load documentation via the internet which is always slow (but also not affecting the insert behaviour)
• Completions (%) - How many completions (overall) are inserted unresolved

The gist remains that unresolved item insertion (with above consequences) happens rarely but to very many users.

Extension	Sessions (%)	AvgResolve (ms)	Completions (%)
vscode.typescript-language-features	22.10%	48.08	3.96%
rust-lang.rust-analyzer	15.97%	9.35	1.59%
redhat.java	26.21%	45.05	4.56%
ms-vscode.powershell	24.69%	50.92	6.61%
ms-vscode.cpptools	16.21%	44.51	3.65%
ms-python.python	17.25%	28.76	3.17%
ms-dotnettools.csharp	23.72%	17.28	2.86%
Dart-Code.dart-code	27.90%	28.69	4.05%

fyi - @jdneo, @333fred, @DanTup

[DanTup]

Thanks for the numbers!

This might be slightly off-topic for this issue, because for Dart we currently only compare about supporting command in resolve (not changing newText) discussed in #184924 (which could be changed to a set of edits rather than a command - see #185772).. but I'm interested in understanding the Dart numbers above.

Completions (%) - How many completions (overall) are inserted unresolved

Am I right in thinking that VS Code doesn't know whether resolve is adding anything here (because we have no "requiresResolve" flag on a completion item)?

So if on average only 10% of completions actually change edits/commands during resolve and this number is 4%, it's actually only (on average(!)) 0.4% of completions that would be affected?

[jrieken]

Am I right in thinking that VS Code doesn't know whether resolve is adding anything here

Let's keep this off-topic but yes we only know that the provider wants to resolve (otherwise it wouldn't have that capability) but we do not know what it actually does, only the provider knows

[jdneo]

Hi @jrieken, some question about the data.

1. Is the column of Completions (%) counts over all of the completions or it's the percent happens in the left mentioned sessions?

2. By saying unresolved, does that mean the resolve request is sent out but the response is not received? Or it doesn't care whether the resolve request is sent, as long as the response is not received, it is unresolved?

[jrieken]

It's over all completions, so 4.56% of all java completions are inserted before resolving is done (response not yet received)

[jdneo]

May I ask what's VS Code's perspective about the data? What kind of bar is considered acceptable for the 'unresolved' rate?

[BoykoAlex]

@jrieken It seems to me that additionalEdits and the command don't wait for the main edit to be applied... Is this correct?
My use case is i need to add a suffix at the end of main edit. It is expensive to compute the suffix for many completion items therefore I'm trying to limit it only to a resolved completion. Once the suffix is applied I move the cursor to the position right after the suffix with a command. This works fine in Eclipse (LSP4E) but in VSCode suffix is placed before the main edit :-\ And I also see the cursor not moved to the end... so a bit of a mess... and seems due to things done in a random order.
Furthermore, it seems that if i click on the proposal in the list without moving the cursor over the completion item, the item isn't resolved and hence additionalEdit is not applied because it has never been computed via resolve completion item request.

[arunchndr]

ms-dotnettools.csharp | 23.72% | 17.28 | 2.86%

@jrieken is there a dashboard that we can use as a starting point to expand on the completion telemetry?