Skip to content

Emit parsing errors as diagnostic records #1130

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.

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 10 commits into from
Feb 11, 2019
6 changes: 5 additions & 1 deletion Engine/Commands/InvokeScriptAnalyzerCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public string[] IncludeRule
/// <summary>
/// IncludeRule: Array of the severity types to be enabled.
/// </summary>
[ValidateSet("Warning", "Error", "Information", IgnoreCase = true)]
[ValidateSet("Warning", "Error", "Information", "ParseError", IgnoreCase = true)]
[Parameter(Mandatory = false)]
[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
public string[] Severity
Expand Down Expand Up @@ -432,6 +432,7 @@ private void WriteToOutput(IEnumerable<DiagnosticRecord> diagnosticRecords)
var errorCount = 0;
var warningCount = 0;
var infoCount = 0;
var parseErrorCount = 0;

foreach (DiagnosticRecord diagnostic in diagnosticRecords)
{
Expand All @@ -447,6 +448,9 @@ private void WriteToOutput(IEnumerable<DiagnosticRecord> diagnosticRecords)
case DiagnosticSeverity.Error:
errorCount++;
break;
case DiagnosticSeverity.ParseError:
parseErrorCount++;
break;
default:
throw new ArgumentOutOfRangeException(nameof(diagnostic.Severity), $"Severity '{diagnostic.Severity}' is unknown");
}
Expand Down
5 changes: 5 additions & 0 deletions Engine/Generic/DiagnosticRecord.cs
Original file line number Diff line number Diff line change
Expand Up @@ -142,5 +142,10 @@ public enum DiagnosticSeverity : uint
/// ERROR: This diagnostic is likely to cause a problem or does not follow PowerShell's required guidelines.
/// </summary>
Error = 2,

/// <summary>
/// ERROR: This diagnostic is caused by an actual parsing error, and is generated only by the engine.
/// </summary>
ParseError = 3,
};
}
5 changes: 5 additions & 0 deletions Engine/Generic/RuleSeverity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,10 @@ public enum RuleSeverity : uint
/// ERROR: This warning is likely to cause a problem or does not follow PowerShell's required guidelines.
/// </summary>
Error = 2,

/// <summary>
/// ERROR: This diagnostic is caused by an actual parsing error, and is generated only by the engine.
/// </summary>
ParseError = 3,
};
}
115 changes: 61 additions & 54 deletions Engine/ScriptAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public sealed class ScriptAnalyzer

private IOutputWriter outputWriter;
private Dictionary<string, object> settings;
private readonly Regex s_aboutHelpRegex = new Regex("^about_.*help\\.txt$", RegexOptions.IgnoreCase | RegexOptions.Compiled);
#if !CORECLR
private CompositionContainer container;
#endif // !CORECLR
Expand Down Expand Up @@ -1526,23 +1527,26 @@ public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefini

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

if (relevantParseErrors != null && relevantParseErrors.Count > 0)
// Add parse errors first if requested!
if ( relevantParseErrors != null && (severity == null || severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase)))
{
foreach (var parseError in relevantParseErrors)
var results = new List<DiagnosticRecord>();
foreach ( ParseError parseError in relevantParseErrors )
{
string parseErrorMessage = String.Format(CultureInfo.CurrentCulture, Strings.ParseErrorFormatForScriptDefinition, parseError.Message.TrimEnd('.'), parseError.Extent.StartLineNumber, parseError.Extent.StartColumnNumber);
this.outputWriter.WriteError(new ErrorRecord(new ParseException(parseErrorMessage), parseErrorMessage, ErrorCategory.ParserError, parseError.ErrorId));
results.Add(new DiagnosticRecord(
parseError.Message,
parseError.Extent,
parseError.ErrorId.ToString(),
DiagnosticSeverity.ParseError,
String.Empty // no script file
)
);
}
diagnosticRecords.AddRange(results);
}

if (relevantParseErrors != null && relevantParseErrors.Count > 10)
{
string manyParseErrorMessage = String.Format(CultureInfo.CurrentCulture, Strings.ParserErrorMessageForScriptDefinition);
this.outputWriter.WriteError(new ErrorRecord(new ParseException(manyParseErrorMessage), manyParseErrorMessage, ErrorCategory.ParserError, scriptDefinition));

return new List<DiagnosticRecord>();
}

// now, analyze the script definition
return diagnosticRecords.Concat(this.AnalyzeSyntaxTree(scriptAst, scriptTokens, String.Empty));
}

Expand Down Expand Up @@ -1839,49 +1843,8 @@ private IEnumerable<DiagnosticRecord> AnalyzeFile(string filePath)
this.outputWriter.WriteVerbose(string.Format(CultureInfo.CurrentCulture, Strings.VerboseFileMessage, filePath));
var diagnosticRecords = new List<DiagnosticRecord>();

//Parse the file
if (File.Exists(filePath))
{
// processing for non help script
if (!(Path.GetFileName(filePath).ToLower().StartsWith("about_") && Path.GetFileName(filePath).ToLower().EndsWith(".help.txt")))
{
try
{
scriptAst = Parser.ParseFile(filePath, out scriptTokens, out errors);
}
catch (Exception e)
{
this.outputWriter.WriteWarning(e.ToString());
return null;
}
#if !PSV3
//try parsing again
if (TrySaveModules(errors, scriptAst))
{
scriptAst = Parser.ParseFile(filePath, out scriptTokens, out errors);
}
#endif //!PSV3
var relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords);

//Runspace.DefaultRunspace = oldDefault;
if (relevantParseErrors != null && relevantParseErrors.Count > 0)
{
foreach (var parseError in relevantParseErrors)
{
string parseErrorMessage = String.Format(CultureInfo.CurrentCulture, Strings.ParserErrorFormat, parseError.Extent.File, parseError.Message.TrimEnd('.'), parseError.Extent.StartLineNumber, parseError.Extent.StartColumnNumber);
this.outputWriter.WriteError(new ErrorRecord(new ParseException(parseErrorMessage), parseErrorMessage, ErrorCategory.ParserError, parseError.ErrorId));
}
}

if (relevantParseErrors != null && relevantParseErrors.Count > 10)
{
string manyParseErrorMessage = String.Format(CultureInfo.CurrentCulture, Strings.ParserErrorMessage, System.IO.Path.GetFileName(filePath));
this.outputWriter.WriteError(new ErrorRecord(new ParseException(manyParseErrorMessage), manyParseErrorMessage, ErrorCategory.ParserError, filePath));
return new List<DiagnosticRecord>();
}
}
}
else
// If the file doesn't exist, return
if (! File.Exists(filePath))
{
this.outputWriter.ThrowTerminatingError(new ErrorRecord(new FileNotFoundException(),
string.Format(CultureInfo.CurrentCulture, Strings.InvalidPath, filePath),
Expand All @@ -1890,6 +1853,50 @@ 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 ( s_aboutHelpRegex.IsMatch(Path.GetFileName(filePath)) )
{
return diagnosticRecords.Concat(this.AnalyzeSyntaxTree(scriptAst, scriptTokens, filePath));
}

// Process script
try
{
scriptAst = Parser.ParseFile(filePath, out scriptTokens, out errors);
}
catch (Exception e)
{
this.outputWriter.WriteWarning(e.ToString());
return null;
}
#if !PSV3
//try parsing again
if (TrySaveModules(errors, scriptAst))
{
scriptAst = Parser.ParseFile(filePath, out scriptTokens, out errors);
}
#endif //!PSV3
IEnumerable<ParseError> relevantParseErrors = RemoveTypeNotFoundParseErrors(errors, out diagnosticRecords);

// First, add all parse errors if they've been requested
if ( relevantParseErrors != null && (severity == null || severity.Contains("ParseError", StringComparer.OrdinalIgnoreCase)))
{
List<DiagnosticRecord> results = new List<DiagnosticRecord>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

foreach ( ParseError parseError in relevantParseErrors )
{
string parseErrorMessage = String.Format(CultureInfo.CurrentCulture, Strings.ParseErrorFormatForScriptDefinition, parseError.Message.TrimEnd('.'), parseError.Extent.StartLineNumber, parseError.Extent.StartColumnNumber);
results.Add(new DiagnosticRecord(
parseError.Message,
parseError.Extent,
parseError.ErrorId.ToString(),
DiagnosticSeverity.ParseError,
filePath)
);
}
diagnosticRecords.AddRange(results);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

return diagnosticRecords.Concat(this.AnalyzeSyntaxTree(scriptAst, scriptTokens, filePath));
}

Expand Down
39 changes: 39 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,43 @@ Get-TestFailures

[Back to ToC](#table-of-contents)

Parser Errors
=============

In prior versions of ScriptAnalyer, errors found during parsing were reported as errors and diagnostic records were not created.
ScriptAnalyzer now emits parser errors as diagnostic records in the output stream with other diagnostic records.

```powershell
PS> Invoke-ScriptAnalyzer -ScriptDefinition '"b" = "b"; function eliminate-file () { }'

RuleName Severity ScriptName Line Message
-------- -------- ---------- ---- -------
InvalidLeftHandSide ParseError 1 The assignment expression is not
valid. The input to an
assignment operator must be an
object that is able to accept
assignments, such as a variable
or a property.
PSUseApprovedVerbs Warning 1 The cmdlet 'eliminate-file' uses an
unapproved verb.
```

The RuleName is set to the `ErrorId` of the parser error.

If ParseErrors would like to be suppressed, do not include it as a value in the `-Severity` parameter.

```powershell
PS> Invoke-ScriptAnalyzer -ScriptDefinition '"b" = "b"; function eliminate-file () { }' -Severity Warning

RuleName Severity ScriptName Line Message
-------- -------- ---------- ---- -------
PSUseApprovedVerbs Warning 1 The cmdlet 'eliminate-file' uses an
unapproved verb.
```




Suppressing Rules
=================

Expand Down Expand Up @@ -272,6 +309,8 @@ Param()

**Note**: Rule suppression is currently supported only for built-in rules.

**Note**: Parser Errors cannot be suppressed via the `SuppressMessageAttribute`

[Back to ToC](#table-of-contents)

Settings Support in ScriptAnalyzer
Expand Down
51 changes: 42 additions & 9 deletions Tests/Engine/InvokeScriptAnalyzer.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -109,24 +109,31 @@ Describe "Test available parameters" {

Describe "Test ScriptDefinition" {
Context "When given a script definition" {
It "Does not run rules on script with more than 10 parser errors" {
$moreThanTenErrors = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue -ScriptDefinition (Get-Content -Raw "$directory\CSharp.ps1")
$moreThanTenErrors.Count | Should -Be 0
It "Runs rules on script with more than 10 parser errors" {
# this is a script with 12 parse errors
$script = ');' * 12
$moreThanTenErrors = Invoke-ScriptAnalyzer -ScriptDefinition $script
$moreThanTenErrors.Count | Should -Be 12
}
}
}

Describe "Test Path" {
Context "When given a single file" {
It "Has the same effect as without Path parameter" {
$withPath = Invoke-ScriptAnalyzer $directory\TestScript.ps1
$withoutPath = Invoke-ScriptAnalyzer -Path $directory\TestScript.ps1
$withPath.Count -eq $withoutPath.Count | Should -BeTrue
$scriptPath = Join-Path $directory "TestScript.ps1"
$withPath = Invoke-ScriptAnalyzer $scriptPath
$withoutPath = Invoke-ScriptAnalyzer -Path $scriptPath
$withPath.Count | Should -Be $withoutPath.Count
}
}

It "Does not run rules on script with more than 10 parser errors" {
$moreThanTenErrors = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\CSharp.ps1
$moreThanTenErrors.Count | Should -Be 0
Context "When there are more than 10 errors in a file" {
It "All errors are found in a file" {
# this is a script with 12 parse errors
1..12 | Foreach-Object { ');' } | Out-File -Encoding ASCII "${TestDrive}\badfile.ps1"
$moreThanTenErrors = Invoke-ScriptAnalyzer -Path "${TestDrive}\badfile.ps1"
@($moreThanTenErrors).Count | Should -Be 12
}
}

Expand Down Expand Up @@ -306,6 +313,32 @@ Describe "Test Exclude And Include" {1
}

Describe "Test Severity" {
Context "Each severity can be chosen in any combination" {
BeforeAll {
$Severities = "ParseError","Error","Warning","Information"
# end space is important
$script = '$a=;ConvertTo-SecureString -Force -AsPlainText "bad practice" '
$testcases = @{ Severity = "ParseError" }, @{ Severity = "Error" },
@{ Severity = "Warning" }, @{ Severity = "Information" },
@{ Severity = "ParseError", "Error" }, @{ Severity = "ParseError","Information" },
@{ Severity = "Information", "Warning", "Error" }
}

It "Can retrieve specific severity <severity>" -testcase $testcases {
param ( $severity )
$result = Invoke-ScriptAnalyzer -ScriptDefinition $script -Severity $severity
if ( $severity -is [array] ) {
@($result).Count | Should -Be @($severity).Count
foreach ( $sev in $severity ) {
$result.Severity | Should -Contain $sev
}
}
else {
$result.Severity | Should -Be $severity
}
}
}

Context "When used correctly" {
It "works with one argument" {
$errors = Invoke-ScriptAnalyzer $directory\TestScript.ps1 -Severity Information
Expand Down
2 changes: 1 addition & 1 deletion Tests/Engine/LibraryUsage.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ function Invoke-ScriptAnalyzer {
[Parameter(Mandatory = $false)]
[string[]] $IncludeRule = $null,

[ValidateSet("Warning", "Error", "Information", IgnoreCase = $true)]
[ValidateSet("Warning", "Error", "Information", "ParseError", IgnoreCase = $true)]
[Parameter(Mandatory = $false)]
[string[]] $Severity = $null,

Expand Down
17 changes: 12 additions & 5 deletions Tests/Engine/ModuleDependencyHandler.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,12 @@ Describe "Resolve DSC Resource Dependency" {

Context "Invoke-ScriptAnalyzer without switch" {
It "Has parse errors" -skip:$skipTest {
$dr = Invoke-ScriptAnalyzer -Path $violationFilePath -ErrorVariable parseErrors -ErrorAction SilentlyContinue
$parseErrors.Count | Should -Be 1
$dr = Invoke-ScriptAnalyzer -Path $violationFilePath -ErrorVariable analyzerErrors -ErrorAction SilentlyContinue
$analyzerErrors.Count | Should -Be 0

$dr |
Where-Object { $_.Severity -eq "ParseError" } |
Get-Count | Should -Be 1
}
}

Expand Down Expand Up @@ -182,10 +186,13 @@ Describe "Resolve DSC Resource Dependency" {
Copy-Item -Recurse -Path $modulePath -Destination $tempModulePath
}

It "Doesn't have parse errors" -skip:$skipTest {
It "has a single parse error" -skip:$skipTest {
# invoke script analyzer
$dr = Invoke-ScriptAnalyzer -Path $violationFilePath -ErrorVariable parseErrors -ErrorAction SilentlyContinue
$dr.Count | Should -Be 0
$dr = Invoke-ScriptAnalyzer -Path $violationFilePath -ErrorVariable analyzerErrors -ErrorAction SilentlyContinue
$analyzerErrors.Count | Should -Be 0
$dr |
Where-Object { $_.Severity -eq "ParseError" } |
Get-Count | Should -Be 1
}

It "Keeps PSModulePath unchanged before and after invocation" -skip:$skipTest {
Expand Down
1 change: 1 addition & 0 deletions Tests/Rules/AlignAssignmentStatement.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ Configuration Sample_ChangeDescriptionAndPermissions
# NonExistentModule is not really avaiable to load. Therefore we set erroraction to
# SilentlyContinue
Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings -ErrorAction SilentlyContinue |
Where-Object { $_.Severity -ne "ParseError" } |
Get-Count |
Should -Be 4
}
Expand Down
16 changes: 9 additions & 7 deletions Tests/Rules/UseUTF8EncodingForHelpFile.tests.ps1
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
$violationMessage = "File about_utf16.help.txt has to use UTF8 instead of System.Text.UTF32Encoding encoding because it is a powershell help file."
$violationName = "PSUseUTF8EncodingForHelpFile"
$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
$violations = Invoke-ScriptAnalyzer $directory\about_utf16.help.txt | Where-Object {$_.RuleName -eq $violationName}
$noViolations = Invoke-ScriptAnalyzer $directory\about_utf8.help.txt | Where-Object {$_.RuleName -eq $violationName}
$notHelpFileViolations = Invoke-ScriptAnalyzer $directory\utf16.txt | Where-Object {$_.RuleName -eq $violationName}

$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
Describe "UseUTF8EncodingForHelpFile" {
BeforeAll {
$violationMessage = "File about_utf16.help.txt has to use UTF8 instead of System.Text.UTF32Encoding encoding because it is a powershell help file."
$violationName = "PSUseUTF8EncodingForHelpFile"
$violations = Invoke-ScriptAnalyzer $directory\about_utf16.help.txt | Where-Object {$_.RuleName -eq $violationName}
$noViolations = Invoke-ScriptAnalyzer $directory\about_utf8.help.txt | Where-Object {$_.RuleName -eq $violationName}
$notHelpFileViolations = Invoke-ScriptAnalyzer $directory\utf16.txt | Where-Object {$_.RuleName -eq $violationName}
}

Context "When there are violations" {
It "has 1 avoid use utf8 encoding violation" {
$violations.Count | Should -Be 1
Expand Down
Loading