Use a flag to control whether the textEdit of completion will be calculated in resolve request

[akurtakov]

I have looked at the test https://github.com/eclipse/eclipse.jdt.ls/blob/abdc0f87ab9313c1127fd7f263a26c722157424f/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandlerTest.java#L1024 and was surprised to see that the CompletionItem doesn't has TextEdit but just insertText . This effectively means that the completion will make it sysoutSystem.out.println(); which is not what's intended IIUC.

[akurtakov]

Discovered while looking at redhat-developer/eclipseide-jdtls#46 .

[fbricon]

Mmm sysout snippet (and others) work as expected in vscode. Is vscode more clever or ?

[akurtakov]

Either smarter or smth else jumps in meanwhile to give it more complete instructions. Can you please share the lsp messages for this completion? Bonus points for teaching me how to see lsp communication in vscode myself :) .

[fbricon]

In vscode, set "java.trace.server":"verbose", then open the Output view, select "Language Support for Java".

Tested with the latest pre-release.

[Trace - 11:46:29] Sending request 'textDocument/completion - (568)'.
Params: {
    "textDocument": {
        "uri": "file:///Users/fbricon/Dev/souk/quakuus/src/main/java/org/fred/vsc/test.java"
    },
    "position": {
        "line": 24,
        "character": 48
    },
    "context": {
        "triggerKind": 1
    }
}


[Trace - 11:46:29] Received notification 'window/logMessage'.
Params: {
    "type": 3,
    "message": "Apr 7, 2023, 11:46:29 AM >> document/completion"
}


[Info  - 11:46:29] Apr 7, 2023, 11:46:29 AM >> document/completion
[Trace - 11:46:29] Sending request 'workspace/executeCommand - (569)'.
Params: {
    "command": "microprofile/java/completion",
    "arguments": [
        {
            "uri": "file:///Users/fbricon/Dev/souk/quakuus/src/main/java/org/fred/vsc/test.java",
            "position": {
                "line": 24,
                "character": 48
            }
        }
    ]
}


[Trace - 11:46:29] Received notification 'window/logMessage'.
Params: {
    "type": 3,
    "message": "Apr 7, 2023, 11:46:29 AM Completion request completed"
}


[Info  - 11:46:29] Apr 7, 2023, 11:46:29 AM Completion request completed
[Trace - 11:46:29] Received response 'textDocument/completion - (568)' in 24ms.
Result: {
    "isIncomplete": false,
    "items": [
        {
            "label": "sysout",
            "kind": 15,
            "detail": "print to standard out",
            "insertText": "System.out.println(${0});",
            "insertTextFormat": 2,
            "textEditText": "System.out.println(${0});",
            "command": {
                "title": "",
                "command": "java.completion.onDidSelect",
                "arguments": [
                    "15",
                    "0"
                ]
            },
            "data": {
                "pid": "0",
                "rid": "15",
                "uri": "file:///Users/fbricon/Dev/souk/quakuus/src/main/java/org/fred/vsc/test.java",
                "COMPLETION_EXECUTION_TIME": "19"
            }
        }
    ],
    "itemDefaults": {}
}


[Trace - 11:46:29] Received notification 'window/logMessage'.
Params: {
    "type": 3,
    "message": "Apr 7, 2023, 11:46:29 AM >> workspace/executeCommand microprofile/java/completion"
}


[Info  - 11:46:29] Apr 7, 2023, 11:46:29 AM >> workspace/executeCommand microprofile/java/completion
[Trace - 11:46:29] Received response 'workspace/executeCommand - (569)' in 27ms.
Result: {
    "cursorContext": {
        "kind": 2000,
        "prefix": "sysout"
    }
}


[Trace - 11:46:29] Sending request 'completionItem/resolve - (570)'.
Params: {
    "label": "sysout",
    "detail": "print to standard out",
    "insertTextFormat": 2,
    "insertText": "System.out.println(${0});",
    "kind": 15,
    "command": {
        "title": "",
        "command": "java.completion.onDidSelect",
        "arguments": [
            "15",
            "0"
        ]
    },
    "data": {
        "pid": "0",
        "rid": "15",
        "uri": "file:///Users/fbricon/Dev/souk/quakuus/src/main/java/org/fred/vsc/test.java",
        "COMPLETION_EXECUTION_TIME": "19"
    }
}


[Trace - 11:46:29] Received notification 'window/logMessage'.
Params: {
    "type": 3,
    "message": "Apr 7, 2023, 11:46:29 AM >> document/resolveCompletionItem"
}


[Info  - 11:46:29] Apr 7, 2023, 11:46:29 AM >> document/resolveCompletionItem
[Trace - 11:46:29] Received response 'completionItem/resolve - (570)' in 12ms.
Result: {
    "label": "sysout",
    "kind": 15,
    "detail": "print to standard out",
    "documentation": {
        "kind": "markdown",
        "value": "```java\nSystem.out.println();\n```"
    },
    "insertText": "System.out.println(${0});",
    "insertTextFormat": 2,
    "textEdit": {
        "range": {
            "start": {
                "line": 24,
                "character": 48
            },
            "end": {
                "line": 24,
                "character": 48
            }
        },
        "newText": "System.out.println(${0});"
    },
    "command": {
        "title": "",
        "command": "java.completion.onDidSelect",
        "arguments": [
            "15",
            "0"
        ]
    }
}

So I see the TextEdit is now computed in the resolve call, which I thought was disallowed by the spec (cc @rgrunber @testforstephen ). I'm assuming vscode-java sends a special clientCapabilty to do that. But that means it should still fall back to computing the TextEdit during the completion call, if no capability is declared.

[akurtakov]

And this TextEdit has exactly 0 characters length:

 "textEdit": {
        "range": {
            "start": {
                "line": 24,
                "character": 48
            },
            "end": {
                "line": 24,
                "character": 48
            }
        },

This sounds wrong too.

[fbricon]

@puremourning @mfussenegger are you guys seeing this snippet completion issue too?

[mfussenegger]

I noticed a change after itemDefaults support got merged.

defaults.editRange is set, but there was no textEdit.newText or item.textEditText available for snippets.

I wasn't sure if that's okay by the spec so I ended up handling it in nvim-lsp-compl by falling back to insertText

I didn't try with the stock core lsp client functionality in Neovim which doesn't support itemDefaults, but I'm not aware of any bug reports so far in regards to this.

[mickaelistria]

And this TextEdit has exactly 0 characters length:

This is the particular reason cause of redhat-developer/eclipseide-jdtls#46 .

[puremourning]

A 0-length text edit range is an insert, right. Not strictly wrong in all cases.

@fbricon thanks for the heads up but we don't support snippets at all in YCM/ycmd (we're not barbarians 🙃 !). I'll try our regression tests against latest release just in case though. Probably next week.

[jdneo]

So, I see the TextEdit is now computed in the resolve call, which I thought was disallowed by the spec.

See: #1857. It was changed due to perf issue. To be honest it's violating spec (though for a long time till now, it's just working like a charm in VS Code).

[puremourning]

it's violating spec (though for a long time till now, it's just working like a charm in VS Code).

Sigh.

[rgrunber]

Yup 0-length is valid, and mentioned in the spec. See https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEdit .

As for the problem being observed when we added itemDefault to the completion list, there was a regression with snippets that should have been fixed shortly after with #2561 .

As for the snippets themselves, I had a quick look and it doesn't look like JDT-LS supports snippets without resolve. We seem to always set the data fields in anticipating of the resolve call. https://github.com/eclipse/eclipse.jdt.ls/blob/abdc0f87ab9313c1127fd7f263a26c722157424f/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/SnippetCompletionProposal.java#L322-L326 I could definitely see it being easier for clients to not have to implement resolve (so we should support that) but can't vouch for how well it may perform.

[jdneo]

I could definitely see it being easier for clients to not have to implement resolve (so we should support that) but can't vouch for how well it may perform.

@rgrunber What about introducing a flag to control whether the completion will be lazy resolved. (like the discussion before: #2535 (comment)) We can turn it on in VS Code. Other clients can have their own decision. Then everyone will be happy.

[puremourning]

Why bother? The spec accounts for this:

By default the request can only delay the computation of the detail and documentation properties. 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.

If vscode supports resolving textEdit the. It should announce that. If not, raise a bug against vscode?

[jdneo]

It's not a bug of VS Code.

VS Code as a client declares that it does not support update textEdit in the resolve phase. We still do that because of the performance consideration.

If the textEdit of all the completion items are calculated during textDocument/completion, the performance will be bad.

Personally, I don't want to change the behavior to get that perf penalty, because the current implementation works for a long time. That's why I propose a flag to control it.

[mfussenegger]

That's why I propose a flag to control it.

A flag would be good. Something working in VS code doesn't mean it will work in other editors, especially if you're already aware that it is violating the specification.

It's not a bug of VS Code.
VS Code as a client declares that it does not support update textEdit in the resolve phase.

If it works in vscode, why don't they add the resolveSupport capability for it? Then it would automatically work in other editors too which set the capability. No need for custom flags.

[puremourning]

If it works in vscode, why don't they add the resolveSupport capability for it? Then it would automatically work in other editors too which set the capability. No need for custom flags.

Yeah, exactly - that was my point about vscode being wrong in this case. If it "works", then it should declare it and no random out-of-band flags would be required

[jdneo]

If it works in vscode, why don't they add the resolveSupport capability for it?

Hmm. My personal understanding: There is no guarantee that it will always work. (Though it looks like in most cases it's working). It seems that there are some cases that lazily resolving TextEdit causes problems:

1. The content of textEdit is different from insertText and the user commits a completion item before the resolve response comes. (e.g. those server side snippets)
2. Clients may act differently on text filtering when the textEdit is available or not.

Completion item provides an insertText / label without a text edit: in the model the client should filter against what the user has already typed using the word boundary rules of the language (e.g. resolving the word under the cursor position). The reason for this mode is that it makes it extremely easy for a server to implement a basic completion list and get it filtered on the client.

Completion Item with text edits: in this mode the server tells the client that it actually knows what it is doing. If you create a completion item with a text edit at the current cursor position no word guessing takes place and no filtering should happen. This mode can be combined with a sort text and filter text to customize two things. If the text edit is a replace edit then the range denotes the word used for filtering. If the replace changes the text it most likely makes sense to specify a filter text to be used.

[mickaelistria]

Instead of a new flag, can't jdt.ls use the clientId to detect it's VSCode and implement the vscode-specific tweak; and in other cases stick to default behavior as defined by the spec?

[fbricon]

Instead of a new flag, can't jdt.ls use the clientId to detect it's VSCode and implement the vscode-specific tweak; and in other cases stick to default behavior as defined by the spec?

No. Feature flags provide the exact same thing and stay client agnostic.

[puremourning]

And there's already a feature flag for this in the spec :)

[jdneo]

Looks like we agree that using a flag to control the behavior. I can take a look at this in next week.