Skip to content

Go To Definition works with different Ast types #706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 25, 2018
Merged

Go To Definition works with different Ast types #706

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4bea2fe
better go to definition
TylerLeonhardt Jul 18, 2018
dcd687e
switched to an astvisitor
TylerLeonhardt Jul 18, 2018
241e50a
add a few more overrides
TylerLeonhardt Jul 18, 2018
5d07e71
only look at LHS
TylerLeonhardt Jul 19, 2018
0501314
fix indent
TylerLeonhardt Jul 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 59 additions & 8 deletions src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
//

using System;
using System.Collections.Generic;
using System.Linq;
using System.Management.Automation.Language;

namespace Microsoft.PowerShell.EditorServices
Expand Down Expand Up @@ -76,19 +78,68 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst fun
/// or a decision to continue if it wasn't found</returns>
public override AstVisitAction VisitAssignmentStatement(AssignmentStatementAst assignmentStatementAst)
{
var variableExprAst = assignmentStatementAst.Left as VariableExpressionAst;
if (variableExprAst == null ||
variableName == null ||
!variableExprAst.VariablePath.UserPath.Equals(
if (variableName == null)
{
return AstVisitAction.Continue;
}

// The AssignmentStatementAst could contain either of the following Ast types:
// VariableExpressionAst, ArrayLiteralAst, ConvertExpressionAst, AttributedExpressionAst
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gah comment is wrong

// We might need to recurse down the tree to find the VariableExpressionAst we're looking for
List<VariableExpressionAst> asts = FindMatchingAsts(assignmentStatementAst.Left, variableName);
if (asts.Count > 0)
{
FoundDeclaration = new SymbolReference(SymbolType.Variable, asts.FirstOrDefault().Extent);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

return AstVisitAction.StopVisit;
}
return AstVisitAction.Continue;
}

/// <summary>
/// Takes in an ExpressionAst and recurses until it finds all VariableExpressionAst and returns the list of them.
/// We expect this ExpressionAst to be either: VariableExpressionAst, ArrayLiteralAst, ConvertExpressionAst, AttributedExpressionAst
/// </summary>
/// <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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Historical note: We think this should work)

{
if (variableExprAst.VariablePath.UserPath.Equals(
variableName,
StringComparison.OrdinalIgnoreCase))
{
return new List<VariableExpressionAst>{ variableExprAst };
Copy link
Collaborator

@SeeminglyScience SeeminglyScience Jul 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return new List<VariableExpressionAst>();
}

// VariableExpressionAsts could be an element of an ArrayLiteralAst. This adds all the elements to the call stack.
var arrayLiteralAst = ast as ArrayLiteralAst;
if (arrayLiteralAst != null)
{
return AstVisitAction.Continue;
return (arrayLiteralAst.Elements.SelectMany(e => FindMatchingAsts(e, variableName)).ToList());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// TODO also find instances of set-variable
FoundDeclaration = new SymbolReference(SymbolType.Variable, variableExprAst.Extent);
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but apparently this works

($a, $b) = 1, 2

So replace it with ParenExpressionAst :P

{
return FindMatchingAsts(convertExprAst.Child, variableName);
}

// The AttributedExpressionAst (any attribute for example `[NotNull()]$foo = "asdf"`) could contain a VariableExpressionAst so we recurse down
var attributedExprAst = ast as AttributedExpressionAst;
if (attributedExprAst != null && attributedExprAst.Child != null)
{
return FindMatchingAsts(attributedExprAst.Child, variableName);
};

// We shouldn't ever get here, but in case a new Ast type is added to PowerShell, this fails gracefully
return new List<VariableExpressionAst>();
}
}
}