Skip to content

Fix formatter crash when script contains parse errors #728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 28, 2018
Merged

Fix formatter crash when script contains parse errors #728

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f51560
Fix formatter crash when script contains parse errors
Aug 22, 2018
29008ce
Fix possible null references, add comments, improve variable types
Aug 22, 2018
811a03d
Address feedback on nulls and style
Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 127 additions & 40 deletions src/PowerShellEditorServices/Analysis/AnalysisService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use Array.Empty<PSObject>() here? That way the empty array will be cached (or pulled from cache)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that -- so long as .NET 4.5.1 supports that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aww :/

Probably not worth setting up then since we'll get it when we move to standard.


/// <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];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echo: Array.Empty<ScriptFileMarker>()


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>
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
Expand All @@ -342,7 +394,7 @@ private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
else
{
// Return an empty marker list
return new ScriptFileMarker[0];
return s_emptyScriptMarkerResult;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

Expand All @@ -360,7 +412,7 @@ private async Task<ScriptFileMarker[]> GetSemanticMarkersAsync<TSettings>(
else
{
// Return an empty marker list
return new ScriptFileMarker[0];
return s_emptyScriptMarkerResult;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

Expand All @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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)
{
Expand All @@ -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(() =>
{
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this class could be put in some utility class rather than in this file - perhaps if InvokePowerShellAsync is used elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but InvokePowerShellAsync is a method on ScriptAnalysis. I didn't want to engage in heavy refactoring

{
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>
Expand Down