Go To Definition works with different Ast types #706

TylerLeonhardt · 2018-07-18T06:12:13Z

The issue was that when you hit F12 (aka go to definition) on any of the following scenarios except for the first one, they would fail to find the definition.

$var1 = "string"
$var1

[string] $var2 = "string"
$var2

[NJsonSchema.Annotations.NotNull()] $var3 = "string"
$var3

$a, $var4 = "string", "sss"
$var4

$a, [string] $var5 = "string", "sss"
$var5

All these scenarios now work with this PR.

We needed to recurse the AST to find the VariableExpressionAst because it could be nested in another Ast type.

SeeminglyScience

This is great! Awesome work 😄

Just a couple optional suggestions for optimizing.

SeeminglyScience · 2018-07-18T11:27:00Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+            List<VariableExpressionAst> asts = FindMatchingAsts(assignmentStatementAst.Left, variableName);
+            if (asts.Count > 0)
+            {
+                FoundDeclaration = new SymbolReference(SymbolType.Variable, asts.FirstOrDefault().Extent);


Will this only check the first variable when the lhs is an array literal? If so we should enumerate until we find a match or the list is completed.

May be worth looking at this method (in typical fashion, it's private, but still may be helpful):
https://github.com/PowerShell/PowerShell/blob/a3786158ca51cd65388743f900b69ec9e253c3d9/src/System.Management.Automation/engine/parser/ast.cs#L7367-L7379

@SeeminglyScience that will grab the first in an array literal, I guess. But we probably want the LAST hit from an array literal.

$a, [string] $b = "asdf", 5, $b = 26, 5 $b

FindMatchingAsts for $b will give the 2 hits in a list...

we want $b to jump to $b = 26 not [string] $b = "asdf" so I'll have it grab the last instead of the first - good catch.

@rjmholt looks like they went iterative. I like that, but it's a little extra code when we could just continue to recurse (since we're already doing it). Thanks for the reference though :) let me know if you feel strongly otherwise

I prefer the clarity of recursion actually. But thought it was worth noting that an implementation already exists

@tylerl0706 Initially I thought the method was just returning all VariableExpressionAst's, so my comment doesn't make a lot of sense :P

That said, I would stick with getting the first assignment.

Take this example

$a, [string] $b, $b = 1, 2, 3

Here we know with certainty what the value of the RHS is. So we can assume that the value of $b is going to be 3. However, the declaration is still the middle variable. If you run that snippet, $b is typed as a string even though the value is 3. We also won't know in most cases what the value or length of the RHS will be, so it's not a safe assumption that it will have any value.

It'll also save a little bit of processing time in some (very) fringe cases (assuming the switch to the yield style generalized method)

SeeminglyScience · 2018-07-18T11:27:52Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

                    variableName,
                    StringComparison.OrdinalIgnoreCase))
+                {
+                    return new List<VariableExpressionAst>{ variableExprAst };


Neat! I didn't know you could omit the parenthesis 😃

SeeminglyScience · 2018-07-18T11:29:45Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+
+            // VaraibleExpressionAst case - aka base case. This will return a list with the variableExpressionAst in it or an empty list if it's not the right one.
+            var variableExprAst = ast as VariableExpressionAst;
+            if (variableExprAst != null)


Since the variable will only be used within the if block, an is assignment could be used here instead. (e.g. if (ast is VariableExpressionAst variableExprAst) {})

Or possibly an assignment switch

switch (ast) { case ConvertExpressionAst convertExpression: { FindMatchingAsts(convertExpressionAst); break; } }

Yeah I reckon a type switch is probably clearest here -- it's probably how the PowerShell compiler itself would work if it were written today.

I saw a bunch of articles that said this wasn't possible hmm - I'll give it a go

The MS Docs seem to call it pattern matching. (Which is guess is approximately true...)

Ohhhh, but this may not be possible in all the .NET versions we support

(Historical note: We think this should work)

SeeminglyScience · 2018-07-18T11:40:40Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

            {
-                return AstVisitAction.Continue;
+                return (arrayLiteralAst.Elements.SelectMany(e => FindMatchingAsts(e, variableName)).ToList());


This isn't super important but this method might be a lot more efficient in assignments with a lot of decoration if it used yield.

If you change the return type to IEnumerable<VariableExpressionAst> and change the other return statements to something like

foreach (var foundVariableAst in FindMatchingAsts(convertExprAst.Child)) { yield return foundVariableAst; } yield break;

Then you could remove the variable name parameter and make this a more general utility function. You could then do the actual test in the caller while still iterating only as much as you need to.

rjmholt · 2018-07-18T16:26:47Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+            List<VariableExpressionAst> asts = FindMatchingAsts(assignmentStatementAst.Left, variableName);
+            if (asts.Count > 0)
+            {
+                FoundDeclaration = new SymbolReference(SymbolType.Variable, asts.FirstOrDefault().Extent);


May be worth looking at this method (in typical fashion, it's private, but still may be helpful):
https://github.com/PowerShell/PowerShell/blob/a3786158ca51cd65388743f900b69ec9e253c3d9/src/System.Management.Automation/engine/parser/ast.cs#L7367-L7379

rjmholt · 2018-07-18T16:28:59Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+
+            // VaraibleExpressionAst case - aka base case. This will return a list with the variableExpressionAst in it or an empty list if it's not the right one.
+            var variableExprAst = ast as VariableExpressionAst;
+            if (variableExprAst != null)


Yeah I reckon a type switch is probably clearest here -- it's probably how the PowerShell compiler itself would work if it were written today.

rjmholt · 2018-07-18T16:29:37Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+        /// <param name="ast">An ExpressionAst</param>
+        /// <param name="variableName">The name of the variable we are trying to find</param>
+        /// <returns>A list of ExpressionAsts that match the variable name provided</returns>
+        private static List<VariableExpressionAst> FindMatchingAsts (ExpressionAst ast, string variableName) {


May want to remove the space between the method name and the opening paren here

rjmholt · 2018-07-18T16:31:18Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

-            return AstVisitAction.StopVisit;
+            // The ConvertExpressionAst (static casting for example `[string]$foo = "asdf"`) could contain a VariableExpressionAst so we recurse down
+            var convertExprAst = ast as ConvertExpressionAst;
+            if (convertExprAst != null && convertExprAst.Child != null)


Ideally we're moving to the switch, but the null check here could be compressed to convertExprAst?.Child != null

I think we can actually skip the null on Child, it shouldn't be null.

Incidentally, I didn't realize ConvertExpressionAst inherited AttributedExpressionAst, so @tylerl0706 you probably only need to check AttributedExpressionAst and ArrayLiteralAst

Oh but apparently this works

($a, $b) = 1, 2

So replace it with ParenExpressionAst :P

TylerLeonhardt · 2018-07-18T22:34:36Z

@rjmholt + @SeeminglyScience pretty big refactor in that last commit - can you both re-review? I switched to using an AstVisitor (Thanks Patrick for helping me! :) )

TylerLeonhardt · 2018-07-18T22:35:43Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

-            FoundDeclaration = new SymbolReference(SymbolType.Variable, variableExprAst.Extent);
-            return AstVisitAction.StopVisit;
+            // The AssignmentStatementAst could contain either of the following Ast types:
+            // VariableExpressionAst, ArrayLiteralAst, ConvertExpressionAst, AttributedExpressionAst


gah comment is wrong

rjmholt · 2018-07-18T22:47:20Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+            {
+                if(variableExpressionAst.VariablePath.UserPath.Equals(variableName, StringComparison.OrdinalIgnoreCase))
+                {
+                    // TODO also find instances of set-variable


Code for this already exists too: https://github.com/PowerShell/PowerShell/blob/a3786158ca51cd65388743f900b69ec9e253c3d9/src/System.Management.Automation/engine/parser/VariableAnalysis.cs#L38-L311

nice link - we'd pretty much need that entire class and rework it to do what we do. I've grabbed a few quick wins from it and applied them.

Set-Variable is particularly annoying because it's not an assignmentstatementast same with dot sourcing.

Yeah exactly. I think we'll solve 99% with this, no reason to overcomplicate it

SeeminglyScience

Awesome job! This is much cleaner :)

One possible optimization and one nit pick.

SeeminglyScience · 2018-07-19T02:55:10Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

-            return AstVisitAction.StopVisit;
+            // We want to check VariableExpressionAsts from within this AssignmentStatementAst so we visit it.
+            FindDeclarationVariableExpressionVisitor visitor = new FindDeclarationVariableExpressionVisitor(symbolRef);
+            assignmentStatementAst.Visit(visitor);


Is it possible to make this assignmentStatementAst.Left.Visit(visitor) and drop the other methods in FindDeclarationVariableExpressionVisitor?

Also if you go this route, add an override for VisitMemberExpression and have it return skip children. If I remember correctly that should be the only type of expression that can be on the lhs and also contain a child variable expression that would be invalid.

Actually also VisitIndexExpression

SeeminglyScience · 2018-07-19T02:55:32Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+        /// or a decision to continue if it wasn't found</returns>
+            public override AstVisitAction VisitVariableExpression(VariableExpressionAst variableExpressionAst)
+            {
+                if(variableExpressionAst.VariablePath.UserPath.Equals(variableName, StringComparison.OrdinalIgnoreCase))


nit: Space after if

rjmholt

LGTM!

rkeithhill · 2018-07-20T04:01:41Z

src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs

+        /// </summary>
+        /// <param name="variableExpressionAst">A VariableExpressionAst</param>
+        /// <returns>A decision to stop searching if the right VariableExpressionAst was found,
+        /// or a decision to continue if it wasn't found</returns>


Is this just GH's diff viewer or does the comment above not align with the method below?

rkeithhill

LGTM other than comment about "comment" alignment.

better go to definition

4bea2fe

TylerLeonhardt requested review from rkeithhill, rjmholt and SeeminglyScience July 18, 2018 06:12

SeeminglyScience approved these changes Jul 18, 2018

View reviewed changes

rjmholt reviewed Jul 18, 2018

View reviewed changes

switched to an astvisitor

dcd687e

TylerLeonhardt commented Jul 18, 2018

View reviewed changes

rjmholt reviewed Jul 18, 2018

View reviewed changes

add a few more overrides

241e50a

SeeminglyScience approved these changes Jul 19, 2018

View reviewed changes

only look at LHS

5d07e71

rjmholt approved these changes Jul 20, 2018

View reviewed changes

rkeithhill reviewed Jul 20, 2018

View reviewed changes

rkeithhill approved these changes Jul 20, 2018

View reviewed changes

fix indent

0501314

TylerLeonhardt merged commit 5717ad0 into PowerShell:master Jul 25, 2018

TylerLeonhardt deleted the better-goto branch July 25, 2018 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go To Definition works with different Ast types #706

Go To Definition works with different Ast types #706

TylerLeonhardt commented Jul 18, 2018 •

edited

Loading

SeeminglyScience left a comment

SeeminglyScience Jul 18, 2018

rjmholt Jul 18, 2018

TylerLeonhardt Jul 18, 2018

rjmholt Jul 18, 2018

SeeminglyScience Jul 18, 2018

SeeminglyScience Jul 18, 2018 •

edited

Loading

SeeminglyScience Jul 18, 2018

rjmholt Jul 18, 2018

TylerLeonhardt Jul 18, 2018

rjmholt Jul 18, 2018

rjmholt Jul 18, 2018

rjmholt Jul 18, 2018

SeeminglyScience Jul 18, 2018

rjmholt Jul 18, 2018

rjmholt Jul 18, 2018

rjmholt Jul 18, 2018

rjmholt Jul 18, 2018

SeeminglyScience Jul 18, 2018

SeeminglyScience Jul 18, 2018

TylerLeonhardt commented Jul 18, 2018 •

edited

Loading

TylerLeonhardt Jul 18, 2018

rjmholt Jul 18, 2018

TylerLeonhardt Jul 18, 2018

TylerLeonhardt Jul 18, 2018 •

edited

Loading

rjmholt Jul 18, 2018

SeeminglyScience left a comment

SeeminglyScience Jul 19, 2018

SeeminglyScience Jul 19, 2018 •

edited

Loading

SeeminglyScience Jul 19, 2018

SeeminglyScience Jul 19, 2018

rjmholt left a comment

rkeithhill Jul 20, 2018

rkeithhill left a comment

Go To Definition works with different Ast types #706

Go To Definition works with different Ast types #706

Conversation

TylerLeonhardt commented Jul 18, 2018 • edited Loading

SeeminglyScience left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jul 18, 2018 •

edited

Loading

SeeminglyScience Jul 18, 2018 •

edited

Loading

TylerLeonhardt commented Jul 18, 2018 •

edited

Loading

TylerLeonhardt Jul 18, 2018 •

edited

Loading

SeeminglyScience Jul 19, 2018 •

edited

Loading