Clarify magic indentation fixes applied to multi-line text edits in CompletionItem

[kdvolder]

It looks like when I create a completion item which contains newlines, so expands to more than one line of actual text... vscode applies some kind of magic indentation fixes to my edit (rather than taking my edit at face value and replacing my text exactly as given).

This is disturbing really because my 'newText' already contains exactly the needed spaces to get the desired/correct indentation. I'm seeing that in vscode additional spaces are added on top of that which actually messes things up.

E.g my language server sends a completion like this:

  {
       "label":"services",
       "kind":5,
       "sortText":"00014",
       "filterText":"services",
       "textEdit": {
            "range":{
                  "start":{"line":2,"character":4},
                  "end":{"line":2,"character":4}
       },
      "newText":"services:\n      - "
 }

There are precisely 4 spaces between the \n and the -.

However, when the completion is invoked it looks like 8 spaces get added into the document instead.

Really, I'd prefer if 'newText' doesn't get subjected to this kind of magic stuff. But if there is indeed magic stuff happening, then we probably should have clear descriptions in the LS spec to explain exactly what magics to expect.

PS: This is somewhat related to #82 (both issues concern a different kind of undocumented 'magic interpretation' applied to the contents of the TextEdit'snewText field).

[dbaeumer]

@jrieken can you please comment. Is there a way to avoid the formatting (e.g. by using the new range property ?)

[dbaeumer]

Moving to January. Need to first understand what exactly is happening and then need to think about a spec.

[jrieken]

The magic is that VS Code prefixes new lines in snippets and completions with the indentation of the 'reference line', that is the line on which the operation was started. So, if your reference line is indented with 4 spaces each new line in your snippet is prefixed with 4 spaces. This is in line with other editors and makes it easy to create snippets and completions without always checking the indentation properties of the current document.

[kdvolder]

Cool, okay. That clarifies it.

I think it it would be convenient if there was a way to enable/disable this magic, maybe by adding a boolean field 'enableMagicIndent' or something like that to the CompletionItem (or to the list to avoid repeating it for every item). To preserve backwards compatibility this field could be optional and the magic indent could be enabled by default if the field is absent.

[kdvolder]

Cool, okay. That clarifies it.

Actually... just a little more nitpicking. I am wondering... how does cursor position (before edit) play into this?

For example, let's say that cursor is shown as <*> and we have this in the editor prior to applying the edit:

   <*>   foo

I.e. there are 3 spaces in front of the cursor and 3 spaces after the cursor. The reference line is indented by 6 spaces (first-non-whitepace char is the 'f' from foo which is preceded by 6 spaces).

It would seem most logical to me 'magic indent' in this case should be 3 spaces rather than 6 (i.e the text after the cursor is not considered). But the simple phrasing of the magic rule as given by @jrieken, above would imply its actually 6 spaces.

[kdvolder]

I'm going to re-iterate my request to disable the 'magic indents'. I'm doing an editor for yml, which is very finicky around proper indentation levels (the indentations have semantic meaning!). I'm finding the magics are making it impossible in some cases to implement the correct/desired behavior.

Here is a concrete example (cursor position is shown by <*>)

resources:
- name: source-repo
  type: 
    <*>

Invoking content assist is supposed to expand into this (where the $1, $2 and $3 are tabstops in a snippet)

resources:
- name: source-repo
  type: 
    pool
  source:
    uri: $1
    branch: $2
    pool: $3

Unfortunately this is not possible because the insertText with 'magic indents' applied will always end up indented more than the reference indent (i.e. indentation of pool). Its not possible to communicate the intent to apply a negative relative indent to source.

So can we please, please, please add a switch to the protocol that allows to turn this magic off?

[kdvolder]

Afterthought... It occurred to me that maybe this:

Its not possible to communicate the intent to apply a negative relative indent

Might not be true. Maybe a \b (backspace) character which is kind of a negative space could be added to the front of a line to communicate this intent.

Alas... doing that with vscode's client just inserts the control characters into the editor text (which has some mildly confusing effects).

[dbaeumer]

Moving to 4.0 to consider the capability PR.

[dbaeumer]

We should first clarify the current behavior.

[kdvolder]

We should first clarify the current behavior.

I tried to do that in the PR as well. Though the explanation is part of the capability description.

My apologies for taking so long with the CLA. When I have to ask permission from legal, things tend to get annoyingly complicated. If you think there's a good chance you'll accept the PR for 4.0 (?). I'll prioritize the paper work I have to do for this. So let me know how you feel about the chances for this PR if you can.

[dbaeumer]

I think we need to do two steps:

• clarify the current behavior
• allow in 4.0 to control the behavior.

If you don't sign the CLA it will now mean that the feature request is dropped :-)

[mickaelistria]

To me, this is not a bug in LSP, but a bug in VSCode.
The documentation nowhere says anything about such extra-indentation, and just says that the TextEdit are expected to apply from last to first. As such, VSCode interpretation of TextEdit is over-doing work with VSCode specific choices, but this is not really because of protocol.
If the same thing were done with Eclipse LSP4E because of some additional handling of editorconfig, it would more likely be seen as a bug in LSP4E.

VSCode-based language servers may decide to read some extra configuration files to apply some indent, but those should be internally and reflected in the TextEdit.

So instead of tweaking the protocol or adding more clarification or more complexity by allowing multiple cases, what about just fixing VSCode? (You'll see I'm placing a major bait for troll about governance and vendor-neutrality here, but I think it can be useful to discuss it at some point and this bug is a great example)

[puremourning]

Most vim clients probably don’t support snippet insertion. But if they do it would have to be as a bolt-on that happened outside of the completion system. Completion strings in vim may not contain new lines. Wel, they can but Vim inserts ^@ in their place.

I tied to do this in ycmd but it is a real pain.

[mickaelistria]

@dbaeumer I agree handling of auto-indent must be spec'd and that we can find a clarification without adding a capability nor extra types nor even an opt-in/out strategy. I think it's more than a single item to spec if we want to support most cases.

If we want spec that auto-indent is applied by default (opt-out), we should spec that:

1. When text edit is multiple lines, the client must repeat the indentation of the first line when completion item is applied to all new lines
2. '\b' means removing a level of indentation (can be a space or a tab)
3. clients are free to change the line-ending and to replace '\t' indent characters by combination of spaces.

If we want to spec auto-indent is off by default (opt-in), we should spec that:

1. When text edit is multiple line, not extra-character is expected to be added as indentation of new lines
2. Clients are free to change the line-ending and to replace '\t' indent characters by combination of spaces.

[kiennq]

@dbaeumer Is there any news on this? Applying auto indentation can sometimes be really slow in some editor (like Emacs with org-mode) so I wonder if we can have a more simpler indentation that language server expect client to insert.
Doing magic indentation on client side may have side effect of what's expected format by client and server may not be in sync, so a textDocument/rangeFormatting can completely overwrite spaces/tabs in the text edit buffer.

I saw this propose in VsCode that mentions

/**
 * Keep whitespace as is. By default, the editor adjusts leading
 * whitespace of new lines so that they match the indentation of
 * the line for which the item is accepeted.
*/

so the editor side indentation should be changing the leading whitespace of new lines to match with the line where item is accepted?

[dbaeumer]

It got added to VS Code as a boolean (https://github.com/microsoft/vscode/blob/master/src/vs/vscode.d.ts#L3870). Adopting the boolean would be straight forward and I think specing the behavior as well. Things get more complicated if we want to support the \b and \t as suggested here #83 (comment). IMO this wouldn't be necessary since the server could always provide the right indentation.

[kdvolder]

Things get more complicated if we want to support the \b and \t as suggested here #83 (comment). IMO this wouldn't be necessary since the server could always provide the right indentation.

I agree. I think I may at some point have planted the idea of using \b as a negative indent signal. It is something I did try out (and it didn't work :-). However the only reason I tried to use \b in this way was because there was no way to disable the magic indents. If magic indents would be disabled then I wouldn't have any need to express a 'negative indent' relative to the magic indentation level.

[dbaeumer]

I addressed this in the following way:

• the client announces now a insertTextMode in the completion client capabilities. Currently support are asIs and adjustIndentation. This value signals the default behavior of the client
• a completion item can have a insertTextMode property to overwrite a default client behavior if supported by the client. The support is signaled by the client capability completionItem.insertTextModeSupport

[rwols]

I would actually argue against having newlines in completion text edits. It just doesn't come across as very intuitive to me. The most natural completions are the ones that are shortest.

It's also not clear to the user whether selecting e.g. a completion of type "function" would now result in additional newlines or not. It makes the process of completing somewhat unpredictable.

(for completions of type "snippet", it is of course to be expected that it may contain newlines)

[puremourning]

I would actually argue against having newlines in completion text edits. It just doesn't come across as very intuitive to me. The most natural completions are the ones that are shortest.

Agree, completions shouldn't contain newlines, because what gets inserted doesn't match what's shown in the menu, which is highly confusing.

Alas, we are where we are with this one. In the client, we just have to filter out completions with suggestions containing newlines.

[kdvolder]

Agree, completions shouldn't contain newlines, because what gets inserted doesn't match what's shown in the menu, which is highly confusing.

I completely disagree.

You are entitled to your opinion of course, and I'd say you are free not to create such completions in your language server, if you are making one. I think this kind of choice should not be forced onto a server by the protocol or the client. The protocol / client should instead empower server authors to make this decision as it makes sense to them for their specific language.

BTW: 'Snippet / template proposals' are naturally multi-lined snippets of text and they are i.m.o a very useful type of completion.

The most natural completions are the ones that are shortest.

I question that. I don't know what it means to be 'natural'. However when talking about usefulness, e.g. in terms of how much a completion helps me as user by avoiding writing out small or larger bits of code manually... clearly the more useful completions are the larger ones.

That is precisely why multi-line 'snippet' completions are so useful in practice because they can insert larger templates of boilerplate code that would be rather laborious to type out in full by hand (or even when aided by many little completion steps invoked repeatedly).

[mickaelistria]

I totally support @kdvolder here. In Eclipse IDE, we have plenty completion proposals that spans across multiple line, respect style and so on, basically bootstrapping some skeletons of relatively complex code that are "natural" to place as such location in such context. If you remove such features from Eclipse IDE for Java, it would suddenly seem seems much weaker than it is now.
I expect that more languages may like to take advantage of it; and that any decent editor should be capable of properly handling multi-line text insertions/completions.
In any case, the fact that there is at least 1 serious and mature use-case for multi-line completion/text edits is enough to not try to dodge this issue, and instead to focus on finding a decent solution. To me, pretending insertions should be single-line is just a way to put some extra constraint to find a "cheaper" solution on short term and delay a bigger effort later trying to get rid of the constraint); not really a way to find the best solution.

[puremourning]

BTW: 'Snippet / template proposals' are naturally multi-lined snippets of text and they are i.m.o a very useful type of completion.

I agree that these are useful in, but I'm sort of of the opinion that it feels awkward in an auto-completion menu. feels like the kind of thing that lives in a different client-gesture (like 'expand snippet' or a code-action type thing).

Taking the one I know, jdt.ls returns a completion item with the menu text of get which creates a getter method when selected. It's kind of jarring when all other completions in the same menu just essentially insert the text selected.

As you say, opinions clearly differ on this.

And like I said, the cat's out of the bag; servers are already doing this, and I 100% agree that the semantics and the bahaviours associated should be prescribed such that client authors like myself don't have to parse the vscode source code to find out what the de facto behaviour should be. I'm not completely convinced that the insertTextMode does that, but it's certainly a good signal.

Perhaps servers could be encouraged not to send multi-line suggestions when clients don't advertise support for adjustIndentation client capability. Though I fear this is a forlorn hope.

[kdvolder]

@dbaeumer The wording on how magic indents are applied/defined/computed is a bit too vague i.m.o. For example if you scrolled way up there is this question for clarification I raised:

#83 (comment)

There are probably different cases to consider depending on where the cursor is w.r.t to the line into which we are inserting/applying an edit. If you don't want to have clients doing each something slightly different, then it probably needs to be specific explicitly.

[kdvolder]

As you say, opinions clearly differ on this.

Agreed, and that is exactly why the client/protocol should not force this choice onto a server implementor.

[dbaeumer]

@kdvolder clarified it. I speced it to use 3 spaces taking your example since this is what I as a user would expect to happen.

[kdvolder]

Okay so that clarifies it for the situation where cursor is inside the leading whitespace. But what about the case when it is not? The wording we had before seemed more applicable/specific to that, whereas the new wording now is clear for when the cursor is preceded by only white spaces, but I think it is now ambiguous when the cursor is at the end of or in the middle of a non-whitespace area.

So specifically, for this example:

    some<*>

What is the meaning of 'indentation upto the cursor'? I can think of two possible interpretations. One gives 7 (3 leading spaces + 4 letters) and the other gives 3. I'm pretty sure (?) that you want the result to be 3 (i.e. counting only the 3 leading spaces) but I do not think that is totally clear from the wording.

Another potentially confusing issue is whether the magic indent are really applied relative to the 'cursor' or rather whether is based on the insertion position of a text edit. (I beleave vscode probably uses the positions in the edit as queues for computing the magic indents, not the place the cursor was in, though we may expect these to be somewhat related. Nevertheless, language server produces these edits and so they are really independent of the cursor position that client keeps track of.

So I think where the word 'cursor' is used now in the description, it probably actually means 'insertion position' (which is the same as cursor position in many/most cases but not necessarily all cases).

[kdvolder]

I suggest some very specific wording like:

The editor adjusts leading whitespace of new lines, by copying exactly the leading whitespace on the line to which the edit is applied, upto the edit's starting postion (for a text edit) or the insertion position (for insertions).