(PowerShellGH-813) Refactor the FoldingReference arrays into it's own class

glennsarti · glennsarti · commit e2dc02b4f082 · 2018-12-13T11:04:44.000+08:00
TODO ..  Basically stop creating intermediate arrays and just use an extended Hash object
diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs
@@ -1388,7 +1388,7 @@ private FoldingRange[] Fold(
             if (this.currentSettings.CodeFolding.ShowLastLine) { endLineOffset = -1; }
             foreach (FoldingReference fold in FoldingOperations.FoldableRegions(
                 script.ScriptTokens,
-                script.ScriptAst))
+                script.ScriptAst).ToArray())
             {
                 result.Add(new FoldingRange {
                     EndCharacter   = fold.EndCharacter,
diff --git a/src/PowerShellEditorServices/Language/AstOperations.cs b/src/PowerShellEditorServices/Language/AstOperations.cs
@@ -336,12 +336,10 @@ static public string[] FindDotSourcedIncludes(Ast scriptAst, string psScriptRoot
         /// </summary>
         /// <param name="scriptAst">The abstract syntax tree of the given script</param>
         /// <returns>A collection of FoldingReference objects</returns>
-        public static IEnumerable<FoldingReference> FindFoldsInDocument(Ast scriptAst)
+        public static void FindFoldsInDocument(Ast scriptAst, ref FoldingReferenceList refList)
         {
-            FindFoldsVisitor findFoldsVisitor = new FindFoldsVisitor();
+            FindFoldsVisitor findFoldsVisitor = new FindFoldsVisitor(ref refList);
             scriptAst.Visit(findFoldsVisitor);
-
-            return findFoldsVisitor.FoldableRegions;
         }
 
     }
diff --git a/src/PowerShellEditorServices/Language/FindFoldsVisitor.cs b/src/PowerShellEditorServices/Language/FindFoldsVisitor.cs
@@ -16,11 +16,11 @@ internal class FindFoldsVisitor : AstVisitor
     {
         private const string RegionKindNone = null;
 
-        public List<FoldingReference> FoldableRegions { get; }
+        private FoldingReferenceList _refList;
 
-        public FindFoldsVisitor()
+        public FindFoldsVisitor(ref FoldingReferenceList refList)
         {
-            this.FoldableRegions = new List<FoldingReference>();
+            this._refList = refList;
         }
 
         /// <summary>
@@ -55,7 +55,7 @@ public override AstVisitAction VisitArrayExpression(ArrayExpressionAst objAst)
         {
             if (IsValidFoldingExtent(objAst.Extent))
             {
-                this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
+                this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
             }
             return AstVisitAction.Continue;
         }
@@ -64,7 +64,7 @@ public override AstVisitAction VisitHashtable(HashtableAst objAst)
         {
             if (IsValidFoldingExtent(objAst.Extent))
             {
-                this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
+                this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
             }
             return AstVisitAction.Continue;
         }
@@ -76,7 +76,7 @@ public override AstVisitAction VisitStatementBlock(StatementBlockAst objAst)
             if (objAst.Parent is ArrayExpressionAst) { return AstVisitAction.Continue; }
             if (IsValidFoldingExtent(objAst.Extent))
             {
-                this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
+                this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
             }
             return AstVisitAction.Continue;
         }
@@ -89,7 +89,7 @@ public override AstVisitAction VisitScriptBlock(ScriptBlockAst objAst)
             if (objAst.Parent is ScriptBlockExpressionAst) { return AstVisitAction.Continue; }
             if (IsValidFoldingExtent(objAst.Extent))
             {
-                this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
+                this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
             }
             return AstVisitAction.Continue;
         }
@@ -106,7 +106,7 @@ public override AstVisitAction VisitScriptBlockExpression(ScriptBlockExpressionA
                     foldRef.StartCharacter--;
                     foldRef.EndCharacter++;
                 }
-                this.FoldableRegions.Add(foldRef);
+                this._refList.SafeAdd(foldRef);
             }
             return AstVisitAction.Continue;
         }
@@ -115,7 +115,7 @@ public override AstVisitAction VisitStringConstantExpression(StringConstantExpre
         {
             if (IsValidFoldingExtent(objAst.Extent))
             {
-                this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
+                this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
             }
 
             return AstVisitAction.Continue;
@@ -125,7 +125,7 @@ public override AstVisitAction VisitSubExpression(SubExpressionAst objAst)
         {
             if (IsValidFoldingExtent(objAst.Extent))
             {
-                this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
+                this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
             }
             return AstVisitAction.Continue;
         }
@@ -134,7 +134,7 @@ public override AstVisitAction VisitVariableExpression(VariableExpressionAst obj
         {
             if (IsValidFoldingExtent(objAst.Extent))
             {
-                this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
+                this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
             }
             return AstVisitAction.Continue;
         }
diff --git a/src/PowerShellEditorServices/Language/FoldingOperations.cs b/src/PowerShellEditorServices/Language/FoldingOperations.cs
@@ -18,38 +18,19 @@ internal static class FoldingOperations
         /// Extracts all of the unique foldable regions in a script given a script AST and the list tokens
         /// used to generate the AST
         /// </summary>
-        internal static FoldingReference[] FoldableRegions(
+        internal static FoldingReferenceList FoldableRegions(
             Token[] tokens,
             Ast scriptAst)
         {
-            List<FoldingReference> foldableRegions = new List<FoldingReference>();
+            var foldableRegions = new FoldingReferenceList();
 
             // Add regions from AST
-            foldableRegions.AddRange(AstOperations.FindFoldsInDocument(scriptAst));
+            AstOperations.FindFoldsInDocument(scriptAst, ref foldableRegions);
 
             // Add regions from Tokens
-            foldableRegions.AddRange(TokenOperations.FoldableRegions(tokens));
+            TokenOperations.FoldableRegions(tokens, ref foldableRegions);
 
-            // Sort the FoldingReferences, starting at the top of the document,
-            // and ensure that, in the case of multiple ranges starting the same line,
-            // that the largest range (i.e. most number of lines spanned) is sorted
-            // first. This is needed to detect duplicate regions. The first in the list
-            // will be used and subsequent duplicates ignored.
-            foldableRegions.Sort();
-
-            // It's possible to have duplicate or overlapping ranges, that is, regions which have the same starting
-            // line number as the previous region. Therefore only emit ranges which have a different starting line
-            // than the previous range.
-            foldableRegions.RemoveAll( (FoldingReference item) => {
-                // Note - I'm not happy with searching here, but as the RemoveAll
-                // doesn't expose the index in the List, we need to calculate it. Fortunately the
-                // list is sorted at this point, so we can use BinarySearch.
-                int index = foldableRegions.BinarySearch(item);
-                if (index == 0) { return false; }
-                return (item.StartLine == foldableRegions[index - 1].StartLine);
-            });
-
-            return foldableRegions.ToArray();
+            return foldableRegions;
         }
     }
 }
diff --git a/src/PowerShellEditorServices/Language/FoldingReference.cs b/src/PowerShellEditorServices/Language/FoldingReference.cs
@@ -4,6 +4,7 @@
 //
 
 using System;
+using System.Collections.Generic;
 
 namespace Microsoft.PowerShell.EditorServices
 {
@@ -60,4 +61,33 @@ public int CompareTo(FoldingReference that) {
             return string.Compare(this.Kind, that.Kind);
         }
     }
+
+    /// <summary>
+    /// A class that holds a list of FoldingReferences and ensures that when adding a reference that the
+    /// folding rules are obeyed, e.g. Only one fold per start line
+    /// </summary>
+    public class FoldingReferenceList: System.Collections.Generic.Dictionary<int, FoldingReference>
+    {
+        /// <summary>
+        /// Adds a FoldingReference to the list and enforces ordering rules e.g. Only one fold per start line
+        /// </summary>
+        public void SafeAdd(FoldingReference item)
+        {
+            if (item == null) { return; }
+            FoldingReference currentItem;
+            this.TryGetValue(item.StartLine, out currentItem);
+            // Only add the item if it hasn't been seen before or it's the largest range
+            if ((currentItem == null) || (currentItem.CompareTo(item) == 1)) { this[item.StartLine] = item; }
+        }
+
+        /// <summary>
+        /// Helper method to easily convert the Dictionary Values into an array
+        /// </summary>
+        public FoldingReference[] ToArray()
+        {
+            var result = new FoldingReference[this.Count];
+            this.Values.CopyTo(result, 0);
+            return result;
+        }
+    }
 }
diff --git a/src/PowerShellEditorServices/Language/TokenOperations.cs b/src/PowerShellEditorServices/Language/TokenOperations.cs
@@ -33,10 +33,10 @@ internal static class TokenOperations
         /// <summary>
         /// Extracts all of the unique foldable regions in a script given the list tokens
         /// </summary>
-        internal static FoldingReference[] FoldableRegions(
-            Token[] tokens)
+        internal static void FoldableRegions(
+            Token[] tokens,
+            ref FoldingReferenceList refList)
         {
-            var foldableRegions = new List<FoldingReference>();
             var tokenCommentRegionStack = new Stack<Token>();
             Token blockStartToken = null;
             int blockNextLine = -1;
@@ -48,8 +48,7 @@ internal static FoldingReference[] FoldableRegions(
                 // Processing for comment regions <# -> #>
                 if (token.Extent.StartLineNumber != token.Extent.EndLineNumber)
                 {
-                    FoldingReference foldRef = CreateFoldingReference(token, token, RegionKindComment);
-                    if (foldRef != null) { foldableRegions.Add(foldRef); }
+                    refList.SafeAdd(CreateFoldingReference(token, token, RegionKindComment));
                     continue;
                 }
 
@@ -67,8 +66,7 @@ internal static FoldingReference[] FoldableRegions(
                         // Mismatched regions in the script can cause bad stacks.
                         if (tokenCommentRegionStack.Count > 0)
                         {
-                            FoldingReference foldRef = CreateFoldingReference(tokenCommentRegionStack.Pop(), token, RegionKindRegion);
-                            if (foldRef != null) { foldableRegions.Add(foldRef); }
+                            refList.SafeAdd(CreateFoldingReference(tokenCommentRegionStack.Pop(), token, RegionKindRegion));
                         }
                         continue;
                     }
@@ -77,8 +75,7 @@ internal static FoldingReference[] FoldableRegions(
                     int thisLine = token.Extent.StartLineNumber - 1;
                     if ((blockStartToken != null) && (thisLine != blockNextLine))
                     {
-                        FoldingReference foldRef = CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment);
-                        if (foldRef != null) { foldableRegions.Add(foldRef); }
+                        refList.SafeAdd(CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment));
                         blockStartToken = token;
                     }
                     if (blockStartToken == null) { blockStartToken = token; }
@@ -89,11 +86,8 @@ internal static FoldingReference[] FoldableRegions(
             // comment block simply ends at the end of document
             if (blockStartToken != null)
             {
-                FoldingReference foldRef = CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment);
-                if (foldRef != null) { foldableRegions.Add(foldRef); }
+                refList.SafeAdd(CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment));
             }
-
-            return foldableRegions.ToArray();
         }
 
         /// <summary>
diff --git a/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs b/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs
@@ -20,9 +20,12 @@ private FoldingReference[] GetRegions(string text) {
                 text,
                 Version.Parse("5.0"));
 
-            return Microsoft.PowerShell.EditorServices.FoldingOperations.FoldableRegions(
+            var result = Microsoft.PowerShell.EditorServices.FoldingOperations.FoldableRegions(
                 scriptFile.ScriptTokens,
-                scriptFile.ScriptAst);
+                scriptFile.ScriptAst).ToArray();
+            // The foldable regions need to be deterministic for testing so sort the array.
+            Array.Sort(result);
+            return result;
         }
 
         /// <summary>

Original file line number	Diff line number	Diff line change
`@@ -1388,7 +1388,7 @@ private FoldingRange[] Fold(`
`1388`	`1388`	`if (this.currentSettings.CodeFolding.ShowLastLine) { endLineOffset = -1; }`
`1389`	`1389`	`foreach (FoldingReference fold in FoldingOperations.FoldableRegions(`
`1390`	`1390`	`script.ScriptTokens,`
`1391`		`- script.ScriptAst))`
	`1391`	`+ script.ScriptAst).ToArray())`
`1392`	`1392`	`{`
`1393`	`1393`	`result.Add(new FoldingRange {`
`1394`	`1394`	`EndCharacter = fold.EndCharacter,`
Original file line number	Diff line number	Diff line change
`@@ -336,12 +336,10 @@ static public string[] FindDotSourcedIncludes(Ast scriptAst, string psScriptRoot`
`336`	`336`	`/// </summary>`
`337`	`337`	`/// <param name="scriptAst">The abstract syntax tree of the given script</param>`
`338`	`338`	`/// <returns>A collection of FoldingReference objects</returns>`
`339`		`- public static IEnumerable<FoldingReference> FindFoldsInDocument(Ast scriptAst)`
	`339`	`+ public static void FindFoldsInDocument(Ast scriptAst, ref FoldingReferenceList refList)`
`340`	`340`	`{`
`341`		`- FindFoldsVisitor findFoldsVisitor = new FindFoldsVisitor();`
	`341`	`+ FindFoldsVisitor findFoldsVisitor = new FindFoldsVisitor(ref refList);`
`342`	`342`	`scriptAst.Visit(findFoldsVisitor);`
`343`		`-`
`344`		`- return findFoldsVisitor.FoldableRegions;`
`345`	`343`	`}`
`346`	`344`
`347`	`345`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,11 +16,11 @@ internal class FindFoldsVisitor : AstVisitor`
`16`	`16`	`{`
`17`	`17`	`private const string RegionKindNone = null;`
`18`	`18`
`19`		`- public List<FoldingReference> FoldableRegions { get; }`
	`19`	`+ private FoldingReferenceList _refList;`
`20`	`20`
`21`		`- public FindFoldsVisitor()`
	`21`	`+ public FindFoldsVisitor(ref FoldingReferenceList refList)`
`22`	`22`	`{`
`23`		`- this.FoldableRegions = new List<FoldingReference>();`
	`23`	`+ this._refList = refList;`
`24`	`24`	`}`
`25`	`25`
`26`	`26`	`/// <summary>`
`@@ -55,7 +55,7 @@ public override AstVisitAction VisitArrayExpression(ArrayExpressionAst objAst)`
`55`	`55`	`{`
`56`	`56`	`if (IsValidFoldingExtent(objAst.Extent))`
`57`	`57`	`{`
`58`		`- this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));`
	`58`	`+ this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));`
`59`	`59`	`}`
`60`	`60`	`return AstVisitAction.Continue;`
`61`	`61`	`}`
`@@ -64,7 +64,7 @@ public override AstVisitAction VisitHashtable(HashtableAst objAst)`
`64`	`64`	`{`
`65`	`65`	`if (IsValidFoldingExtent(objAst.Extent))`
`66`	`66`	`{`
`67`		`- this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));`
	`67`	`+ this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));`
`68`	`68`	`}`
`69`	`69`	`return AstVisitAction.Continue;`
`70`	`70`	`}`
`@@ -76,7 +76,7 @@ public override AstVisitAction VisitStatementBlock(StatementBlockAst objAst)`
`76`	`76`	`if (objAst.Parent is ArrayExpressionAst) { return AstVisitAction.Continue; }`
`77`	`77`	`if (IsValidFoldingExtent(objAst.Extent))`
`78`	`78`	`{`
`79`		`- this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));`
	`79`	`+ this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));`
`80`	`80`	`}`
`81`	`81`	`return AstVisitAction.Continue;`
`82`	`82`	`}`
`@@ -89,7 +89,7 @@ public override AstVisitAction VisitScriptBlock(ScriptBlockAst objAst)`
`89`	`89`	`if (objAst.Parent is ScriptBlockExpressionAst) { return AstVisitAction.Continue; }`
`90`	`90`	`if (IsValidFoldingExtent(objAst.Extent))`
`91`	`91`	`{`
`92`		`- this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));`
	`92`	`+ this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));`
`93`	`93`	`}`
`94`	`94`	`return AstVisitAction.Continue;`
`95`	`95`	`}`
`@@ -106,7 +106,7 @@ public override AstVisitAction VisitScriptBlockExpression(ScriptBlockExpressionA`
`106`	`106`	`foldRef.StartCharacter--;`
`107`	`107`	`foldRef.EndCharacter++;`
`108`	`108`	`}`
`109`		`- this.FoldableRegions.Add(foldRef);`
	`109`	`+ this._refList.SafeAdd(foldRef);`
`110`	`110`	`}`
`111`	`111`	`return AstVisitAction.Continue;`
`112`	`112`	`}`
`@@ -115,7 +115,7 @@ public override AstVisitAction VisitStringConstantExpression(StringConstantExpre`
`115`	`115`	`{`
`116`	`116`	`if (IsValidFoldingExtent(objAst.Extent))`
`117`	`117`	`{`
`118`		`- this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));`
	`118`	`+ this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));`
`119`	`119`	`}`
`120`	`120`
`121`	`121`	`return AstVisitAction.Continue;`
`@@ -125,7 +125,7 @@ public override AstVisitAction VisitSubExpression(SubExpressionAst objAst)`
`125`	`125`	`{`
`126`	`126`	`if (IsValidFoldingExtent(objAst.Extent))`
`127`	`127`	`{`
`128`		`- this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));`
	`128`	`+ this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));`
`129`	`129`	`}`
`130`	`130`	`return AstVisitAction.Continue;`
`131`	`131`	`}`
`@@ -134,7 +134,7 @@ public override AstVisitAction VisitVariableExpression(VariableExpressionAst obj`
`134`	`134`	`{`
`135`	`135`	`if (IsValidFoldingExtent(objAst.Extent))`
`136`	`136`	`{`
`137`		`- this.FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));`
	`137`	`+ this._refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));`
`138`	`138`	`}`
`139`	`139`	`return AstVisitAction.Continue;`
`140`	`140`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,10 +33,10 @@ internal static class TokenOperations`
`33`	`33`	`/// <summary>`
`34`	`34`	`/// Extracts all of the unique foldable regions in a script given the list tokens`
`35`	`35`	`/// </summary>`
`36`		`- internal static FoldingReference[] FoldableRegions(`
`37`		`- Token[] tokens)`
	`36`	`+ internal static void FoldableRegions(`
	`37`	`+ Token[] tokens,`
	`38`	`+ ref FoldingReferenceList refList)`
`38`	`39`	`{`
`39`		`- var foldableRegions = new List<FoldingReference>();`
`40`	`40`	`var tokenCommentRegionStack = new Stack<Token>();`
`41`	`41`	`Token blockStartToken = null;`
`42`	`42`	`int blockNextLine = -1;`
`@@ -48,8 +48,7 @@ internal static FoldingReference[] FoldableRegions(`
`48`	`48`	`// Processing for comment regions <# -> #>`
`49`	`49`	`if (token.Extent.StartLineNumber != token.Extent.EndLineNumber)`
`50`	`50`	`{`
`51`		`- FoldingReference foldRef = CreateFoldingReference(token, token, RegionKindComment);`
`52`		`- if (foldRef != null) { foldableRegions.Add(foldRef); }`
	`51`	`+ refList.SafeAdd(CreateFoldingReference(token, token, RegionKindComment));`
`53`	`52`	`continue;`
`54`	`53`	`}`
`55`	`54`
`@@ -67,8 +66,7 @@ internal static FoldingReference[] FoldableRegions(`
`67`	`66`	`// Mismatched regions in the script can cause bad stacks.`
`68`	`67`	`if (tokenCommentRegionStack.Count > 0)`
`69`	`68`	`{`
`70`		`- FoldingReference foldRef = CreateFoldingReference(tokenCommentRegionStack.Pop(), token, RegionKindRegion);`
`71`		`- if (foldRef != null) { foldableRegions.Add(foldRef); }`
	`69`	`+ refList.SafeAdd(CreateFoldingReference(tokenCommentRegionStack.Pop(), token, RegionKindRegion));`
`72`	`70`	`}`
`73`	`71`	`continue;`
`74`	`72`	`}`
`@@ -77,8 +75,7 @@ internal static FoldingReference[] FoldableRegions(`
`77`	`75`	`int thisLine = token.Extent.StartLineNumber - 1;`
`78`	`76`	`if ((blockStartToken != null) && (thisLine != blockNextLine))`
`79`	`77`	`{`
`80`		`- FoldingReference foldRef = CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment);`
`81`		`- if (foldRef != null) { foldableRegions.Add(foldRef); }`
	`78`	`+ refList.SafeAdd(CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment));`
`82`	`79`	`blockStartToken = token;`
`83`	`80`	`}`
`84`	`81`	`if (blockStartToken == null) { blockStartToken = token; }`
`@@ -89,11 +86,8 @@ internal static FoldingReference[] FoldableRegions(`
`89`	`86`	`// comment block simply ends at the end of document`
`90`	`87`	`if (blockStartToken != null)`
`91`	`88`	`{`
`92`		`- FoldingReference foldRef = CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment);`
`93`		`- if (foldRef != null) { foldableRegions.Add(foldRef); }`
	`89`	`+ refList.SafeAdd(CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment));`
`94`	`90`	`}`
`95`		`-`
`96`		`- return foldableRegions.ToArray();`
`97`	`91`	`}`
`98`	`92`
`99`	`93`	`/// <summary>`