-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LSP] Completion/resolve computes text changes agains incorrect snapshot #60598
Comments
@333fred what did you do in this case? Do you rewind, get the change and map forward? |
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?
Formatting is not just indentation, so I think this depends on how formatting options work. eg, if C# .editorconfig says to format as 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. |
We resolve up front. Remember that vscode doesn't support async text edits, only additional text edits can be resolved. |
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.
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. |
Surprising, but I agree, it seems like we don't need to worry about that.
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 |
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. |
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. |
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
This calculates the completion list with provider data using a span to replace containing just
a
.Filter down to the
await
item by typingw
The list is not recalculated. A
completionItem/resolve
request comes in. We find the matchingawait
completion item. Now when we calculate the text change to insertawait
, the provider uses the range from the original list and tells us to replacea
withawait
leading the final text to beSolutions
cc @CyrusNajmabadi @NTaylorMullen
Deeper into the rabbit hole...
The text was updated successfully, but these errors were encountered: