From 73b6030f8b0af2e8b3917c0a31735da1371d85ca Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Fri, 7 Dec 2018 18:02:35 +0800 Subject: [PATCH 1/2] (GH-813) Use AST for code folding The AST contains the most correct version of how a script is interpreted. This includes regions of text. Currently the code folder only uses the Tokens which requires the folder to re-implement some of the AST behaviour e.g. matching token pairs for arrays etc. The code folder should be implemented using as much of the AST as possible. This commit; * Moves most of the region detection to use the AST Extents and uses a new FindFoldsASTVisitor. * Modifies the tests and language server to use the new method fold detection class. * Moved the code to modify the end line of folding regions to the language server code. --- .../Server/LanguageServer.cs | 17 +- .../Language/AstOperations.cs | 14 + .../Language/FindFoldsVisitor.cs | 152 ++++++++++ .../Language/FoldingOperations.cs | 55 ++++ .../Language/TokenOperations.cs | 263 ++++-------------- .../Language/TokenOperationsTests.cs | 97 ++++--- 6 files changed, 352 insertions(+), 246 deletions(-) create mode 100644 src/PowerShellEditorServices/Language/FindFoldsVisitor.cs create mode 100644 src/PowerShellEditorServices/Language/FoldingOperations.cs diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index 81fc92341..cb2443c46 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -535,7 +535,7 @@ private async Task HandleGetCommandRequestAsync( { PSCommand psCommand = new PSCommand(); if (!string.IsNullOrEmpty(param)) - { + { psCommand.AddCommand("Microsoft.PowerShell.Core\\Get-Command").AddArgument(param); } else @@ -1267,7 +1267,7 @@ protected async Task HandleCodeActionRequest( } } - // Add "show documentation" commands last so they appear at the bottom of the client UI. + // Add "show documentation" commands last so they appear at the bottom of the client UI. // These commands do not require code fixes. Sometimes we get a batch of diagnostics // to create commands for. No need to create multiple show doc commands for the same rule. var ruleNamesProcessed = new HashSet(); @@ -1382,13 +1382,16 @@ private FoldingRange[] Fold( // TODO Should be using dynamic registrations if (!this.currentSettings.CodeFolding.Enable) { return null; } var result = new List(); - foreach (FoldingReference fold in TokenOperations.FoldableRegions( - editorSession.Workspace.GetFile(documentUri).ScriptTokens, - this.currentSettings.CodeFolding.ShowLastLine)) + ScriptFile script = editorSession.Workspace.GetFile(documentUri); + // If we're showing the last line, decrement the Endline of all regions by one. + int endLineOffset = this.currentSettings.CodeFolding.ShowLastLine ? -1 : 0; + foreach (FoldingReference fold in FoldingOperations.FoldableRegions( + script.ScriptTokens, + script.ScriptAst)) { result.Add(new FoldingRange { EndCharacter = fold.EndCharacter, - EndLine = fold.EndLine, + EndLine = fold.EndLine + endLineOffset, Kind = fold.Kind, StartCharacter = fold.StartCharacter, StartLine = fold.StartLine @@ -1734,7 +1737,7 @@ await eventSender( }); } - // Generate a unique id that is used as a key to look up the associated code action (code fix) when + // Generate a unique id that is used as a key to look up the associated code action (code fix) when // we receive and process the textDocument/codeAction message. private static string GetUniqueIdFromDiagnostic(Diagnostic diagnostic) { diff --git a/src/PowerShellEditorServices/Language/AstOperations.cs b/src/PowerShellEditorServices/Language/AstOperations.cs index 9076b1cb9..7331e93d5 100644 --- a/src/PowerShellEditorServices/Language/AstOperations.cs +++ b/src/PowerShellEditorServices/Language/AstOperations.cs @@ -330,5 +330,19 @@ static public string[] FindDotSourcedIncludes(Ast scriptAst, string psScriptRoot return dotSourcedVisitor.DotSourcedFiles.ToArray(); } + + /// + /// Finds all foldable regions in a script based on AST + /// + /// The abstract syntax tree of the given script + /// A collection of FoldingReference objects + public static IEnumerable FindFoldsInDocument(Ast scriptAst) + { + FindFoldsVisitor findFoldsVisitor = new FindFoldsVisitor(); + scriptAst.Visit(findFoldsVisitor); + + return findFoldsVisitor.FoldableRegions; + } + } } diff --git a/src/PowerShellEditorServices/Language/FindFoldsVisitor.cs b/src/PowerShellEditorServices/Language/FindFoldsVisitor.cs new file mode 100644 index 000000000..9eede5db4 --- /dev/null +++ b/src/PowerShellEditorServices/Language/FindFoldsVisitor.cs @@ -0,0 +1,152 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +using System; +using System.Collections.Generic; +using System.Management.Automation.Language; + +namespace Microsoft.PowerShell.EditorServices +{ + /// + /// The visitor used to find the all folding regions in an AST + /// + internal class FindFoldsVisitor : AstVisitor2 + { + private const string RegionKindNone = null; + + public List FoldableRegions { get; } + + public FindFoldsVisitor() + { + FoldableRegions = new List(); + } + + /// + /// Returns whether an Extent could be used as a valid folding region + /// + private bool IsValidFoldingExtent( + IScriptExtent extent) + { + // The extent must span at least one line + return extent.EndLineNumber > extent.StartLineNumber; + } + + /// + /// Creates an instance of a FoldingReference object from a script extent + /// + private FoldingReference CreateFoldingReference( + IScriptExtent extent, + string matchKind) + { + // Extents are base 1, but LSP is base 0, so minus 1 off all lines and character positions + return new FoldingReference + { + StartLine = extent.StartLineNumber - 1, + StartCharacter = extent.StartColumnNumber - 1, + EndLine = extent.EndLineNumber - 1, + EndCharacter = extent.EndColumnNumber - 1, + Kind = matchKind + }; + } + + // AST object visitor methods + public override AstVisitAction VisitArrayExpression(ArrayExpressionAst objAst) + { + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitHashtable(HashtableAst objAst) + { + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitStatementBlock(StatementBlockAst objAst) + { + // These parent visitors will get this AST Object. No need to process it + if (objAst.Parent == null) { return AstVisitAction.Continue; } + if (objAst.Parent is ArrayExpressionAst) { return AstVisitAction.Continue; } + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitScriptBlock(ScriptBlockAst objAst) + { + // If the Parent object is null then this represents the entire script. We don't want to fold that + if (objAst.Parent == null) { return AstVisitAction.Continue; } + // The ScriptBlockExpressionAst visitor will get this AST Object. No need to process it + if (objAst.Parent is ScriptBlockExpressionAst) { return AstVisitAction.Continue; } + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitScriptBlockExpression(ScriptBlockExpressionAst objAst) + { + if (IsValidFoldingExtent(objAst.Extent)) { + FoldingReference foldRef = CreateFoldingReference(objAst.ScriptBlock.Extent, RegionKindNone); + if (objAst.Parent == null) { return AstVisitAction.Continue; } + if (objAst.Parent is InvokeMemberExpressionAst) { + // This is a bit naive. The ScriptBlockExpressionAst Extent does not include the actual { and } + // characters so the StartCharacter and EndCharacter indexes are out by one. This could be a bug in + // PowerShell Parser. This is just a workaround + foldRef.StartCharacter--; + foldRef.EndCharacter++; + } + FoldableRegions.Add(foldRef); + } + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitStringConstantExpression(StringConstantExpressionAst objAst) + { + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitSubExpression(SubExpressionAst objAst) + { + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitTypeDefinition(TypeDefinitionAst objAst) + { + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + return AstVisitAction.Continue; + } + + public override AstVisitAction VisitVariableExpression(VariableExpressionAst objAst) + { + if (IsValidFoldingExtent(objAst.Extent)) + { + FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + } + return AstVisitAction.Continue; + } + } +} diff --git a/src/PowerShellEditorServices/Language/FoldingOperations.cs b/src/PowerShellEditorServices/Language/FoldingOperations.cs new file mode 100644 index 000000000..063bae694 --- /dev/null +++ b/src/PowerShellEditorServices/Language/FoldingOperations.cs @@ -0,0 +1,55 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. +// + +using System; +using System.Collections.Generic; +using System.Management.Automation.Language; + +namespace Microsoft.PowerShell.EditorServices +{ + /// + /// Provides common operations for code folding in a script + /// + 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 + /// + internal static FoldingReference[] FoldableRegions( + Token[] tokens, + Ast scriptAst) + { + var foldableRegions = new List(); + + // Add regions from AST + foldableRegions.AddRange(AstOperations.FindFoldsInDocument(scriptAst)); + + // Add regions from Tokens + foldableRegions.AddRange(TokenOperations.FoldableRegions(tokens)); + + // 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(); + } + } +} diff --git a/src/PowerShellEditorServices/Language/TokenOperations.cs b/src/PowerShellEditorServices/Language/TokenOperations.cs index 3003cd6e8..4802bb85b 100644 --- a/src/PowerShellEditorServices/Language/TokenOperations.cs +++ b/src/PowerShellEditorServices/Language/TokenOperations.cs @@ -10,7 +10,6 @@ namespace Microsoft.PowerShell.EditorServices { - /// /// Provides common operations for the tokens of a parsed script. /// @@ -21,100 +20,75 @@ internal static class TokenOperations private const string RegionKindRegion = "region"; private const string RegionKindNone = null; - // Opening tokens for { } and @{ } - private static readonly TokenKind[] s_openingBraces = new [] - { - TokenKind.LCurly, - TokenKind.AtCurly - }; - - // Opening tokens for ( ), @( ), $( ) - private static readonly TokenKind[] s_openingParens = new [] - { - TokenKind.LParen, - TokenKind.AtParen, - TokenKind.DollarParen - }; + // These regular expressions are used to match lines which mark the start and end of region comment in a PowerShell + // script. They are based on the defaults in the VS Code Language Configuration at; + // https://github.com/Microsoft/vscode/blob/64186b0a26/extensions/powershell/language-configuration.json#L26-L31 + static private readonly Regex s_startRegionTextRegex = new Regex( + @"^\s*#region\b", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + static private readonly Regex s_endRegionTextRegex = new Regex( + @"^\s*#endregion\b", + RegexOptions.IgnoreCase | RegexOptions.Compiled); /// /// Extracts all of the unique foldable regions in a script given the list tokens /// internal static FoldingReference[] FoldableRegions( - Token[] tokens, - bool ShowLastLine) + Token[] tokens) { - List foldableRegions = new List(); - - // Find matching braces { -> } - // Find matching hashes @{ -> } - foldableRegions.AddRange( - MatchTokenElements(tokens, s_openingBraces, TokenKind.RCurly, RegionKindNone) - ); - - // Find matching parentheses ( -> ) - // Find matching array literals @( -> ) - // Find matching subexpressions $( -> ) - foldableRegions.AddRange( - MatchTokenElements(tokens, s_openingParens, TokenKind.RParen, RegionKindNone) - ); - - // Find contiguous here strings @' -> '@ - foldableRegions.AddRange( - MatchTokenElement(tokens, TokenKind.HereStringLiteral, RegionKindNone) - ); - - // Find unopinionated variable names ${ \n \n } - foldableRegions.AddRange( - MatchTokenElement(tokens, TokenKind.Variable, RegionKindNone) - ); + var foldableRegions = new List(); + var tokenCommentRegionStack = new Stack(); + Token blockStartToken = null; + int blockNextLine = -1; - // Find contiguous here strings @" -> "@ - foldableRegions.AddRange( - MatchTokenElement(tokens, TokenKind.HereStringExpandable, RegionKindNone) - ); - - // Find matching comment regions #region -> #endregion - foldableRegions.AddRange( - MatchCustomCommentRegionTokenElements(tokens, RegionKindRegion) - ); - - // Find blocks of line comments # comment1\n# comment2\n... - foldableRegions.AddRange( - MatchBlockCommentTokenElement(tokens, RegionKindComment) - ); - - // Find comments regions <# -> #> - foldableRegions.AddRange( - MatchTokenElement(tokens, TokenKind.Comment, RegionKindComment) - ); - - // Remove any null entries. Nulls appear if the folding reference is invalid - // or missing - foldableRegions.RemoveAll(item => item == null); - - // 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); - }); + for (int index = 0; index < tokens.Length; index++) + { + Token token = tokens[index]; + if (token.Kind != TokenKind.Comment) { continue; } + // Processing for comment regions <# -> #> + if (token.Extent.StartLineNumber != token.Extent.EndLineNumber) + { + FoldingReference foldRef = CreateFoldingReference(token, token, RegionKindComment); + if (foldRef != null) { foldableRegions.Add(foldRef); } + continue; + } - // Some editors have different folding UI, sometimes the lastline should be displayed - // If we do want to show the last line, just change the region to be one line less - if (ShowLastLine) { - foldableRegions.ForEach( item => { item.EndLine--; }); + if (!IsBlockComment(index, tokens)) { continue; } + // Regex's are very expensive. Use them sparingly! + // Processing for # region -> # endregion + if (s_startRegionTextRegex.IsMatch(token.Text)) + { + tokenCommentRegionStack.Push(token); + continue; + } + if (s_endRegionTextRegex.IsMatch(token.Text)) + { + // 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); } + } + continue; + } + // If it's neither a start or end region then it could be block line comment + // Processing for blocks of line comments # comment1\n# comment2\n... + int thisLine = token.Extent.StartLineNumber - 1; + if ((blockStartToken != null) && (thisLine != blockNextLine)) + { + FoldingReference foldRef = CreateFoldingReference(blockStartToken, blockNextLine - 1, RegionKindComment); + if (foldRef != null) { foldableRegions.Add(foldRef); } + blockStartToken = token; + } + if (blockStartToken == null) { blockStartToken = token; } + blockNextLine = thisLine + 1; + } + // If we exit the token array and we're still processing comment lines, then the + // 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); } } return foldableRegions.ToArray(); @@ -160,47 +134,6 @@ static private FoldingReference CreateFoldingReference( }; } - /// - /// Given an array of tokens, find matching regions which start (array of tokens) and end with a different TokenKind - /// - static private List MatchTokenElements( - Token[] tokens, - TokenKind[] startTokenKind, - TokenKind endTokenKind, - string matchKind) - { - List result = new List(); - Stack tokenStack = new Stack(); - foreach (Token token in tokens) - { - if (Array.IndexOf(startTokenKind, token.Kind) != -1) { - tokenStack.Push(token); - } - if ((tokenStack.Count > 0) && (token.Kind == endTokenKind)) { - result.Add(CreateFoldingReference(tokenStack.Pop(), token, matchKind)); - } - } - return result; - } - - /// - /// Given an array of token, finds a specific token - /// - static private List MatchTokenElement( - Token[] tokens, - TokenKind tokenKind, - string matchKind) - { - List result = new List(); - foreach (Token token in tokens) - { - if ((token.Kind == tokenKind) && (token.Extent.StartLineNumber != token.Extent.EndLineNumber)) { - result.Add(CreateFoldingReference(token, token, matchKind)); - } - } - return result; - } - /// /// Returns true if a Token is a block comment; /// - Must be a TokenKind.comment @@ -215,79 +148,5 @@ static private bool IsBlockComment(int index, Token[] tokens) { if (tokens[index - 1].Kind != TokenKind.NewLine) { return false; } return thisToken.Text.StartsWith("#"); } - - // This regular expressions is used to detect a line comment (as opposed to an inline comment), that is not a region - // block directive i.e. - // - No text between the beginning of the line and `#` - // - Comment does start with region - // - Comment does start with endregion - static private readonly Regex s_nonRegionLineCommentRegex = new Regex( - @"\s*#(?!region\b|endregion\b)", - RegexOptions.IgnoreCase | RegexOptions.Compiled); - - /// - /// Finding blocks of comment tokens is more complicated as the newline characters are not - /// classed as comments. To workaround this we search for valid block comments (See IsBlockCmment) - /// and then determine contiguous line numbers from there - /// - static private List MatchBlockCommentTokenElement( - Token[] tokens, - string matchKind) - { - List result = new List(); - Token startToken = null; - int nextLine = -1; - for (int index = 0; index < tokens.Length; index++) - { - Token thisToken = tokens[index]; - if (IsBlockComment(index, tokens) && s_nonRegionLineCommentRegex.IsMatch(thisToken.Text)) { - int thisLine = thisToken.Extent.StartLineNumber - 1; - if ((startToken != null) && (thisLine != nextLine)) { - result.Add(CreateFoldingReference(startToken, nextLine - 1, matchKind)); - startToken = thisToken; - } - if (startToken == null) { startToken = thisToken; } - nextLine = thisLine + 1; - } - } - // If we exit the token array and we're still processing comment lines, then the - // comment block simply ends at the end of document - if (startToken != null) { - result.Add(CreateFoldingReference(startToken, nextLine - 1, matchKind)); - } - return result; - } - - /// - /// Given a list of tokens, find the tokens that are comments and - /// the comment text is either `# region` or `# endregion`, and then use a stack to determine - /// the ranges they span - /// - static private List MatchCustomCommentRegionTokenElements( - Token[] tokens, - string matchKind) - { - // These regular expressions are used to match lines which mark the start and end of region comment in a PowerShell - // script. They are based on the defaults in the VS Code Language Configuration at; - // https://github.com/Microsoft/vscode/blob/64186b0a26/extensions/powershell/language-configuration.json#L26-L31 - string startRegionText = @"^\s*#region\b"; - string endRegionText = @"^\s*#endregion\b"; - - List result = new List(); - Stack tokenStack = new Stack(); - for (int index = 0; index < tokens.Length; index++) - { - if (IsBlockComment(index, tokens)) { - Token token = tokens[index]; - if (Regex.IsMatch(token.Text, startRegionText, RegexOptions.IgnoreCase)) { - tokenStack.Push(token); - } - if ((tokenStack.Count > 0) && (Regex.IsMatch(token.Text, endRegionText, RegexOptions.IgnoreCase))) { - result.Add(CreateFoldingReference(tokenStack.Pop(), token, matchKind)); - } - } - } - return result; - } } } diff --git a/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs b/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs index 3ce157837..58b0e1b32 100644 --- a/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs +++ b/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs @@ -13,13 +13,16 @@ public class TokenOperationsTests /// /// Helper method to create a stub script file and then call FoldableRegions /// - private FoldingReference[] GetRegions(string text, bool showLastLine = true) { + private FoldingReference[] GetRegions(string text) { ScriptFile scriptFile = new ScriptFile( "testfile", "clienttestfile", text, Version.Parse("5.0")); - return Microsoft.PowerShell.EditorServices.TokenOperations.FoldableRegions(scriptFile.ScriptTokens, showLastLine); + + return Microsoft.PowerShell.EditorServices.FoldingOperations.FoldableRegions( + scriptFile.ScriptTokens, + scriptFile.ScriptAst); } /// @@ -122,22 +125,22 @@ double quoted herestrings should also fold valid} = 5 "; private FoldingReference[] expectedAllInOneScriptFolds = { - CreateFoldingReference(0, 0, 3, 10, "region"), - CreateFoldingReference(1, 0, 2, 2, "comment"), - CreateFoldingReference(10, 0, 14, 2, "comment"), - CreateFoldingReference(16, 30, 62, 1, null), - CreateFoldingReference(17, 0, 21, 2, "comment"), - CreateFoldingReference(23, 7, 25, 2, null), - CreateFoldingReference(31, 5, 33, 2, null), - CreateFoldingReference(38, 2, 39, 0, "comment"), - CreateFoldingReference(42, 2, 51, 14, "region"), - CreateFoldingReference(44, 4, 47, 14, "region"), - CreateFoldingReference(54, 7, 55, 3, null), - CreateFoldingReference(59, 7, 61, 3, null), - CreateFoldingReference(67, 0, 68, 0, "comment"), - CreateFoldingReference(70, 0, 74, 26, "region"), - CreateFoldingReference(71, 0, 72, 0, "comment"), - CreateFoldingReference(78, 0, 79, 6, null), + CreateFoldingReference(0, 0, 4, 10, "region"), + CreateFoldingReference(1, 0, 3, 2, "comment"), + CreateFoldingReference(10, 0, 15, 2, "comment"), + CreateFoldingReference(16, 30, 63, 1, null), + CreateFoldingReference(17, 0, 22, 2, "comment"), + CreateFoldingReference(23, 7, 26, 2, null), + CreateFoldingReference(31, 5, 34, 2, null), + CreateFoldingReference(38, 2, 40, 0, "comment"), + CreateFoldingReference(42, 2, 52, 14, "region"), + CreateFoldingReference(44, 4, 48, 14, "region"), + CreateFoldingReference(54, 7, 56, 3, null), + CreateFoldingReference(59, 7, 62, 3, null), + CreateFoldingReference(67, 0, 69, 0, "comment"), + CreateFoldingReference(70, 0, 75, 26, "region"), + CreateFoldingReference(71, 0, 73, 0, "comment"), + CreateFoldingReference(78, 0, 80, 6, null), }; /// @@ -147,6 +150,8 @@ private void AssertFoldingReferenceArrays( FoldingReference[] expected, FoldingReference[] actual) { + int minValue = expected.Length; + if (minValue < actual.Length) { minValue = actual.Length; } for (int index = 0; index < expected.Length; index++) { Assert.Equal(expected[index], actual[index]); @@ -178,19 +183,6 @@ public void LaguageServiceFindsFoldablRegionsWithCRLF() { AssertFoldingReferenceArrays(expectedAllInOneScriptFolds, result); } - [Fact] - public void LaguageServiceFindsFoldablRegionsWithoutLastLine() { - FoldingReference[] result = GetRegions(allInOneScript, false); - // Incrememnt the end line of the expected regions by one as we will - // be hiding the last line - FoldingReference[] expectedFolds = expectedAllInOneScriptFolds.Clone() as FoldingReference[]; - for (int index = 0; index < expectedFolds.Length; index++) - { - expectedFolds[index].EndLine++; - } - AssertFoldingReferenceArrays(expectedFolds, result); - } - [Fact] public void LaguageServiceFindsFoldablRegionsWithMismatchedRegions() { string testString = @@ -203,7 +195,7 @@ public void LaguageServiceFindsFoldablRegionsWithMismatchedRegions() { #region should not fold - mismatched "; FoldingReference[] expectedFolds = { - CreateFoldingReference(2, 0, 3, 10, "region") + CreateFoldingReference(2, 0, 4, 10, "region") }; FoldingReference[] result = GetRegions(testString); @@ -220,8 +212,8 @@ public void LaguageServiceFindsFoldablRegionsWithDuplicateRegions() { }) "; FoldingReference[] expectedFolds = { - CreateFoldingReference(1, 64, 1, 27, null), - CreateFoldingReference(2, 35, 3, 2, null) + CreateFoldingReference(1, 64, 2, 27, null), + CreateFoldingReference(2, 35, 4, 2, null) }; FoldingReference[] result = GetRegions(testString); @@ -245,9 +237,40 @@ public void LaguageServiceFindsFoldablRegionsWithSameEndToken() { ) "; FoldingReference[] expectedFolds = { - CreateFoldingReference(0, 19, 4, 1, null), - CreateFoldingReference(2, 9, 3, 5, null), - CreateFoldingReference(7, 5, 8, 1, null) + CreateFoldingReference(0, 19, 5, 1, null), + CreateFoldingReference(2, 9, 4, 5, null), + CreateFoldingReference(7, 5, 9, 1, null) + }; + + FoldingReference[] result = GetRegions(testString); + + AssertFoldingReferenceArrays(expectedFolds, result); + } + + // A simple PowerShell Classes test + [Fact] + public void LaguageServiceFindsFoldablRegionsWithClasses() { + string testString = +@"class TestClass { + [string[]] $TestProperty = @( + 'first', + 'second', + 'third') + + [string] TestMethod() { + return $this.TestProperty[0] + } +} +"; + // FoldingReference[] expectedFolds = { + // CreateFoldingReference(0, 16, 8, 1, null), + // CreateFoldingReference(1, 31, 3, 16, null), + // CreateFoldingReference(6, 26, 7, 5, null) + // }; + FoldingReference[] expectedFolds = { + CreateFoldingReference(0, 0, 9, 1, null), + CreateFoldingReference(1, 31, 4, 16, null), + CreateFoldingReference(6, 26, 8, 5, null) }; FoldingReference[] result = GetRegions(testString); From e4744ca38cfdeeb62fd599b62c0c9629239ee9e3 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Thu, 13 Dec 2018 11:04:44 +0800 Subject: [PATCH 2/2] (GH-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. --- .../Server/LanguageServer.cs | 2 +- .../Language/AstOperations.cs | 7 ++--- .../Language/FindFoldsVisitor.cs | 24 +++++++-------- .../Language/FoldingOperations.cs | 29 ++++-------------- .../Language/FoldingReference.cs | 30 +++++++++++++++++++ .../Language/TokenOperations.cs | 20 +++++-------- .../Language/TokenOperationsTests.cs | 7 +++-- 7 files changed, 63 insertions(+), 56 deletions(-) diff --git a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs index cb2443c46..a697aa5c8 100644 --- a/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs +++ b/src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs @@ -1387,7 +1387,7 @@ private FoldingRange[] Fold( int endLineOffset = this.currentSettings.CodeFolding.ShowLastLine ? -1 : 0; 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 index 7331e93d5..ae1e8647e 100644 --- a/src/PowerShellEditorServices/Language/AstOperations.cs +++ b/src/PowerShellEditorServices/Language/AstOperations.cs @@ -335,13 +335,12 @@ static public string[] FindDotSourcedIncludes(Ast scriptAst, string psScriptRoot /// Finds all foldable regions in a script based on AST /// /// The abstract syntax tree of the given script + /// The FoldingReferenceList object to add the folds to /// A collection of FoldingReference objects - public static IEnumerable 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 index 9eede5db4..60d606173 100644 --- a/src/PowerShellEditorServices/Language/FindFoldsVisitor.cs +++ b/src/PowerShellEditorServices/Language/FindFoldsVisitor.cs @@ -16,11 +16,11 @@ internal class FindFoldsVisitor : AstVisitor2 { private const string RegionKindNone = null; - public List FoldableRegions { get; } + private FoldingReferenceList _refList; - public FindFoldsVisitor() + public FindFoldsVisitor(ref FoldingReferenceList refList) { - FoldableRegions = new List(); + _refList = refList; } /// @@ -56,7 +56,7 @@ public override AstVisitAction VisitArrayExpression(ArrayExpressionAst objAst) { if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; } @@ -65,7 +65,7 @@ public override AstVisitAction VisitHashtable(HashtableAst objAst) { if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; } @@ -77,7 +77,7 @@ public override AstVisitAction VisitStatementBlock(StatementBlockAst objAst) if (objAst.Parent is ArrayExpressionAst) { return AstVisitAction.Continue; } if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; } @@ -90,7 +90,7 @@ public override AstVisitAction VisitScriptBlock(ScriptBlockAst objAst) if (objAst.Parent is ScriptBlockExpressionAst) { return AstVisitAction.Continue; } if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; } @@ -107,7 +107,7 @@ public override AstVisitAction VisitScriptBlockExpression(ScriptBlockExpressionA foldRef.StartCharacter--; foldRef.EndCharacter++; } - FoldableRegions.Add(foldRef); + _refList.SafeAdd(foldRef); } return AstVisitAction.Continue; } @@ -116,7 +116,7 @@ public override AstVisitAction VisitStringConstantExpression(StringConstantExpre { if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; @@ -126,7 +126,7 @@ public override AstVisitAction VisitSubExpression(SubExpressionAst objAst) { if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; } @@ -135,7 +135,7 @@ public override AstVisitAction VisitTypeDefinition(TypeDefinitionAst objAst) { if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; } @@ -144,7 +144,7 @@ public override AstVisitAction VisitVariableExpression(VariableExpressionAst obj { if (IsValidFoldingExtent(objAst.Extent)) { - FoldableRegions.Add(CreateFoldingReference(objAst.Extent, RegionKindNone)); + _refList.SafeAdd(CreateFoldingReference(objAst.Extent, RegionKindNone)); } return AstVisitAction.Continue; } diff --git a/src/PowerShellEditorServices/Language/FoldingOperations.cs b/src/PowerShellEditorServices/Language/FoldingOperations.cs index 063bae694..7eb96e892 100644 --- 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 /// - internal static FoldingReference[] FoldableRegions( + internal static FoldingReferenceList FoldableRegions( Token[] tokens, Ast scriptAst) { - var foldableRegions = new List(); + 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 index 54e3401df..0a611e998 100644 --- 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); } } + + /// + /// 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 + /// + public class FoldingReferenceList : Dictionary + { + /// + /// Adds a FoldingReference to the list and enforces ordering rules e.g. Only one fold per start line + /// + public void SafeAdd(FoldingReference item) + { + if (item == null) { return; } + FoldingReference currentItem; + 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; } + } + + /// + /// Helper method to easily convert the Dictionary Values into an array + /// + public FoldingReference[] ToArray() + { + var result = new FoldingReference[Count]; + Values.CopyTo(result, 0); + return result; + } + } } diff --git a/src/PowerShellEditorServices/Language/TokenOperations.cs b/src/PowerShellEditorServices/Language/TokenOperations.cs index 4802bb85b..fb543019f 100644 --- a/src/PowerShellEditorServices/Language/TokenOperations.cs +++ b/src/PowerShellEditorServices/Language/TokenOperations.cs @@ -33,10 +33,10 @@ internal static class TokenOperations /// /// Extracts all of the unique foldable regions in a script given the list tokens /// - internal static FoldingReference[] FoldableRegions( - Token[] tokens) + internal static void FoldableRegions( + Token[] tokens, + ref FoldingReferenceList refList) { - var foldableRegions = new List(); var tokenCommentRegionStack = new Stack(); 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; } @@ -66,8 +65,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; } @@ -76,8 +74,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; } @@ -87,11 +84,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(); } /// diff --git a/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs b/test/PowerShellEditorServices.Test/Language/TokenOperationsTests.cs index 58b0e1b32..7403d9a11 100644 --- 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; } ///