Formatter: Recycle parsed AST and tokens in between rule invocations when no correction was applied to improve performance (#1462)

* Recycle Parse result for formatter, if a rule does not return a DiagnosticRecord and therefore does not require re-parsing

* fix issue where the parsing stage can only be skipped in the first loop iteration of the Fix API

* Adapt LibraryUsage.tests.ps1

* Remove unnecessary call to ToList() and Count() where object is already a list

* re-trigger ci

Co-authored-by: Christoph Bergmeister <christoph.bergmeister@bjss.com>

Author: bergmeister

Engine/Commands/InvokeScriptAnalyzerCommand.cs

@@ -422,7 +422,7 @@ private void ProcessInput()
             }
             else if (String.Equals(this.ParameterSetName, "ScriptDefinition", StringComparison.OrdinalIgnoreCase))
             {
-                diagnosticsList = ScriptAnalyzer.Instance.AnalyzeScriptDefinition(scriptDefinition);
+                diagnosticsList = ScriptAnalyzer.Instance.AnalyzeScriptDefinition(scriptDefinition, out _, out _);
                 WriteToOutput(diagnosticsList);
             }
         }

Engine/Formatter.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections;
 using System.Management.Automation;
+using System.Management.Automation.Language;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
 {
@@ -46,6 +47,9 @@ public static string Format<TCmdlet>(
             };
 
             var text = new EditableText(scriptDefinition);
+            ScriptBlockAst scriptAst = null;
+            Token[] scriptTokens = null;
+            bool skipParsing = false;
             foreach (var rule in ruleOrder)
             {
                 if (!settings.RuleArguments.ContainsKey(rule))
@@ -57,9 +61,8 @@ public static string Format<TCmdlet>(
                 ScriptAnalyzer.Instance.UpdateSettings(currentSettings);
                 ScriptAnalyzer.Instance.Initialize(cmdlet, null, null, null, null, true, false);
 
-                Range updatedRange;
-                bool fixesWereApplied;
-                text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange, out fixesWereApplied, skipVariableAnalysis: true);
+                text = ScriptAnalyzer.Instance.Fix(text, range, skipParsing, out Range updatedRange, out bool fixesWereApplied, ref scriptAst, ref scriptTokens, skipVariableAnalysis: true);
+                skipParsing = !fixesWereApplied;
                 range = updatedRange;
             }
 

Engine/ScriptAnalyzer.cs

@@ -1478,8 +1478,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeAndFixPath(string path, Func<string,
                 if (shouldProcess(scriptFilePath, $"Analyzing and fixing file {scriptFilePath}"))
                 {
                     var fileEncoding = GetFileEncoding(scriptFilePath);
-                    bool fixesWereApplied;
-                    var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding), out fixesWereApplied);
+                    var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding), out bool fixesWereApplied);
                     if (fixesWereApplied)
                     {
                         File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding);
@@ -1497,12 +1496,15 @@ public IEnumerable<DiagnosticRecord> AnalyzeAndFixPath(string path, Func<string,
         /// <summary>
         /// Analyzes a script definition in the form of a string input
         /// </summary>
-        /// <param name="scriptDefinition">The script to be analyzed</param>
+        /// <param name="scriptDefinition">The script to be analyzed.</param>
+        /// <param name="scriptAst">Parsed AST of <paramref name="scriptDefinition"/>.</param>
+        /// <param name="scriptTokens">Parsed tokens of <paramref name="scriptDefinition"/.></param>
+        /// <param name="skipVariableAnalysis">Whether variable analysis can be skipped (applicable if rules do not use variable analysis APIs).</param>
         /// <returns></returns>
-        public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, bool skipVariableAnalysis = false)
+        public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, out ScriptBlockAst scriptAst, out Token[] scriptTokens, bool skipVariableAnalysis = false)
         {
-            ScriptBlockAst scriptAst = null;
-            Token[] scriptTokens = null;
+            scriptAst = null;
+            scriptTokens = null;
             ParseError[] errors = null;
 
             this.outputWriter.WriteVerbose(string.Format(CultureInfo.CurrentCulture, Strings.VerboseScriptDefinitionMessage));
@@ -1546,7 +1548,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefini
         /// Fix the violations in the given script text.
         /// </summary>
         /// <param name="scriptDefinition">The script text to be fixed.</param>
-        /// <param name="updatedRange">Whether any warnings were fixed.</param>
+        /// <param name="fixesWereApplied">Whether any warnings were fixed.</param>
         /// <returns>The fixed script text.</returns>
         public string Fix(string scriptDefinition, out bool fixesWereApplied)
         {
@@ -1555,20 +1557,24 @@ public string Fix(string scriptDefinition, out bool fixesWereApplied)
                 throw new ArgumentNullException(nameof(scriptDefinition));
             }
 
-            Range updatedRange;
-            return Fix(new EditableText(scriptDefinition), null, out updatedRange, out fixesWereApplied).ToString();
+            ScriptBlockAst scriptAst = null;
+            Token[] scriptTokens = null;
+            return Fix(new EditableText(scriptDefinition), range: null, skipParsing: false, out _, out fixesWereApplied, ref scriptAst, ref scriptTokens).ToString();
         }
 
         /// <summary>
         /// Fix the violations in the given script text.
         /// </summary>
-        /// <param name="text">An object of type `EditableText` that encapsulates the script text to be fixed.</param>
+        /// <param name="text">An object of type <see cref="EditableText"/> that encapsulates the script text to be fixed.</param>
         /// <param name="range">The range in which the fixes are allowed.</param>
+        /// <param name="skipParsing">Whether to use the <paramref name="scriptAst"/> and <paramref name="scriptTokens"/> parameters instead of parsing the <paramref name="text"/> parameter.</param>
         /// <param name="updatedRange">The updated range after the fixes have been applied.</param>
-        /// <param name="updatedRange">Whether any warnings were fixed.</param>
+        /// <param name="fixesWereApplied">Whether any warnings were fixed.</param>
+        /// <param name="scriptAst">Optionally pre-parsed AST.</param>
+        /// <param name="scriptTokens">Optionally pre-parsed tokens.</param>
         /// <param name="skipVariableAnalysis">Whether to skip variable analysis.</param>
         /// <returns>The same instance of `EditableText` that was passed to the method, but the instance encapsulates the fixed script text. This helps in chaining the Fix method.</returns>
-        public EditableText Fix(EditableText text, Range range, out Range updatedRange, out bool fixesWereApplied, bool skipVariableAnalysis = false)
+        public EditableText Fix(EditableText text, Range range, bool skipParsing, out Range updatedRange, out bool fixesWereApplied, ref ScriptBlockAst scriptAst, ref Token[] scriptTokens, bool skipVariableAnalysis = false)
         {
             if (text == null)
             {
@@ -1590,7 +1596,15 @@ public EditableText Fix(EditableText text, Range range, out Range updatedRange,
             var previousUnusedCorrections = 0;
             do
             {
-                var records = AnalyzeScriptDefinition(text.ToString(), skipVariableAnalysis);
+                IEnumerable<DiagnosticRecord> records;
+                if (skipParsing && previousUnusedCorrections == 0)
+                {
+                    records = AnalyzeSyntaxTree(scriptAst, scriptTokens, String.Empty, skipVariableAnalysis);
+                }
+                else
+                {
+                    records = AnalyzeScriptDefinition(text.ToString(), out scriptAst, out scriptTokens, skipVariableAnalysis);
+                }
                 var corrections = records
                     .Select(r => r.SuggestedCorrections)
                     .Where(sc => sc != null && sc.Any())
@@ -1684,22 +1698,21 @@ private static Range SnapToEdges(EditableText text, Range range)
         }
 
         private static IEnumerable<CorrectionExtent> GetCorrectionExtentsForFix(
-            IEnumerable<CorrectionExtent> correctionExtents)
+            List<CorrectionExtent> correctionExtents)
         {
-            var ceList = correctionExtents.ToList();
-            ceList.Sort((x, y) =>
+            correctionExtents.Sort((x, y) =>
             {
                 return x.StartLineNumber < y.StartLineNumber ?
                             1 :
                             (x.StartLineNumber == y.StartLineNumber ? 0 : -1);
             });
 
-            return ceList.GroupBy(ce => ce.StartLineNumber).Select(g => g.First());
+            return correctionExtents.GroupBy(ce => ce.StartLineNumber).Select(g => g.First());
         }
 
         private static EditableText Fix(
             EditableText text,
-            IEnumerable<CorrectionExtent> correctionExtents,
+            List<CorrectionExtent> correctionExtents,
             out int unusedCorrections)
         {
             var correctionsToUse = GetCorrectionExtentsForFix(correctionExtents);
@@ -1710,7 +1723,7 @@ private static EditableText Fix(
                 text.ApplyEdit(correction);
             }
 
-            unusedCorrections = correctionExtents.Count() - count;
+            unusedCorrections = correctionExtents.Count - count;
             return text;
         }
 

Tests/Engine/LibraryUsage.tests.ps1

@@ -94,7 +94,7 @@ function Invoke-ScriptAnalyzer {
     }
     else
     {
-        $results = $scriptAnalyzer.AnalyzeScriptDefinition($ScriptDefinition);
+        $results = $scriptAnalyzer.AnalyzeScriptDefinition($ScriptDefinition, [ref] $null, [ref] $null)
 	}
 
 	$results