Add name validation and related tests, also rearrange/rename some test fixtures

Author: JustinGrote

src/PowerShellEditorServices/Services/TextDocument/RenameService.cs

@@ -69,6 +69,7 @@ ILanguageServerConfiguration config
 
         // Since LSP 3.16 we can simply basically return a DefaultBehavior true or null to signal to the client that the position is valid for rename and it should use its default selection criteria (which is probably the language semantic highlighting or grammar). For the current scope of the rename provider, this should be fine, but we have the option to supply the specific range in the future for special cases.
         RangeOrPlaceholderRange renameSupported = new(new RenameDefaultBehavior() { DefaultBehavior = true });
+
         return (renameResponse?.Changes?[request.TextDocument.Uri].ToArray().Length > 0)
             ? renameSupported
             : null;
@@ -95,9 +96,8 @@ or CommandAst
             => RenameFunction(tokenToRename, scriptFile.ScriptAst, request),
 
             VariableExpressionAst
-            or ParameterAst
             or CommandParameterAst
-            or AssignmentStatementAst
+            or StringConstantExpressionAst
             => RenameVariable(tokenToRename, scriptFile.ScriptAst, request, options.createParameterAlias),
 
             _ => throw new InvalidOperationException("This should not happen as PrepareRename should have already checked for viability. File an issue if you see this.")
@@ -114,24 +114,14 @@ or AssignmentStatementAst
 
     // TODO: We can probably merge these two methods with Generic Type constraints since they are factored into overloading
 
-    internal static TextEdit[] RenameFunction(Ast target, Ast scriptAst, RenameParams renameParams)
+    private static TextEdit[] RenameFunction(Ast target, Ast scriptAst, RenameParams renameParams)
     {
-        if (target is not (FunctionDefinitionAst or CommandAst))
-        {
-            throw new HandlerErrorException($"Asked to rename a function but the target is not a viable function type: {target.GetType()}. This is a bug, file an issue if you see this.");
-        }
-
         RenameFunctionVisitor visitor = new(target, renameParams.NewName);
         return visitor.VisitAndGetEdits(scriptAst);
     }
 
-    internal static TextEdit[] RenameVariable(Ast symbol, Ast scriptAst, RenameParams requestParams, bool createParameterAlias)
+    private static TextEdit[] RenameVariable(Ast symbol, Ast scriptAst, RenameParams requestParams, bool createParameterAlias)
     {
-        if (symbol is not (VariableExpressionAst or ParameterAst or CommandParameterAst or StringConstantExpressionAst))
-        {
-            throw new HandlerErrorException($"Asked to rename a variable but the target is not a viable variable type: {symbol.GetType()}. This is a bug, file an issue if you see this.");
-        }
-
         NewRenameVariableVisitor visitor = new(
             symbol, requestParams.NewName
         );
@@ -144,22 +134,33 @@ internal static TextEdit[] RenameVariable(Ast symbol, Ast scriptAst, RenameParam
     /// <returns>Ast of the token or null if no renamable symbol was found</returns>
     internal static Ast? FindRenamableSymbol(ScriptFile scriptFile, ScriptPositionAdapter position)
     {
-        Ast? ast = scriptFile.ScriptAst.FindClosest(position,
-        [
+        List<Type> renameableAstTypes = [
             // Functions
             typeof(FunctionDefinitionAst),
             typeof(CommandAst),
 
             // Variables
             typeof(VariableExpressionAst),
-            typeof(CommandParameterAst)
-            // FIXME: Splat parameter in hashtable
-        ]);
+            typeof(CommandParameterAst),
+            typeof(StringConstantExpressionAst)
+        ];
+        Ast? ast = scriptFile.ScriptAst.FindClosest(position, renameableAstTypes.ToArray());
+
+        if (ast is StringConstantExpressionAst stringAst)
+        {
+            // Only splat string parameters should be considered for evaluation.
+            if (stringAst.FindSplatParameterReference() is not null) { return stringAst; }
+            // Otherwise redo the search without stringConstant, so the most specific is a command, etc.
+            renameableAstTypes.Remove(typeof(StringConstantExpressionAst));
+            ast = scriptFile.ScriptAst.FindClosest(position, renameableAstTypes.ToArray());
+        }
+
+        // Performance optimizations
 
         // Only the function name is valid for rename, not other components
         if (ast is FunctionDefinitionAst funcDefAst)
         {
-            if (!GetFunctionNameExtent(funcDefAst).Contains(position))
+            if (!funcDefAst.GetFunctionNameExtent().Contains(position))
             {
                 return null;
             }
@@ -179,23 +180,9 @@ internal static TextEdit[] RenameVariable(Ast symbol, Ast scriptAst, RenameParam
             }
         }
 
-        return ast;
-    }
 
 
-    /// <summary>
-    /// Return an extent that only contains the position of the name of the function, for Client highlighting purposes.
-    /// </summary>
-    internal static ScriptExtentAdapter GetFunctionNameExtent(FunctionDefinitionAst ast)
-    {
-        string name = ast.Name;
-        // FIXME: Gather dynamically from the AST and include backticks and whatnot that might be present
-        int funcLength = "function ".Length;
-        ScriptExtentAdapter funcExtent = new(ast.Extent);
-        funcExtent.Start = funcExtent.Start.Delta(0, funcLength);
-        funcExtent.End = funcExtent.Start.Delta(0, name.Length);
-
-        return funcExtent;
+        return ast;
     }
 
     /// <summary>
@@ -322,7 +309,7 @@ internal AstVisitAction Visit(Ast ast)
             {
                 FunctionDefinitionAst f => f,
                 CommandAst command => CurrentDocument.FindFunctionDefinition(command)
-                    ?? throw new HandlerErrorException("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 HandlerErrorException("The command to rename does not have a function definition. Renaming a function is only supported when the function is defined within an accessible scope"),
                 _ => throw new Exception($"Unsupported AST type {target.GetType()} encountered")
             };
         };
@@ -373,7 +360,12 @@ private TextEdit GetRenameFunctionEdit(Ast candidate)
                 throw new InvalidOperationException("GetRenameFunctionEdit was called on an Ast that was not the target. This is a bug and you should file an issue.");
             }
 
-            ScriptExtentAdapter functionNameExtent = RenameService.GetFunctionNameExtent(funcDef);
+            if (!IsValidFunctionName(newName))
+            {
+                throw new HandlerErrorException($"{newName} is not a valid function name.");
+            }
+
+            ScriptExtentAdapter functionNameExtent = funcDef.GetFunctionNameExtent();
 
             return new TextEdit()
             {
@@ -399,6 +391,19 @@ private TextEdit GetRenameFunctionEdit(Ast candidate)
             Range = new ScriptExtentAdapter(funcName.Extent)
         };
     }
+
+    internal static bool IsValidFunctionName(string name)
+    {
+        // Allows us to supply function:varname or varname and get a proper result
+        string candidate = "function " + name.TrimStart('$').TrimStart('-') + " {}";
+        Parser.ParseInput(candidate, out Token[] tokens, out _);
+        return tokens.Length == 5
+            && tokens[0].Kind == TokenKind.Function
+            && tokens[1].Kind == TokenKind.Identifier
+            && tokens[2].Kind == TokenKind.LCurly
+            && tokens[3].Kind == TokenKind.RCurly
+            && tokens[4].Kind == TokenKind.EndOfInput;
+    }
 }
 
 internal class NewRenameVariableVisitor(Ast target, string newName, bool skipVerify = false) : RenameVisitorBase
@@ -430,7 +435,7 @@ internal AstVisitAction Visit(Ast ast)
             VariableDefinition = target.GetTopVariableAssignment();
             if (VariableDefinition is null)
             {
-                throw new HandlerErrorException("The element to rename does not have a definition. Renaming an element is only supported when the element is defined within the same scope");
+                throw new HandlerErrorException("The variable element to rename does not have a definition. Renaming an element is only supported when the variable element is defined within an accessible scope");
             }
         }
         else if (CurrentDocument != ast.GetHighestParent())
@@ -464,24 +469,51 @@ private TextEdit GetRenameVariableEdit(Ast ast)
     {
         return ast switch
         {
-            VariableExpressionAst var => new TextEdit
-            {
-                NewText = '$' + NewName,
-                Range = new ScriptExtentAdapter(var.Extent)
-            },
-            CommandParameterAst param => new TextEdit
-            {
-                NewText = '-' + NewName,
-                Range = new ScriptExtentAdapter(param.Extent)
-            },
-            StringConstantExpressionAst stringAst => new TextEdit
-            {
-                NewText = NewName,
-                Range = new ScriptExtentAdapter(stringAst.Extent)
-            },
+            VariableExpressionAst var => !IsValidVariableName(NewName)
+                ? throw new HandlerErrorException($"${NewName} is not a valid variable name.")
+                : new TextEdit
+                {
+                    NewText = '$' + NewName,
+                    Range = new ScriptExtentAdapter(var.Extent)
+                },
+            StringConstantExpressionAst stringAst => !IsValidVariableName(NewName)
+                ? throw new Exception($"{NewName} is not a valid variable name.")
+                : new TextEdit
+                {
+                    NewText = NewName,
+                    Range = new ScriptExtentAdapter(stringAst.Extent)
+                },
+            CommandParameterAst param => !IsValidCommandParameterName(NewName)
+                ? throw new Exception($"-{NewName} is not a valid command parameter name.")
+                : new TextEdit
+                {
+                    NewText = '-' + NewName,
+                    Range = new ScriptExtentAdapter(param.Extent)
+                },
             _ => throw new InvalidOperationException($"GetRenameVariableEdit was called on an Ast that was not the target. This is a bug and you should file an issue.")
         };
     }
+
+    internal static bool IsValidVariableName(string name)
+    {
+        // Allows us to supply $varname or varname and get a proper result
+        string candidate = '$' + name.TrimStart('$').TrimStart('-');
+        Parser.ParseInput(candidate, out Token[] tokens, out _);
+        return tokens.Length is 2
+            && tokens[0].Kind == TokenKind.Variable
+            && tokens[1].Kind == TokenKind.EndOfInput;
+    }
+
+    internal static bool IsValidCommandParameterName(string name)
+    {
+        // Allows us to supply -varname or varname and get a proper result
+        string candidate = "Command -" + name.TrimStart('$').TrimStart('-');
+        Parser.ParseInput(candidate, out Token[] tokens, out _);
+        return tokens.Length == 3
+            && tokens[0].Kind == TokenKind.Command
+            && tokens[1].Kind == TokenKind.Parameter
+            && tokens[2].Kind == TokenKind.EndOfInput;
+    }
 }
 
 /// <summary>

src/PowerShellEditorServices/Utility/AstExtensions.cs

@@ -523,4 +523,19 @@ public static bool HasParent(this Ast ast, Ast parent)
         return false;
     }
 
+
+    /// <summary>
+    /// Return an extent that only contains the position of the name of the function, for Client highlighting purposes.
+    /// </summary>
+    internal static ScriptExtentAdapter GetFunctionNameExtent(this FunctionDefinitionAst ast)
+    {
+        string name = ast.Name;
+        // FIXME: Gather dynamically from the AST and include backticks and whatnot that might be present
+        int funcLength = "function ".Length;
+        ScriptExtentAdapter funcExtent = new(ast.Extent);
+        funcExtent.Start = funcExtent.Start.Delta(0, funcLength);
+        funcExtent.End = funcExtent.Start.Delta(0, name.Length);
+
+        return funcExtent;
+    }
 }

test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/FunctionsSingle.ps1 renamed to test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/FunctionSimple.ps1

@@ -10,6 +10,10 @@ public class RefactorFunctionTestCases
     /// </summary>
     public static RenameTestTarget[] TestCases =
     [
+        new("FunctionSimple.ps1",                      Line:  1, Column: 11 ),
+        new("FunctionSimple.ps1",                      Line:  1, Column:  1, NoResult: true  ),
+        new("FunctionSimple.ps1",                      Line:  2, Column:  4, NoResult: true  ),
+        new("FunctionSimple.ps1",                      Line:  1, Column: 11, NewName: "Bad Name", ShouldThrow: true ),
         new("FunctionCallWIthinStringExpression.ps1",  Line:  1, Column: 10 ),
         new("FunctionCmdlet.ps1",                      Line:  1, Column: 10 ),
         new("FunctionForeach.ps1",                     Line: 11, Column:  5 ),
@@ -21,7 +25,6 @@ public class RefactorFunctionTestCases
         new("FunctionOuterHasNestedFunction.ps1",      Line:  1, Column: 10 ),
         new("FunctionSameName.ps1",                    Line:  3, Column: 14 ),
         new("FunctionScriptblock.ps1",                 Line:  5, Column:  5 ),
-        new("FunctionsSingle.ps1",                     Line:  1, Column: 11 ),
         new("FunctionWithInnerFunction.ps1",           Line:  5, Column:  5 ),
         new("FunctionWithInternalCalls.ps1",           Line:  3, Column:  6 ),
     ];

test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/FunctionsSingleRenamed.ps1 renamed to test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/FunctionSimpleRenamed.ps1

@@ -28,21 +28,24 @@ public class RenameTestTarget
     public string NewName = "Renamed";
 
     public bool ShouldFail;
+    public bool ShouldThrow;
 
     /// <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>
-    /// <param name="ShouldFail">This test case should not succeed and return either null or a handler error</param>
-    public RenameTestTarget(string FileName, int Line, int Column, string NewName = "Renamed", bool ShouldFail = false)
+    /// <param name="NoResult">This test case should return null (cannot be renamed)</param>
+    /// <param name="ShouldThrow">This test case should throw a HandlerErrorException meaning user needs to be alerted in a custom way</param>
+    public RenameTestTarget(string FileName, int Line, int Column, string NewName = "Renamed", bool NoResult = false, bool ShouldThrow = false)
     {
         this.FileName = FileName;
         this.Line = Line;
         this.Column = Column;
         this.NewName = NewName;
-        this.ShouldFail = ShouldFail;
+        this.ShouldFail = NoResult;
+        this.ShouldThrow = ShouldThrow;
     }
     public RenameTestTarget() { }
 
-    public override string ToString() => $"{FileName.Substring(0, FileName.Length - 4)} {Line}:{Column} N:{NewName} F:{ShouldFail}";
+    public override string ToString() => $"{FileName.Substring(0, FileName.Length - 4)} {Line}:{Column} N:{NewName} F:{ShouldFail} T:{ShouldThrow}";
 }

test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/RefactorFunctionTestCases.cs renamed to test/PowerShellEditorServices.Test.Shared/Refactoring/Functions/_RefactorFunctionTestCases.cs

@@ -5,9 +5,11 @@ public class RefactorVariableTestCases
 {
     public static RenameTestTarget[] TestCases =
     [
-        new ("SimpleVariableAssignment.ps1",                   Line:  1, Column:  1, NewName: "$Renamed"),
-        new ("SimpleVariableAssignment.ps1",                   Line:  1, Column:  1),
-        new ("SimpleVariableAssignment.ps1",                   Line:  2, Column:  1, NewName: "Wrong", ShouldFail: true),
+        new ("VariableSimpleAssignment.ps1",                   Line:  1, Column:  1),
+        new ("VariableSimpleAssignment.ps1",                   Line:  1, Column:  1, NewName: "$Renamed"),
+        new ("VariableSimpleAssignment.ps1",                   Line:  1, Column:  1, NewName: "$Bad Name", ShouldThrow: true),
+        new ("VariableSimpleAssignment.ps1",                   Line:  1, Column:  1, NewName: "Bad Name", ShouldThrow: true),
+        new ("VariableSimpleAssignment.ps1",                   Line:  1, Column:  6, NoResult: true),
         new ("VariableCommandParameter.ps1",                   Line:  3, Column: 17),
         new ("VariableCommandParameter.ps1",                   Line: 10, Column: 10),
         new ("VariableCommandParameterSplatted.ps1",           Line:  3, Column: 19 ),
@@ -32,6 +34,5 @@ public class RefactorVariableTestCases
         new ("VariableWithinCommandAstScriptBlock.ps1",        Line:  3, Column: 75),
         new ("VariableWithinForeachObject.ps1",                Line:  2, Column:  1),
         new ("VariableWithinHastableExpression.ps1",           Line:  3, Column: 46),
-        new ("ParameterUndefinedFunction.ps1",                 Line:  1, Column: 39, ShouldFail: true),
     ];
 }

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

@@ -72,9 +72,9 @@ public async Task FindsFunction(RenameTestTarget s)
         {
             result = await testHandler.Handle(testParams, CancellationToken.None);
         }
-        catch (HandlerErrorException)
+        catch (HandlerErrorException err)
         {
-            Assert.True(s.ShouldFail);
+            Assert.True(s.ShouldThrow, $"Unexpected HandlerErrorException: {err.Message}");
             return;
         }
         if (s.ShouldFail)
@@ -100,7 +100,7 @@ public async Task FindsVariable(RenameTestTarget s)
         }
         catch (HandlerErrorException err)
         {
-            Assert.True(s.ShouldFail, err.Message);
+            Assert.True(s.ShouldThrow, $"Unexpected HandlerErrorException: {err.Message}");
             return;
         }
         if (s.ShouldFail)
@@ -213,6 +213,7 @@ public void Serialize(IXunitSerializationInfo info)
         info.AddValue(nameof(Column), Column);
         info.AddValue(nameof(NewName), NewName);
         info.AddValue(nameof(ShouldFail), ShouldFail);
+        info.AddValue(nameof(ShouldThrow), ShouldThrow);
     }
 
     public void Deserialize(IXunitSerializationInfo info)
@@ -222,6 +223,7 @@ public void Deserialize(IXunitSerializationInfo info)
         Column = info.GetValue<int>(nameof(Column));
         NewName = info.GetValue<string>(nameof(NewName));
         ShouldFail = info.GetValue<bool>(nameof(ShouldFail));
+        ShouldThrow = info.GetValue<bool>(nameof(ShouldThrow));
     }
 
     public static RenameTestTargetSerializable FromRenameTestTarget(RenameTestTarget t)
@@ -231,6 +233,7 @@ public static RenameTestTargetSerializable FromRenameTestTarget(RenameTestTarget
             Column = t.Column,
             Line = t.Line,
             NewName = t.NewName,
-            ShouldFail = t.ShouldFail
+            ShouldFail = t.ShouldFail,
+            ShouldThrow = t.ShouldThrow
         };
 }