Roslyn: Completion autocompletes keywords in Doc Comments in CodeSpaces

[gundermanc]

Version Used:

Steps to Reproduce:

1. Open a C# project
2. Over a function/method, start typing in a doc comment XML entry, such as a <paramref>.
3. Type certain words like 'List'.

Expected Behavior:
Completion is present only for referenced params, doc comment constructs etc.

Actual Behavior:
Typing certain words like 'List' seems to invoke completion with a non-empty list, resulting in words getting committed unexpectedly on period and space.

It seems like some of these words are keywords which are unexpectedly visible in the completion list.

This looks like one case:

[gundermanc]

Originally reported by @pieandcakes. cc: @AmadeusW

[AmadeusW]

@allisonchou @dibarbet looks like Roslyn should provide empty completion list

[CyrusNajmabadi]

We have completion in doc comments. But it looks like we don't do it on an identifier being typed. Does LSP let us know if the request was triggered for typing, versus explicit user request?

For example:

[CyrusNajmabadi]

Looks like we only do this for "completoin on typing" if the user typed a <:

c# if (trigger.Kind == CompletionTriggerKind.Insertion && trigger.Character != '<') { // With the use of IsTriggerAfterSpaceOrStartOfWordCharacter, the code below is much // too aggressive at suggesting tags, so exit early before degrading the experience return null; }

So something is weird here. Are these not 'insertion' completions?

[CyrusNajmabadi]

This appears to be a codespaces/LSP issue. The completion request we get is:

So we're getting an 'invoke' request, even though it should be a TriggerCharacter request. So roslyn is treating this as requested. We're acting as if the user hit ctrl-space and we're providing the appropriate items in that case.

@gundermanc @AmadeusW can you direct as appropriate?

[gundermanc]

@AmadeusW is probably best contact for this one. Any chance this is message synchronization related? Is the repro consistent, even if typing very slowly with multiple seconds between messages?

[dibarbet]

@CyrusNajmabadi
In the base LSP spec there is no difference between a manual invocation vs typing an identifier invocation - see CompletionTriggerKind - https://microsoft.github.io/language-server-protocol/specification#textDocument_completion

/**
     * Completion was triggered by typing an identifier (24x7 code
     * complete), manual invocation (e.g Ctrl+Space) or via API.
     */
    export const Invoked: 1 = 1;

VS used to not abide by the spec and pass us the Triggered with the TriggerCharacter, but that changed recently so that they respect the spec. @AmadeusW is adding a new extension property context._ms_invokeKind that will specify if it was explicitly requested, from typing, or from deletion.

So we'll have to update to respect that property once it is available.

A partially related issue we may encounter is that the LSP client requests completion on more than just the word start character depending on how fast the user types, but roslyn we don't provide results for those - http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp.Features/Completion/CompletionProviders/CompletionUtilities.cs,58 .
e.g. typing "int", client can request for all "i", "n", "t" if typing slow enough, but roslyn only provides results on "i". This can lead to weird stale completion issues if we use the proper trigger kind.

[CyrusNajmabadi]

A partially related issue we may encounter is that the LSP client requests completion on more than just the word start character depending on how fast the user types, but roslyn we don't provide results for those - http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp.Features/Completion/CompletionProviders/CompletionUtilities.cs,58 .
e.g. typing "int", client can request for all "i", "n", "t" if typing slow enough,

Why si this a problem? they asked about 'i', and we should provide completion on 'i'. it shouldn't matter what gets typed next. This is a core principle of async-completion. Whatever follows is merely filter characters.

Any other model means you can't trust muscle memory in terms of what will happen.

[CyrusNajmabadi]

VS used to not abide by the spec and pass us the Triggered with the TriggerCharacter, but that changed recently so that they respect the spec.

I see. I don't see any additional entries on that item. So i would say this is by-design for now until we get the data in on the context-object.

In the future, perhaps we should first add hte new item, then have people switch to it, then change the default? I can see pros/cons to the different approaches here. However, right now we seem to be in a bind as there's no way to get the right behavior because the new API isn't available.

[CyrusNajmabadi]

@AmadeusW is probably best contact for this one. Any chance this is message synchronization related? Is the repro consistent, even if typing very slowly with multiple seconds between messages?

Yes, it's consistent on my end. I don't even need to type multiple requests. I just type 'l' and we get the lack of trigger-character information.

[gundermanc]

Why si this a problem? they asked about 'i', and we should provide completion on 'i'. it shouldn't matter what gets typed next. This is a core principle of async-completion. Whatever follows is merely filter characters.

LSP spec is that a request is _supposed_ to happen on the server's text version at _enqueue time_. So, even if the user is typing quickly, the request sent when 'i' is typed should return results for 'i', not 'iterator', or whatever the more complete token is.

Our LSP client _should_ be maintaining that guarantee already in the local (non-LiveShare/CodeSpaces) scenario. If it's not, that's a bug for @AmadeusW.

There are a series of bugs when LiveShare comes into the picture though that can let an LSP request run on a _newer_ text version. I suspect this is what @dibarbet is referring to.

The goal of the LSP/LiveShare integration refactor is to fix this particular issue: https://microsoft.sharepoint.com/:w:/t/MS-LSP2/EdeqP_IoJvlJgnALJeCsh-sBeXZfpaFAqiYgCR3e_RL4jg?e=twVNvm.

[CyrusNajmabadi]

https://microsoft.sharepoint.com/:w:/t/MS-LSP2/EdeqP_IoJvlJgnALJeCsh-sBeXZfpaFAqiYgCR3e_RL4jg?e=twVNvm.

this link is unavailable. Can you put in a public location (or on devdiv.visualstudio.com)?

[dibarbet]

Why si this a problem? they asked about 'i', and we should provide completion on 'i'. it shouldn't matter what gets typed next. This is a core principle of async-completion. Whatever follows is merely filter characters.

Any other model means you can't trust muscle memory in terms of what will happen.

It may just be a bug on the editor side that they don't filter the list from "i", but I think they were expecting us to return items for "n" and "t" as well. So question for @AmadeusW: When we switch back to using the new property to retrieve the triggered on typing msKind, what is the contract for returning items? Should we be returning results for all typed chars (e.g. "n" and "t") instead of just "i" and filtering down?

@gundermanc I'm not sure there is necessarily a sync bug here - we're returning the right items for just "i", and nothing for "n" and "t" as expected, although the responses for "n" and "t" can return to the client first because it does no computation, just returns nothing.

[gundermanc]

we're returning the right items for "i", and nothing for "n" and "t" as expected

Oh I see, completion is a much simpler protocol than I was expecting and only takes a single character rather than a token like I would have expected.

This does seem like a bug on the platform side of things though. Requesting completions for 'i' if the user types 'int' probably makes sense because we can filter down that list based on the preceding characters in the buffer, but it doesn't look like it makes sense to ask for 'n' if it's not the first character of the word. Is that by design @AmadeusW?

the responses for "n" and "t" can return to the client first because it does no computation, just returns nothing.

JSON RPC has a message id for the response. I would think we should be able to tolerate out of order responses if the client is behaving correctly.