Unintended whitespace removal

[rwols]

Bug:

• macOS 10.13.5, clangd language server using a C++ codebase.

The problem here is that a whitespace character is removed, while that should not happen. @nanoant, I remember seeing a pull request from you where you tried to change the insertion behavior for Powershell. Can this be an unintended side-effect?

[nanoant]

@rwols LSP plugin does not control how to completion gets inserted, this is completely up-to SublimeText. My change #295 was trimming the completion text according to expected insertion place, but not altering insertion point, so it cannot remove space that is already there. Beyond this point LSP plugin no longer control what happens. You can enable debugging to confirm that const is the completion that is generated, and there're no weird characters.
Also make sure that you have no other plugins that can interfere, e.g. listen to document change events are do re-format or something that can lead to removal of this space.

Btw. I have tried to reproduce the problem, but for me it works as intended, see:

[predragnikolic]

Maybe related to this, if not, I will delete this comment.

Using felixfbecker/php-language-server, I have a problem where the $ sign removes after completion.

Note After installing this php server, I noticed an error in the console when I needed to get completions. So I tried to fix it, below is the fix, and then I noticed the $ sign being removed after completion.

             # changed this in plugin/completion.py
-            items = sorted(items, key=lambda item: item.get("sortText", item["label"]))
+            items = sorted(items, key=lambda item: item.get("sortText") or item["label"])

After changing this code in plugin/completion.py, it completes as expected.

-        insert_text = self.text_edit_text(item) or item.get("insertText") or label
+        insert_text = item.get("insertText") or label

[nanoant]

@predragnikolic I have managed to reproduce the problem. I will try to work on the fix this weekend. Indeed there is some problem that $ get swallowed in some cases with PHP and my #295.

[predragnikolic]

Ok Adam, no hurries :-)

[nanoant]

@predragnikolic @rwols @tomv564 Sorry to say that, but it seems that Sublime autocompletion behavior is kind of erratic (at least) as I have described in my comment at sublimehq/sublime_text#1281. ST is replacing more than the prefix, which is rather unexpected - leading to swallowing $.
The single reason it was working before #295 is that https://github.com/felixfbecker/php-language-server is returning 'insertText': None hence before #295 LSP was always using a 'label': '$someParam' as a replacement text, Sublime was swallowing the prefix plus $ before it, leading to somewhat correct final result, see:

LSP: on_query_completions(prefix="", locations=[45])
LSP:  --> textDocument/completion
LSP: item: {'kind': 6, 'textEdit': {'newText': 'someParam', 'range': {'end': {'line': 3, 'character': 2}, 'start': {'line': 3, 'character': 2}}}, 'filterText': None, 'additionalTextEdits': None, 'label': '$someParam', 'detail': 'null', 'insertText': None, 'documentation': None, 'data': None, 'sortText': None, 'command': None}

Now #295 introduced a mitigation that textEdit that starts before the prefix, it trimmed to the prefix, however for non-empty prefix, e.g. so https://github.com/felixfbecker/php-language-server is sending textEdit insert AFTER the prefix with meParam, see:

LSP: on_query_completions(prefix="so", locations=[47])
LSP:  --> textDocument/completion
LSP: item: {'textEdit': {'newText': 'meParam', 'range': {'end': {'character': 4, 'line': 3}, 'start': {'character': 4, 'line': 3}}}, 'label': '$someParam', 'filterText': None, 'data': None, 'command': None, 'detail': 'null', 'insertText': None, 'additionalTextEdits': None, 'sortText': None, 'documentation': None, 'kind': 6}

This is IMHO correct server behavior, but this is unfortunately (or fortunately?!) not handled by my text_edit_text on plugin/completion.py, so $ gets swallowed by Sublime erratic inconsistent behavior just on empty prefix 🙄. On non-empty we still fallback to the label text.

Nevertheless, I believe for sake of correctness we should handle both cases in text_edit_text, so when edit_range.start.col <= last_col we trim the response to prefix start -- done, otherwise if edit_range.start.col <= last_col + len(last_prefix) we append (partially) the prefix -- new.

This all will work under the assumption that Sublime replaces the prefix, and nothing less or more, which is not true at the moment sublimehq/sublime_text#1281 🤣

[rwols]

So I finally figured out how this happens. clangd recently changed how it returns completion labels. There's this experimental feature where an #include <...> is added at the top of the file if an include is missing for a certain completion. The way this is indicated is as follows:

• If no #include <...> line will be added at the top of the file, a space is prepended to the label.
• If an #include <...> line will be added at the top of the file, a circular dot character is prepended to the label.

So, we end up with completions for ST like this

[" this\tImage *", "this"],
[" const", "const"]
etc

Now remember that, according to the ST API, ST sees the first element simply as the completion trigger (it is not a label). So when I type the word const (with some indentation, say), ST will think that I typed [space]const and replace that string with const.

I guess a deeper problem is that there is really no place to put the label of an LSP completion. We put it in the the completion trigger for ST but we then assume that the label is going to have the same prefix as the the completion trigger.

[rwols]

Made a workaround for clangd for this issue here: https://reviews.llvm.org/D49967, so from my point of view we can close this issue. Users with a fairly new clangd source should add the command-line option -header-insertion-decorators=0 in their user preferences. I should probably make a pull request to update the docs.

[tomv564]

Nice research. Specifically to clangds desire to decorate completion items with leading spaces/characters - the protocol suggests the label must be insertable:

interface CompletionItem {
	/**
	 * The label of this completion item. By default
	 * also the text that is inserted when selecting
	 * this completion.
	 */
	label: string;

https://microsoft.github.io/language-server-protocol/specification#textDocument_completion

[rwols]

the protocol suggests the label must be insertable

How do you infer this from the docs? If an insertText property is present then that should be inserted, not the label.

[tomv564]

Ah right, forgot that it's the server that calls the shots here :)

[tomv564]

Looking at #294 I see that both examples would be broken by the mismatching label field returned (true for $true and USERPROFILE for $env:USERPROFILE).

I think LSP should use the same value for trigger and content, where the simplest completions for these language servers would be:
PHP: Return [label, label]
Powershell: Return [insertText, insertText]

I'm not convinced by the use of textEdit, especially given the work needed to stick the $ back on someParam in PHP completions. (And VS Code and Sublime's behaviour with insertText mostly matches). Therefore, I will move it behind a flag for now.

Some additional observations on #294:

• I assume the usectrl+space was by lack of a trigger character set up for $ ? Recent versions of LSP will set up the $ trigger character automatically, in which case you would likely never get a completion for just the "USERPROFILE" portion.
• Completion results that span word boundaries like ($env:USERPROFILE) still fail to match their trigger

$| -> choose $env:USERPROFILE -> $$env:USERPROFILE
$env:U| choose $env:USERPROFILE -> $env:USERPROFILE

Other languages servers tend to only complete from word boundaries, but for powershell this could be addressed by removing : from "word_separators":

>>> sublime.active_window().active_view().settings().get("word_separators")
'./\\()"\'-:,.;<>~!@#$%^&*|+=[]{}`~?'
>>> sublime.active_window().active_view().settings().set("word_separators", './\\()"\'-,.;<>~!@#$%^&*|+=[]{}`~?')

[nanoant]

I'm not convinced by the use of textEdit, especially given the work needed to stick the $ back on someParam in PHP completions. (And VS Code and Sublime's behaviour with insertText mostly matches). Therefore, I will move it behind a flag for now.

No pbm about making textEdit support behind a flag, at least until Sublime's completion behavior becomes well defined or Sublime allows custom edit action on completion selection.

However, I don't understand your statement that Sublime and VS Code insertText match. How so, if (AFAIK) VS Code has proper (full) support for textEdit and it is preferred over insertText - so it can apply edit exactly as it is returned by LSP, whereas Sublime plugin API does not offer triggering unrestricted edit on completion selection.

And for PHP and sticking $ back, it is not fault of LSP plugin or LSP server, it is Sublime that does replacement with something else than completion prefix which is erratic and undocumented behavior.

And if I read LSP proto docs correctly, label is insertable as long as insertText or editText are NOT
present, otherwise label can be just a phony label for proper insertion.

And last thing in defence of textEdit is that it exactly specifies what has to be replaced on completion, hence there is no need to know or deal with word separators, etc.

[tomv564]

Flag complete_using_text_edit has now been added. I think now that the significance of the trigger field is known (thanks @rwols !) the remaining bigger issue is with the word separators.

[nanoant]

@tomv564 Thanks for your support!

[tomv564]

Didn't get a chance to reply to your comments @nanoant, but I share your conclusions about the protocol and the best way to use it (textEdit). Unfortunately as long as we return completion content instead of inserting completions ourselves, we will have issues when sublime recognises word boundaries :/