Fix formatter crash when script contains parse errors by rjmholt · Pull Request #728 · PowerShell/PowerShellEditorServices

rjmholt · 2018-08-22T20:42:49Z

Addresses PowerShell/PSScriptAnalyzer#1057 and PowerShell/PSScriptAnalyzer#826 (comment).

When the formatter was run, it we would try to format even a document with parse errors. Now if there are parse errors, the formatter will do nothing (silently). Ideally we should make VSCode report that formatting failed and why, although this would mean returning a different format result.

TylerLeonhardt

Looking good. Just a couple questions before I sign off

TylerLeonhardt · 2018-08-23T02:17:36Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

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


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

Yeah started out writing a different message

TylerLeonhardt · 2018-08-23T02:23:30Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+        /// Wraps the result of an execution of PowerShell to send back through
+        /// asynchronous calls.
+        /// </summary>
+        private class PowerShellResult


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

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

TylerLeonhardt · 2018-08-23T02:41:25Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

                }

-                var result = new PSObject[0];
+                PowerShellResult result = null;


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

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??

TylerLeonhardt · 2018-08-23T02:46:45Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

-                    result = powerShell.Invoke()?.ToArray();
+                    PSObject[] output = powerShell.Invoke()?.ToArray();
+                    ErrorRecord[] errors = powerShell.Streams?.Error?.ToArray();
+                    result = new PowerShellResult(output, errors, powerShell.HadErrors);


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

Looks like we think they can't actually be null. Would be so much nicer if the type system were able to track that...

rkeithhill · 2018-08-23T02:58:55Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

            {
                // Return an empty marker list
-                return new ScriptFileMarker[0];
+                return s_emptyScriptMarkerResult;


rkeithhill · 2018-08-23T02:59:07Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

            {
                // Return an empty marker list
-                return new ScriptFileMarker[0];
+                return s_emptyScriptMarkerResult;


rkeithhill

LGTM

SeeminglyScience

LGTM, just a couple things to look at.

SeeminglyScience · 2018-08-23T11:28:48Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+        /// 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[]


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", };

SeeminglyScience · 2018-08-23T11:30:23Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+        /// <summary>
+        /// An empty diagnostic result to return when a script fails analysis.
+        /// </summary>
+        private static readonly PSObject[] s_emptyDiagnosticResult = new PSObject[0];


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

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

Looks like it doesn't.

Aww :/

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

SeeminglyScience · 2018-08-23T11:30:42Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

+        /// <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];


echo: Array.Empty<ScriptFileMarker>()

SeeminglyScience · 2018-08-23T11:41:33Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

                {
-                    result = powerShell.Invoke()?.ToArray();
+                    PSObject[] output = powerShell.Invoke()?.ToArray();
+                    ErrorRecord[] errors = powerShell.Streams?.Error?.ToArray();


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.

That's what I was hoping for

TylerLeonhardt · 2018-08-27T17:25:42Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

        }

-        private PowerShellResult InvokePowerShell(string command, IDictionary<string, object> paramArgMap)
+        private PowerShellResult InvokePowerShell(string command, IDictionary<string, object> paramArgMap = null)


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

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.

TylerLeonhardt · 2018-08-27T17:26:05Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

        }

-        private async Task<PowerShellResult> InvokePowerShellAsync(string command, IDictionary<string, object> paramArgMap)
+        private async Task<PowerShellResult> InvokePowerShellAsync(string command, IDictionary<string, object> paramArgMap = null)


consider an empty dictionary instead of null

TylerLeonhardt

LGTM just a small nit

Robert Holt added 2 commits August 22, 2018 13:33

Fix formatter crash when script contains parse errors

6f51560

Fix possible null references, add comments, improve variable types

29008ce

rjmholt requested review from SeeminglyScience, TylerLeonhardt and rkeithhill August 22, 2018 20:42

TylerLeonhardt reviewed Aug 23, 2018

View reviewed changes

rkeithhill reviewed Aug 23, 2018

View reviewed changes

rkeithhill approved these changes Aug 23, 2018

View reviewed changes

SeeminglyScience approved these changes Aug 23, 2018

View reviewed changes

Address feedback on nulls and style

811a03d

TylerLeonhardt reviewed Aug 27, 2018

View reviewed changes

TylerLeonhardt approved these changes Aug 27, 2018

View reviewed changes

rjmholt merged commit 8e29349 into PowerShell:master Aug 28, 2018

rjmholt deleted the fix-formatter-crash branch December 12, 2018 06:00

Conversation

rjmholt commented Aug 22, 2018

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill left a comment

Choose a reason for hiding this comment

Uh oh!

SeeminglyScience left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

rjmholt Aug 27, 2018 •

edited

Loading