Only do context sensitive import rewrite when resolving completion items

[jdneo]

This change fixes the performance downgrade introduced by #2232.

How to reproduce the performance downgrade?

1. Prepare a large java file, for example, the attached file in Make the debounce time of publish diagnostic job adaptive #2443.
2. Manually trigger the completion via keyboard shortcut (ctrl + space) after S
   
3. A very long-time lag can be observed. While if the completion is triggered directly by typing S, the time is normal.

Why same completion requests can result different performance?

The request is the same, but at the server side, state of SharedASTProviderCore is different.

• When the completion is triggered directly by typing S, the document is changed, SharedASTProviderCore is not ready to give a parsed AST tree.
• When it's triggered by ctrl + space, usually the SharedASTProviderCore is ready

This causes the argument passed to importRewrite.addImport() are different.

To verify this, we can set a breakpoint at
https://github.com/eclipse/eclipse.jdt.ls/blob/438e6192cc327960f82d3086bc5f374a5cfaeca7/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalReplacementProvider.java#L1039

Usually, if the textDocument/completion request is sent by typing S, the breakpoint won't be hit.

It raises another question: if the context is null when the completion is triggered by typing events, why the fix still works?

The answer is: The text edit will be corrected during completionItem/resolve request. When the resolving request comes, especially when the item is not the first one, SharedASTProviderCore has enough time to prepare the AST, so the context won't be null.

Same thing applies to triggering completion via ctrl + space. This non-null context causes much more calculation to get a more accurate completion. As a tradeoff, we lose the performance.

About LSP violation

Unfortunately, updating text edit during completionItem/resolve request violates the LSP spec.

All other properties (usually sortText, filterText, insertText and textEdit) must be provided in the textDocument/completion response and must not be changed during resolve.

A direct idea is to append additional text edit during resolve request to avoid such violation. Unfortunately, this is still unacceptable:

Additional text edits should be used to change text unrelated to the current cursor position (for example adding an import statement at the top of the file if the completion item will insert an unqualified type).

So far, I couldn't figure out a way to fix the perf downgrade as well as the spec violation. Current change fixes the perf issue but the spec violation is still there.

Considering that the case mentioned in #2232 is a rare case, and usually the resolve response should be arrived before user commits a completion item. I think it's acceptable.

Signed-off-by: Sheng Chen sheche@microsoft.com

[snjeza]

Unfortunately, updating text edit during completionItem/resolve request violates the LSP spec.
All other properties (usually sortText, filterText, insertText and textEdit) must be provided in the textDocument/completion response and must not be changed during resolve.

@jdneo You may want to take a look at

Since 3.16.0 the client can signal that it can resolve more properties lazily. This is done using the completionItem#resolveSupport client capability which lists all properties that can be filled in during a ‘completionItem/resolve’ request

[jdneo]

@snjeza Yes, I noticed that as well. And that really makes me a little bit confused.

My understand for that is all other properties can be updated during resolve except for sortText, filterText, insertText and textEdit.

[snjeza]

My understand for that is all other properties can be updated during resolve except for sortText, filterText, insertText and textEdit.

I think you can define these properties using completionItem#resolveSupport. Defaults are [ "documentation", "detail","additionalTextEdits"]

"capabilities": {
    ...
    "textDocument": {
        "completion": {
            ...
            "completionItem": {
                ...
                "resolveSupport": {
                    "properties": [
                        "documentation",
                        "detail",
                        "additionalTextEdits"
                    ]
                },
                ...
            }
            ...
        }
        ...
    }
    ...
}

Since 3.16.0 the client can signal that it can resolve more properties lazily. This is done using the completionItem#resolveSupport client capability which lists all properties that can be filled in during a ‘completionItem/resolve’ request. All other properties (usually sortText, filterText, insertText and textEdit) must be provided in the textDocument/completion response and must not be changed during resolve.

You should add textEdit to resolveSupport in vscode-java and check it in Java LS for other clients.

[Eskibear]

You should add textEdit to resolveSupport in vscode-java and check it in Java LS for other clients.

AFAIK completionItem#resolveSupport is client capability, and the defaults are defined in vscode-languageclient lib, which indicates vscode's capability to deal with completion items.

If I remember clearly, vscode doesn't guarentee you have correct behavior in some edge cases.
See #1864

But personally I'm ok with the "tradeoff" if it speeds up the completion a lot.

[rgrunber]

Change works well for me.

Feel free to merge once comments are addressed.

[jdneo]

One more update for the LSP spec.

See the discussion here: microsoft/language-server-protocol#1669.

This discussion shows that, the correctness of textedit for each completion item is not a hard requirement anymore. It can be updated during the resolve request. - (as long as the resolve request can be done before user commits the item.)

I feel that there is some chance here to make the completion perf great again! 🤣