Support suppressing commit character from being inserted during completion

[DanTup]

I have a completion provider that inserts parens when completing method calls, eg. myFunc(). Today I tried adding ( as a commit character so that if I've typed myFun and press ( then it'll still complete. However this results in myFunc()() because the completion text includes the parens, the commit character then gets added, and then the automatic closing paren.

Although I could remove the parens from the completion, then they wouldn't be inserted if you hit <enter> either.

@mjbvz suggested raising this as he may have had a similar issue with snippets in JS/TS.

Is there a good workaround for this, or can the API be extended in some way to handle this?

[jrieken]

needs thinking

[DanTup]

I'm a bit confused by the existing behaviour here... I have this code:

a.noSuchMe

In my completion the user sees noSuchMethod(...). Snippet looks like noSuchMethod(${1:invocation})${2:}

If they complete with ( then I end up with:

a.noSuchMethod((invocation))

I'm not sure how the inserted parents (from the commit character) ended up in the place they did. Are they being wrapped around the first placeholder?

[jrieken]

Closing this as designed because that's what commit characters are supposed to do, accept the completion and type that character. See https://github.com/Microsoft/vscode/blob/86230c239c24b17ba2c48533cb5d8075b1b4fed6/src/vs/vscode.d.ts#L3231-L3236.

[DanTup]

I understand this is designed, but it seems restrictive which is what this request was about. There is an assumption that the commit character will always be the character wanted at the very end of the completion (which isn't even the common case - the common case wants a closing paren on the end, which just happens to be taken care of by the auto-insertion of it).

There's also still the question in the last comment about about why parens are wrapped around the snippet which seems really confusing.

[mjbvz]

Root cause of #19820 too. I think this is worth revisiting as the current behavior does cause issues and I cannot find a workaround. We've had to disable ( as a commit character for js/ts when completing function calls (which is when having ( as a commit character would arguable be most useful)

I was about to file a new issue for this but am reopening this one instead since exactly matches the JS/TS extension problem too

[jrieken]

@gjsjohnmurray from #79373 (comment)

I acknowledge that altering this behaviour would result in a breaking change. So I propose enhancing commitCharacters to accept as an alternative an array of objects structured as {character: string, inject: boolean}

[jrieken]

Based on @gjsjohnmurray idea, this is the plan

• vscode: extend the API like so: commitCharacters?: string[] | ReadonlyArray<{character:string, insert:boolean}>
• vscode: add internal editor API so that a will-type listener can prevent typing
• extensions: adopt new API

[Gama11]

This seems closely related to #70824 and microsoft/language-server-protocol#793. Perhaps in the future the {character:string, insert:boolean} structure could also include insertText and additionalTextEdits?

[mjbvz]

I like the proposal from an api perspective. My only suggestion would be to change the api to make the insert field optional:

commitCharacters?: string[] | ReadonlyArray<{ character:string, commitWithoutInsert?: boolean}>

commitWithoutInsert probably needs a better name

[DanTup]

I'm starting to have second thoughts about what I asked for here... I'm not sure it's the best way. Imagine I have a function foo(bar) and I want to provider a snippet placeholder for the args:

foo(${1:bar})${2:}

A common complaint is when a user is writing code like this:

myCollection.map(foo);

The press <enter> on foo and we end up inserting foo(bar) and they have to delete stuff. I think the real solution is that we can do something different based on the trigger character, eg.:

• If the user presses <enter> just insert foo
• If the user presses ( then insert foo(${1:bar))

Currently this isn't possible because we can't have conditional edits based on the commit character. We can run commands, but a) they don't get the commit character (we could potentially look at the character before the cursor, but that would be fragile) and b) the docs say that commands shouldn't modify the same document (they should use additionalTextEdits).

Is there a way of doing this I've overlooked?

(FWIW, the main reason I'm inserting arg placeholders is because the rendering of signature help is a bit of a mess and very difficult to read).

[dbaeumer]

This came up in a discussion with VS and @jrieken and I discussed about this. To improve the naming we could define the following literal { character: string, insert: boolean }. Observe that insert is NOT optional. So we can use a positive name since it absence is not possible and therefore doesn't need to default to false. The value case is covered by using a string anyways.

[DanTup]

My thoughts:

a) the completion payload can already get huge with commit characters on every item, this could make it significantly larger
b) it doesn't help with being able to conditionally insert argument placeholders (see my comment above) - the user needs to opt-in to "always having parens/args inserted" whereas typically commit characters would allow them to be conditional on whether you used the commit character or not

It would be nice if you didn't need to choose between "being able to complete with parens" and "being able to complete with argument placeholders". Ideally you'd be able to use ( as a signal that the user wants to invoke, for example:

• complete with <enter> to insert print
• complete with ( to insert `print(${1:message));

(I don't know how to implement this without making the payload yet larger again though 😬)

[dbaeumer]

@DanTup IMO there are two different requests here:

• avoid inserting the commit character
• use a different text edit depending on the commit character. This is more like the insert / replace support the completion item has now (e.g. do something different on enter or ctrl+enter).

IMO we should keep the two separate because we can make progress on the first without solving the second. The first alone has valid use cases in HTML and XAML.

[DanTup]

Yeah that's fair, though I thought it was worth bringing up in case the solution to the second might influence how the first is solved.

As some general feedback and a bit of a tangent, commit characters have been somewhat disappointing to me in VS Code. I've tried to use them both in Dart and TypeScript and always hit frustrating issues that result in me turning them back off (I don't think it's just my Dart implementation, since TS has issues too). I don't recall ever being annoyed by them this much in C#+VS - I don't know if that's because VS Code's API makes things more restrictive, or I'm just misremembering.

[sam-mccall]

Bumping this we've assumed commit characters already worked this way, and have been suddenly bitten (clangd/vscode-clangd#357). We're probably going to work around by suppressing all commit characters in our vscode plugin.

---

We've been sending a large set of allCommitCharacters including (. VSCode was ignoring these (microsoft/vscode-languageserver-node#673), other clients were making this work somehow (heuristics?) or also ignoring allCommitCharacters.

We didn't realize that the commit character would be inserted (it makes sense, except that it seems to leave little room for commit characters to be useful). And we can't retroactively change the behavior of released language servers, so we're stuck trying to disable all commit characters.