Fix symbol highlight when hovering function name (#1890)

* move helper to VistiorUtils class * find function name if indented or after newline * extend tests to cover function definition name * extend test to include end position * Fix `postion` to `position` typo Co-authored-by: Andy Jordan <[email protected]>
PowerShell · Aug 15, 2022 · 87cd721 · 87cd721
1 parent b0bfce7
commit 87cd721
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 51 deletions.
diff --git a/src/PowerShellEditorServices/Services/Symbols/Vistors/FindReferencesVisitor.cs b/src/PowerShellEditorServices/Services/Symbols/Vistors/FindReferencesVisitor.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Management.Automation.Language;
+using Microsoft.PowerShell.EditorServices.Utility;
 
 namespace Microsoft.PowerShell.EditorServices.Services.Symbols
 {
@@ -116,7 +117,7 @@ public override AstVisitAction VisitCommand(CommandAst commandAst)
         /// <returns>A visit action that continues the search for references</returns>
         public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst functionDefinitionAst)
         {
-            (int startColumnNumber, int startLineNumber) = GetStartColumnAndLineNumbersFromAst(functionDefinitionAst);
+            (int startColumnNumber, int startLineNumber) = VisitorUtils.GetNameStartColumnAndLineNumbersFromAst(functionDefinitionAst);
 
             IScriptExtent nameExtent = new ScriptExtent()
             {
@@ -167,39 +168,5 @@ public override AstVisitAction VisitVariableExpression(VariableExpressionAst var
             }
             return AstVisitAction.Continue;
         }
-
-        // Computes where the start of the actual function name is.
-        private static (int, int) GetStartColumnAndLineNumbersFromAst(FunctionDefinitionAst ast)
-        {
-            int startColumnNumber = ast.Extent.StartColumnNumber;
-            int startLineNumber = ast.Extent.StartLineNumber;
-            int astOffset = ast.IsFilter ? "filter".Length : ast.IsWorkflow ? "workflow".Length : "function".Length;
-            string astText = ast.Extent.Text;
-            // The line offset represents the offset on the line that we're on where as
-            // astOffset is the offset on the entire text of the AST.
-            int lineOffset = astOffset;
-            for (; astOffset < astText.Length; astOffset++, lineOffset++)
-            {
-                if (astText[astOffset] == '\n')
-                {
-                    // reset numbers since we are operating on a different line and increment the line number.
-                    startColumnNumber = 0;
-                    startLineNumber++;
-                    lineOffset = 0;
-                }
-                else if (astText[astOffset] == '\r')
-                {
-                    // Do nothing with carriage returns... we only look for line feeds since those
-                    // are used on every platform.
-                }
-                else if (!char.IsWhiteSpace(astText[astOffset]))
-                {
-                    // This is the start of the function name so we've found our start column and line number.
-                    break;
-                }
-            }
-
-            return (startColumnNumber + lineOffset, startLineNumber);
-        }
     }
 }
diff --git a/src/PowerShellEditorServices/Services/Symbols/Vistors/FindSymbolVisitor.cs b/src/PowerShellEditorServices/Services/Symbols/Vistors/FindSymbolVisitor.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System.Management.Automation.Language;
+using Microsoft.PowerShell.EditorServices.Utility;
 
 namespace Microsoft.PowerShell.EditorServices.Services.Symbols
 {
@@ -57,22 +58,28 @@ public override AstVisitAction VisitCommand(CommandAst commandAst)
         /// or a decision to continue if it wasn't found</returns>
         public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst functionDefinitionAst)
         {
-            int startColumnNumber = 1;
+            int startLineNumber = functionDefinitionAst.Extent.StartLineNumber;
+            int startColumnNumber = functionDefinitionAst.Extent.StartColumnNumber;
+            int endLineNumber = functionDefinitionAst.Extent.EndLineNumber;
+            int endColumnNumber = functionDefinitionAst.Extent.EndColumnNumber;
 
             if (!includeFunctionDefinitions)
             {
-                startColumnNumber =
-                    functionDefinitionAst.Extent.Text.IndexOf(
-                        functionDefinitionAst.Name) + 1;
+                // We only want the function name
+                (int startColumn, int startLine) = VisitorUtils.GetNameStartColumnAndLineNumbersFromAst(functionDefinitionAst);
+                startLineNumber = startLine;
+                startColumnNumber = startColumn;
+                endLineNumber = startLine;
+                endColumnNumber = startColumn + functionDefinitionAst.Name.Length;
             }
 
             IScriptExtent nameExtent = new ScriptExtent()
             {
                 Text = functionDefinitionAst.Name,
-                StartLineNumber = functionDefinitionAst.Extent.StartLineNumber,
-                EndLineNumber = functionDefinitionAst.Extent.EndLineNumber,
+                StartLineNumber = startLineNumber,
+                EndLineNumber = endLineNumber,
                 StartColumnNumber = startColumnNumber,
-                EndColumnNumber = startColumnNumber + functionDefinitionAst.Name.Length,
+                EndColumnNumber = endColumnNumber,
                 File = functionDefinitionAst.Extent.File
             };
 

diff --git a/src/PowerShellEditorServices/Utility/VisitorUtils.cs b/src/PowerShellEditorServices/Utility/VisitorUtils.cs
@@ -0,0 +1,51 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using System.Management.Automation.Language;
+
+namespace Microsoft.PowerShell.EditorServices.Utility
+{
+    /// <summary>
+    /// General common utilities for AST visitors to prevent reimplementation.
+    /// </summary>
+    internal static class VisitorUtils
+    {
+        /// <summary>
+        /// Calculates the start line and column of the actual function name in a function definition AST.
+        /// </summary>
+        /// <param name="ast">A FunctionDefinitionAst object in the script's AST</param>
+        /// <returns>A tuple with start column and line for the function name</returns>
+        internal static (int startColumn, int startLine) GetNameStartColumnAndLineNumbersFromAst(FunctionDefinitionAst ast)
+        {
+            int startColumnNumber = ast.Extent.StartColumnNumber;
+            int startLineNumber = ast.Extent.StartLineNumber;
+            int astOffset = ast.IsFilter ? "filter".Length : ast.IsWorkflow ? "workflow".Length : "function".Length;
+            string astText = ast.Extent.Text;
+            // The line offset represents the offset on the line that we're on where as
+            // astOffset is the offset on the entire text of the AST.
+            int lineOffset = astOffset;
+            for (; astOffset < astText.Length; astOffset++, lineOffset++)
+            {
+                if (astText[astOffset] == '\n')
+                {
+                    // reset numbers since we are operating on a different line and increment the line number.
+                    startColumnNumber = 0;
+                    startLineNumber++;
+                    lineOffset = 0;
+                }
+                else if (astText[astOffset] == '\r')
+                {
+                    // Do nothing with carriage returns... we only look for line feeds since those
+                    // are used on every platform.
+                }
+                else if (!char.IsWhiteSpace(astText[astOffset]))
+                {
+                    // This is the start of the function name so we've found our start column and line number.
+                    break;
+                }
+            }
+
+            return (startColumnNumber + lineOffset, startLineNumber);
+        }
+    }
+}
diff --git a/test/PowerShellEditorServices.Test/Services/Symbols/AstOperationsTests.cs b/test/PowerShellEditorServices.Test/Services/Symbols/AstOperationsTests.cs
@@ -35,23 +35,30 @@ function          FunctionWithExtraSpace
 
 
     FunctionNameOnDifferentLine
+
+            function IndentedFunction  { } IndentedFunction
 ";
         private static readonly ScriptBlockAst s_ast = (ScriptBlockAst)ScriptBlock.Create(s_scriptString).Ast;
 
         [Theory]
+        [InlineData(1, 15, "BasicFunction")]
         [InlineData(2, 3, "BasicFunction")]
+        [InlineData(4, 31, "FunctionWithExtraSpace")]
         [InlineData(7, 18, "FunctionWithExtraSpace")]
+        [InlineData(12, 22, "FunctionNameOnDifferentLine")]
         [InlineData(22, 13, "FunctionNameOnDifferentLine")]
-        public void CanFindSymbolAtPostion(int lineNumber, int columnNumber, string expectedName)
+        [InlineData(24, 30, "IndentedFunction")]
+        [InlineData(24, 52, "IndentedFunction")]
+        public void CanFindSymbolAtPosition(int lineNumber, int columnNumber, string expectedName)
         {
             SymbolReference reference = AstOperations.FindSymbolAtPosition(s_ast, lineNumber, columnNumber);
             Assert.NotNull(reference);
             Assert.Equal(expectedName, reference.SymbolName);
         }
 
         [Theory]
-        [MemberData(nameof(FindReferencesOfSymbolAtPostionData))]
-        public void CanFindReferencesOfSymbolAtPostion(int lineNumber, int columnNumber, Position[] positions)
+        [MemberData(nameof(FindReferencesOfSymbolAtPositionData))]
+        public void CanFindReferencesOfSymbolAtPosition(int lineNumber, int columnNumber, Range[] symbolRange)
         {
             SymbolReference symbol = AstOperations.FindSymbolAtPosition(s_ast, lineNumber, columnNumber);
 
@@ -60,18 +67,25 @@ public void CanFindReferencesOfSymbolAtPostion(int lineNumber, int columnNumber,
             int positionsIndex = 0;
             foreach (SymbolReference reference in references)
             {
-                Assert.Equal(positions[positionsIndex].Line, reference.ScriptRegion.StartLineNumber);
-                Assert.Equal(positions[positionsIndex].Character, reference.ScriptRegion.StartColumnNumber);
+                Assert.Equal(symbolRange[positionsIndex].Start.Line, reference.ScriptRegion.StartLineNumber);
+                Assert.Equal(symbolRange[positionsIndex].Start.Character, reference.ScriptRegion.StartColumnNumber);
+                Assert.Equal(symbolRange[positionsIndex].End.Line, reference.ScriptRegion.EndLineNumber);
+                Assert.Equal(symbolRange[positionsIndex].End.Character, reference.ScriptRegion.EndColumnNumber);
 
                 positionsIndex++;
             }
         }
 
-        public static object[][] FindReferencesOfSymbolAtPostionData { get; } = new object[][]
+        public static object[][] FindReferencesOfSymbolAtPositionData { get; } = new object[][]
         {
-            new object[] { 2, 3, new[] { new Position(1, 10), new Position(2, 1) } },
-            new object[] { 7, 18, new[] { new Position(4, 19), new Position(7, 3) } },
-            new object[] { 22, 13, new[] { new Position(12, 8), new Position(22, 5) } },
+            new object[] { 1, 15, new[] { new Range(1, 10, 1, 23), new Range(2, 1, 2, 14) } },
+            new object[] { 2, 3, new[] { new Range(1, 10, 1, 23), new Range(2, 1, 2, 14) } },
+            new object[] { 4, 31, new[] { new Range(4, 19, 4, 41), new Range(7, 3, 7, 25) } },
+            new object[] { 7, 18, new[] { new Range(4, 19, 4, 41), new Range(7, 3, 7, 25) } },
+            new object[] { 22, 13, new[] { new Range(12, 8, 12, 35), new Range(22, 5, 22, 32) } },
+            new object[] { 12, 22, new[] { new Range(12, 8, 12, 35), new Range(22, 5, 22, 32) } },
+            new object[] { 24, 30, new[] { new Range(24, 22, 24, 38), new Range(24, 44, 24, 60) } },
+            new object[] { 24, 52, new[] { new Range(24, 22, 24, 38), new Range(24, 44, 24, 60) } },
         };
     }
 }