From 6f515609a09e1ef88c97d9ee5cfbbfc15c3fcfdf Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 22 Aug 2018 13:33:43 -0700 Subject: [PATCH 1/3] Fix formatter crash when script contains parse errors --- .../Analysis/AnalysisService.cs | 138 +++++++++++++----- 1 file changed, 103 insertions(+), 35 deletions(-) diff --git a/src/PowerShellEditorServices/Analysis/AnalysisService.cs b/src/PowerShellEditorServices/Analysis/AnalysisService.cs index 0d9763b9e..ed7c0059b 100644 --- a/src/PowerShellEditorServices/Analysis/AnalysisService.cs +++ b/src/PowerShellEditorServices/Analysis/AnalysisService.cs @@ -24,6 +24,47 @@ namespace Microsoft.PowerShell.EditorServices /// public class AnalysisService : IDisposable { + #region Static fields + + /// + /// Defines the list of Script Analyzer rules to include by default if + /// no settings file is specified. + /// + private static readonly string[] s_includedRules = new string[] + { + "PSUseToExportFieldsInManifest", + "PSMisleadingBacktick", + "PSAvoidUsingCmdletAliases", + "PSUseApprovedVerbs", + "PSAvoidUsingPlainTextForPassword", + "PSReservedCmdletChar", + "PSReservedParams", + "PSShouldProcess", + "PSMissingModuleManifestField", + "PSAvoidDefaultValueSwitchParameter", + "PSUseDeclaredVarsMoreThanAssignments", + "PSPossibleIncorrectComparisonWithNull", + "PSAvoidDefaultValueForMandatoryParameter", + "PSPossibleIncorrectUsageOfRedirectionOperator" + }; + + /// + /// An empty diagnostic result to return when a script fails analysis. + /// + private static readonly PSObject[] s_emptyDiagnosticResult = new PSObject[0]; + + /// + /// An empty script marker result to return when no script markers can be returned. + /// + private static readonly ScriptFileMarker[] s_emptyScriptMarkerResult = new ScriptFileMarker[0]; + + /// + /// The indentation to add when the logger lists errors. + /// + private static readonly string s_indentJoin = Environment.NewLine + " "; + + #endregion // Static fields + #region Private Fields /// @@ -53,31 +94,8 @@ public class AnalysisService : IDisposable /// private PSModuleInfo _pssaModuleInfo; - /// - /// Defines the list of Script Analyzer rules to include by default if - /// no settings file is specified. - /// - 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 /// @@ -282,7 +300,7 @@ public async Task GetSemanticMarkersAsync( public IEnumerable GetPSScriptAnalyzerRules() { List ruleNames = new List(); - var ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary()); + var ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary()).Output; foreach (var rule in ruleObjects) { ruleNames.Add((string)rule.Members["RuleName"].Value); @@ -319,8 +337,35 @@ public async Task 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 +387,7 @@ private async Task GetSemanticMarkersAsync( else { // Return an empty marker list - return new ScriptFileMarker[0]; + return s_emptyScriptMarkerResult; } } @@ -360,7 +405,7 @@ private async Task GetSemanticMarkersAsync( else { // Return an empty marker list - return new ScriptFileMarker[0]; + return s_emptyScriptMarkerResult; } } @@ -382,7 +427,7 @@ private void LogAvailablePssaFeatures() { PSObject[] modules = InvokePowerShell( "Get-Module", - new Dictionary{ {"Name", PSSA_MODULE_NAME} }); + new Dictionary{ {"Name", PSSA_MODULE_NAME} }).Output; _pssaModuleInfo = modules .Select(m => m.BaseObject) @@ -426,7 +471,7 @@ private async Task GetDiagnosticRecordsAsync( 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 +497,15 @@ private async Task GetDiagnosticRecordsAsync( settingArgument = rules; } - diagnosticRecords = await InvokePowerShellAsync( + PowerShellResult result = await InvokePowerShellAsync( "Invoke-ScriptAnalyzer", new Dictionary { { "ScriptDefinition", scriptContent }, { settingParameter, settingArgument } }); + + diagnosticRecords = result.Output; } _logger.Write( @@ -468,7 +515,7 @@ private async Task GetDiagnosticRecordsAsync( return diagnosticRecords; } - private PSObject[] InvokePowerShell(string command, IDictionary paramArgMap) + private PowerShellResult InvokePowerShell(string command, IDictionary paramArgMap) { using (var powerShell = System.Management.Automation.PowerShell.Create()) { @@ -479,10 +526,12 @@ private PSObject[] InvokePowerShell(string command, IDictionary 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 +552,7 @@ private PSObject[] InvokePowerShell(string command, IDictionary } } - private async Task InvokePowerShellAsync(string command, IDictionary paramArgMap) + private async Task InvokePowerShellAsync(string command, IDictionary paramArgMap) { var task = Task.Run(() => { @@ -590,6 +639,25 @@ public void Dispose() } #endregion + + 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; } + } } /// From 29008ce8436c142bc8c236b146910aa99c8c8ed4 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 22 Aug 2018 13:38:27 -0700 Subject: [PATCH 2/3] Fix possible null references, add comments, improve variable types --- .../Analysis/AnalysisService.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/PowerShellEditorServices/Analysis/AnalysisService.cs b/src/PowerShellEditorServices/Analysis/AnalysisService.cs index ed7c0059b..dd079b10a 100644 --- a/src/PowerShellEditorServices/Analysis/AnalysisService.cs +++ b/src/PowerShellEditorServices/Analysis/AnalysisService.cs @@ -299,8 +299,8 @@ public async Task GetSemanticMarkersAsync( /// public IEnumerable GetPSScriptAnalyzerRules() { - List ruleNames = new List(); - var ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary()).Output; + var ruleNames = new List(); + IEnumerable ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary()).Output; foreach (var rule in ruleObjects) { ruleNames.Add((string)rule.Members["RuleName"].Value); @@ -427,7 +427,7 @@ private void LogAvailablePssaFeatures() { PSObject[] modules = InvokePowerShell( "Get-Module", - new Dictionary{ {"Name", PSSA_MODULE_NAME} }).Output; + new Dictionary{ {"Name", PSSA_MODULE_NAME} })?.Output; _pssaModuleInfo = modules .Select(m => m.BaseObject) @@ -505,7 +505,7 @@ private async Task GetDiagnosticRecordsAsync( { settingParameter, settingArgument } }); - diagnosticRecords = result.Output; + diagnosticRecords = result?.Output; } _logger.Write( @@ -640,6 +640,10 @@ public void Dispose() #endregion + /// + /// Wraps the result of an execution of PowerShell to send back through + /// asynchronous calls. + /// private class PowerShellResult { public PowerShellResult( From 811a03d0fe57860ffd93f522b9f8d156c1257909 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Thu, 23 Aug 2018 10:10:50 -0700 Subject: [PATCH 3/3] Address feedback on nulls and style --- .../Analysis/AnalysisService.cs | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/src/PowerShellEditorServices/Analysis/AnalysisService.cs b/src/PowerShellEditorServices/Analysis/AnalysisService.cs index dd079b10a..40b6327e5 100644 --- a/src/PowerShellEditorServices/Analysis/AnalysisService.cs +++ b/src/PowerShellEditorServices/Analysis/AnalysisService.cs @@ -30,8 +30,7 @@ public class AnalysisService : IDisposable /// Defines the list of Script Analyzer rules to include by default if /// no settings file is specified. /// - private static readonly string[] s_includedRules = new string[] - { + private static readonly string[] s_includedRules = { "PSUseToExportFieldsInManifest", "PSMisleadingBacktick", "PSAvoidUsingCmdletAliases", @@ -58,6 +57,8 @@ public class AnalysisService : IDisposable /// private static readonly ScriptFileMarker[] s_emptyScriptMarkerResult = new ScriptFileMarker[0]; + private static readonly string[] s_emptyGetRuleResult = new string[0]; + /// /// The indentation to add when the logger lists errors. /// @@ -299,9 +300,15 @@ public async Task GetSemanticMarkersAsync( /// public IEnumerable GetPSScriptAnalyzerRules() { + PowerShellResult getRuleResult = InvokePowerShell("Get-ScriptAnalyzerRule"); + if (getRuleResult == null) + { + _logger.Write(LogLevel.Warning, "Get-ScriptAnalyzerRule returned null result"); + return s_emptyGetRuleResult; + } + var ruleNames = new List(); - IEnumerable ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary()).Output; - foreach (var rule in ruleObjects) + foreach (var rule in getRuleResult.Output) { ruleNames.Add((string)rule.Members["RuleName"].Value); } @@ -341,7 +348,7 @@ public async Task Format( if (result == null) { - _logger.Write(LogLevel.Error, $"Formatter returned null result"); + _logger.Write(LogLevel.Error, "Formatter returned null result"); return null; } @@ -425,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{ {"Name", PSSA_MODULE_NAME} })?.Output; + new Dictionary{ {"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() .FirstOrDefault(); @@ -515,22 +527,25 @@ private async Task GetDiagnosticRecordsAsync( return diagnosticRecords; } - private PowerShellResult InvokePowerShell(string command, IDictionary paramArgMap) + private PowerShellResult InvokePowerShell(string command, IDictionary 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 kvp in paramArgMap) + { + powerShell.AddParameter(kvp.Key, kvp.Value); + } } PowerShellResult result = null; try { - PSObject[] output = powerShell.Invoke()?.ToArray(); - ErrorRecord[] errors = powerShell.Streams?.Error?.ToArray(); + PSObject[] output = powerShell.Invoke().ToArray(); + ErrorRecord[] errors = powerShell.Streams.Error.ToArray(); result = new PowerShellResult(output, errors, powerShell.HadErrors); } catch (CommandNotFoundException ex) @@ -552,7 +567,7 @@ private PowerShellResult InvokePowerShell(string command, IDictionary InvokePowerShellAsync(string command, IDictionary paramArgMap) + private async Task InvokePowerShellAsync(string command, IDictionary paramArgMap = null) { var task = Task.Run(() => {