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.

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
Changes from 4 commits
Commits
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
74 changes: 64 additions & 10 deletions src/PowerShellEditorServices/Language/FindDeclarationVisitor.cs
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 @@ -54,7 +56,7 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst fun
};

if (symbolRef.SymbolType.Equals(SymbolType.Function) &&
nameExtent.Text.Equals(symbolRef.ScriptRegion.Text, StringComparison.CurrentCultureIgnoreCase))
nameExtent.Text.Equals(symbolRef.ScriptRegion.Text, StringComparison.CurrentCultureIgnoreCase))
{
this.FoundDeclaration =
new SymbolReference(
Expand All @@ -76,19 +78,71 @@ 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(
variableName,
StringComparison.OrdinalIgnoreCase))
if (variableName == null)
{
return AstVisitAction.Continue;
}

// TODO also find instances of set-variable
FoundDeclaration = new SymbolReference(SymbolType.Variable, variableExprAst.Extent);
return AstVisitAction.StopVisit;
// We want to check VariableExpressionAsts from within this AssignmentStatementAst so we visit it.
FindDeclarationVariableExpressionVisitor visitor = new FindDeclarationVariableExpressionVisitor(symbolRef);
assignmentStatementAst.Left.Visit(visitor);

if (visitor.FoundDeclaration != null)
{
FoundDeclaration = visitor.FoundDeclaration;
return AstVisitAction.StopVisit;
}
return AstVisitAction.Continue;
}

/// <summary>
/// The private visitor used to find the variable expression that matches a symbol
/// </summary>
private class FindDeclarationVariableExpressionVisitor : AstVisitor
{
private SymbolReference symbolRef;
private string variableName;

public SymbolReference FoundDeclaration{ get; private set; }

public FindDeclarationVariableExpressionVisitor(SymbolReference symbolRef)
{
this.symbolRef = symbolRef;
if (this.symbolRef.SymbolType == SymbolType.Variable)
{
// converts `$varName` to `varName` or of the form ${varName} to varName
variableName = symbolRef.SymbolName.TrimStart('$').Trim('{', '}');
}
}

/// <summary>
/// Check if the VariableExpressionAst has the same name as that of symbolRef.
/// </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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

public override AstVisitAction VisitVariableExpression(VariableExpressionAst variableExpressionAst)
{
if (variableExpressionAst.VariablePath.UserPath.Equals(variableName, StringComparison.OrdinalIgnoreCase))
{
// TODO also find instances of set-variable
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@TylerLeonhardt TylerLeonhardt Jul 18, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

FoundDeclaration = new SymbolReference(SymbolType.Variable, variableExpressionAst.Extent);
return AstVisitAction.StopVisit;
}
return AstVisitAction.Continue;
}

public override AstVisitAction VisitMemberExpression(MemberExpressionAst functionDefinitionAst)
{
// We don't want to discover any variables in member expressisons (`$something.Foo`)
return AstVisitAction.SkipChildren;
}

public override AstVisitAction VisitIndexExpression(IndexExpressionAst functionDefinitionAst)
{
// We don't want to discover any variables in index expressions (`$something[0]`)
return AstVisitAction.SkipChildren;
}
}
}
}