Breakout and simplify matching logic, fixes more test cases

Author: JustinGrote

src/PowerShellEditorServices/Language/AstExtensions.cs

@@ -72,54 +72,48 @@ public static class AstExtensions
 
     public static FunctionDefinitionAst? FindFunctionDefinition(this Ast ast, CommandAst command)
     {
-        string? name = command.GetCommandName();
+        string? name = command.GetCommandName().ToLower();
         if (name is null) { return null; }
 
-        List<FunctionDefinitionAst> FunctionDefinitions = ast.FindAll(ast =>
+        FunctionDefinitionAst[] candidateFuncDefs = ast.FindAll(ast =>
         {
-            return ast is FunctionDefinitionAst funcDef &&
-            funcDef.Name.ToLower() == name &&
-            (funcDef.Extent.EndLineNumber < command.Extent.StartLineNumber ||
-            (funcDef.Extent.EndColumnNumber <= command.Extent.StartColumnNumber &&
-            funcDef.Extent.EndLineNumber <= command.Extent.StartLineNumber));
-        }, true).Cast<FunctionDefinitionAst>().ToList();
-
-        // return the function def if we only have one match
-        if (FunctionDefinitions.Count == 1)
-        {
-            return FunctionDefinitions[0];
-        }
-        // Determine which function definition is the right one
-        FunctionDefinitionAst? CorrectDefinition = null;
-        for (int i = FunctionDefinitions.Count - 1; i >= 0; i--)
-        {
-            FunctionDefinitionAst element = FunctionDefinitions[i];
+            if (ast is not FunctionDefinitionAst funcDef)
+            {
+                return false;
+            }
 
-            Ast parent = element.Parent;
-            // walk backwards till we hit a functiondefinition if any
-            while (null != parent)
+            if (funcDef.Name.ToLower() != name)
             {
-                if (parent is FunctionDefinitionAst)
-                {
-                    break;
-                }
-                parent = parent.Parent;
+                return false;
             }
-            // we have hit the global scope of the script file
-            if (null == parent)
+
+            // If the function is recursive (calls itself), its parent is a match unless a more specific in-scope function definition comes next (this is a "bad practice" edge case)
+            // TODO: Consider a simple "contains" match
+            if (command.HasParent(funcDef))
             {
-                CorrectDefinition = element;
-                break;
+                return true;
             }
 
-            if (command?.Parent == parent)
+            if
+            (
+                // TODO: Replace with a position match
+                funcDef.Extent.EndLineNumber > command.Extent.StartLineNumber
+                ||
+                (
+                    funcDef.Extent.EndLineNumber == command.Extent.StartLineNumber
+                    && funcDef.Extent.EndColumnNumber >= command.Extent.StartColumnNumber
+                )
+            )
             {
-                CorrectDefinition = (FunctionDefinitionAst)parent;
+                return false;
             }
-        }
-        return CorrectDefinition;
-    }
 
+            return command.HasParent(funcDef.Parent); // The command is in the same scope as the function definition
+        }, true).Cast<FunctionDefinitionAst>().ToArray();
+
+        // There should only be one match most of the time, the only other cases is when a function is defined multiple times (bad practice). If there are multiple definitions, the candidate "closest" to the command, which would be the last one found, is the appropriate one
+        return candidateFuncDefs.LastOrDefault();
+    }
 
     public static Ast[] FindParents(this Ast ast, params Type[] type)
     {

src/PowerShellEditorServices/Services/TextDocument/RenameService.cs

@@ -206,7 +206,7 @@ public class RenameFunctionVisitor(Ast target, string newName, bool skipVerify =
 {
     public List<TextEdit> Edits { get; } = new();
     private Ast? CurrentDocument;
-    private string OldName = string.Empty;
+    private FunctionDefinitionAst? FunctionToRename;
 
     // Wire up our visitor to the relevant AST types we are potentially renaming
     public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst ast) => Visit(ast);
@@ -223,15 +223,13 @@ public AstVisitAction Visit(Ast ast)
                 throw new TargetSymbolNotFoundException("The target this visitor would rename is not present in the AST. This is a bug and you should file an issue");
             }
 
-            FunctionDefinitionAst functionDef = target switch
+            FunctionToRename = target switch
             {
                 FunctionDefinitionAst f => f,
                 CommandAst command => CurrentDocument.FindFunctionDefinition(command)
                     ?? throw new TargetSymbolNotFoundException("The command to rename does not have a function definition. Renaming a function is only supported when the function is defined within the same scope"),
                 _ => throw new Exception($"Unsupported AST type {target.GetType()} encountered")
             };
-
-            OldName = functionDef.Name;
         };
 
         if (CurrentDocument != ast.GetHighestParent())
@@ -241,7 +239,7 @@ public AstVisitAction Visit(Ast ast)
 
         if (ShouldRename(ast))
         {
-            Edits.Add(GetRenameFunctionEdits(ast));
+            Edits.Add(GetRenameFunctionEdit(ast));
             return AstVisitAction.Continue;
         }
         else
@@ -254,10 +252,10 @@ public AstVisitAction Visit(Ast ast)
 
     private bool ShouldRename(Ast candidate)
     {
-        // There should be only one function definition and if it is not our target, it may be a duplicately named function
+        // Rename our original function definition. There may be duplicate definitions of the same name
         if (candidate is FunctionDefinitionAst funcDef)
         {
-            return funcDef == target;
+            return funcDef == FunctionToRename;
         }
 
         // Should only be CommandAst (function calls) from this point forward in the visit.
@@ -266,36 +264,20 @@ private bool ShouldRename(Ast candidate)
             throw new InvalidOperationException($"ShouldRename for a function had an Unexpected Ast Type {candidate.GetType()}. This is a bug and you should file an issue.");
         }
 
-        if (command.GetCommandName().ToLower() != OldName.ToLower())
-        {
-            return false;
-        }
-
-        // TODO: Use position comparisons here
-        // Command calls must always come after the function definitions
-        if (
-            target.Extent.StartLineNumber > command.Extent.StartLineNumber
-            || (
-                target.Extent.StartLineNumber == command.Extent.StartLineNumber
-                && target.Extent.StartColumnNumber >= command.Extent.StartColumnNumber
-            )
-        )
+        if (CurrentDocument is null)
         {
-            return false;
+            throw new InvalidOperationException("CurrentDoc should always be set by now from first Visit. This is a bug and you should file an issue.");
         }
 
-        // If the command is defined in the same parent scope as the function
-        return command.HasParent(target.Parent);
-
-        // If we get this far, we hit an edge case
-        throw new InvalidOperationException("ShouldRename for a function could not determine the viability of a rename. This is a bug and you should file an issue.");
+        // Match up the command to its function definition
+        return CurrentDocument.FindFunctionDefinition(command) == FunctionToRename;
     }
 
-    private TextEdit GetRenameFunctionEdits(Ast candidate)
+    private TextEdit GetRenameFunctionEdit(Ast candidate)
     {
         if (candidate is FunctionDefinitionAst funcDef)
         {
-            if (funcDef != target)
+            if (funcDef != FunctionToRename)
             {
                 throw new InvalidOperationException("GetRenameFunctionEdit was called on an Ast that was not the target. This is a bug and you should file an issue.");
             }
@@ -315,22 +297,16 @@ private TextEdit GetRenameFunctionEdits(Ast candidate)
             throw new InvalidOperationException($"Expected a command but got {candidate.GetType()}");
         }
 
-        if (command.GetCommandName()?.ToLower() == OldName.ToLower() &&
-            target.Extent.StartLineNumber <= command.Extent.StartLineNumber)
+        if (command.CommandElements[0] is not StringConstantExpressionAst funcName)
         {
-            if (command.CommandElements[0] is not StringConstantExpressionAst funcName)
-            {
-                throw new InvalidOperationException("Command element should always have a string expresssion as its first item. This is a bug and you should report it.");
-            }
-
-            return new TextEdit()
-            {
-                NewText = newName,
-                Range = new ScriptExtentAdapter(funcName.Extent)
-            };
+            throw new InvalidOperationException("Command element should always have a string expresssion as its first item. This is a bug and you should report it.");
         }
 
-        throw new InvalidOperationException("GetRenameFunctionEdit was not provided a FuncitonDefinition or a CommandAst");
+        return new TextEdit()
+        {
+            NewText = newName,
+            Range = new ScriptExtentAdapter(funcName.Extent)
+        };
     }
 
     public TextEdit[] VisitAndGetEdits(Ast ast)