Expose document symbols to Razor cohosting #74730
Conversation
| @@ -25,7 +25,7 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler | |||
| /// </summary> | |||
| [ExportCSharpVisualBasicStatelessLspService(typeof(DocumentSymbolsHandler)), Shared] | |||
| [Method(Methods.TextDocumentDocumentSymbolName)] | |||
| internal sealed class DocumentSymbolsHandler : ILspServiceDocumentRequestHandler<RoslynDocumentSymbolParams, object[]> | |||
| internal sealed class DocumentSymbolsHandler : ILspServiceDocumentRequestHandler<RoslynDocumentSymbolParams, SumType<RoslynDocumentSymbol[], SymbolInformation[]>> | |||
There was a problem hiding this comment.
could this be SumType<DocumentSymbol[], SymbolInformation[]> (and return RoslynDocumentSymbol) or does that not work?
There was a problem hiding this comment.
That's what I had at first, but the Document Outline tests failed. Guessing it is missing a converter, but it wasn't clear to me why that uses LSP at all, so didn't want to poke the bear :)
There was a problem hiding this comment.
I would guess the document outline tests just need to request the base type, and then cast the result it gets back here https://github.com/dotnet/roslyn/blob/main/src/VisualStudio/CSharp/Test/DocumentOutline/DocumentOutlineTestsBase.cs#L89
There was a problem hiding this comment.
It's an actual serialization issue, the Glyph property is just lost. I could probably add a call to AddOrReplaceConverter<DocumentSymbol, RoslynDocumentSymbol>(); here though if you like?
There was a problem hiding this comment.
if that works, fine with me. otherwise OK with this too
There was a problem hiding this comment.
I added the converter, seems to work fine. It's cleaner on the Razor side too, which is nice.
There was a problem hiding this comment.
lol, that caused a different set of test failures 🤦♂️
| public static async Task<SumType<DocumentSymbol[], SymbolInformation[]>> GetDocumentSymbolsAsync(Document document, bool useHierarchicalSymbols, CancellationToken cancellationToken) | ||
| { | ||
| // The symbol information service in Roslyn lives in EditorFeatures and has VS dependencies. for glyph images, | ||
| // so isn't available in OOP. The default implementation is available in OOP, but not in the Roslyn MEF composition, |
There was a problem hiding this comment.
but not in the Roslyn MEF composition
Hmm - not sure why this isn't the case. I think its regular workspace service. Not 100% sure but I would have thought it should be available.
There was a problem hiding this comment.
oh is the protocol project not in the OOP mef composition?
There was a problem hiding this comment.
Yes, precisely. That might come up again in other endpoints so could be something we need to talk about in future, but document symbol didn't seem important enough to block on it
| { | ||
| var document = context.GetRequiredDocument(); | ||
| var clientCapabilities = context.GetRequiredClientCapabilities(); | ||
| var useHierarchicalSymbols = clientCapabilities.TextDocument?.DocumentSymbol?.HierarchicalDocumentSymbolSupport == true || request.UseHierarchicalSymbols; |
There was a problem hiding this comment.
It seems weird that even if the client doesn't support HierarchicalSymbols we'd still use it if the request said to?
There was a problem hiding this comment.
The Document Outline window in Roslyn calls the document symbols LSP endpoint, and wants hierarchical results all the time, but the VS LSP client doesn't support them. In the LSP spec the non-hierarchical result is deprecated and I should probably log an issue on the platform to move off it, and then a lot of this nonsense can go away.
There was a problem hiding this comment.
this is for document outline. base vs client doesn't have hierarchical support IIRC, but we need it for our custom doc outline support.
Fixes #10689 Roslyn side: dotnet/roslyn#74730 Looks good, despite the Roslyn side having VS deps for images: 
Roslyn side of dotnet/razor#10689
Razor side: dotnet/razor#10728
Slightly annoying having to work around the VS dependency for images, but seems to look good in VS regardless:
