Partial fix for https://github.com/dotnet/vscode-csharp/issues/7678

[alexgav]

Summary of the changes

• Remove explicit commit character set for VSCode to avoid inserting those characters twice

Fixes:
dotnet/vscode-csharp#7678
dotnet/vscode-csharp#3647

This is a reasonably complete fix for the issue. I understand behavior better now, with some issues stemming from the fact that VSCode doesn't support VSInternalXXX types and some of the defaults that get produced during conversion form "formal" LSP to VS extension in LSP were part of the problem.

Here's what I am seeing while investigating the issue and my reasoning for this fix.

When examining completion list entries in plain HTML file in VSCode, I am seeing that no commit characters are specified

while in Razor file, via our RazorCompletionEndpoint, we specify [' ', '>', ';', '=']

Furthermore, VSCode returns attribute completion insertion text as a snippet <attributename>={0}, e.g.

Since in VSCode we end up with src="=", it appears that commit character gets inserted as the value of the text at the caret/cursor position. I am saying "it appears" because I haven't actually debugged VSCode HTML server and I am basing my conclusions on observations and tests only.

This fix removes all commit characters specified in RazorCompletionProvider (we probably could've removed just = for this specific bug). After this fix, typing src in Razor file in VSCode and commiting with = results in src=. So snippet text is not inserted. That's actually fine because that's only the first part of the fix.

The second part of the fix is implementing onAutoInsert in VSCode in C# extension / dev kit. I confirmed that VSCode HTML language server does use autoinsert to insert the "" after the equals (by adding "html.trace.server": "verbose" to the settings and watching LSP requests/responses).

Furthermore, with or without this fix, autoinsert should be implement in C# extension, because if you start with text <img src without completion being up and type =, quotes do not currently get inserted (as expected, since we don't yet implement autoinsert).

So I think this fix is OK as the first part (unless Dirk disagrees or has other suggestions), but regardless, we should still implement autoinsert.

[DustinCampbell]

Just so I understand, will this break the commit user experience when typing C# in a Razor file in VS Code?

[DustinCampbell]

IMO, this is kind of a point-in-time comment that might be confusing when somebody else is reading it in a year.

[alexgav]

What would make it clearer? Or do you suggest dropping the comment completely?

[DustinCampbell]

If languageServerFEatureOptions.UseVsCodeCompletionTriggerCharacters isn't set, there isn't reason to create commitCharacters, since it'll just end up setting AllCommitCharacters to [] below.

[alexgav]

Agreed, pushed a commit to fix this. Thanks!

[davidwengier]

When examining completion list entries in plain HTML file in VSCode, I am seeing that no commit characters are specified

Is it possible they're specifying the commit characters on the completion list itself, rather than each item? Or would that have been shown in the extension/popup you screenshotted?

After this fix, typing src in Razor file in VSCode and commiting with = results in src=. So snippet text is not inserted. That's actually fine because that's only the first part of the fix.

I don't think we should take this until the second part is ready, so they can go in together. Given we release VS Code weekly, even if this PR makes ever user who reported the issue happy, I think it probably makes more users unhappy by regressing other scenarios.

[davidwengier]

Does our CommitElementsWithSpace option still work with this change?

[alexgav]

CommitElementsWithSpace is already broken without this change. We are currently always committing elements with space in VS Code even though that setting is "false". That happens because specifying commit characters via capabilities ends up bypassing the code we use to remove Space from the specified commit characters. That code works only when Space is specified by the specific instance of the completion list or specific completion items in that list. We should discuss if that option makes sense in VS Code given behavior differences in how aggressively completion list is shown with low quality matches in VS Code. I would suggest we don't combine that work with this fix because once again, that options is broken already, without this PR. I am glad you brought it up though, thank you.

[alexgav]

Just so I understand, will this break the commit user experience when typing C# in a Razor file in VS Code?

Thanks Dustin, I'm glad you asked. While testing this, I realized commit user experience when typing C# in a Razor file in VS Code was broken already. There are a couple of issue that I found:

1. In VS Code we always think completion list is invoked Explicitly because VS Code uses CompletionContext rather than VSInternalCompletionContext which misses InvokeKind, and InvokeKind is set to Explicit by default

https://github.com/dotnet/roslyn/blob/f18f6e222e67f4bfaacb19e7b7b9f89ba707096e/src/LanguageServer/Protocol/Protocol/Internal/VSInternalCompletionContext.cs#L22

which causes us to think that simply typing in C# block in a Razor file is Explicit completion invocation and causes us to create empty Razor completion list. That empty completion list was getting merged incorrectly (for VS Code) by our ComletionListMerger rather than simply returning C# completion list here as intended in that case

razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListMerger.cs

Line 28 in 8babb3a

return delegatedCompletionList;

I fixed that for now by also checking for empty completion list. We should chat in LSP sync-up Tuesday whether better indication of actual InvokeKind is possible in VS Code.

2. VS Code does not use VSInternalCompletionList.CommitCharacters, so this code in CompletionListMerger

razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListMerger.cs

Line 53 in 8babb3a

CommitCharacters = mergedCommitCharacters,

is also not good enough for VS Code. Roslyn doesn't even return that in VS Code case here

https://github.com/dotnet/vscode-csharp/blob/13325dd7bb9535e4ce6371604948ade004ed7421/src/razor/src/completion/completionHandler.ts#L252C75-L252C76

but it does return CompletionList.ItemDefaults.CommitCharacters, so we shouldn't discard that when available because that's the only data VS Code uses.

I pushed a commit that fixes both # 1 and # 2. Now (even with the initial change in this PR) we have correct commit behavior in C# in VS Code. Again, this was already broken prior to this PR.

[alexgav]

Is it possible they're specifying the commit characters on the completion list itself, rather than each item? Or would that have been shown in the extension/popup you screenshotted?

It would've been shown. There is no VSInternalCompletionList.CommitCharacters in VSCode, but there is CompletionList.ItemDefaults.CommitCharacters. If the latter (the only possibility in VSCode) is specified, they would've been shown. BTW, Ctrl+/ is the key combination to show the data in the screenshots.

I don't think we should take this until the second part is ready, so they can go in together. Given we release VS Code weekly, even if this PR makes ever user who reported the issue happy, I think it probably makes more users unhappy by regressing other scenarios.

I respectfully yet strongly disagree. I feel that implementing onAutoInsert in VSCode shouldn't be a part of a bug fix and is to a degree orthogonal to it. The current fix improves the behavior a lot both for this case and for case when Space completes unwanted completion item. We have a number of user complaints about it. Current behavior (without this fix) inserts unwanted garbage completely breaking user workflow by forcing them to delete the unwanted text that was erroneously inserting. Inserting nothing is much better than inserting unwanted text and causing the user to delete it before re-typing it.