search

PowerShell / PowerShellEditorServices

A common platform for PowerShell development support in any editor or application!
MIT License
622 stars 213 forks source link

Integrating class symbol support #1984

Closed andyleejordan closed 1 year ago

andyleejordan commented 1 year ago

This is the full (admittedly still a bit of a work-in-progress PR) that integrates https://github.com/PowerShell/PowerShellEditorServices/pull/1886 into main.

fflaten commented 1 year ago

This is coming along very well! I'll try to consolidate my thoughts in this PR 🙂

This PR would also fix #1889 and #1904 🎉 image

fflaten commented 1 year ago

Latest commit fixed the reference-issue with large files 🎉 I've updated the previous post

andyleejordan commented 1 year ago

Ok @fflaten and @SeeminglyScience try it out!

Need to filter on symboltype when searching symbols.

I'm not quite sure how to do this so that it's totally right. Want to punt until after this PR and come fix it for me? 😄

Consider matching [classname]::new() with classname() constructors?

Definitely punting this.

Only show variable assignment symbols for top-level assignments like stable?

Any idea how to do this? I talked with Patrick and there's not really a right way to distinguish between the initial assignment and later assignments, they all just trigger VisitVariableExpression and then we set isDeclaration: variableExpressionAst.Parent is AssignmentStatementAst or ParameterAst.

Outline, workspace symbol search and go to definition jumps

I set workspace and definition locations to NameRegion and multiple-definitions still work (I kinda liked it the other way around lol) but am not sure how to fix jumping from the outline view because it needs the whole range in order to figure out what's a child of what. Once we backlink symbols to their parents in the future, that's easily fixed, but for now 🤷

I just want to get the tests passing and get this merged at this point so we can put it in preview, and you can open PRs all you want to it 🥲

fflaten commented 1 year ago

Ok @fflaten and @SeeminglyScience try it out!

Awesome. Just had a few minutes now, but will give it a good try later today.

Need to filter on symboltype when searching symbols.

I'm not quite sure how to do this so that it's totally right. Want to punt until after this PR and come fix it for me? 😄

See comment. Can also be fixed later 👍

Only show variable assignment symbols for top-level assignments like stable?

Any idea how to do this? I talked with Patrick and there's not really a right way to distinguish between the initial assignment and later assignments...

I agree. Was thinking more about hiding assignments outside top-level (or all?) in a few handlers to avoid a complete mess. At least in WorkspaceSymbolsHandler as searching for name or result will return hundreds of results 😅 Outline has a tree-view, but could be noisy.

I'm 50/50 on go to definition as we show all references in stable and currently in PR return all method/constructors regardless of overload. So maybe more consistent to keep all variable assignments in that handler regardless of noise?

If we'd want to stick with top-level only anywhere my PoC earlier was something like this:

// SymbolReference.cs
        public bool IsDeclaration { get; init; }

+       public SymbolReference? Parent { get; internal set; }

// ReferenceTable.cs
    private bool _isInited;

+   private SymbolReference _previousSymbol;

...
    private AstVisitAction AddReference(SymbolReference symbol)
    {
        // We have to exclude implicit things like `$this` that don't actually exist.
        if (symbol.ScriptRegion.IsEmpty())
        {
            return AstVisitAction.Continue;
        }

+        // Find parent node
+        for (SymbolReference? p = _previousSymbol; p is not null; p = p.Parent)
+        {
+            if (p.ScriptRegion.ContainsPosition(symbol.ScriptRegion.StartLineNumber, symbol.ScriptRegion.StartColumnNumber))
+            {
+                symbol.Parent = p;
+                break;
+            }
+        }
+        _previousSymbol = symbol;

        _symbolReferences.AddOrUpdate(
...

// In handler
+    if (r.SymbolType is SymbolType.Variable && r.Parent is not null)
+    {
+        continue;
+    }

Outline, workspace symbol search and go to definition jumps

I set workspace and definition locations to NameRegion and multiple-definitions still work (I kinda liked it the other way around lol) but am not sure how to fix jumping from the outline view because it needs the whole range in order to figure out what's a child of what. Once we backlink symbols to their parents in the future, that's easily fixed, but for now 🤷

Outline just needs to start on the name. There's no children between keyword and symbolname so should be fine as temp solution. See comment.

I just want to get the tests passing and get this merged at this point so we can put it in preview, and you can open PRs all you want to it 🥲

I just got a flashback to writing the old PR - and I completely support this. 😄 We'll be able to iron out the rest in preview or before.

fflaten commented 1 year ago

Tested (no code review). I think it looks good - excellent work @andschwa !

I'd prefer to get the symbol type matching in now as that's a straight up bug. See review comment, incl. update about using the last where-epxression for all four places.

As for the variable assignment-discussion we can see what people say in preview-release. I'm curious and will look at python extension to see how they've approached it 🙂

andyleejordan commented 1 year ago

Ok @fflaten got that taken care of, and I fixed a bug I found when @SeeminglyScience decided to skip children when visiting parameters. I removed that and instead skip parameters in the variable expression visitor (so they're not double visited), but we do want to visit the children (specifically the type constraints!).

andyleejordan commented 1 year ago

Thanks @fflaten and @SeeminglyScience, resolved those things!