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 2 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
144 changes: 108 additions & 36 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,47 @@ 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 = new string[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't know how everyone feels about this, but you can ditch the new string[] and just keep the braces for a field initialization. e.g

private static readonly string[] s_includedRules =
{
    "MyRules",
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh!

{
"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>()


/// <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 +94,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,8 +299,8 @@ 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>());
var ruleNames = new List<string>();
IEnumerable<PSObject> ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary<string, object>()).Output;
foreach (var rule in ruleObjects)
{
ruleNames.Add((string)rule.Members["RuleName"].Value);
Expand Down Expand Up @@ -319,8 +337,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");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: do you need the $? It doesn't seem to be a template string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah started out writing a different message

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 +387,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 +405,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 @@ -382,7 +427,7 @@ private void LogAvailablePssaFeatures()
{
PSObject[] modules = InvokePowerShell(
"Get-Module",
new Dictionary<string, object>{ {"Name", PSSA_MODULE_NAME} });
new Dictionary<string, object>{ {"Name", PSSA_MODULE_NAME} })?.Output;

_pssaModuleInfo = modules
.Select(m => m.BaseObject)
Expand Down Expand Up @@ -426,7 +471,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 +497,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,7 +515,7 @@ 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)
{
using (var powerShell = System.Management.Automation.PowerShell.Create())
{
Expand All @@ -479,10 +526,12 @@ private PSObject[] InvokePowerShell(string command, IDictionary<string, object>
powerShell.AddParameter(kvp.Key, kvp.Value);
}

var result = new PSObject[0];
PowerShellResult result = null;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be set to null? Just want to make sure we don't cause a null pointer exception somewhere else :)

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 think it has to be set to something here, and creating a new PowerShell result would require creating two new arrays as well. Doesn't seem right.

I think I've dealt with the possibility that any result here could return null at the call site.

But, perhaps it's better to do something here like throw an analysis exception, or something else we catch??

try
{
result = powerShell.Invoke()?.ToArray();
PSObject[] output = powerShell.Invoke()?.ToArray();
ErrorRecord[] errors = powerShell.Streams?.Error?.ToArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be able to drop the null conditionals, as far as I know Streams and Error are always populated.

Come to think of it, I don't think powerShell.Invoke() can return null either, just an empty collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I was hoping for

result = new PowerShellResult(output, errors, powerShell.HadErrors);
Copy link
Member

Choose a reason for hiding this comment

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

I notice that both output and errors could potentially be set to null here... Is that going to be ok?

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 we think they can't actually be null. Would be so much nicer if the type system were able to track that...

}
catch (CommandNotFoundException ex)
{
Expand All @@ -503,7 +552,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)
{
var task = Task.Run(() =>
{
Expand Down Expand Up @@ -590,6 +639,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