Cohosting Support for Go to Definition #10750
Conversation
This change adds all of the boilerplate needed to handle C# Go to Definition
There was a problem hiding this comment.
I suspect you might have had to jump through slightly less hoops by using VS protocol types instead of Roslyn types, but it wouldn't be 0 hoops, and all of the hoops will go away eventually anyway.
Unfortunately, there are two well-hidden features in the old SingleServerDelegatingEndpoint and you've fallen into both traps. Feel free not to rearchitect the world to make these traps harder to fall into in this PR, but at the same time, I know what you're like 😛
Overall though, definitely looks good. I'd probably be happy to approve with skipped tests for those two features, and have them done in a follow up, if you were in a "re-architect the world" mood. I could see some better APIs on IDocumentMappingService to make that behaviour easier to access potentially.
...soft.CodeAnalysis.Razor.Workspaces/GoToDefinition/AbstractRazorComponentDefinitionService.cs
Outdated
Show resolved
Hide resolved
| var csharpText = codeDocument.GetCSharpSourceText(); | ||
| var syntaxTree = CSharpSyntaxTree.ParseText(csharpText, cancellationToken: cancellationToken); | ||
| var root = await syntaxTree.GetRootAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Totally fine to log as a follow up, but this is a great example of where we can make things much better for cohosting. eg, if we had an abstract method on the service to get the SyntaxRoot from the code document, and then have this method take the syntax root and RazorCSharpDocument as parameters, means we can avoid having to realize the file as a string and re-parse in cohosting.
There was a problem hiding this comment.
I definitely agree that we should do better. In this case, I was simply copying your original code from here. 😛
The approach that you described wasn't clear to me. I might be conflating terminology, but I think you're suggesting that we would use a RazorCodeDocument to retrieve the root for a C# SyntaxTree? That feels an odd mixture of currency to me.
There was a problem hiding this comment.
Thanks for chatting with me offline. I understand your suggestion now, but I'll hold off until we get things ported over. As you said in our chat, parity first.
src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/GoToDefinition/RemoteGoToDefinitionService.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/GoToDefinition/RemoteGoToDefinitionService.cs
Show resolved
Hide resolved
|
All right @davidwengier. Take two! |
Enjoy! |
That's totally fair. I think there's a likely future where this gets cleaned up further. After all, the |
…x tree (#10765) This PR is a classic self-nerd-snipe, and resolves this comment: #10750 (comment) Also inadvertently found a slight "bug" with the existing go to def code in cohosting. Bug is in quotes because the actual user behaviour probably was identical in 99% of cases.
Fixes #10631
Here's support for Go to Definition under co-hosting. Tests are still incoming (and I'll add them before merging), but I wanted to get this out for review to see if there's anything I missed or should implement differently.