Adding cohost completion handler #11048
Conversation
…letionItemProvider to common layer.
…ded by RemoteCompletionService
Switch passed in and returned types from Roslyn to VS Platform LSP types since that's what all of the common completion code in the Workspaces layer uses. We will need to convert returned Roslyn completion items to VS platform LSP completion items.
…et used in cohosting
…e workspaces layer (to be used in cohosting later)
…g HTML completion item labels to RazorCompletionListProvider
…r and minor cleanup
… layer and use it in cohost completion request
Also simplifying parameters passed to the response re-writers to only what's needed.
…hooked up and we are getting VSInternalCompletionContext in CompletionParams
…ng them They all already checked (or should've checked) for language and were operating on either C# or HTML, never on both. HTML re-writer will get called from the client and can be much simpler. In cohosting it doesn't make sense to have them all in one list since C# will get called in OOP and HTML on the client (in VS).
… it up in cohosting
Renamed some fields and variables dealing with "add snippets" options and added comments. We currently have two options that mean "add snippets" - one for the delegated completion, and one for Razor completion. The values of those don't correlate. The Razor one is always true in LSP and Cohost, always false for legacy editor. The delegation one actually depends on the position.
Also adding a snippet completion provider test and markup transition test
…ces test projects The tests were left behind (some in this PR, some in prior PRs) when the code was moved to Workspaces layer. This commit addresses most of them other than in Delegation subworkspace
We had inconsistent handling of null completion item labels between our response re-writers. Some handled null labels, others would through. Since label shouldn't be null (non-nullable), I adjusted the tests not to use null labels. Also the tests previously passed because they created DelegatedCompletionListProvider with only a selected DelegatedResponseRewriter. Now the DelegatedCompletionHelper will apply all response re-writers for the correct language (either C# or HTML), which is what the product actually does, so I feel that's fine. It exposed these test failures due to inconsistent null label handling
… in RemoteCompletionService
Switching AllTriggerCharacters to string[] since we only use it for registration/capability data, which needs string[]. and we never do look ups via Contains. Also removing rendundant property and calculations in CompletionListProvider
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/DocumentMapping/DocumentPositionInfo.cs
Outdated
Show resolved
Hide resolved
...zor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/Competion/CompletionPositionInfo.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionEndpoint.cs
Outdated
Show resolved
Hide resolved
...Analysis.Razor.Workspaces/Completion/Delegation/DelegatedCSharpCompletionResponseRewriter.cs
Outdated
Show resolved
Hide resolved
...c/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/Delegation/DelegatedCompletionHelper.cs
Outdated
Show resolved
Hide resolved
| @@ -10,12 +10,12 @@ | |||
|
|
|||
| <ItemGroup> | |||
| <None Include="xunit.runner.json" CopyToOutputDirectory="PreserveNewest" /> | |||
| <Compile Include="..\OSSkipConditionFactAttribute.cs" LinkBase="Shared" /> | |||
There was a problem hiding this comment.
This seems related to the build failure. I'm guessing it was removed because of the new project reference? Perhaps instead of referencing a test project from another, whatever is needed from it could be move to the common project?
| TriggerKind = RoslynCompletionTriggerKind.TriggerCharacter | ||
| }, | ||
| expectedItemLabels: ["char", "DateTime", "Exception"], | ||
| expectedItemCount: 996); |
There was a problem hiding this comment.
Is validating the count valuable? Seems pretty fragile.
There was a problem hiding this comment.
As I mentioned in one of the comments, Roslyn doesn't seem to always return all items initially (possibly related to our async code generation or something like that?). Waiting for expected number of items ensures we are waiting for everything to be ready (not sure if there is a better event to wait for?). I agree it may end up being fragile. If it is, we could switch from checking for exact match to checking that returned count is greater than the baseline number. I'd like to leave it as is for now and see if it causes issues, in which case we can modify the check.
| TriggerCharacter = "@", | ||
| TriggerKind = RoslynCompletionTriggerKind.TriggerCharacter | ||
| }, | ||
| expectedItemLabels: ["using", "using directive ...", "page", "page directive ..."], |
There was a problem hiding this comment.
Should this include a couple of C# completion items too, just to prevent regressions?
There was a problem hiding this comment.
A shower thought: Slightly at odds perhaps with other comments in this file, but I think given that we own the directive completions, it would not be a bad thing if this test included all of the results we expect our directive completion to produce. Would be good for preventing regression, and identifying FUSE issues in future when we switch over to it formally.
| public async Task CSharpClassesAtTransition() | ||
| { | ||
| await VerifyCompletionListAsync( | ||
| input: $""" |
There was a problem hiding this comment.
All of the raw strings in this file are interpolated, none of the raw strings in this file contain any interpolation 😁
| Context = completionContext | ||
| }; | ||
|
|
||
| // Roslyn doesn't always return all items right away, so using retry logic |
There was a problem hiding this comment.
This is very surprising. Is this copied from the existing tests, or is this something you are seeing in these tests specifically?
There was a problem hiding this comment.
I did see it in these tests specifically. E.g. at the "@" I initially got something like 556 items, but later 1000 items (max items returned by Roslyn). Adding re-try made tests run reliably. I am not sure why.
There was a problem hiding this comment.
Seems like that could be an issue with our test code, but not sure. Were the items we were expecting not present when it return 556? Seems like a fair number of completion items
Summary of the changes
I tried making each commit small and serving a single purpose, so looking commit-by-commit could be helpful, though returned data in each individual commit is not going to be correct, especially in the first few commits. A lot of merging, filtering and ordering was added in the last few commits to ensure correct final completion list is returned.
One unfortunate thing here is conversion between VS and Roslyn LSP types that we have to do until we move to Roslyn LSP types everywhere in our code. Completion code uses LSP types throughout most of the common code much more so than other requests, and while we need common code for non-cohost LSP endpoint, most of it has to use VS LSP types for now.
Fixes: #10697