CompletionItem is underspecified: replacement text replaces *what* range?

[ljw1004]

interface CompletionItem {
  // Label. By default also the text that is inserted.
  label: string;

  // A string that should be inserted a document when selecting this completion.
  // When `falsy` the label is used
  insertText?: string;
}

The LSP spec talks about the text that will be inserted. But VSCode only does insertion in some cases. More generally, VSCode autocomplete does replacement. Here's what VSCode does when you do an autocomplete when the caret is in this position foo.ba|or... [see also VSCode source code vs/workbench/api/node/extHostLanguageFeatures.ts and vs/workbench/api/node/extHostTypeConverters.ts.]

1. someone prior to LSP has set up the LanguageConfiguration.wordPattern regular expression.
2. When autocomplete is invoked, it uses wordPattern to figure out the word containing the caret, in this example baor.
3. It uses the label || insertText to replace that part of the current word that's to the left of the caret, in this case ba.

I think it's unsatisfactory that LSP's completion response should depend on the externally-provided LanguageConfiguration.wordPattern when it doesn't know what wordPattern was provided to VSCode, doesn't even know if it's being invoked by VSCode or some other client, doesn't know what that client does. I happened to stumble across the PHP wordpattern at https://github.com/Microsoft/vscode/blob/master/extensions/php/src/phpMain.ts#L29 but my LSP server shouldn't have to depend on that file!

Concern 1: Some clients like IntelliJ, Eclipse and VisualStudio can actually have autocomplete replace the entire word baor, not just the bit to the left ba. I think the language server needs to know what text it's going to replace, so it can offer up its insertText correctly.

Concern 2: Some clients like IntelliJ, Eclipse have autocomplete insert the function-calls with snippets, e.g. at this.f| it will autocomplete to this.foo($$i, $$j). This differs from VSCode which only autocompletes to this.foo. I think that in LSP it can be up to the language server whether or not to insert parentheses+arguments. But again I want my language server to know what span it will replace.

Concern 3: I'm working in a language with a variety of separators -- a::b, a->b, a\b, a.b -- and I don't know if the LanguageConfiguration.wordPattern will accommodate all of these. The VSCode docs for wordPattern talk about the difference between "positive wordpatterns" (e.g. a word is the regex [a-zA-Z0-9_]+) vs "negative wordpatterns" (e.g. a word is any string of characters surrounded by blanks or one of a finite list of punctuation characters). It says that the negative wordpattern is better if your language supports unicode. So honestly it's anyone's guess what the current wordpattern will be.

PROPOSAL 1: Maybe we should say that label / insertText is actively bad practice because the language server can't know exactly what span this insertText will replace. And every language server should instead use textEdit to be sure to cover the correct range. (But this is a shame because it doesn't work with multiple cursors, and it wouldn't allow the client to offer the user a choice between replacing vs inserting behavior, like IntelliJ+Eclipse currently do).

PROPOSAL 2: Maybe we should say that the LSP itself provides the autocomplete-wordpattern regex to the editor. (This would be tough because it would have to be a wordpattern regex that works across all the different language regex interpreters that clients might be written in).

PROPOSAL 3: Maybe the textDocument/completion request should include the characters that will be replaced if insertText is used.

ADDITIONAL PROPOSAL: Maybe we should stop calling it insertText in LSP, because it's a REPLACEMENT text not an insert text.

[mickaelistria]

The current version of the protocol also say that CompletionItem can contain a textEdit. TextEdits have the necessary smartness. The best approach would be to use TextEdits over insertText anyway. Maybe the insertText field should be deprecated and have a note about preferring textEdit?

[ljw1004]

@mickaelistria what you wrote is exactly my Proposal1. Its downsides are (1) it doesn't work with multiple cursors, (2) it doesn't allow clients to offer users the choice of doing either replace or insert, as Eclipse and IntelliJ currently do.

[mickaelistria]

@ljw1004 in such case, dealing with replace vs insert would most likely be a language server setting. I don't think it should be part of the CompletionItem. On a side note, in Eclipse IDE the insert/replace setting is not available for most text editors, only JDT and a few others do support it, but usually users aren't given choice and take the completion as it; using LS is very similar, so it's not a major inconsistency.

[dbaeumer]

A couple of comments:

• I checked with @jrieken and multi cursor works with text edits
• one reason why VS Code does smartness with the insertText (e.g remove the prefix of the current word from the insertText) is because of multi cursor. Consider this example in the JS file

con<main cursor>
<cursor>
<cursor>

open code complete and select console. This will result in

console
console
console

I think the best solution will be proposal 3. I will provide a default range that is used as a replacement range if only a insertText is provided.

Objections?

[dbaeumer]

May be a global flag is enough. Then server could use the text edit instead to ensure it behaves the same on all clients.

[kdvolder]

My vote is for 'Proposal 1' (from @ljw1004). I.e. just deprecate the use of insertText. Personally, I found the potential confusion around what does the 'insertText' replace (if anything at all) enough of a reason that I already concluded we should use textEdit exclusively so we can be precise about what part of the document to replace when applying the completion. So our language servers never use the insertText precisely because I wasn't sure what it would do.

Perhaps we should just make that official rather than pile more complexity in a place where its not needed (since textEdit mechanic is quite adequate already).

[kdvolder]

@ljw1004 Mentions two downsides:

(1) it doesn't work with multiple cursors

According to @jrieken, who should know, it can be made to work. So that objection is of the table.

(2) it doesn't allow clients to offer users the choice of doing either replace or insert, as Eclipse and IntelliJ currently do.

Personally, I don't think that's really a problem. In my experience virtually nobody wants to change that preference, (because, essentially, changing it from its default value just makes the editor completions work incorrectly).

[ljw1004]

@dbaeumer @jrieken Could you explain what you mean when you say "multi cursor works with text edits"? I still believe it doesn't. Let's look at your concrete example:

0: con<main cursor>
1: <cursor>
2: <cursor>

We send an autocomplete request for line 0 character 3. If it gives the suggestion "console" via a TextEdit, then this will necessarily be a TextEdit solely for line 0, so when you accept it then you get this:

1: console<main cursor>
2: <cursor>
3: <cursor>

But if it gives the suggestion "console" via a insertText then it will end up applying to all three cursors as desired, so you get this:

1: console<main cursor>
2: console<cursor>
3: console<cursor>

That's why I say that TextEdit doesn't work (i.e. doesn't deliver the desired user experience for autocomplete) when used with multiple cursors.

[dbaeumer]

@jrieken can you please clarify. We discussed this yesterday over slack.

[dbaeumer]

@jrieken let me hack a small example. May be I convert things wrongly :-)

[jrieken]

We send an autocomplete request for line 0 character 3. If it gives the suggestion "console" via a TextEdit, then this will necessarily be a TextEdit solely for line 0, so when you accept ...

The API hides the fact that there are multiple cursors and makes this an editor responsibility. Otherwise the provideCompletionItems be spec'd with an array of positions, not just a single position. I still believe that was a smart decision because the API/protocol shouldn't care about that.

Now, how does VS Code actually implement this? In the editor we have the notion of an active/primary cursor and when making a completion request we use its position. The result might be an edit as outlined below (replace 'con' with 'console'). We then map that edit onto secondary cursors and use a simple rule: Compare the text-to-replace from the primary cursor ('con' in this sample) with the text-to-replace of the current cursor. If its the same value use a replace edit, otherwise fallback to an insert edit.

Line 124 is the primary cursor, the others are secondary. The edit on line 123 will replace the same text which makes this a replace-edit, the other won't making them an insert-edit.

Code reference: https://github.com/Microsoft/vscode/blob/master/src/vs/editor/contrib/snippet/snippetSession.ts#L281

@ljw1004 In case you actually tried this in VS Code and in case it didn't work, then please file a bug. Thanks

[dbaeumer]

I tried the following extension code and it works correctly in terms of multi cursor and text edit (note than in the VS Code API it is range and insertText)

'use strict';
// The module 'vscode' contains the VS Code extensibility API
// Import the module and reference it with the alias vscode in your code below
import * as vscode from 'vscode';

// this method is called when your extension is activated
// your extension is activated the very first time the command is executed
export function activate(context: vscode.ExtensionContext) {

    vscode.languages.registerCompletionItemProvider('plaintext', {
        provideCompletionItems: (textDocument, position) => {
            if (position.line !== 0 || position.character !== 3) {
                return undefined;
            }
            const result: vscode.CompletionItem[] = [];
            let item: vscode.CompletionItem = new vscode.CompletionItem('console');
            item.insertText = 'console';
            item.range = new vscode.Range(0, 0, 0, 3);
            result.push(item)
            return result;
        }
    })

}

// this method is called when your extension is deactivated
export function deactivate() {
}

@jrieken you are off the hook

[dbaeumer]

Will try a language server now.

[dbaeumer]

OK. I tried the LSP case as well and it works for me. I have the lsp-sample from here and exchanged the existing completion item handler with:

connection.onCompletion((params: TextDocumentPositionParams): CompletionItem[] => {
	let result: CompletionItem[] = [];
	if (params.position.line === 0 && params.position.character === 3) {
		let item: CompletionItem = {
			label: 'console',
			kind: CompletionItemKind.Function,
			textEdit: TextEdit.replace({ start: { line: 0, character: 0}, end: { line: 0, character: 3 } }, 'console')
		}
		result.push(item);
	}
	return result;
});

Running this correctly works with multi cursor:

Given all this I will do the following: make it clear that the insertText might be subject to interpretation and deprecate it. I agree that using text edits is the right approach here anyways. I will also include a global capability to single if the insertText is subject to interpretation.

[ljw1004]

Thanks for the explanation @jrieken, and thanks for your analysis and conclusions @dbaeumer. They sound good.

Q1. TextEdit is used for multiple methods - textDocument/WillSaveWaitUntil, textDocument/formatting, textDocument/onTypeFormatting, workspace/applyEdit, textDocument/rename, textDocument/completion. Does the algorithm you described for applying TextEdit to multiple cursors apply to all of these? or solely to Completion?

Q2. I'm struggling to figure out how this would apply when multiple cursors overlap with the TextEdit. Imagine the string abcd with primary cursor after b and secondary cursor after c, and the TextEdit says to replace abcd with wxyz. What transformation is applied to the secondary cursor?

Q3. @jrieken, I get the gist of what you're describing, but not precisely enough to be able to implement it myself. For instance, I don't know the definition of a replace vs insert application of a TextEdit. Could you point me to some code so I can follow along?

Q4. This is a real semantic thing. Prior to multiple cursors, if you had con|, then the LSP server could happily produce either a TextEdit that says to insert the letters sole, or one to replace con with console, and the two would have been indistinguishable. Now with multiple cursors, the difference is practically distinguishable. @dbaeumer I think some aspect of the behavior should be part of the LSP spec, so that LSP server authors are at least sure they are implementing textDocument/completion in a way that editors (including VSCode) will leverage properly. It might be that the spec simply says "LSP servers are encouraged to respond to textDocument/completion with a TextEdit that replaces any existing partial identifier with a whole one because editors will use this well in situations with multiple cursors". Or it might be that the spec actually specifies what the editors will do in that situation.

Q5. @dbaeumer do you expect all editors which support multiple cursors to also use the same algorithm when applying a completion response? Or is it up to them what to do?

Q6. I'm still not sure what you mean by "is insertText subject to interpretation"...

[jrieken]

Q1. TextEdit is used for multiple methods

The TextEdit on a completion item is actually deprecated, not solely but also because of that. The multi cursor insertion only applies to completions and snippets.

[dbaeumer]

Q1: as Joh said no. TextEdit is a concept that replaces text. It has no special sematic in terms of cursor(s). The sematic comes with a completion item. This is why for example the range for a completion item needs to be on one line and must contain the cursor. All other requests that return text edits might have a different semantic regarding cursors and that semantic is not specified at all in the LSP on purpose. How a tool handles cursors for example while applying the edits of a format request is up to the tool to decide.

Q2: VS Code rejects this completion item. However other tools might do something different.

Q3: @jrieken on the VS Code completion side this is a CompletionItem with an insertText and the range either being an insert position or a replace range.

Q4: this is still true for the primary cursor. I am against saying anything about secondary cursors in the LSP. I think we should leave this up to the tool. I see that in VS Code there will be a semantic difference between a replace for con with console and an insert of sole for the secondary cursors but specifying this would all tools force to implement the same multi cursor behavior. And may be there is a tool that does smart things with insert of sole but not with a replace of con with console. So I simply opt to spec that the completion item is to be applied to the main cursor location and that it is up for the tool to decide what to do with secondary cursors. If we start to talk about secondary cursors we then in consequence need to pass them in the completion request params. Otherwise we will have the next round of things being underspecified.

Q5: no, only for the primary location.

Q6: insertText as you pointed out is not always an insert. It can be a replacement. So there might be some interpretation of the operation depending on the cursor location. Take the following example:

function foo() {
}

function foobar() {
}

function foobarbaz() {
}

foo<cursor>bar

A tool when the completion item foobarbaz is selected and the insertText is foobarbaz could complete this to foobarbaz. So it could even interpret things in front of the cursor which VS Code doesn't.

So besides what I proposed I would add some sentences that a completion items is made for a primary cursor location and that it is up to the tool to decide what it does with the comletion item on secondary cursor locations.

[kdvolder]

@ljw1004 Writes:

But if it gives the suggestion "console" via a insertText then it will end up applying to all three cursors
as desired, so you get this:

1: console<main cursor>
2: console<cursor>
3: console<cursor>

I don't understand that, if a insertext is 'console' and the text already there at the first cursor is 'con' then wouldn't the end result be

1: conconsole<main cursor>
2: console<cursor>
3: console<cursor>

So that is not 'the desired result at all'.

Actually, for the result to be 'console' then the server would probably return an insert text of 'sole' (because con is already there only 'sole' has to be inserted to complete it) and so, the result with multi-cursors would be

1: console<main cursor>
2: sole<cursor>
3: sole<cursor>

This is not the desired result either.

So I'm a bit confused to be honest... it would seem to me insertText doesn't work with multicursor?

[dbaeumer]

@kdvolder this is why we deprecated insertText in favor of text edit. The tool might do some smartness with insertText as VS Code does. I added this as a comment to insertText property (which by the way is deprecated). If the completionItem gives a insert edit with a range and a text then console is inserted and the result is conconsole

[dbaeumer]

In my opinion not further action is needed. I added comments to the spec about the insert text and to favor the text edit. Please ping if you disagree.