completion: calculating textEdit for constructor proposals is slow

[testforstephen]

Sample:

@SpringBootApplication(proxyBeanMethods = false)
public class PetClinicApplication {

	public static void main(String[] args) {
		SpringApplication.run(PetClinicApplication.class, args);
		new S|
	}
}

Since one type could have multiple constructors, the list of constructor proposals could be much larger than the list of type names. Calculating textEdit can be very slow for large result list.

[Eskibear]

I notice some change about textDocument/completion request in LSP 3.16.

LSP 3.15:

The request can only delay the computation of the detail and documentation properties. Other properties like sortText, filterText, insertText, textEdit and additionalTextEdits must be provided in the textDocument/completion response and must not be changed during resolve.

LSP 3.16:

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.

So I think for the moment it's not breaking the protocol if we:

• fill insertText in completion stage.
• fill textEdit (time consuming) in resolve stage.

The proposal is to move textEdit calculation from completion stage into resolve stage, as what we did long time ago. And in completion stage we provide simple insertText to prevent regression. The biggest advantage is, we would largely improve the performance via a systematic way, instead of tweaking it case by case. Meanwhile, following LSP we don't change any property of completion item during resolve.

/** * An edit which is applied to a document when selecting this completion. * When an edit is provided the value of insertText is ignored. .... textEdit?: TextEdit | InsertReplaceEdit;

Commonly after resolve, textEdit has a higher priority than insertText, so there should be no difference with current behavior. In the edge case where resolve response is not correctly received, it still have insertText to fallback to.

[Eskibear]

/* The insertText is subject to interpretation by the client side. * Some tools might not take the string literally. For example * VS Code when code complete is requested in this example * con<cursor position> and a completion item with an insertText of * console is provided it will only insert sole. Therefore it is * recommended to use textEdit instead since it avoids additional client * side interpretation. */ insertText?: string;

Because how to insert insertText is determined by client, completion related to full qualified name breaks when testing on vscode when I tried implementing above proposal. It affects completion related to qualified names.

E.g. taking import java.sq^ for example.

import java.sq^ 

select 'java.sql', now you get wrong result.

import java.java.sql.*

Root cause:

Previously when a textEdit (range matches "java.sq", newText = java.sql) is included in completion stage, vscode learns from textEdit.range it's going to replace "java.sq" with "java.sql", so everything works fine, and the result is "import java.sql".

With proposed changes above, when completion returns an item with insertText = java.sql but not textEdit, "sq" is deducted as the text to replace. Even through a textEdit (range matching "java.sq", newText = java.sql) is returned in resolve stage, vscode still replace "sq" with "java.sql", and the result wrongly becomes "import java.java.sql".

[fbricon]

@Eskibear you're making some convenient interpretations of the spec ;-) I'm not convinced this doesn't violate the spec, as this is what we were doing in the past and were asked to stop doing it. Can we get some feedback from @dbaeumer?

I'm all for lazily computing expensive stuff, but this might break other clients (vim, emacs...), or at least degrade the completion UX. @mfussenegger @yyoncho what say you?

[fbricon]

@Eskibear here are the completionItem#resolveSupport values vscode sends to jdt.ls during initialization:

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

So no textEdit there.

[Eskibear]

@fbricon Yes, you are right. The "complete insertText + resolve textEdit" approach happens to work in some cases, but the correctness is not guaranteed. I created the #1868 mainly to see how much we can improve and to restart this discussion of this issue.

So far since there're some clients (e.g. vscode) supporting resolve additionalTextEdit, anohter approach is to "complete textEdit (simple name only) + resolve additionalTextEdit", as described in https://github.com/microsoft/vscode/issues/96779#issuecomment-627954429 . E.g.

• For "new Str^", candidate is "String(byte[]: bytes)". we use "String" as textEdit, and (byte[]: bytes) as additionalTextEdit

But a blocking thing is, additionalTextEdit cannot overlap or even next to textEdit per LSP, and there might be some problems applying multiple textEdits to the same position.

/** * An optional array of additional text edits that are applied when * selecting this completion. Edits must not overlap (including the same * insert position) with the main edit nor with themselves. * * 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). */ additionalTextEdits?: TextEdit[];

[mfussenegger]

Stock neovim lsp doesn't have resolveSupport at all and the built-in completion uses textEdit if available, with fallback to insertText and label (in that order). So it wouldn't break, but would degrade the functionality.

There are some plugins which add resolveSupport, but as far as I know it is limited to documentation, detail and additionalTextEdits. None that I know of support lazy resolving of textEdit. And as far as I'm aware it would be non-trivial to support, given how the completion infrastructure of neovim is laid out.

[dbaeumer]

I think https://github.com/eclipse/eclipse.jdt.ls/issues/1864#issuecomment-915098967 answers it. It depends on the client which properties it allows and VS Code DOESN'T support resolving the text edit lazy.

[Eskibear]

I simply think if I commit the completion before textEdit is resolved, client can simply use insertText(or label) and ignore the later resolved textEdit...which is not a big problem to me. I believe that's a systematic way to solve performance issue. And it doesn't make sense to calculate textEdit for every candidate before I even select them.

it would be non-trivial to support, given how the completion infrastructure of neovim is laid out.

• Additional text edits should be used to change text unrelated to thecurrent cursor position

VS Code DOESN'T support resolving the text edit lazy

I'm not familiar with client implementation. BUT given the fact that NONE of LSP spec and clients (vscode and vim-plugins) supports lazy reoslve text edits, I believe there must be some technical difficulties. Is there any material describing the blocking cases for me to learn from?

[mfussenegger]

I'm not familiar with client implementation. BUT given the fact that NONE of LSP spec and clients (vscode and vim-plugins) supports lazy reoslve text edits, I believe there must be some technical difficulties. Is there any material describing the blocking cases for me to learn from?

Can only speak for neovim. The function/API to show completion candidates doesn't (easily) support asynchronously extending or changing completion candidates. Instead there is a function that takes a list of candidates and shows them. Currently it uses the textEdit or insertText or label information when it presents the choices.

But even if it had - or if the other APIs are used that give more flexibility, textEdit contains the following remark:

 * An edit which is applied to a document when selecting this completion.
	 * When an edit is provided the value of `insertText` is ignored.

I interpret that as "textEdit has higher priority than insertText". So assuming that textEdit could be lazy, it would either have to fetch all missing textEdit properties before showing the completion items. That would potentially end up increasing the latency and would also imply an overhead for cases where the language server is not going to provide a textEdit value anyways.

Alternatively it could first display the (potentially wrong) insertText values and in the background fetch the textEdit values and then replace update completion items with new values. But depending on how the values would change it could mess with the users who might have continued to type.

[dbaeumer]

In VS Code it is used to do property prefix matching since the text edit describes the replace region in the document when applying the text edit. Without that info filtering will not work correctly

[fbricon]

Reopening as I closed the wrong issue (although it's related to #1846)