Fixup Tests, still a bug in rename logic with functions

Author: JustinGrote

src/PowerShellEditorServices/Services/PowerShell/Utility/IScriptExtentExtensions.cs

@@ -1,13 +1,14 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-using System.Management.Automation.Language;
-using Microsoft.PowerShell.EditorServices.Services;
+// using System.Management.Automation.Language;
+// using Microsoft.PowerShell.EditorServices.Services;
 
-namespace PowerShellEditorServices.Services.PowerShell.Utility
-{
-    public static class IScriptExtentExtensions
-    {
-        public static bool Contains(this IScriptExtent extent, ScriptPositionAdapter position) => ScriptExtentAdapter.ContainsPosition(new(extent), position);
-    }
-}
+// namespace PowerShellEditorServices.Services.PowerShell.Utility
+// {
+//     public static class IScriptExtentExtensions
+//     {
+//         public static bool Contains(this IScriptExtent extent, IScriptExtent position)
+//             => ScriptExtentAdapter.ContainsPosition(extent, position);
+//     }
+// }

src/PowerShellEditorServices/Services/TextDocument/Services/RenameService.cs

@@ -70,8 +70,10 @@ ILanguageServerConfiguration config
         }
 
         ScriptPositionAdapter position = request.Position;
-        Ast target = FindRenamableSymbol(scriptFile, position);
+        Ast? target = FindRenamableSymbol(scriptFile, position);
         if (target is null) { return null; }
+
+        // Will implicitly convert to RangeOrPlaceholder and adjust to 0-based
         return target switch
         {
             FunctionDefinitionAst funcAst => GetFunctionNameExtent(funcAst),
@@ -85,7 +87,7 @@ ILanguageServerConfiguration config
         ScriptFile scriptFile = workspaceService.GetFile(request.TextDocument.Uri);
         ScriptPositionAdapter position = request.Position;
 
-        Ast tokenToRename = FindRenamableSymbol(scriptFile, position);
+        Ast? tokenToRename = FindRenamableSymbol(scriptFile, position);
         if (tokenToRename is null) { return null; }
 
         // TODO: Potentially future cross-file support
@@ -151,55 +153,35 @@ internal static TextEdit[] RenameVariable(Ast symbol, Ast scriptAst, RenameParam
         return [];
     }
 
-
     /// <summary>
-    /// Finds a renamable symbol at a given position in a script file.
+    /// Finds the most specific renamable symbol at the given position
     /// </summary>
     /// <returns>Ast of the token or null if no renamable symbol was found</returns>
-    internal static Ast FindRenamableSymbol(ScriptFile scriptFile, ScriptPositionAdapter position)
+    internal static Ast? FindRenamableSymbol(ScriptFile scriptFile, ScriptPositionAdapter position)
     {
-        int line = position.Line;
-        int column = position.Column;
-
-        // Cannot use generic here as our desired ASTs do not share a common parent
-        Ast token = scriptFile.ScriptAst.Find(ast =>
+        Ast? ast = scriptFile.ScriptAst.FindAtPosition(position,
+        [
+            // Filters just the ASTs that are candidates for rename
+            typeof(FunctionDefinitionAst),
+            typeof(VariableExpressionAst),
+            typeof(CommandParameterAst),
+            typeof(ParameterAst),
+            typeof(StringConstantExpressionAst),
+            typeof(CommandAst)
+        ]);
+
+        // Special detection for Function calls that dont follow verb-noun syntax e.g. DoThing
+        // It's not foolproof but should work in most cases where it is explicit (e.g. not & $x)
+        if (ast is StringConstantExpressionAst stringAst)
         {
-            // Skip all statements that end before our target line or start after our target line. This is a performance optimization.
-            if (ast.Extent.EndLineNumber < line || ast.Extent.StartLineNumber > line) { return false; }
-
-            // Supported types, filters out scriptblocks and whatnot
-            if (ast is not (
-                FunctionDefinitionAst
-                or VariableExpressionAst
-                or CommandParameterAst
-                or ParameterAst
-                or StringConstantExpressionAst
-                or CommandAst
-            ))
-            {
-                return false;
-            }
-
-            // Special detection for Function calls that dont follow verb-noun syntax e.g. DoThing
-            // It's not foolproof but should work in most cases where it is explicit (e.g. not & $x)
-            if (ast is StringConstantExpressionAst stringAst)
-            {
-                if (stringAst.Parent is not CommandAst parent) { return false; }
-                if (parent.GetCommandName() != stringAst.Value) { return false; }
-            }
-
-            ScriptExtentAdapter target = ast switch
-            {
-                FunctionDefinitionAst funcAst => GetFunctionNameExtent(funcAst),
-                _ => new ScriptExtentAdapter(ast.Extent)
-            };
-
-            return target.Contains(position);
-        }, true);
+            if (stringAst.Parent is not CommandAst parent) { return null; }
+            if (parent.GetCommandName() != stringAst.Value) { return null; }
+        }
 
-        return token;
+        return ast;
     }
 
+
     private static ScriptExtentAdapter GetFunctionNameExtent(FunctionDefinitionAst ast)
     {
         string name = ast.Name;
@@ -221,6 +203,63 @@ public class RenameSymbolOptions
     public bool CreateAlias { get; set; }
 }
 
+
+public static class AstExtensions
+{
+    /// <summary>
+    /// Finds the most specific Ast at the given script position, or returns null if none found.<br/>
+    /// For example, if the position is on a variable expression within a function definition,
+    /// the variable will be returned even if the function definition is found first.
+    /// </summary>
+    internal static Ast? FindAtPosition(this Ast ast, IScriptPosition position, Type[]? allowedTypes)
+    {
+        // Short circuit quickly if the position is not in the provided range, no need to traverse if not
+        // TODO: Maybe this should be an exception instead? I mean technically its not found but if you gave a position outside the file something very wrong probably happened.
+        if (!new ScriptExtentAdapter(ast.Extent).Contains(position)) { return null; }
+
+        // This will be updated with each loop, and re-Find to dig deeper
+        Ast? mostSpecificAst = null;
+
+        do
+        {
+            ast = ast.Find(currentAst =>
+            {
+                if (currentAst == mostSpecificAst) { return false; }
+
+                int line = position.LineNumber;
+                int column = position.ColumnNumber;
+
+                // Performance optimization, skip statements that don't contain the position
+                if (
+                    currentAst.Extent.EndLineNumber < line
+                    || currentAst.Extent.StartLineNumber > line
+                    || (currentAst.Extent.EndLineNumber == line && currentAst.Extent.EndColumnNumber < column)
+                    || (currentAst.Extent.StartLineNumber == line && currentAst.Extent.StartColumnNumber > column)
+                )
+                {
+                    return false;
+                }
+
+                if (allowedTypes is not null && !allowedTypes.Contains(currentAst.GetType()))
+                {
+                    return false;
+                }
+
+                if (new ScriptExtentAdapter(currentAst.Extent).Contains(position))
+                {
+                    mostSpecificAst = currentAst;
+                    return true; //Stops the find
+                }
+
+                return false;
+            }, true);
+        } while (ast is not null);
+
+        return mostSpecificAst;
+    }
+
+}
+
 internal class Utilities
 {
     public static Ast? GetAstAtPositionOfType(int StartLineNumber, int StartColumnNumber, Ast ScriptAst, params Type[] type)
@@ -385,10 +424,10 @@ public static bool AssertContainsDotSourced(Ast ScriptAst)
 public record ScriptPositionAdapter(IScriptPosition position) : IScriptPosition, IComparable<ScriptPositionAdapter>, IComparable<Position>, IComparable<ScriptPosition>
 {
     public int Line => position.LineNumber;
-    public int Column => position.ColumnNumber;
-    public int Character => position.ColumnNumber;
     public int LineNumber => position.LineNumber;
+    public int Column => position.ColumnNumber;
     public int ColumnNumber => position.ColumnNumber;
+    public int Character => position.ColumnNumber;
 
     public string File => position.File;
     string IScriptPosition.Line => position.Line;
@@ -457,13 +496,32 @@ internal record ScriptExtentAdapter(IScriptExtent extent) : IScriptExtent
     public IScriptPosition EndScriptPosition => End;
     public int EndColumnNumber => End.ColumnNumber;
     public int EndLineNumber => End.LineNumber;
-    public int StartOffset => extent.EndOffset;
+    public int StartOffset => extent.StartOffset;
     public int EndOffset => extent.EndOffset;
     public string File => extent.File;
     public int StartColumnNumber => extent.StartColumnNumber;
     public int StartLineNumber => extent.StartLineNumber;
     public string Text => extent.Text;
 
-    public bool Contains(Position position) => ContainsPosition(this, position);
-    public static bool ContainsPosition(ScriptExtentAdapter range, ScriptPositionAdapter position) => Range.ContainsPosition(range, position);
+    public bool Contains(IScriptPosition position) => Contains((ScriptPositionAdapter)position);
+
+    public bool Contains(ScriptPositionAdapter position)
+    {
+        if (position.Line < Start.Line || position.Line > End.Line)
+        {
+            return false;
+        }
+
+        if (position.Line == Start.Line && position.Character < Start.Character)
+        {
+            return false;
+        }
+
+        if (position.Line == End.Line && position.Character > End.Character)
+        {
+            return false;
+        }
+
+        return true;
+    }
 }

test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/RefactorFunctionTestCases.cs

@@ -7,7 +7,7 @@ public class RefactorFunctionTestCases
 {
     public static RenameTestTarget[] TestCases =
     [
-        new("FunctionsSingle.ps1",                     Line: 1,  Column:  5 ),
+        new("FunctionsSingle.ps1",                     Line: 1,  Column: 11 ),
         new("FunctionMultipleOccurrences.ps1",         Line: 1,  Column:  5 ),
         new("FunctionInnerIsNested.ps1",               Line: 5,  Column:  5, "bar"),
         new("FunctionOuterHasNestedFunction.ps1",      Line: 10, Column:  1 ),
@@ -19,7 +19,7 @@ public class RefactorFunctionTestCases
         new("FunctionLoop.ps1",                        Line: 5,  Column:  5 ),
         new("FunctionForeach.ps1",                     Line: 5,  Column: 11 ),
         new("FunctionForeachObject.ps1",               Line: 5,  Column: 11 ),
-        new("FunctionCallWIthinStringExpression.ps1",  Line: 10, Column:  1 ),
-        new("FunctionNestedRedefinition.ps1",          Line: 15, Column: 13 )
+        new("FunctionCallWIthinStringExpression.ps1",  Line: 1,  Column: 10 ),
+        new("FunctionNestedRedefinition.ps1",          Line: 13, Column: 15 )
     ];
 }

test/PowerShellEditorServices.Test.Shared/Refactoring/RenameTestTarget.cs

@@ -8,11 +8,37 @@ namespace PowerShellEditorServices.Test.Shared.Refactoring;
 /// <summary>
 /// Describes a test case for renaming a file
 /// </summary>
-/// <param name="FileName">The test case file name e.g. testScript.ps1</param>
-/// <param name="Line">The line where the cursor should be positioned for the rename</param>
-/// <param name="Column">The column/character indent where ther cursor should be positioned for the rename</param>
-/// <param name="NewName">What the target symbol represented by the line and column should be renamed to. Defaults to "Renamed" if not specified</param>
-public record RenameTestTarget(string FileName = "UNKNOWN", int Line = -1, int Column = -1, string NewName = "Renamed")
+public class RenameTestTarget
 {
+    /// <summary>
+    /// The test case file name e.g. testScript.ps1
+    /// </summary>
+    public string FileName { get; set; } = "UNKNOWN";
+    /// <summary>
+    /// The line where the cursor should be positioned for the rename
+    /// </summary>
+    public int Line { get; set; } = -1;
+    /// <summary>
+    /// The column/character indent where ther cursor should be positioned for the rename
+    /// </summary>
+    public int Column { get; set; } = -1;
+    /// <summary>
+    /// What the target symbol represented by the line and column should be renamed to. Defaults to "Renamed" if not specified
+    /// </summary>
+    public string NewName = "Renamed";
+
+    /// <param name="FileName">The test case file name e.g. testScript.ps1</param>
+    /// <param name="Line">The line where the cursor should be positioned for the rename</param>
+    /// <param name="Column">The column/character indent where ther cursor should be positioned for the rename</param>
+    /// <param name="NewName">What the target symbol represented by the line and column should be renamed to. Defaults to "Renamed" if not specified</param>
+    public RenameTestTarget(string FileName, int Line, int Column, string NewName = "Renamed")
+    {
+        this.FileName = FileName;
+        this.Line = Line;
+        this.Column = Column;
+        this.NewName = NewName;
+    }
+    public RenameTestTarget() { }
+
     public override string ToString() => $"{FileName.Substring(0, FileName.Length - 4)}";
 }