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?