-
Notifications
You must be signed in to change notification settings - Fork 407
Description
The -Severity parameter of Invoke-ScriptAnalyzer does not do what it says it does.
Parameter help for the -Severity parameter says:
After running Script Analyzer with all rules, this parameter selects rule violations with the specified severity.
...
The parameter filters the rules violations only after running all rules.
...
In reality the parameter filters rules and not diagnostics; both have the concept of Severity (RuleSeverity, and DiagnosticSeverity)
How the -Severity parameter is ultimately used:
-
The user passes
-SeveritytoInvoke-ScriptAnalyzer. Value validated to be one of"Warning", "Error", "Information", "ParseError". -
The
ScriptAnalyzersingleton instance is initialised with this-Severitystring[]value, which is stored as a private member of the singleton object (severity). -
When analyzing a script, the engine determines which rules to run. It uses
IsRuleAllowed()to check each rule. -
IsRuleAllowed()gets a list ofallowedSeverities. It does so by callingGetAllowedSeveritiesInInt().Each
severity(an array of strings) is parsed into the underlyinguintvalue of theDiagnosticSeverityenum type.(I believe this should actually be
RuleSeverityenum. It works as both enums have identical members and underlyinguintvalues).
PSScriptAnalyzer/Engine/ScriptAnalyzer.cs
Lines 1929 to 1934 in aba29c3
IEnumerable<uint> GetAllowedSeveritiesInInt() { return severity != null ? severity.Select(item => (uint)Enum.Parse(typeof(DiagnosticSeverity), item, true)) : null; } -
IsRuleAllowed()then callsIsSeverityAllowed(allowedSeverities, rule)as part of it's decision making on whether to execute a rule. Passing in the list ofallowedSeveritiesand the currentrulebeing considered. -
IsSeverityAllowed(..)then checks ifallowedSeveritiescontains therules severity (callingrule.GetSeverity()and casting it to auint). If it does the rule is allowed to run.
PSScriptAnalyzer/Engine/ScriptAnalyzer.cs
Lines 1914 to 1921 in aba29c3
private bool IsSeverityAllowed(IEnumerable<uint> allowedSeverities, IRule rule) { return severity == null || (allowedSeverities != null && rule != null && HasGetSeverity(rule) && allowedSeverities.Contains((uint)rule.GetSeverity())); }
So there are 2 issues that need to be addressed:
-
There is a disparity between the documentation and implementation. Either:
- The documentation needs updating to match reality OR
- The implementation needs to be updated so
-Severityfilters the output ofDiagnosticRecords.
-
GetAllowedSeveritiesInInt()resolves severities usingDiagnosticSeverityand that is then used to compare torule.GetSeverity()which is aRuleSeverity. It works now as both enums are identical in their definition and are being cast to their underlyinguintvalue. If either was updated this could break. It also does not seem intentional.
@bergmeister - Should we update the documentation as a first pass and revisit behaviour later if it's desired? I will also get the enum corrected to RuleSeverity.
This explains the issue described in #2049 - where a custom rule is emitting error-level DiagnosticRecord but is not showing with -Severity Error, but is with -Severity Warning. -Severity is filtering the rules that are run (by their severity) and all custom rules have Warning severity.
PSScriptAnalyzer/Engine/Generic/ExternalRule.cs
Lines 55 to 59 in aba29c3
| //Set the community rule level as warning as the current implementation does not require user to specify rule severity when defining their functions in PS scripts | |
| public RuleSeverity GetSeverity() | |
| { | |
| return RuleSeverity.Warning; | |
| } |
n.b. I have typed the word severity so many times now that it no longer looks like a real word...