Emit parsing errors as diagnostic records #1130

JamesWTruher · 2019-01-17T23:59:12Z

PR Summary

Fixes #1041
Emit parsing errors as diagnostic records.

I'm attempting to bring analyzer into a more lint like behavior which treats parsing/syntax errors as just another type of diagnostic.

Currently, errors during parsing are written as error records, but they should probably just be diagnostic records. This PR creates a new diagnostic record type and emits errors found during parsing into the result stream. This is far more useful to downstream readers (such as VSCode/EditorServices) because there's a single stream for information about the script. Rule violations and syntax errors.

I have also removed the current limitation of any time there are more than 10 parse errors, the analyzer simply returns, which seems to me to be pretty undesirable. We will now emit diagnostic records for all parse errors.

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…r than emitting error records Embedded tools such as VSCode have real difficulty when there are parser errors. We need to be sure to just return what we find and let the down stream consumers filter.

I've noticed it's sometimes hard to determine went wrong in the build, this may help a bit

TylerLeonhardt · 2019-01-23T06:44:40Z

Engine/ScriptAnalyzer.cs

+                            filePath)
+                        );
+                }
+                diagnosticRecords.AddRange(results);


Any reason you add all the results to a temp array and then use AddRange instead of just adding directly to diagnosticRecords?

earlier debugging processes allowed me to see the new parser errors more easily

bergmeister · 2019-01-23T06:53:24Z

From a high level this makes sense. There are a few tests to be fixed/looked at first.
Can one suppress those special diagnostic records? Does this work together with the -Severity parameter on Invoke-ScriptAnalyzer?
I'll do a proper review once the PR is ready.

Some tests are checking ErrorVariable for parse errors which are now part of the output stream

All findings are returned by default, but if you exclude ParseErrors in -Severity we won't emit them.

Force array of output

…ror severity

JamesWTruher · 2019-02-07T20:44:01Z

@bergmeister - suggestions for documentation additions?

bergmeister · 2019-02-07T20:58:23Z

Engine/ScriptAnalyzer.cs

@@ -1526,23 +1526,27 @@ public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefini

            var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out List<DiagnosticRecord> diagnosticRecords);

-            if (relevantParseErrors != null && relevantParseErrors.Count > 0)
+            int emitParseErrors = severity == null ? 1 : severity.Count(item => item == "ParseError");


converting this to a boolean and using Any() instead of .Count() would be better

Engine/ScriptAnalyzer.cs

bergmeister · 2019-02-07T21:16:55Z

Looks very nice. I also checked how it looks with the integration into VS-Code and it looks so much better. But should we maybe prepend ParseError: to it?

bergmeister · 2019-02-07T21:27:42Z

@JamesWTruher How would people suppress it in vscode (where no command line exists and most people do not use settings files)

rjmholt · 2019-02-07T20:55:42Z

Engine/ScriptAnalyzer.cs

-            if (relevantParseErrors != null && relevantParseErrors.Count > 0)
+            int emitParseErrors = severity == null ? 1 : severity.Count(item => item == "ParseError");
+            // Add parse errors first if requested!
+            if ( relevantParseErrors != null && emitParseErrors == 1)


Rather than create the variable above which seems to be a boolean in wolf's clothing, I'd just inline the test here:

Suggested change

if ( relevantParseErrors != null && emitParseErrors == 1)

if ( relevantParseErrors != null && (severity == null || severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase)))

@rjmholt I made those changes

rjmholt · 2019-02-07T20:57:17Z

Engine/ScriptAnalyzer.cs

+                        parseError.Extent,
+                        parseError.ErrorId.ToString(),
+                        DiagnosticSeverity.ParseError,
+                        "" // no script file


Suggested change

"" // no script file

string.Empty // no script file

rjmholt · 2019-02-07T20:57:39Z

Engine/ScriptAnalyzer.cs

            {
-                foreach (var parseError in relevantParseErrors)
+                List<DiagnosticRecord> results = new List<DiagnosticRecord>();


Suggested change

List<DiagnosticRecord> results = new List<DiagnosticRecord>();

var results = new List<DiagnosticRecord>();

rjmholt · 2019-02-07T20:58:29Z

Engine/ScriptAnalyzer.cs

            {
-                foreach (var parseError in relevantParseErrors)
+                List<DiagnosticRecord> results = new List<DiagnosticRecord>();
+                foreach ( var parseError in relevantParseErrors )


Suggested change

foreach ( var parseError in relevantParseErrors )

foreach (ParseError parseError in relevantParseErrors)

@rjmholt fixed

rjmholt · 2019-02-07T21:05:55Z

Engine/ScriptAnalyzer.cs

@@ -1890,6 +1853,51 @@ private IEnumerable<DiagnosticRecord> AnalyzeFile(string filePath)
                return null;
            }

+            // short-circuited processing for a help file
+            // no parsing can really be done, but there are other rules to run (specifically encoding).
+            if ( Regex.Matches(Path.GetFileName(filePath), @"^about_.*help.txt$", RegexOptions.IgnoreCase).Count != 0)


This regex might benefit from being a compiled private readonly static field:

private readonly Regex s_aboutHelpRegex = new Regex("^about_.*help\\.txt$", RegexOptions.IgnoreCase | RegexOptions.Compiled);

Then here:

Suggested change

if ( Regex.Matches(Path.GetFileName(filePath), @"^about_.*help.txt$", RegexOptions.IgnoreCase).Count != 0)

if (s_aboutHelpRegex.IsMatch(Path.GetFileName(filePath)))

EDIT: Put a backlash in for the . before txt to distinguish it from the regex wildcard.

Or the other possibility is just to do:

string fileName = Path.GetFileName(filePath); if (fileName.StartsWith("about_", StringComparison.OrdinalIgnoreCase) && fileName.EndsWith("help.txt", StringComparison.OrdinalIgnoreCase)) { ... }

This is probably the most efficient option, since it doesn't even need to read the whole string.

I went with the regex

rjmholt · 2019-02-07T21:07:31Z

Engine/ScriptAnalyzer.cs

+                scriptAst = Parser.ParseFile(filePath, out scriptTokens, out errors);
+            }
+#endif //!PSV3
+            var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords);


Suggested change

var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords);

IEnumerable<ParseError> relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords);

rjmholt · 2019-02-07T21:08:28Z

Engine/ScriptAnalyzer.cs

+
+            // First, add all parse errors if they've been requested
+            int emitParseErrors = severity == null ? 1 : severity.Count(item => item == "ParseError");
+            if ( relevantParseErrors != null && emitParseErrors == 1 )


Suggested change

if ( relevantParseErrors != null && emitParseErrors == 1 )

if (relevantParseErrors != null && (severity == null || severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase)))

rjmholt · 2019-02-07T21:08:42Z

Engine/ScriptAnalyzer.cs

+            int emitParseErrors = severity == null ? 1 : severity.Count(item => item == "ParseError");
+            if ( relevantParseErrors != null && emitParseErrors == 1 )
+            {
+                List<DiagnosticRecord> results = new List<DiagnosticRecord>();


Suggested change

List<DiagnosticRecord> results = new List<DiagnosticRecord>();

var results = new List<DiagnosticRecord>();

rjmholt · 2019-02-07T21:08:56Z

Engine/ScriptAnalyzer.cs

+            if ( relevantParseErrors != null && emitParseErrors == 1 )
+            {
+                List<DiagnosticRecord> results = new List<DiagnosticRecord>();
+                foreach ( var parseError in relevantParseErrors )


Suggested change

foreach ( var parseError in relevantParseErrors )

foreach (ParseError parseError in relevantParseErrors)

rjmholt · 2019-02-07T21:33:25Z

How would people suppress it in vscode (where no command line exists and most people do not use settings files)

The red squiggly in VSCode comes from PSES, not PSScriptAnalyzer, because those are legitimate parse errors. I'm not sure if we currently have a way of suppressing them. Installing the PowerShell extension and then suppressing parse errors would be like buying a fighter jet and ripping out the flight computer.

bergmeister · 2019-02-07T21:46:19Z

@rjmholt Thanks for the info, that's good to know, I agree, lets keep it how it is, PSES can keep its flight computer ;-)

bergmeister

Some minor comments, and maybe add a few lines into the main readme to explain this new type and that one does not suppress it like PSSA rules.
Otherwise it looks great, feel free to merge afterwards

Update documentation to include new behavior for ParseErrors and fix up logic to be a bit cleaner when emiting ParseErrors

kilasuit · 2019-02-11T21:48:08Z

LGTM

JamesWTruher · 2019-02-11T22:01:43Z

I've looked at implementing support for [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute] and while it's possible, it doesn't work like I want. (the bare attribute can't exist without a param statement). I could also do something like #pragma, etc, but I haven't looked at that. For the moment, -Severity can be set to Warning,Error,Information which will suppress the output.

bergmeister · 2019-02-11T22:09:32Z

Sounds good. We have a separate issue to introduce pragma like region or line based suppression anyway

plastikfan · 2020-09-09T17:17:29Z

How would people suppress it in vscode (where no command line exists and most people do not use settings files)

The red squiggly in VSCode comes from PSES, not PSScriptAnalyzer, because those are legitimate parse errors. I'm not sure if we currently have a way of suppressing them. Installing the PowerShell extension and then suppressing parse errors would be like buying a fighter jet and ripping out the flight computer.

What is PSES? I am experiencing this error in relation to PowerShell class inside a PowerShell module and although my code is correct (as a result of concatenating all source files into a combined .psm1 file), VSC is still flagging in the original source. This is rather unfortunate because I'm not sure how to disable the flagging of this non-error if it isn't being generated by PSSA built into VSC.

rjmholt · 2020-09-09T17:27:26Z

I am experiencing this error in relation to PowerShell class inside a PowerShell module and although my code is correct (as a result of concatenating all source files into a combined .psm1 file), VSC is still flagging in the original source

If you run the PowerShell parser itself on your file, it will raise this error. The PowerShell VSCode extension just reports that. There's no way to suppress a parser error currently, because parser errors are the most serious kind of error. This is to say that what you're seeing isn't an error in any of the tooling -- it's a mismatch between how you're handling the file and what PowerShell expects. The two options I can imagine are:

Open a feature request on the PowerShell extension for VSCode to suppress parser errors
Change the way you handle these scripts in some way (for example, marking them as template files)

The script as you have it won't run as a standalone. That's what the PowerShell extension and all the other tools assess currently.

kilasuit · 2020-09-09T18:54:17Z

What is PSES?

@plastikfan it is PowerShell Editor Services - https://github.com/PowerShell/PowerShellEditorServices

plastikfan · 2020-09-09T19:40:56Z

* Open a feature request on the PowerShell extension for VSCode to suppress parser errors

* Change the way you handle these scripts in some way (for example, marking them as template files)
The script as you have it won't run as a standalone. That's what the PowerShell extension and all the other tools assess currently.

I've decided that the best way forward is to constrain all class references to a single file. So I currently have the need for 2 classes. Instead of implementing them in separate files, I am going to implement them all in the same file including defining a factory function to create the class instances:

controller.ps1:

class Controller {
  [System.Collections.Hashtable]$_passThru;

  Controller(
    [System.Collections.Hashtable]$passThru
  ) {
    $this._passThru = $passThru;
  }
}

class ForeachController : Controller {
  ForeachController(
    [System.Collections.Hashtable]$passThru
  ) {

  }
}

function New-Controller {
  # return appropriate new instance
}

No other module code will make explicit reference to classes. This is the best way I can see going forweard for the time being. It's less than desirable, but at least I don't have to put up with none-errors being reported by VSC and potentially hiding real issues.

So @rjmholt, when you say raise a feature request against the powershell extension for VScode, is this https://github.com/PowerShell/vscode-powershell or if the error is being emitted from PSES, shouldn't I submit to that instead?

rjmholt · 2020-09-09T20:17:16Z

So @rjmholt, when you say raise a feature request against the powershell extension for VScode, is this https://github.com/PowerShell/vscode-powershell or if the error is being emitted from PSES, shouldn't I submit to that instead?

Almost all the issues in the vscode-powershell repo are actually things implemented in PSES, but people were confused by that distinction and redirecting them just increased friction. So you're free to open the issue wherever, but we have more set up to track it properly in vscode-powershell.

plastikfan · 2020-09-09T20:22:36Z

Actually on second thoughts, having chatted some folks on POwerShell group on Gitter, I've decided to avoid using POwerShell classes altogther because there are potentially lots of gotchyas. I'm going to resort to attaching functions to PSCustomObjects and just be done with it. As much as I love PowerShell, the number of gotchyas in it are infuriating, so I'd rather just avoid classes which I have not used in PowerShell up to this point. I could do without the grief!

JustinGrote · 2021-10-22T15:21:33Z

@rjmholt Was an issue ever opened for this? Can we reference it here?

JamesWTruher added 2 commits January 17, 2019 15:50

update analyzer to create diagnostic records for parser errors, rathe…

18c43e6

…r than emitting error records Embedded tools such as VSCode have real difficulty when there are parser errors. We need to be sure to just return what we find and let the down stream consumers filter.

update the output in the case that we don't get a build.

36061f5

I've noticed it's sometimes hard to determine went wrong in the build, this may help a bit

JamesWTruher requested review from bergmeister, rjmholt and TylerLeonhardt January 17, 2019 23:59

TylerLeonhardt reviewed Jan 23, 2019

View reviewed changes

This was referenced Jan 29, 2019

Write-Warning instead of returning artificial DiagnosticRecord when type parsing errors occurs since those DiagnosticRecords cannot be suppressed. #1126

Closed

TypeNotFound suppressions #1041

Closed

Fix test failures

edaca8e

Some tests are checking ErrorVariable for parse errors which are now part of the output stream

JamesWTruher force-pushed the ParseErrorDiagnostic branch from 84ace38 to edaca8e Compare January 31, 2019 22:50

JamesWTruher added 6 commits January 31, 2019 17:13

Make sure that you can suppress ParseErrors

37c095c

All findings are returned by default, but if you exclude ParseErrors in -Severity we won't emit them.

Fix ordering typo, remove parse errors before counting

4c347a1

Change invocation to not ignore errors

b3ef54a

Force array of output

Add test to its own context block

8443abf

Add tests for permutations of Severity which includes the new ParseEr…

f6f3309

…ror severity

Add 'ParseError' to severity parameter for proxy cmdlet

d34b8cb

JamesWTruher changed the title ~~WIP: Emit parsing errors as diagnostic records~~ Emit parsing errors as diagnostic records Feb 7, 2019

bergmeister reviewed Feb 7, 2019

View reviewed changes

Engine/ScriptAnalyzer.cs Outdated Show resolved Hide resolved

rjmholt reviewed Feb 7, 2019

View reviewed changes

bergmeister approved these changes Feb 7, 2019

View reviewed changes

Change check for help file to use static regex

0374851

Update documentation to include new behavior for ParseErrors and fix up logic to be a bit cleaner when emiting ParseErrors

JamesWTruher merged commit c9915b1 into PowerShell:development Feb 11, 2019

bergmeister mentioned this pull request Feb 14, 2019

ScriptAnalyzer needs to emit Diagnostic records for AST parsing errors #264

Closed

bergmeister added the Breaking Change label Mar 14, 2019

	if ( relevantParseErrors != null && emitParseErrors == 1)
	if ( relevantParseErrors != null && (severity == null \|\| severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase)))

	List<DiagnosticRecord> results = new List<DiagnosticRecord>();
	var results = new List<DiagnosticRecord>();

	foreach ( var parseError in relevantParseErrors )
	foreach (ParseError parseError in relevantParseErrors)

	if ( Regex.Matches(Path.GetFileName(filePath), @"^about_.*help.txt$", RegexOptions.IgnoreCase).Count != 0)
	if (s_aboutHelpRegex.IsMatch(Path.GetFileName(filePath)))

	var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords);
	IEnumerable<ParseError> relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords);

	if ( relevantParseErrors != null && emitParseErrors == 1 )
	if (relevantParseErrors != null && (severity == null \|\| severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase)))

Emit parsing errors as diagnostic records #1130

Emit parsing errors as diagnostic records #1130

Conversation

JamesWTruher commented Jan 17, 2019 • edited by bergmeister Loading

PR Summary

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister commented Jan 23, 2019 • edited Loading

JamesWTruher commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergmeister commented Feb 7, 2019 • edited Loading

bergmeister commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

rjmholt Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Feb 7, 2019

bergmeister commented Feb 7, 2019

bergmeister left a comment • edited Loading

Choose a reason for hiding this comment

kilasuit commented Feb 11, 2019

JamesWTruher commented Feb 11, 2019

bergmeister commented Feb 11, 2019

plastikfan commented Sep 9, 2020

rjmholt commented Sep 9, 2020

kilasuit commented Sep 9, 2020 • edited Loading

plastikfan commented Sep 9, 2020

rjmholt commented Sep 9, 2020

plastikfan commented Sep 9, 2020

JustinGrote commented Oct 22, 2021

JamesWTruher commented Jan 17, 2019 •

edited by bergmeister

Loading

bergmeister commented Jan 23, 2019 •

edited

Loading

bergmeister commented Feb 7, 2019 •

edited

Loading

rjmholt Feb 7, 2019 •

edited

Loading

rjmholt Feb 7, 2019 •

edited

Loading

bergmeister left a comment •

edited

Loading

kilasuit commented Sep 9, 2020 •

edited

Loading