[LSP] Completion/resolve computes text changes agains incorrect snapshot

[dibarbet]

So far I've only discovered an actual bug with this in a branch to support await completion in Razor, but the problem exists in general and may be causing issues in the wild.

Problem

Essentially the problem boils down to the fact that our normal completion service GetChangeAsync expects to be passed in a document with text corresponding to the original invocation snapshot and not the snapshot containing the filtered text. In non-LSP completion, we accomplish this by passing the original trigger snapshot, then mapping forward the calculated text changes with tracking spans.

However, completionItem/resolve in LSP passes in the current LSP document that contains the filter text - https://sourceroslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/LanguageServer/Handlers/Completion/CompletionResolveHandler.cs,139 and results in incorrect text edits being returned. For example the async/await provider is implemented according to the contract that the document passed in is the same as the one the list was calculated on.

We have no tracking spans in LSP and didChange does not guarantee that changes map to user typing so the same solution as classic is not viable.

Example

Invoke completion on a

Task M()
{
    a
}

This calculates the completion list with provider data using a span to replace containing just a.

Filter down to the await item by typing w

Task M()
{
    aw
}

The list is not recalculated. A completionItem/resolve request comes in. We find the matching await completion item. Now when we calculate the text change to insert await, the provider uses the range from the original list and tells us to replace a with await leading the final text to be

Task M()
{
    awaitw
}

Solutions

1. Always recompute the entire list in resolve and match items by label. Might be problematic as resolve blocks the UI and completion allowed to be slightly slow. We used to do this but changed in https://github.com/dotnet/roslyn/pull/49762 - I think because the data object being serialized was large. But may no longer be an issue with list data.
2. Store the old document along with the cached completion list and attempt to map the changes forward without tracking spans. Seems theoretically possible if we can constrain the amount of changes we might have to remap.
3. Add a new API for complex text edits to allow providers to opt-in to calculating text changes on an updated document snapshot - then in resolve only ask those providers. One downside here is that Razor is forcing us to provide resolve completions for non-complex text edits (like cast completion) due to not having the ability to performantly format additional text edits in textDocument/completion. And cast completion currently relies on original document state.
   1. Note from Cyrus - we think the cases that Razor actually needs to format a completion item response should be minimal - essentially only multi-line changes like override completion. Things like async/await completio just insert tokens which shouldnt need formatting? cc @davidwengier . If that is true providing all non-complex edits upfront is reasonable, and then this solution works.

cc @CyrusNajmabadi @NTaylorMullen

Deeper into the rabbit hole...

[dibarbet]

@333fred what did you do in this case? Do you rewind, get the change and map forward?

[davidwengier]

we think the cases that Razor actually needs to format a completion item response should be minimal

Does this hold with semantic snippets in the mix? Also isn't completion an extension point in Roslyn, and therefore this would be a problem with C# LSP (ie, not just razor) for any number of community completion providers?

which shouldn't need formatting?

Formatting is not just indentation, so I think this depends on how formatting options work. eg, if C# .editorconfig says to format as ((byte)b) but Razor .editorconfig says to format as ( (byte) b ), how will that work? Is Roslyn going to format the completion result that way because of some magic? Or does Razor need to format it after we get back the edit?

Note that I don't see either of the above necessarily as blockers, and I'm happy to move something forward in even a small way, just thinking out loud. I must admit, assuming I read and understood things correctly, number 2 was the first thing that came to mind, since it seems to match the local scenario.

[333fred]

@333fred what did you do in this case? Do you rewind, get the change and map forward?

We resolve up front. Remember that vscode doesn't support async text edits, only additional text edits can be resolved.

[dibarbet]

we think the cases that Razor actually needs to format a completion item response should be minimal

Does this hold with semantic snippets in the mix? Also isn't completion an extension point in Roslyn, and therefore this would be a problem with C# LSP (ie, not just razor) for any number of community completion providers?

which shouldn't need formatting?

Formatting is not just indentation, so I think this depends on how formatting options work. eg, if C# .editorconfig says to format as ((byte)b) but Razor .editorconfig says to format as ( (byte) b ), how will that work? Is Roslyn going to format the completion result that way because of some magic? Or does Razor need to format it after we get back the edit?

So I think this may be fine, hear me out. Consider the case you mentioned where casts should have spacing. Today in Roslyn completion we already do not format this according to the formatting option. Instead when you insert this and continue typing, once you hit a semicolon we do go back and format it according to the rules. See gif

It seems like this could be true for razor too - where completion generally doesn't do much other than fixup indentation for multi-line edits, and handle everything else via format on type. Now you mention third party providers which is totally fair - we may want to expose this IsComplexEdits flag and pass that along to razor to tell you when you might have to format.

Note that I don't see either of the above necessarily as blockers, and I'm happy to move something forward in even a small way, just thinking out loud. I must admit, assuming I read and understood things correctly, number 2 was the first thing that came to mind, since it seems to match the local scenario.

Yeah - however 2 is significantly easier in the existing editor because of tracking spans. We can't easily replicate them in LSP because didCHange does not necessarily correspond to exact typing. The client is free to basically send us whatever it wants. We may be able to do something simple if we rely on client behavior for completion resolve.

[davidwengier]

So I think this may be fine, hear me out

Surprising, but I agree, it seems like we don't need to worry about that.

We can't easily replicate them in LSP because didCHange does not necessarily correspond to exact typing

Is it too risky to assume that it does though? If some other change occurred, then surely the client would have re-request completion, rather than just completion resolve, in the same way that edits cause it to throw away old formatting responses etc. I can't imagine that any change between a completion and a completion resolve would be allowed, other than filtering.

[dibarbet]

Is it too risky to assume that it does though? If some other change occurred, then surely the client would have re-request completion, rather than just completion resolve, in the same way that edits cause it to throw away old formatting responses etc. I can't imagine that any change between a completion and a completion resolve would be allowed, other than filtering.

Right - I think I could come up with something reasonable that assumes that the client reasonably behaves around not re-using old completion lists / cancelling and re-querying when 'too much' changes

[dibarbet]

A 4th potentially crazy idea - have the client do the mapping forward based on some kind of property we send back in resolve. The client has all the knowledge of tracking spans to map things forward.

Drawbacks - would require a new property which is probably not applicable to public LSP. But then again we don't resolve on commit in public LSP anyway.

[dibarbet]

Actually this is sounding less and less crazy - we can send back text edits that are versioned - https://microsoft.github.io/language-server-protocol/specification#textDocumentEdit

If the client gets an edit from an older version, it knows it needs to map it forward.