Support hierarchical document symbols in LSP #4661

MikhailArkhipov · 2018-08-17T19:03:14Z

Per LSP 4.4+, VS Code 1.26+
microsoft/vscode-python#2384

Questions:

Do we need old DocumentSymbol method? VS does not appear to use it.
Just change IServer or keep method and make IServer2?
Better way to establish child-parent relations?

zooba · 2018-08-17T19:37:51Z

OutOfProcProjectAnalyzer.GetNavigations() should be reimplemented in terms of DocumentSymbol, but currently it's not using it.

It might be nice to add a recursion limit parameter here, firstly because it is probably possible to get an infinitely recursive structure (from analysis), but also just to save time/space when returning the symbols. VS is unlikely to request more than two levels deep, for example.

zooba · 2018-08-17T19:39:08Z

Python/Product/Analysis/LanguageServer/Server.WorkspaceSymbols.cs

                case PythonMemberType.Unknown: return SymbolKind.None;
-                case PythonMemberType.Class: return SymbolKind.Class;
-                case PythonMemberType.Instance: return SymbolKind.Object;
+                case PythonMemberType.Class: return m.Scope is FunctionScope ? SymbolKind.Variable : SymbolKind.Class;


This should be a constant mapping. If ~~nested~~inner classes have an incorrect member type, that is probably its own bug.

Alternatively, handle this extra check earlier, so that Scope doesn't have to be passed around here.

It a minor nit. In class method first argument comes as class although technically it is an argument in the signature. It looks a bit odd in the outline as a class inside a function. Or maybe its OK?

Well, you'd certainly want it to look like a class in this case, so perhaps the answer is to explicitly handle arguments differently?

def make_type(): class MyClass: pass return MyClass

zooba · 2018-08-17T19:40:01Z

Python/Product/Analysis/LanguageServer/Server.WorkspaceSymbols.cs

@@ -50,6 +51,14 @@ await GetModuleVariablesAsync(entry as ProjectEntry, opts, @params.query, 50)
                .ToArray();
        }

+        public async Task<DocumentSymbol[]> NewDocumentSymbol(DocumentSymbolParams @params, CancellationToken cancellationToken) {


Ah, I see, this is the newer one. Personally I'd prefer a suffix 2 or 3_10 (for the LSP version), or a descriptive prefix (HierarchyDocumentSymbol?)

Handle trace logging consistently in VSC

MikhailArkhipov · 2018-08-20T22:24:38Z

There are some problems rewriting GetNavigations since LS does not return types like staticmethod or classmethod, it returns SymbolKind. We can return them as additional custom fields though.

zooba

I think the logic for finding the hierarchy could be simpler if you start with an AST walk and then just go to the analysis for names you can't tell the type of. This is fundamentally what we do for analysis classifications - you may even be able to reuse that walker.

zooba · 2018-08-21T17:39:09Z

Python/Product/Analysis/LanguageServer/IServer.cs

+
+    public interface IServer2 : IServer {
+        Task<DocumentSymbol[]> HierarchicalDocumentSymbol(DocumentSymbolParams @params, CancellationToken cancellationToken);
+    }


We're likely to end up with a lot of IServer# interfaces if they only gain one method at a time... but maybe that's how we have to do it given how frequently this releases?

Arguably, if there's no user for it, maybe we could batch up interface additions?

Extension (IC) will be using it most probably since they need symbols with types.

Do they need to use it via the interface? Or are they downcasting to the Server class anyway?

They use IServer except there is some odd code that casts it to Server to see it is actual Python.

zooba · 2018-08-21T21:06:42Z

Python/Product/Analysis/LanguageServer/Server.WorkspaceSymbols.cs

+
+        private static string GetFunctionKind(MemberResult m) {
+            if (m.MemberType == PythonMemberType.Function) {
+                var funcDef = m.Values.FirstOrDefault(x => x is IPythonFunction)?.PythonType as FunctionDefinition;


This always results in null - you will want m.Values.OfType<FunctionInfo>() and then check their .IsStatic/.IsClassMethod/.IsProperty member.

zooba · 2018-08-21T21:09:29Z

Python/Product/Analysis/LanguageServer/Server.WorkspaceSymbols.cs

+            var topLevel = new List<MemberResult>();
+
+            foreach (var m in members) {
+                var parent = members.FirstOrDefault(x => x.Scope?.Node == m.Scope?.OuterScope?.Node && x.Name == m.Scope?.Name);


We should be able to avoid this check by more carefully collecting values from scopes, yes? And save adding scope references to the MemberResult struct?

Or arguably we can extract nearly enough information from an AST walk (the names at least), and then use a direct analysis lookup to resolve those that we don't know for sure (like we do in the analysis classifier).

Yes, I am actually going to write document symbols fetch using AST, but that is going to be September. microsoft/vscode-python#2430

MemberResult is a public API, so adding/removing a field will be a breaking change.

We'll need to revisit public surface going forward, add more interfaces since it gets harder to make changes as we go.

Can make MemberResult2 deriving from the base technically

AlexanderSher · 2018-08-23T19:43:55Z

Python/Product/Analysis/LanguageServer/Server.WorkspaceSymbols.cs

+            var opts = GetMemberOptions.ExcludeBuiltins | GetMemberOptions.DeclaredOnly;
+            var entry = ProjectFiles.GetEntry(@params.textDocument);
+
+            var members = await GetModuleVariablesAsync(entry as ProjectEntry, opts, string.Empty, 50);


cancellationToken

MikhailArkhipov added 3 commits August 16, 2018 16:27

Hierarchical outline

6a8043c

Merge branch 'master' of https://github.com/Microsoft/PTVS into hier

6304d81

Support hierarchy

0a869ed

zooba reviewed Aug 17, 2018

View reviewed changes

MikhailArkhipov added 4 commits August 20, 2018 09:34

PR feedback

80d6cac

Merge branch 'hier' of https://github.com/MikhailArkhipov/PTVS into hier

903e073

Add symbol depth limit

0e4ab8e

Handle trace logging consistently in VSC

Undo

69459c6

MikhailArkhipov changed the title ~~WIP: Support hierarchical document symbols~~ Support hierarchical document symbols in LSP Aug 20, 2018

MikhailArkhipov added 2 commits August 20, 2018 16:30

Use DocumentSymbol in PTVS

ec6d8e1

Add IServer2 interface

1dddbf2

MikhailArkhipov mentioned this pull request Aug 21, 2018

Update DocumentSymbolProviders to return DocumentSymbols microsoft/vscode-python#2100

Closed

zooba reviewed Aug 21, 2018

View reviewed changes

MikhailArkhipov added 3 commits August 22, 2018 09:49

PR feedback

e2ae6de

PTVS fix

b50ae3f

null check

6c91a24

AlexanderSher reviewed Aug 23, 2018

View reviewed changes

AlexanderSher approved these changes Aug 23, 2018

View reviewed changes

Add cancellation

f644447

MikhailArkhipov merged commit 91426f5 into microsoft:master Aug 23, 2018

This was referenced Aug 23, 2018

Build 2018.8.0-alpha0: outline: __init__ method attributes incorrect microsoft/vscode-python#2384

Closed

Language Server: Outline says No Symbols Found for the current doc. start type and symbols appear microsoft/vscode-python#2334

Closed

MikhailArkhipov deleted the hier branch August 23, 2018 22:26

Support hierarchical document symbols in LSP #4661

Support hierarchical document symbols in LSP #4661

Uh oh!

Conversation

MikhailArkhipov commented Aug 17, 2018

Uh oh!

zooba commented Aug 17, 2018

Uh oh!

zooba Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikhailArkhipov commented Aug 20, 2018

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zooba Aug 17, 2018 •

edited

Loading