Fix formatter crash when script contains parse errors (#728)

* Fix formatter crash when script contains parse errors

* Fix possible null references, add comments, improve variable types

Author: rjmholt

src/PowerShellEditorServices/Analysis/AnalysisService.cs

@@ -24,6 +24,48 @@ namespace Microsoft.PowerShell.EditorServices
     /// </summary>
     public class AnalysisService : IDisposable
     {
+        #region Static fields
+
+        /// <summary>
+        /// Defines the list of Script Analyzer rules to include by default if
+        /// no settings file is specified.
+        /// </summary>
+        private static readonly string[] s_includedRules = {
+            "PSUseToExportFieldsInManifest",
+            "PSMisleadingBacktick",
+            "PSAvoidUsingCmdletAliases",
+            "PSUseApprovedVerbs",
+            "PSAvoidUsingPlainTextForPassword",
+            "PSReservedCmdletChar",
+            "PSReservedParams",
+            "PSShouldProcess",
+            "PSMissingModuleManifestField",
+            "PSAvoidDefaultValueSwitchParameter",
+            "PSUseDeclaredVarsMoreThanAssignments",
+            "PSPossibleIncorrectComparisonWithNull",
+            "PSAvoidDefaultValueForMandatoryParameter",
+            "PSPossibleIncorrectUsageOfRedirectionOperator"
+        };
+
+        /// <summary>
+        /// An empty diagnostic result to return when a script fails analysis.
+        /// </summary>
+        private static readonly PSObject[] s_emptyDiagnosticResult = new PSObject[0];
+
+        /// <summary>
+        /// An empty script marker result to return when no script markers can be returned.
+        /// </summary>
+        private static readonly ScriptFileMarker[] s_emptyScriptMarkerResult = new ScriptFileMarker[0];
+
+        private static readonly string[] s_emptyGetRuleResult = new string[0];
+
+        /// <summary>
+        /// The indentation to add when the logger lists errors.
+        /// </summary>
+        private static readonly string s_indentJoin = Environment.NewLine + "    ";
+
+        #endregion // Static fields
+
         #region Private Fields
 
         /// <summary>
@@ -53,31 +95,8 @@ public class AnalysisService : IDisposable
         /// </summary>
         private PSModuleInfo _pssaModuleInfo;
 
-        /// <summary>
-        /// Defines the list of Script Analyzer rules to include by default if
-        /// no settings file is specified.
-        /// </summary>
-        private static readonly string[] s_includedRules = new string[]
-        {
-            "PSUseToExportFieldsInManifest",
-            "PSMisleadingBacktick",
-            "PSAvoidUsingCmdletAliases",
-            "PSUseApprovedVerbs",
-            "PSAvoidUsingPlainTextForPassword",
-            "PSReservedCmdletChar",
-            "PSReservedParams",
-            "PSShouldProcess",
-            "PSMissingModuleManifestField",
-            "PSAvoidDefaultValueSwitchParameter",
-            "PSUseDeclaredVarsMoreThanAssignments",
-            "PSPossibleIncorrectComparisonWithNull",
-            "PSAvoidDefaultValueForMandatoryParameter",
-            "PSPossibleIncorrectUsageOfRedirectionOperator"
-        };
-
         #endregion // Private Fields
 
-
         #region Properties
 
         /// <summary>
@@ -281,9 +300,15 @@ public async Task<ScriptFileMarker[]> GetSemanticMarkersAsync(
         /// </summary>
         public IEnumerable<string> GetPSScriptAnalyzerRules()
         {
-            List<string> ruleNames = new List<string>();
-            var ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary<string, object>());
-            foreach (var rule in ruleObjects)
+            PowerShellResult getRuleResult = InvokePowerShell("Get-ScriptAnalyzerRule");
+            if (getRuleResult == null)
+            {
+                _logger.Write(LogLevel.Warning, "Get-ScriptAnalyzerRule returned null result");
+                return s_emptyGetRuleResult;
+            }
+
+            var ruleNames = new List<string>();
+            foreach (var rule in getRuleResult.Output)
             {
                 ruleNames.Add((string)rule.Members["RuleName"].Value);
             }
@@ -319,8 +344,35 @@ public async Task<string> Format(
                 argsDict.Add("Range", rangeList);
             }
 
-            var result = await InvokePowerShellAsync("Invoke-Formatter", argsDict);
-            return result?.Select(r => r?.ImmediateBaseObject as string).FirstOrDefault();
+            PowerShellResult result = await InvokePowerShellAsync("Invoke-Formatter", argsDict);
+
+            if (result == null)
+            {
+                _logger.Write(LogLevel.Error, "Formatter returned null result");
+                return null;
+            }
+
+            if (result.HasErrors)
+            {
+                var errorBuilder = new StringBuilder().Append(s_indentJoin);
+                foreach (ErrorRecord err in result.Errors)
+                {
+                    errorBuilder.Append(err).Append(s_indentJoin);
+                }
+                _logger.Write(LogLevel.Warning, $"Errors found while formatting file: {errorBuilder}");
+                return null;
+            }
+
+            foreach (PSObject resultObj in result.Output)
+            {
+                string formatResult = resultObj?.BaseObject as string;
+                if (formatResult != null)
+                {
+                    return formatResult;
+                }
+            }
+
+            return null;
         }
 
         #endregion // public methods
@@ -342,7 +394,7 @@ private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
             else
             {
                 // Return an empty marker list
-                return new ScriptFileMarker[0];
+                return s_emptyScriptMarkerResult;
             }
         }
 
@@ -360,7 +412,7 @@ private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
             else
             {
                 // Return an empty marker list
-                return new ScriptFileMarker[0];
+                return s_emptyScriptMarkerResult;
             }
         }
 
@@ -380,11 +432,16 @@ private void LogAvailablePssaFeatures()
             // If we already know the module that was imported, save some work
             if (_pssaModuleInfo == null)
             {
-                PSObject[] modules = InvokePowerShell(
+                PowerShellResult getModuleResult = InvokePowerShell(
                     "Get-Module",
                     new Dictionary<string, object>{ {"Name", PSSA_MODULE_NAME} });
 
-                _pssaModuleInfo = modules
+                if (getModuleResult == null)
+                {
+                    throw new AnalysisServiceLoadException("Get-Module call to find PSScriptAnalyzer module failed");
+                }
+
+                _pssaModuleInfo = getModuleResult.Output
                     .Select(m => m.BaseObject)
                     .OfType<PSModuleInfo>()
                     .FirstOrDefault();
@@ -426,7 +483,7 @@ private async Task<PSObject[]> GetDiagnosticRecordsAsync<TSettings>(
              string[] rules,
              TSettings settings) where TSettings : class
         {
-            var diagnosticRecords = new PSObject[0];
+            var diagnosticRecords = s_emptyDiagnosticResult;
 
             // When a new, empty file is created there are by definition no issues.
             // Furthermore, if you call Invoke-ScriptAnalyzer with an empty ScriptDefinition
@@ -452,13 +509,15 @@ private async Task<PSObject[]> GetDiagnosticRecordsAsync<TSettings>(
                     settingArgument = rules;
                 }
 
-                diagnosticRecords = await InvokePowerShellAsync(
+                PowerShellResult result = await InvokePowerShellAsync(
                     "Invoke-ScriptAnalyzer",
                     new Dictionary<string, object>
                     {
                         { "ScriptDefinition", scriptContent },
                         { settingParameter, settingArgument }
                     });
+
+                diagnosticRecords = result?.Output;
             }
 
             _logger.Write(
@@ -468,21 +527,26 @@ private async Task<PSObject[]> GetDiagnosticRecordsAsync<TSettings>(
             return diagnosticRecords;
         }
 
-        private PSObject[] InvokePowerShell(string command, IDictionary<string, object> paramArgMap)
+        private PowerShellResult InvokePowerShell(string command, IDictionary<string, object> paramArgMap = null)
         {
             using (var powerShell = System.Management.Automation.PowerShell.Create())
             {
                 powerShell.RunspacePool = _analysisRunspacePool;
                 powerShell.AddCommand(command);
-                foreach (var kvp in paramArgMap)
+                if (paramArgMap != null)
                 {
-                    powerShell.AddParameter(kvp.Key, kvp.Value);
+                    foreach (KeyValuePair<string, object> kvp in paramArgMap)
+                    {
+                        powerShell.AddParameter(kvp.Key, kvp.Value);
+                    }
                 }
 
-                var result = new PSObject[0];
+                PowerShellResult result = null;
                 try
                 {
-                    result = powerShell.Invoke()?.ToArray();
+                    PSObject[] output = powerShell.Invoke().ToArray();
+                    ErrorRecord[] errors = powerShell.Streams.Error.ToArray();
+                    result = new PowerShellResult(output, errors, powerShell.HadErrors);
                 }
                 catch (CommandNotFoundException ex)
                 {
@@ -503,7 +567,7 @@ private PSObject[] InvokePowerShell(string command, IDictionary<string, object>
             }
         }
 
-        private async Task<PSObject[]> InvokePowerShellAsync(string command, IDictionary<string, object> paramArgMap)
+        private async Task<PowerShellResult> InvokePowerShellAsync(string command, IDictionary<string, object> paramArgMap = null)
         {
             var task = Task.Run(() =>
             {
@@ -590,6 +654,29 @@ public void Dispose()
         }
 
         #endregion
+
+        /// <summary>
+        /// Wraps the result of an execution of PowerShell to send back through
+        /// asynchronous calls.
+        /// </summary>
+        private class PowerShellResult
+        {
+            public PowerShellResult(
+                PSObject[] output,
+                ErrorRecord[] errors,
+                bool hasErrors)
+            {
+                Output = output;
+                Errors = errors;
+                HasErrors = hasErrors;
+            }
+
+            public PSObject[] Output { get; }
+
+            public ErrorRecord[] Errors { get; }
+
+            public bool HasErrors { get; }
+        }
     }
 
     /// <summary>