C# LSP Regex completion does not work as expected.

[NTaylorMullen]

Version Used:

Steps to Reproduce:

1. Enable C# LSP feature flag
2. Try and commit any sort of Regex completion item

For instance:

Expected Behavior: commit works as expected

Actual Behavior: Extra content is duplicated / labels seem to apply? Gif is kinda long but shows a lot.

[CyrusNajmabadi]

@NTaylorMullen @dibarbet how does completion work in LSP? Does it not use the data in roslyn's completion items?

[dibarbet]

@CyrusNajmabadi we convert the Roslyn completion item to the LSP completion item. I could imagine something going wrong here - https://sourceroslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/LanguageServer/Handlers/Completion/CompletionHandler.cs,209

LSP first looks at the textEdit of the completion item, if it exists it uses that. If no text edit, it will use the insertText to commit. If that doesn't exist, it uses the label.

Just guessing, but potentially it's using the label to commit incorrectly?

[CyrusNajmabadi]

Yes. That seems buggy. It should be using LSPCompletionProvider.GetChangeAsync to determine what actual change should be made.

[dibarbet]

@CyrusNajmabadi note there are potential perf issues with providing text edits instead of insert text - @allisonchou how far did we get in the experiment to always provide text edits in completion? Did it end up being bad for perf?

[CyrusNajmabadi]

Our completio API has no concept of 'insertion text'. If we want to try to have logic htat finds that special data, it must only do so on providers where that is safe. e.g. the provider (or item) needs to opt into that in some way. If it doesn't, we need to fallback to using the actual API we have for computing the change. otherwise, we're just going to get this wrong :)

[allisonchou]

We likely want to implement this work item before turning TextEdits on by default - dotnet/razor#4408
I recall TextEdits making completion perf noticeably slower, the above task should help speed things up. It requires work in both Roslyn and the LSP client however.

[dibarbet]

tracking this in #59773