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 1 commit
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
43 changes: 29 additions & 14 deletions src/PowerShellEditorServices/Analysis/AnalysisService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
/// </summary>
private static readonly string[] s_includedRules = new string[]
{
private static readonly string[] s_includedRules = {
"PSUseToExportFieldsInManifest",
"PSMisleadingBacktick",
"PSAvoidUsingCmdletAliases",
Expand All @@ -58,6 +57,8 @@ public class AnalysisService : IDisposable
/// </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>
Expand Down Expand Up @@ -299,9 +300,15 @@ public async Task<ScriptFileMarker[]> GetSemanticMarkersAsync(
/// </summary>
public IEnumerable<string> 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<string>();
IEnumerable<PSObject> ruleObjects = InvokePowerShell("Get-ScriptAnalyzerRule", new Dictionary<string, object>()).Output;
foreach (var rule in ruleObjects)
foreach (var rule in getRuleResult.Output)
{
ruleNames.Add((string)rule.Members["RuleName"].Value);
}
Expand Down Expand Up @@ -341,7 +348,7 @@ public async Task<string> Format(

if (result == null)
{
_logger.Write(LogLevel.Error, $"Formatter returned null result");
_logger.Write(LogLevel.Error, "Formatter returned null result");
return null;
}

Expand Down Expand Up @@ -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<string, object>{ {"Name", PSSA_MODULE_NAME} })?.Output;
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 @@ -515,22 +527,25 @@ private async Task<PSObject[]> GetDiagnosticRecordsAsync<TSettings>(
return diagnosticRecords;
}

private PowerShellResult InvokePowerShell(string command, IDictionary<string, object> paramArgMap)
private PowerShellResult InvokePowerShell(string command, IDictionary<string, object> paramArgMap = null)
Copy link
Member

Choose a reason for hiding this comment

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

the default param could be an empty dictionary and then you don't need he null check on line 536

Copy link
Contributor Author

@rjmholt rjmholt Aug 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I made this change because one of the calls to this method had to create a new empty dictionary just to call the method. A null check is much cheaper than creating a new dictionary.

I think the question boils down to "do we make the caller or the callee handle the complexity?". Since a caller could always pass in null for the dictionary (and we didn't handle it), I think defaulting to null and making the callee handle it is probably more efficient, safer and easier to use.

{
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);
}
}

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
{
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);
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 @@ -552,7 +567,7 @@ private PowerShellResult InvokePowerShell(string command, IDictionary<string, ob
}
}

private async Task<PowerShellResult> InvokePowerShellAsync(string command, IDictionary<string, object> paramArgMap)
private async Task<PowerShellResult> InvokePowerShellAsync(string command, IDictionary<string, object> paramArgMap = null)
Copy link
Member

Choose a reason for hiding this comment

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

consider an empty dictionary instead of null

{
var task = Task.Run(() =>
{
Expand Down