Language-server-protocol: Clarify the limitations around the edit(s) in CompletionItem

[kdvolder]

Sorry to keep harping on this, but I think it is important.

See discussion in #11. That issue was (rightfully) resolved, but some of the tangential discussion that spun off in it wasn't.

Summary: It seems vscode will check that the main edit in a completion item meets certain requirements. For example:

• its range must be a single line
• its range must overlap the cursor

I argue that these restrictions are misguided and would hamper someone implementing a language server. So in a sense I'm really not arguing to change the spec to include these restrictions.

However @jrieken did say vscode's check for these constraints is intentional, while I disagree imposing these limitations is a good idea... if they are there intentional, then maybe it should be clarified in the spec that such edits are supposed to be considered invalid.

Or, perhaps, should we consider this just a 'implementation restriction' which is specific to vscode's LSP client implementation only? (This isn't great because it would mean language servers that provide edits violating these undocumented restrictions might not work on vscode but may work on other LSP client implementations)

Either way I would like some clarity on this either by an amendment to the spec, or an explicit acknowledgement in response to this ticket that this is just a 'vscode implementation issue' and should not be considered a part of the LSP spec.

[jrieken]

Just to be clear - the restrictions apply only to the _primary edit_. That mean when invoking IntelliSense at position x primary edits must overlap with that position. Otherwise the UX would be uber-confusing. It has nothing to do with additional edits. There is some doc about this here: https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts#L2327

[kdvolder]

Okay, but that link is to vscode's codebase. A similar explanation isn't in the LSP. So my question remains whether this is just a vscode specific 'implementation restriction' or something that should be mandated by the spec.

Also, I understand someone might argue that having the main edit not overlap with the current cursor position could be potentially confusing (although I would assume that the main edit determines where the cursor actually moves to, making it less confusing i.m.o.).

In any case I think whether this is good or bad UX is a judgement call. I don't think it should be up to vscode or LSP to dictate those kinds of restrictions to LS implementors.

For a concrete example of the kind of thing I want to do. Here's a very short video (this is in Eclipse / STS, but we want vscode to get similar support via LSP).

https://drive.google.com/file/d/0B-E3P0EgKBcKMXh4dnRGaEl2eU0/view?usp=sharing

Notice how completion inserts text somewhere (potentialy) far away from the current cursor, removes the text already typed at the cursor, and inserts the completion text in the _right_ place and moves the cursor there. The right place to insert the completion is dictated by pre-existing document structure. Inserting the text at the cursor would simply create invalid yaml with duplicated keys and not be very helpful to the user.

[dbaeumer]

Agree that the documentation needs clarification.

One thing we need to keep in mind is that the more complex we make completion items in the protocol the less likely it is that an editor can implement the protocol part (e.g. the client side)

[kdvolder]

the more complex we make completion items in the protocol the less likely it is that an editor can implement ...

Sure, but in this case I'd assume any editor implementing the protocol (so it already supports the 'extraEdits') must already have the ability to handle these multi-line edits anyway. So the extra checks on the main edit seem to be purely about explicitly disallowing something that the editor can easily do.

[dbaeumer]

I did quite some clarification around this. @kdvolder is there anything that is outstanding from your point ?

[kdvolder]

@dbaeumer I think the description is clear now.

However, I still disagree that this limitation should be warranted by the LSP though. Let me make one more attempt at arguing my point of view.

If you, after considering my argument, haven't changed your mind about it, we can close the ticket. Otherwise, maybe we can discuss it further.

My main motivation is, I still would like to find a way to have completion behavior as shown in the short video (i.e. where a completion item inserts text elsewhere in the document and also moves the cursor to that location. It seems this is currently not possible... because of the limitation on the primary edit and the fact that cursor movement is also tied to the primary edit.

I understand now that this limitation is somehow tied to multi-cursor support in vscode, and, given some other recent discussion on this where you mentioned not wanting to complicate the protocol because of multi-cursor (a point with which I strongly agree), perhaps this particular restrictions really shoudln't be baked into the protocol either. If some client can't handle multi-cursor situations when main edit spans multiple lines, I think a good way for such a client to deal with that situation is simply to ignore the extra cursors in just those specific situations and simply apply these changes to the main cursor only. That would support my use case, and, I think, not really sacrifice anything w.r.t. to allowing a client to make a best effort at supporting multi-cursor if the edits do meet certain restrictions.

I.m.o. we are worrying too much about multi-cursors, which I see as more of an 'edge case'. We should be more concerned that things work smoothly in 'single cursor' situations first. In this case, we are putting in this explicit limitation, just to make multi-cursor support easier, while making other single-cursor use cases impossible, and that seems like a mistake to me.

Anyhow... if this still doesn't change your mind... then thanks for taking the time to consider it, and we can consider the matter closed.

[puremourning]

must already have the ability to handle these multi-line edits anyway.

Actually, no. Vim clients for example are quite restricted in what the completion system can apply. The restriction as it stands actually makes it just about possible to implement LSP in Vim.

I would argue strongly that a completion should absolutely not change the cursor location, and should be restricted to the general notion of completing a word/symbol etc. that the user is trying to type. I accept that there may be ancillary things (like imports for example) that make sense as a consequence of selecting a completion, but he primary thing that is completed should remain the completion of what the user is trying to type.

This is more fundamental than multi cursor IMO.

[kdvolder]

I've just come back to this very annoying limitation. We were trying to implement a workaround. The workaround is very complex. We do this (the steps are not 'chronological' just enumerating the elements of the 'solution'):

1. Tranform our edits into the form that falls within the described limitation (i.e. we iterate all edits and when one hits the current line we collect that edit into one a 'main edit' set. We apply all edits from the main edit together and turn them into a single 'replacement' edit.

2. Any remaining edits falling completely outside the main line become 'additional edits'.

3. We calculate where we want the cursor to end up after applying all the edits. (somewhat tricky because positions are now relative to the changed document, not the original document)

4. We add a 'command' to the completion item to move the cursor to that location.

5. We implement a custom protocol extension that lets us move the cursor (there doesn't seem to be any other way to do that)

6. We have the server register a capability to handle the movecursor command so that command attached to the completion items actually works. The server's command implementation calls the custom 'moveCursor' protocol on the client.

As you can plainly see. This is all really a big PITA. And the implementation has custom protocol bits making it a pain to implement on different clients (the custom bits have to be implemented multiple times). So can you, please, please, please reconsider dropping this annoying 'little' restriction from the spec? That would be so ammazing. Thanks!

I do feel for the likes of VIM which cannot deal with more complex edits. But I guess any client is always free to consider its own capabilities / restrictions, and if it really hasn't got the means to apply more complex edits, then (I got to ask why not?) it can just detect them as 'too complex for me' and filter out any such items it doesn't like.

I do not think it is a good idea to dictate these kinds of restrictions right in the spec. Let the spec be silent on this. Encourage clients to do their best to accomodate whatever edits a server sends its way.

As to the arguments around multi-selection semantics of completion items. I understand. But frankly, personally, I do not care much about that. Simply, because I do not use multi-cursor all that much (if ever!). I would guess that 99% of the times someone invokes a completion, it is invoked with in a single-cursor situation. If you really must consider multi-cursor support. Why not treat it as the special case that I think it is and only enforce limits on completion edits if the current selection is in fact a multi-cursor. I mean... why should there be limits enforced on the other straigtforward and (i.m.o more common and more important) single-cursor use case?

[matklad]

I am hitting this in rust-analyzer. The thing I want to implement is postfix completion:

turning this:

foo.bar()
   .baz()
   .quux().if|

into this:

if foo.bar()
      .baz()
      .quux() {
    |
}

I think the current CompletionItem API makes implementing this cumbersome, if not impossible.

Rust analyzer strives to be "advanced" language server, so I'd love to control everything about edit (including cursor position). The ideal API for me would be

ComplexCompletionItem {
    // The range of the identifier that is being completed. 
    // This is **purely advisory** and can be used, for example, to highlight this range in the editor.
    // Clients can safely ignore this field.
    source_range: Range, 
    // The change to apply to the document when the edit is selected. It 
    // typically replaced identifer at `source_range`, but it can contain arbitrary other edits as well.
    // Text edits are interpreted as snippets and **must** contain `$0` placeholder. 
    edit: TextEdit[]
}

However I also understand that simple edits work in more contexts. I totally buy multiple cursors argument. The VIM case sounds like an editor limitation to me, which ideally should be fixed on the editor's side, but it centrally would be good to support as many editors as we can.

So, a simpler interface makes sense to me as well:

/// This is not as powerful as `ComplexCompletionItem` (it can only edit the single line of text), but
/// when possible, it is advisable to use `SimpleCompletionItem`, because, unlike 
/// `ComplexCompletionItem`, it works with multiple cursors.
SimpleCompletionItem {
    // number of characters before the cursor which should be removed before insert the 
    // replacement is inserted
    prefixLen: number,
    // the text to be substituted instead of the prefix. May contain placeholders like `$0`
    replacement: string,
}

So, something like ComplexCompletionItem | SimpleCompletionItem makes total sense to me! Another question is how do we add this backward-compatibly without making current mess of various ways to specify edit even more complicated...

[puremourning]

Remember that you're implementing a _completion_ not a refactoring tool. I think language server maintainers realised that what the user almost certainly wants to do is complete the word they are typing. I just don't see what you're trying to implement as being a completion of a word, and frankly would be a very surprising result in most editors.

[matklad]

As a user of a code editor, I personally would very much enjoy postfix completion. I think a lot of other people would as well! So I think it is important that LSP makes this possible. Whether we should use completions request for this feature is another question, but I feel that all of keyword/snippet/reference/defenition/postfix completions should go through the unified interface, because the UI for all of them is the same.