Created attachment 143284 [details] wrong text inserted SUMMARY I am using Kate with LSP client addon to edit a Rust source file. When executing a code completion, it inserted unexpected additional text `(as XXXXXX)` into the editor. See the steps below for detail. STEPS TO REPRODUCE 1. Open a Rust source file with LSP client addon enabled and running. 2. Type the following code into the editor. ``` fn main() { let mut s = String::new(); } ``` 3. Type `s.clone` on the empty line and wait for the completion to appear. The first one is `clone (as Clone) [fn(&self) -> Self]` as the screenshot show, which is correct. 4. Press ENTER to confirm that choice. OBSERVED RESULT See the screenshot. The text ` (as Clone)()` get inserted. This is wrong behaviour because the `(as Clone)` is a hint, not actual code. It doesn't compile with that inserted. EXPECTED RESULT It should just be `s.clone()`, without the `(as Clone)` in it. SOFTWARE/OS VERSIONS Arch Linux Linux/KDE Plasma: (available in About System) KDE Plasma Version: 5.23.2 KDE Frameworks Version: 5.87.0 Qt Version: 5.15.2 Compiled kate LSP Client addon from git master branch commit 843b6103fbc7889cf292376b5319bead9d2e0e2d rust-analyzer 2021-11-01 ADDITIONAL INFORMATION
Hi, thanks for the bug report. I tried this with "rust-analyzer fd109fb58 2021-05-10 stable" and seems to work as expected, but the version I have is a bit old. Anyways, we execute completion based on what the LSP tells us. The completion data comes in as json which has a field "insertText" that is inserted once you press "Enter". If the data in that field is bad, you will get bad results (and we can't really do anything about it). Thus I suggest filing a bug with rust-analyzer about this.
(In reply to Waqar Ahmed from comment #1) > json which has a field "insertText" that is inserted once you press "Enter". Hello. I did some logging and research around it. It seems that the field "insertText" is missing in current rust-analyzer, and it's using "textEdit" instead. Here's a CompletionItem JSON object I receive from LSP in function parseDocumentCompletion: "{ "additionalTextEdits": [ ], "deprecated": false, "detail": "fn(&self) -> <Self as ToOwned>::Owned", "documentation": { "kind": "markdown", "value": "Creates owned data from borrowed data, usually by cloning.\n\n# Examples\n\nBasic usage:\n\n```rust\nlet s: &str = \"a\";\nlet ss: String = s.to_owned();\n\nlet v: &[i32] = &[1, 2];\nlet vv: Vec<i32> = v.to_owned();\n```" }, "filterText": "to_owned", "kind": 2, "label": "to_owned (as ToOwned)", "sortText": "ffffffff", "textEdit": { "newText": "to_owned", "range": { "end": { "character": 8, "line": 2 }, "start": { "character": 6, "line": 2 } } } } " I found it in the spec of LSP: https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#completionItem It says: * An edit which is applied to a document when selecting this completion. * When an edit is provided the value of `insertText` is ignored. So we need to implement the support for textEdit field of CompletionItem.
oops -- I copied the JSON for to_owned() instead of clone() in the above snippet -- but it shows the case anyway. The json structure is the same.
Hmm, indeed that would work. However... the sad part is that Kate in its current state.. can't really support that due to xxx reasons. See: https://invent.kde.org/utilities/kate/-/merge_requests/438 where I implemented that but had to revert later. Maybe we can ask them to provide insertText as well (for clients that aren't able to handle textEdits properly atm) or maybe we can work with `filterText` since it is giving the same info but not sure how that would play well with other servers :/ @Mark Nauwelaerts
(In reply to Waqar Ahmed from comment #4) > Hmm, indeed that would work. However... the sad part is that Kate in its > current state.. can't really support that due to xxx reasons. > > See: https://invent.kde.org/utilities/kate/-/merge_requests/438 where I > implemented that but had to revert later. Maybe we can ask them to provide > insertText as well (for clients that aren't able to handle textEdits > properly atm) or maybe we can work with `filterText` since it is giving the > same info but not sure how that would play well with other servers :/ > > @Mark Nauwelaerts I do find that both that MR and the MR in KTE https://invent.kde.org/frameworks/ktexteditor/-/merge_requests/167 got reverted. However I can't find the reason why they got reverted. Is it because it caused performance drop in KDevelop? If we are going to develop a workaround, I propose to use textEdit.newText as insertText, not to use filterText.
Yes, massive perf drop in kdevelop + an uglier completion behavior, the widget would reload on every typed character and it would cause a tiny blink. These are the major issues, there were some other tinier issues as well (which got lost in chat discussions I guess) Indeed. Didn't notice that, "newText" can be used reliably.
(In reply to Waqar Ahmed from comment #6) > the widget would reload on every typed character and it would cause a tiny > blink. Can't we just send a completion request only when the user has stopped typing for a period, say 2 seconds? So that the completion dropdown list won't be visible always and can reduce the amount of requests. I haven't read any of that code. Only my two cents. > Indeed. Didn't notice that, "newText" can be used reliably. Not THAT reliably though. We still can't process the case where some characters need to be deleted or so. But it does solve most common cases.
> Can't we just send a completion request only when the user has stopped typing for a period, say 2 seconds? Sure, but that means the completion widget will appear once you stop + 2 seconds. That I am guessing would be unexpected for most users. It would also be less helpful because you will have to consciously stop to be able to get completions
(In reply to Waqar Ahmed from comment #8) > It would also be less helpful because you will have to consciously stop to be able to > get completions Or we can show the completion only when pressing a key combination? Anyway, there's one thing I didn't get: Why we have to send completion request once per key if we want to support textEdit? Is there some technical or procedural limitation there I didn't notice?
IIRC, that is because the server needs to know how much the user has typed so that it can tell what parts of the word should be replaced by the textEdit. If the server just knows that the user typed "a", even though in reality there is "abc", the results will be incorrect.
(In reply to Waqar Ahmed from comment #10) > If the server just knows that the user typed "a", even though in reality > there is "abc", the results will be incorrect. But that also applies in the current (non-textEdit) scenario. If we don't send the up-to-date input "abc" to LSP server, how can we get completion for it and apply the insertText ?
We have our own mechanisms in place to handle completion execution. That's how it works with insertText. Looking at the code again, perhaps there's a way to make textEdit work, but I need to try it out.
Thanks. Wish you good luck. Meanwhile, I patched it locally to use textEdit.newText as insertText for a workaround.
Can you create a MR for that? It will do for now I think till we land proper support. Also, I wonder if we tell rust-analyzer in capabilities that we support insertText, maybe it will send that as well.
A possibly relevant merge request was started @ https://invent.kde.org/utilities/kate/-/merge_requests/513
(In reply to Waqar Ahmed from comment #14) > Can you create a MR for that? It will do for now I think till we land proper > support. Sure. > Also, I wonder if we tell rust-analyzer in capabilities that we support > insertText, maybe it will send that as well. Sorry, that's beyond my expertise. I'm a C++ coder who just started to learn rust yesterday...
That's okay. Though it's something we need to do in Kate (not rust analyzer).
Git commit 29585b58b65a7e47dc0797c0036311daef73e1e7 by oldherl oh. Committed on 07/11/2021 at 17:00. Pushed by waqar into branch 'master'. LSP Completion: use textEdit.newText as a workaround Some LSP server implementation, such as rust-analyzer 2021-11-01, do not use insertText field, but uses textEdit. Since we don't have real support for textEdit (see Commit 831a0069), the field textEdit.newText is used as a workaround here to prevent a worse fallback to use label as the inserted text. M +6 -0 addons/lspclient/lspclientserver.cpp https://invent.kde.org/utilities/kate/commit/29585b58b65a7e47dc0797c0036311daef73e1e7