Port completion resolve to cohosting #11648
Conversation
We need to take a document for Roslyn to route the request to our server, even if we never use it otherwise
.. and completion list is a particularly painful type to serialize
| @@ -19,23 +20,14 @@ internal static Document GetRequiredDocument(this Project project, DocumentId do | |||
| ?? ThrowHelper.ThrowInvalidOperationException<Document>($"The document {documentId} did not exist in {project.Name}"); | |||
| } | |||
|
|
|||
| public static bool TryGetCSharpDocument(this Project project, Uri csharpDocumentUri, [NotNullWhen(true)] out Document? document) | |||
| public static Task<SourceGeneratedDocument?> TryGetCSharpDocumentFromGeneratedDocumentUriAsync(this Project project, Uri generatedDocumentUri, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
Could you refresh my memory? Why do we get the source generated document and call into our Roslyn EA on the client-side rather than the server-side? I’ve probably already seen this and am just not remembering.
There was a problem hiding this comment.
This method is used both in OOP and devenv. Mostly in OOP, but diagnostics and code actions have to happen in devenv for C#, and then we transfer the results over to OOP for processing.
...isualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostDocumentCompletionEndpoint.cs
Outdated
Show resolved
Hide resolved
| var completionCapability = _clientCapabilitiesService.ClientCapabilities.TextDocument?.Completion as VSInternalCompletionSetting; | ||
|
|
||
| var textDocument = JsonHelpers.ToVsLSP<TextDocumentIdentifier, RoslynTextDocumentIdentifier>(originalTdi).AssumeNotNull(); | ||
| var razorDocumentIdentifier = new TextDocumentIdentifierAndVersion(textDocument, Version: 0); |
There was a problem hiding this comment.
Is "Version: 0" going to bite us later? If so, is there an issue tracking work to fix it?
There was a problem hiding this comment.
No, versions aren't used in cohosting at all, since things run off a snapshot, but the context object stored in the cache is shared. This can likely be cleaned up in future to just store the document identifier in the context, and pass the version number to the things that need it in the language server. I actually think storing a possibly-old version number has caused us bugs in the past, but I didn't want to break into that jail in this PR.
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListMerger.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/RazorCompletionResolveData.cs
Show resolved
Hide resolved
# Conflicts: # src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListCache.cs # src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/ProjectExtensions.cs # src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/SolutionExtensions.cs # src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Remote/IRemoteCompletionService.cs # src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/Completion/RemoteCompletionService.cs # src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostDocumentCompletionEndpoint.cs # src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/Delegation/DelegatedCompletionItemResolverTest.NetFx.cs # src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/CohostDocumentCompletionEndpointTest.cs # src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.Test/Cohost/CohostRenameEndpointTest.cs
The data here used to be serialized by side effect when converting between LSP types
First two commits are from #11619, and like that PR this won't merge (or pass tests) until dotnet/roslyn#77587 merges and is available, but the code won't need to change, and I've manually tested and everything works, and all tests pass on my machine. The power of "Trust me bro!"
Fixes #11101
Fixes #11138
Fixes #10697
Fixes #10947
I managed to stop myself doing anything other than porting resolve, but I learned a lot doing it, most of which horrified me, so will hopefully be able to do more cleanup later. Would be nice if we got to a state where I didn't have to keep saying "this doesn't build yet" first though :P
Commit at a time review will make it easier, but I admit I didn't do a great job here, and the 2nd last commit is definitely in "draw the rest of the owl" territory. This code was written, ported to a branch, then ported to another branch, then written more and by two different people, so it didn't stand much of a chance I'm afraid.