Extract a DocumentSymbolsService from the C# navbar provider #48912

333fred · 2020-10-26T05:36:24Z

This extracts a new IDocumentSymbolsService from the C# navbar item service, for use in both the navbar service and in LSP (breaking the DocumentSymbolHandler's dependency on EditorFeatures). As part of this, I attempted to resolve the issue around many-level hierarchy support, and introduced a new DocumentSymbolInfo class to allow recursively-defined symbols, including locals and local functions. The new service has 2 operation modes: the old navbar service mode, which returns types on the top level, and their top-level members as nested children, and a new hierarchy mode which supports full symbol nesting (including representing nested types as truly nested in their containing types).

I similarly implemented an IDocumentSymbolsService for VB, but I was not able to extract the logic out of the VB navbar service to use the new service. The VB navbar is too tightly coupled to the code generation capabilities it needs to represent many parts of the existing navbars (such as event or constructor generation). As such, while I've tested the VB provider, I don't believe I've exactly replicated the navbar behavior in the legacy mode for the service. I think I've come close, but handling around events and methods that have WithEvents is different.

This extracts a new IDocumentSymbolsService from the C# navbar item service, for use in both the navbar service and in LSP (breaking the DocumentSymbolHandler's dependency on EditorFeatures). As part of this, I attempted to resolve the issue around many-level hierarchy support, and introduced a new DocumentSymbolInfo class to allow recursively-defined symbols, including locals and local functions. The new service has 2 operation modes: the old navbar service mode, which returns types on the top level, and their top-level members as nested children, and a new hierarchy mode which supports full symbol nesting (including representing nested types as truly nested in their containing types). I similarly implemented an IDocumentSymbolsService for VB, but I was not able to extract the logic out of the VB navbar service to use the new service. The VB navbar is too tightly-coupled to the code generation capabilities it needs to represent many parts of the existing navbars (such as event or constructor generation). As such, while I've tested the VB provider, I don't believe I've exactly replicated the navbar behavior in the legacy mode for the service. I think I've come close, but handling around events and methods that have WithEvents is different.

333fred · 2020-10-26T05:41:03Z

@dibarbet there are still some open questions I think:

Should we even make the navbar service use this new document symbol provider, or leave it as is?
What should our behavior around lambdas be? I've excluded lambdas and any potential locals inside from this, as I don't know how we'd represent them in the hierarchy.
What should our behavior around partial types in the same file be? I haven't done any special behavior currently, so DocumentSymbolTests.TestGetDocumentSymbolsAsync__PartialMethods has some odd behavior.
What should be done about VB support? I don't know of any way the symbol handler could register a command to generate methods on click for LSP, are there any plans to handle this at all?
Should we include namespace symbols at all? I went with no, but it wouldn't be hard to add.
I made IDocumentSymbolsService public. I could certainly not make it public if that's preferred, but that was my default.

src/EditorFeatures/CSharp/NavigationBar/CSharpNavigationBarItemService.cs

CyrusNajmabadi · 2020-10-26T15:52:07Z

src/EditorFeatures/CSharp/NavigationBar/CSharpNavigationBarItemService.cs

+            {
+                return SpecializedCollections.EmptyList<NavigationBarItem>();
+            }
+
            var tree = await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);


Suggested change

var tree = await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);

var tree = await document.GetRequiredSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);

CyrusNajmabadi · 2020-10-26T15:52:34Z

src/EditorFeatures/CSharp/NavigationBar/CSharpNavigationBarItemService.cs

+            // If we got members from the symbol service, there had to have been a tree
+            Debug.Assert(tree is not null);


Suggested change

// If we got members from the symbol service, there had to have been a tree

Debug.Assert(tree is not null);

This is exported for C#. it's an invariant that C#/Vb always have syntax trees for all docs.

these comments both still apply.

CyrusNajmabadi · 2020-10-26T15:53:22Z

src/EditorFeatures/CSharp/NavigationBar/CSharpNavigationBarItemService.cs

            var compilation = document.Project.GetCompilationAsync(cancellationToken).WaitAndGetResult(cancellationToken);
+            // If we're being called back, there must have been a compilation to resolve the item in the first place
+            Debug.Assert(compilation is not null);


Suggested change

var compilation = document.Project.GetCompilationAsync(cancellationToken).WaitAndGetResult(cancellationToken);

// If we're being called back, there must have been a compilation to resolve the item in the first place

Debug.Assert(compilation is not null);

var compilation = document.Project.GetRequiredCompilationAsync(cancellationToken).WaitAndGetResult(cancellationToken);

Same thing as above. all C#/VB documents have compilations.

this comment still applies.

src/Features/CSharp/Portable/DocumentSymbols/CSharpDocumentSymbolInfo.cs

src/Features/CSharp/Portable/DocumentSymbols/CSharpDocumentSymbolsService.cs

src/Features/Core/Portable/DocumentSymbols/AbstractDocumentSymbolsService.cs

CyrusNajmabadi · 2020-10-26T16:02:30Z

src/Features/CSharp/Portable/DocumentSymbols/CSharpDocumentSymbolsService.cs

+            if (options == DocumentSymbolsOptions.TypesAndMethodsOnly)
+            {
+                return node is BaseMethodDeclarationSyntax or BasePropertyDeclarationSyntax
+                            or BaseFieldDeclarationSyntax or StatementSyntax or ExpressionSyntax;


it's a bit odd that it says 'TypesAndMethods', but talks about statements/expressions. how do those show as hte parent if we're only considering types/methods?

If we're only talking about types and methods, then there's no need to delve inside members. You can run into statements, such as at the top level, but if we see one we definitely know that there's nothing of interest inside it. I should also rename that enum to TypesAndMembers, it's not method specific.

src/Features/Core/Portable/DocumentSymbols/IDocumentSymbolsService.cs

CyrusNajmabadi · 2020-10-26T16:05:14Z

src/Features/LanguageServer/Protocol/Handler/Symbols/DocumentSymbolsHandler.cs

                }
            }

-            var location = symbol.Locations.FirstOrDefault(l => l.SourceTree.Equals(tree));
-            return location ?? symbol.Locations.FirstOrDefault();
+            return (range, selection);
        }
    }
 }


todo: review this.

src/Features/VisualBasic/Portable/DocumentSymbols/VisualBasicDocumentSymbolInfo.vb

src/Features/VisualBasic/Portable/DocumentSymbols/VisualBasicDocumentSymbolsService.vb

src/Features/Core/Portable/DocumentSymbols/IDocumentSymbolsService.cs

CyrusNajmabadi · 2020-11-03T07:39:22Z

src/Features/CSharp/Portable/DocumentSymbols/CSharpDocumentSymbolsService.cs

+                else
+                {
+                    AddFieldOrLocalSpan(symbol, tree, spans);
+                }


all these different cases are hard to read. ie. if it's a field or a local, except in teh case where it's an enum, otherwise do this.

Can you just break out hte separate cases and deal with each one individually?

CyrusNajmabadi · 2020-11-03T07:39:51Z

src/Features/CSharp/Portable/DocumentSymbols/CSharpDocumentSymbolsService.cs

+                return;
+            }
+
+            var declaringNode = reference.GetSyntax();


shouldn't this take a cancellation token?

CyrusNajmabadi · 2020-11-03T07:41:53Z

src/Features/Core/Portable/DocumentSymbols/AbstractDocumentSymbolsService.cs

+            typesBuilder.Sort((d1, d2) => d1.Text.CompareTo(d2.Text));
+            return typesBuilder.ToImmutable();
+
+            void GetTypesInFile(SemanticModel semanticModel, HashSet<INamedTypeSymbol> types, CancellationToken cancellationToken)


can you make this static, or extract to method? i can't really tell what is captured here and how this affects outer state.

CyrusNajmabadi · 2020-11-03T07:43:10Z

src/Features/Core/Portable/DocumentSymbols/AbstractDocumentSymbolsService.cs

+            }
+        }
+
+        protected static ImmutableArray<TextSpan> GetDeclaringSpans(ISymbol symbol, SyntaxTree tree)


Suggested change

protected static ImmutableArray<TextSpan> GetDeclaringSpans(ISymbol symbol, SyntaxTree tree)

protected static ImmutableArray<TextSpan> GetDeclaringSpansInTree(ISymbol symbol, SyntaxTree tree)

?

CyrusNajmabadi · 2020-11-03T07:43:44Z

src/Features/Core/Portable/DocumentSymbols/AbstractDocumentSymbolsService.cs

+            {
+                if (tree.Equals(location.SourceTree))
+                {
+                    return ImmutableArray.Create(location.SourceSpan);


what if teh symbol has multiple locaitons in the same tree? this will just return the first in Locations. Is that desirable/intentional? Regardless of answer, please doc.

CyrusNajmabadi · 2020-11-03T07:45:20Z

src/Features/Core/Portable/DocumentSymbols/IDocumentSymbolsService.cs

+        /// <see cref="IDocumentSymbolsService.GetSymbolsInDocumentAsync(Document, DocumentSymbolsOptions, CancellationToken)"/>,
+        /// with their top-level members returned as the <see cref="DocumentSymbolInfo.ChildrenSymbols"/> of those types.
+        /// No info on locals or local functions is returned. Members not in the given document can be returned. This format
+        /// matches the behavior of the Visual Studio toolbars.


intead of matching the nav bars, can we just return the full hierarchy, and let the navbar code iself collapse things based on the rich hierarchy data you're returning?

otherwise, i don't see much point here in thse abstractions if we effectively have a bool that says "i'm from navbar (or not)", and then code on the other side that says "oh, for navbar, we get a differnet set of results".

The DocumentSymbolHandler uses this mode if hierarchical support is disabled.

CyrusNajmabadi · 2020-11-03T07:46:14Z

src/Features/Core/Portable/DocumentSymbols/IDocumentSymbolsService.cs

+        /// The range enclosing this symbol not including leading/trailing whitespace
+	    /// but everything else like comments.
+        /// </summary>
+        public ImmutableArray<TextSpan> EnclosingSpans { get; }


why is it 'spans', instead of just one span?

CyrusNajmabadi · 2020-11-03T07:46:36Z

src/Features/Core/Portable/DocumentSymbols/IDocumentSymbolsService.cs

+        /// <summary>
+        /// The range that should be selected and revealed when this symbol is being picked.
+        /// </summary>
+        public ImmutableArray<TextSpan> DeclaringSpans { get; }


why is this an array, instead of one thing?

CyrusNajmabadi · 2020-11-03T07:47:16Z

src/Features/Core/Portable/DocumentSymbols/IDocumentSymbolsService.cs

+        /// <summary>
+        /// Any nested items from the document.
+        /// </summary>
+        public ImmutableArray<DocumentSymbolInfo> ChildrenSymbols { get; }


in general we either use Children or ChildXXX.

CyrusNajmabadi · 2020-11-03T07:47:30Z

src/Features/Core/Portable/DocumentSymbols/IDocumentSymbolsService.cs

+    /// <summary>
+    /// Information about the symbols in this document
+    /// </summary>
+    internal sealed class DocumentSymbolInfo


that this is not a record makes me sad.

I might be able to make this a record. I had originally intended to make this a struct, but when I did I got very weird test failures that DocumentSymbolInfo wasn't able to be loaded by the test assembly.

CyrusNajmabadi · 2020-11-03T07:48:35Z

src/Features/LanguageServer/Protocol/Handler/Symbols/DocumentSymbolsHandler.cs

-            var navBarItems = await navBarService.GetItemsAsync(document, cancellationToken).ConfigureAwait(false);
-            if (navBarItems.Count == 0)
+            var hierarchicalDocumentSymbolSupport = context.ClientCapabilities?.TextDocument?.DocumentSymbol?.HierarchicalDocumentSymbolSupport == true;
+            var documentSymbolsService = document.Project.LanguageServices.GetRequiredService<IDocumentSymbolsService>();


Suggested change

var documentSymbolsService = document.Project.LanguageServices.GetRequiredService<IDocumentSymbolsService>();

var documentSymbolsService = document.GetRequiredLanguageService<IDocumentSymbolsService>();

CyrusNajmabadi · 2020-11-03T07:49:17Z

src/Features/LanguageServer/Protocol/Handler/Symbols/DocumentSymbolsHandler.cs

            {
-                return symbols.ToArrayAndFree();
+                return symbols.ToArrayAndFree()!;


somethind i very odd with these suppressions

CyrusNajmabadi · 2020-11-03T07:50:10Z

src/Features/LanguageServer/Protocol/Handler/Symbols/DocumentSymbolsHandler.cs

            {
-                return symbols.ToArrayAndFree();
+                return symbols.ToArrayAndFree()!;
            }

            var compilation = await document.Project.GetCompilationAsync(cancellationToken).ConfigureAwait(false);


CyrusNajmabadi · 2020-11-03T07:50:41Z

src/Features/LanguageServer/Protocol/Handler/Symbols/DocumentSymbolsHandler.cs

        {
-            if (item.Spans.Count == 0)
+            if (item.DeclaringSpans.IsEmpty)


when can this happen?

CyrusNajmabadi · 2020-11-03T07:51:25Z

src/Features/VisualBasic/Portable/DocumentSymbols/VisualBasicDocumentSymbolsService.vb

+                                                                            Function(s) s.GetSyntax().FullSpan)
+        End Function
+    End Class
+


Suggested change

CyrusNajmabadi · 2020-11-03T07:51:37Z

src/Features/VisualBasic/Portable/DocumentSymbols/VisualBasicDocumentSymbolsService.vb

+        Private Shared Function GetEnclosingSpansForSymbol(
+                symbol As ISymbol,
+            tree As SyntaxTree,
+            declarationService As ISymbolDeclarationService) As ImmutableArray(Of TextSpan)
+            Return declarationService.GetDeclarations(symbol).SelectAsArray(Function(s) s.SyntaxTree.Equals(tree),


Suggested change

Private Shared Function GetEnclosingSpansForSymbol(

symbol As ISymbol,

tree As SyntaxTree,

declarationService As ISymbolDeclarationService) As ImmutableArray(Of TextSpan)

Return declarationService.GetDeclarations(symbol).SelectAsArray(Function(s) s.SyntaxTree.Equals(tree),

Private Shared Function GetEnclosingSpansForSymbol(

symbol As ISymbol,

tree As SyntaxTree,

declarationService As ISymbolDeclarationService) As ImmutableArray(Of TextSpan)

Return declarationService.GetDeclarations(symbol).SelectAsArray(Function(s) s.SyntaxTree.Equals(tree),

CyrusNajmabadi · 2020-11-03T07:51:52Z

src/Features/VisualBasic/Portable/DocumentSymbols/VisualBasicDocumentSymbolsService.vb

+        End Function
+    End Class
+
+End Namespace


todo: review his file.

CyrusNajmabadi · 2020-11-03T07:53:27Z

src/EditorFeatures/CSharp/NavigationBar/CSharpNavigationBarItemService.cs

@@ -28,23 +26,6 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.NavigationBar
    [ExportLanguageService(typeof(INavigationBarItemService), LanguageNames.CSharp), Shared]
    internal class CSharpNavigationBarItemService : AbstractNavigationBarItemService


i don't see the VisualBasicNavigationBarItemService change in this PR.

Correct, there is not.

CyrusNajmabadi

done with pass. would much prefer an approach where there's a single hierarchical provider. and both LSP and NavBars both consume that. Clients can collape the hierarchy to a flatter list if that's what they want, and can filter down to the types of items they want as well based on their own policies.

this would take policy judgements out of the service, and into the feature, and would make the service just responsible for hierarchical symbolic data.

also need to see how this impacts the VB side of things.

333fred · 2020-11-03T18:22:58Z

both LSP and NavBars both consume that.

I do not see a way in which the VB NavBar service could consume this. It's far too coupled to how events work in VB. I could certainly make the proposed structure work with the C# NavBar, but I don't think it's possible to preserve existing behavior in VB.

jasonmalinowski · 2020-11-03T18:30:55Z

@333fred: Last time I looked at this, the navigation bar item service is functioning in dual roles:

Fetching items.
Implementing the logic for navigating to those items.

The former doesn't really have any logical dependency to the editor code; the later does due to some tracking spans that it uses. Splitting it into two services where the former lives in Features and the latter lives in EditorFeatures I think should be doable.

CyrusNajmabadi · 2020-11-03T18:37:55Z

It's far too coupled to how events work in VB.

Can you clarify this? Can the new service return a different type of info that the feature layer can decide what to do with? the LSP versoin can dump them, the VS version can use them.

I do not see a way in which the VB NavBar service could consume this.

Then i don't see a purpose here for this (esp. as we need options to basically say "i want LSP behavior" vs "I want VS behavior"). If we're just going to have a special option for LSP, and we're not going to share this code with C#/VB for VS/LSP, then why not just have a custom LSP service only for that scenario.

In other words, i don't get what this system is buying us given htat we still hvae LSP vs VS, and we still have C# vs VB.

333fred · 2020-11-03T18:49:57Z

Can you clarify this? Can the new service return a different type of info that the feature layer can decide what to do with? the LSP versoin can dump them, the VS version can use them.

My concern is all the event-based logic here: http://sourceroslyn.io/#Microsoft.CodeAnalysis.VisualBasic.EditorFeatures/NavigationBar/VisualBasicNavigationBarItemService.vb,113. The logic around WithEvents and Handles is incredibly VB-specific, and I don't see how it can be exposed in a language-agnostic way without the VB NavBar service basically just using the service to get the types in the file then recomputing all the nested symbols it needs because it needs to determine whether they're WithEvents or Handles, dig out the events they handle, and reorder.

In other words, i don't get what this system is buying us given htat we still hvae LSP vs VS, and we still have C# vs VB.

Well, this does allow us to collapse LSP vs VS, for C#. I don't know if LSP is ever going to be able to replicate the VB behavior: the documentSymbols protocol is fundamentally designed around navigation, not around generation (which the VB NavBar service is designed around). I could certainly back off my attempt to make the C# navbar service consume this, though IMO this new service should still live in the Features layer (just feels like the proper layering, as LSP should be about providing an interface to IDE features, not implementing them itself).

@333fred: Last time I looked at this, the navigation bar item service is functioning in dual roles:
1. Fetching items.

2. Implementing the logic for navigating to those items.
The former doesn't really have any logical dependency to the editor code; the later does due to some tracking spans that it uses. Splitting it into two services where the former lives in Features and the latter lives in EditorFeatures I think should be doable.

In VB, these 2 steps are fundamentally conflated, because it needs to determine a bunch of information about generating items when fetching them.

CyrusNajmabadi · 2020-11-03T19:10:20Z

Well, this does allow us to collapse LSP vs VS, for C#. I don't know if LSP is ever going to be able to replicate the VB behavior:

I'm not asking for LSP to replicate it though. I'm asking for the core service to be a superset of functionality of LSP/VS/C#/VB. Then the particular consumers can decide how to expose that functionality to their platform. LSP, for example, would not expose the event-generation aspect. It would alos expose the full tree. VS would expose the event-generation, but would also collapse the tree to the flat form that it wants to show.

333fred · 2020-11-03T19:27:58Z

I'm asking for the core service to be a superset of functionality of LSP/VS/C#/VB.

I'm straight-up not certain how to do this though 🙂. If you have suggestions on an API shape that would be able to represent the logic that goes on inside the VB handlers without becoming very VB-specific, I'm all ears.

CyrusNajmabadi · 2020-11-03T19:36:58Z

So it looks like the enum items are pretty vanilla: https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/VisualBasic/NavigationBar/GenerateEventHandlerItem.vb

WHy not just return these as items that just have a IAmACodeGeneratingItemAsOpposedToANavigatingItem bit on them. LSP can filter these out. VS can show them. When selected, the item is passed back to the service to say "ok... this wsa selected, do your thang".

333fred · 2021-04-08T04:38:35Z

Superseded by #52481.

333fred requested review from a team as code owners October 26, 2020 05:36

Dotnet-GitSync-Bot added the Area-IDE label Oct 26, 2020

333fred requested a review from dibarbet October 26, 2020 05:37

333fred marked this pull request as draft October 26, 2020 05:37