Make completion capabilities checks more robust #11964
Conversation
This fixes the bug, where we used to not set any data if the delegated server didn't, now we set the one the client expects.
DustinCampbell
left a comment
DustinCampbell
left a comment
There was a problem hiding this comment.
Is it possible to add a regression test?
|
|
||
| public static bool SupportsCompletionListItemDefaultsData(this ClientCapabilities clientCapabilities) | ||
| => clientCapabilities.TextDocument?.Completion?.CompletionListSetting?.ItemDefaults is { } defaults && | ||
| Array.IndexOf(defaults, "data") >= 0; |
There was a problem hiding this comment.
I assume it was to get away from using LINQ on an array.
There was a problem hiding this comment.
Yeah, the old code used Contains, but this file was using Array.IndexOf, so I just followed suit 🤷♂️
| if (clientCapabilities.SupportsAnyCompletionListData()) | ||
| { | ||
| if (completionList.Data is not null) | ||
| if (clientCapabilities.SupportsCompletionListData() || completionList.Data is not null) |
There was a problem hiding this comment.
"Valid" is a funny word when it comes to LSP and serialization. If a client said they didn't support Data on the completion list, then presumably it would have no use as they would never read it, but wouldn't be illegal for someone to send it across the wire. Extra JSON data is not an error in any scenario I've ever seen.
The extra check here is essentially just paranoia: It would take a bad server (sending data back when the client says they don't want it) and a bad client (reading data when they said they didn't), for this to be necessary, but at the end of the day if this method doesn't wrap every bit of data it sees, its a bug waiting to happen. And we hit that bug in lots of our tests, which essentially act as LSP clients, but are exceedingly badly written as far as LSP clients go :)
Good shout, I improved the validation in the resolve test to catch the bug, and expanded it to test VS and VS Code forms. |
4 participants
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2508507
I accidentally broke Html completion when I fixed override completion in VS Code. Previously we would always wrap the Data object, I made it only wrap if the delegated server specified it, but Html (in VS code at least?) doesn't specify any data, which meant we couldn't work out what document the request was for. This fixes it by correctly being guided by the client capabilities as to which data object should be set.