Skip to content

Commit e4744ca

Browse files
glennsartiglennsarti
committed
(PowerShellGH-813) Refactor the FoldingReference arrays and lists into it's own class
Previously the folding provider created many intermediate arrays and lists and required post-processing. This commit changes the behaviour to use an accumlator patter with an extended Dictionary class. This new class adds a `SafeAdd` method to add FoldingRanges, which then has the logic to determine if the range should indeed be added, for example, passing nulls or pre-existing larger ranges. By passing around this list using ByReference we can avoid creating many objects which are just then thrown away. This commit also moves the ShowLastLine code from the FoldingProvider into the Language Server. This reduces the number of array enumerations to one.
1 parent 73b6030 commit e4744ca

File tree

7 files changed

+63
-56
lines changed

7 files changed

+63
-56
lines changed

‎src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1387,7 +1387,7 @@ private FoldingRange[] Fold(
13871387
int endLineOffset = this.currentSettings.CodeFolding.ShowLastLine ? -1 : 0;
13881388
foreach (FoldingReference fold in FoldingOperations.FoldableRegions(
13891389
script.ScriptTokens,
1390-
script.ScriptAst))
1390+
script.ScriptAst).ToArray())
13911391
{
13921392
result.Add(new FoldingRange {
13931393
EndCharacter = fold.EndCharacter,

‎src/PowerShellEditorServices/Language/AstOperations.cs

+3-4
Original file line numberDiff line numberDiff line change
@@ -335,13 +335,12 @@ static public string[] FindDotSourcedIncludes(Ast scriptAst, string psScriptRoot
335335
/// Finds all foldable regions in a script based on AST
336336
/// </summary>
337337
/// <param name="scriptAst">The abstract syntax tree of the given script</param>
338+
/// <param name="refList">The FoldingReferenceList object to add the folds to</param>
338339
/// <returns>A collection of FoldingReference objects</returns>
339-
public static IEnumerable<FoldingReference> FindFoldsInDocument(Ast scriptAst)
340+
public static void FindFoldsInDocument(Ast scriptAst, ref FoldingReferenceList refList)
340341
{
341-
FindFoldsVisitor findFoldsVisitor = new FindFoldsVisitor();
342+
FindFoldsVisitor findFoldsVisitor = new FindFoldsVisitor(ref refList);
342343
scriptAst.Visit(findFoldsVisitor);
343-
344-
return findFoldsVisitor.FoldableRegions;
345344
}
346345

347346
}

‎src/PowerShellEditorServices/Language/FindFoldsVisitor.cs

+12-12
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,11 @@ internal class FindFoldsVisitor : AstVisitor2
1616
{
1717
private const string RegionKindNone = null;
1818

19-
public List<FoldingReference> FoldableRegions { get; }
19+
private FoldingReferenceList _refList;
2020

21-
public FindFoldsVisitor()
21+
public FindFoldsVisitor(ref FoldingReferenceList refList)
2222
{
23-
FoldableRegions = new List<FoldingReference>();
23+
_refList = refList;
2424
}
2525

2626
/// <summary>
@@ -56,7 +56,7 @@ public override AstVisitAction VisitArrayExpression(ArrayExpressionAst objAst)
5656
{
5757
if (IsValidFoldingExtent(objAst.Extent))
5858
{
59-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
59+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
6060
}
6161
return AstVisitAction.Continue;
6262
}
@@ -65,7 +65,7 @@ public override AstVisitAction VisitHashtable(HashtableAst objAst)
6565
{
6666
if (IsValidFoldingExtent(objAst.Extent))
6767
{
68-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
68+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
6969
}
7070
return AstVisitAction.Continue;
7171
}
@@ -77,7 +77,7 @@ public override AstVisitAction VisitStatementBlock(StatementBlockAst objAst)
7777
if (objAst.Parent is ArrayExpressionAst) { return AstVisitAction.Continue; }
7878
if (IsValidFoldingExtent(objAst.Extent))
7979
{
80-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
80+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
8181
}
8282
return AstVisitAction.Continue;
8383
}
@@ -90,7 +90,7 @@ public override AstVisitAction VisitScriptBlock(ScriptBlockAst objAst)
9090
if (objAst.Parent is ScriptBlockExpressionAst) { return AstVisitAction.Continue; }
9191
if (IsValidFoldingExtent(objAst.Extent))
9292
{
93-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
93+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
9494
}
9595
return AstVisitAction.Continue;
9696
}
@@ -107,7 +107,7 @@ public override AstVisitAction VisitScriptBlockExpression(ScriptBlockExpressionA
107107
foldRef.StartCharacter--;
108108
foldRef.EndCharacter++;
109109
}
110-
FoldableRegions.Add(foldRef);
110+
_refList.SafeAdd(foldRef);
111111
}
112112
return AstVisitAction.Continue;
113113
}
@@ -116,7 +116,7 @@ public override AstVisitAction VisitStringConstantExpression(StringConstantExpre
116116
{
117117
if (IsValidFoldingExtent(objAst.Extent))
118118
{
119-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
119+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
120120
}
121121

122122
return AstVisitAction.Continue;
@@ -126,7 +126,7 @@ public override AstVisitAction VisitSubExpression(SubExpressionAst objAst)
126126
{
127127
if (IsValidFoldingExtent(objAst.Extent))
128128
{
129-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
129+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
130130
}
131131
return AstVisitAction.Continue;
132132
}
@@ -135,7 +135,7 @@ public override AstVisitAction VisitTypeDefinition(TypeDefinitionAst objAst)
135135
{
136136
if (IsValidFoldingExtent(objAst.Extent))
137137
{
138-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
138+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
139139
}
140140
return AstVisitAction.Continue;
141141
}
@@ -144,7 +144,7 @@ public override AstVisitAction VisitVariableExpression(VariableExpressionAst obj
144144
{
145145
if (IsValidFoldingExtent(objAst.Extent))
146146
{
147-
FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone));
147+
_refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone));
148148
}
149149
return AstVisitAction.Continue;
150150
}

‎src/PowerShellEditorServices/Language/FoldingOperations.cs

+5-24
Original file line numberDiff line numberDiff line change
@@ -18,38 +18,19 @@ internal static class FoldingOperations
1818
/// Extracts all of the unique foldable regions in a script given a script AST and the list tokens
1919
/// used to generate the AST
2020
/// </summary>
21-
internal static FoldingReference[] FoldableRegions(
21+
internal static FoldingReferenceList FoldableRegions(
2222
Token[] tokens,
2323
Ast scriptAst)
2424
{
25-
var foldableRegions = new List<FoldingReference>();
25+
var foldableRegions = new FoldingReferenceList();
2626

2727
// Add regions from AST
28-
foldableRegions.AddRange(AstOperations.FindFoldsInDocument(scriptAst));
28+
AstOperations.FindFoldsInDocument(scriptAst, ref foldableRegions);
2929

3030
// Add regions from Tokens
31-
foldableRegions.AddRange(TokenOperations.FoldableRegions(tokens));
31+
TokenOperations.FoldableRegions(tokens, ref foldableRegions);
3232

33-
// Sort the FoldingReferences, starting at the top of the document,
34-
// and ensure that, in the case of multiple ranges starting the same line,
35-
// that the largest range (i.e. most number of lines spanned) is sorted
36-
// first. This is needed to detect duplicate regions. The first in the list
37-
// will be used and subsequent duplicates ignored.
38-
foldableRegions.Sort();
39-
40-
// It's possible to have duplicate or overlapping ranges, that is, regions which have the same starting
41-
// line number as the previous region. Therefore only emit ranges which have a different starting line
42-
// than the previous range.
43-
foldableRegions.RemoveAll( (FoldingReference item) => {
44-
// Note - I'm not happy with searching here, but as the RemoveAll
45-
// doesn't expose the index in the List, we need to calculate it. Fortunately the
46-
// list is sorted at this point, so we can use BinarySearch.
47-
int index = foldableRegions.BinarySearch(item);
48-
if (index == 0) { return false; }
49-
return (item.StartLine == foldableRegions[index - 1].StartLine);
50-
});
51-
52-
return foldableRegions.ToArray();
33+
return foldableRegions;
5334
}
5435
}
5536
}

‎src/PowerShellEditorServices/Language/FoldingReference.cs

+30
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
//
55

66
using System;
7+
using System.Collections.Generic;
78

89
namespace Microsoft.PowerShell.EditorServices
910
{
@@ -60,4 +61,33 @@ public int CompareTo(FoldingReference that) {
6061
return string.Compare(this.Kind, that.Kind);
6162
}
6263
}
64+
65+
/// <summary>
66+
/// A class that holds a list of FoldingReferences and ensures that when adding a reference that the
67+
/// folding rules are obeyed, e.g. Only one fold per start line
68+
/// </summary>
69+
public class FoldingReferenceList : Dictionary<int, FoldingReference>
70+
{
71+
/// <summary>
72+
/// Adds a FoldingReference to the list and enforces ordering rules e.g. Only one fold per start line
73+
/// </summary>
74+
public void SafeAdd(FoldingReference item)
75+
{
76+
if (item == null) { return; }
77+
FoldingReference currentItem;
78+
TryGetValue(item.StartLine, out currentItem);
79+
// Only add the item if it hasn't been seen before or it's the largest range
80+
if ((currentItem == null) || (currentItem.CompareTo(item) == 1)) { this[item.StartLine] = item; }
81+
}
82+
83+
/// <summary>
84+
/// Helper method to easily convert the Dictionary Values into an array
85+
/// </summary>
86+
public FoldingReference[] ToArray()
87+
{
88+
var result = new FoldingReference[Count];
89+
Values.CopyTo(result, 0);
90+
return result;
91+
}
92+
}
6393
}

‎src/PowerShellEditorServices/Language/TokenOperations.cs

+7-13
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,10 @@ internal static class TokenOperations
3333
/// <summary>
3434
/// Extracts all of the unique foldable regions in a script given the list tokens
3535
/// </summary>
36-
internal static FoldingReference[] FoldableRegions(
37-
Token[] tokens)
36+
internal static void FoldableRegions(
37+
Token[] tokens,
38+
ref FoldingReferenceList refList)
3839
{
39-
var foldableRegions = new List<FoldingReference>();
4040
var tokenCommentRegionStack = new Stack<Token>();
4141
Token blockStartToken = null;
4242
int blockNextLine = -1;
@@ -48,8 +48,7 @@ internal static FoldingReference[] FoldableRegions(
4848
// Processing for comment regions <# -> #>
4949
if (token.Extent.StartLineNumber != token.Extent.EndLineNumber)
5050
{
51-
FoldingReference foldRef = CreateFoldingReference(token, token, RegionKindComment);
52-
if (foldRef != null) { foldableRegions.Add(foldRef); }
51+
refList.SafeAdd(CreateFoldingReference(token, token, RegionKindComment));
5352
continue;
5453
}
5554

@@ -66,8 +65,7 @@ internal static FoldingReference[] FoldableRegions(
6665
// Mismatched regions in the script can cause bad stacks.
6766
if (tokenCommentRegionStack.Count > 0)
6867
{
69-
FoldingReference foldRef = CreateFoldingReference(tokenCommentRegionStack.Pop(), token, RegionKindRegion);
70-
if (foldRef != null) { foldableRegions.Add(foldRef); }
68+
refList.SafeAdd(CreateFoldingReference(tokenCommentRegionStack.Pop(), token, RegionKindRegion));
7169
}
7270
continue;
7371
}
@@ -76,8 +74,7 @@ internal static FoldingReference[] FoldableRegions(
7674
int thisLine = token.Extent.StartLineNumber - 1;
7775
if ((blockStartToken != null) && (thisLine != blockNextLine))
7876
{
79-
FoldingReference foldRef = CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment);
80-
if (foldRef != null) { foldableRegions.Add(foldRef); }
77+
refList.SafeAdd(CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment));
8178
blockStartToken = token;
8279
}
8380
if (blockStartToken == null) { blockStartToken = token; }
@@ -87,11 +84,8 @@ internal static FoldingReference[] FoldableRegions(
8784
// comment block simply ends at the end of document
8885
if (blockStartToken != null)
8986
{
90-
FoldingReference foldRef = CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment);
91-
if (foldRef != null) { foldableRegions.Add(foldRef); }
87+
refList.SafeAdd(CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment));
9288
}
93-
94-
return foldableRegions.ToArray();
9589
}
9690

9791
/// <summary>

‎test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs

+5-2
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,12 @@ private FoldingReference[] GetRegions(string text) {
2020
text,
2121
Version.Parse("5.0"));
2222

23-
return Microsoft.PowerShell.EditorServices.FoldingOperations.FoldableRegions(
23+
var result = Microsoft.PowerShell.EditorServices.FoldingOperations.FoldableRegions(
2424
scriptFile.ScriptTokens,
25-
scriptFile.ScriptAst);
25+
scriptFile.ScriptAst).ToArray();
26+
// The foldable regions need to be deterministic for testing so sort the array.
27+
Array.Sort(result);
28+
return result;
2629
}
2730

2831
/// <summary>

0 commit comments

Comments
 (0)