Skip to content

Prevent duplicate rule ID suppression errors in -Fix and DSC scenarios #2181

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

Open
liamjpeters wants to merge 1 commit into
base: main
Choose a base branch
from
+80 −16
Open

Prevent duplicate rule ID suppression errors in -Fix and DSC scenarios #2181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 26 additions & 16 deletions Engine/ScriptAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,7 +1450,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeAndFixPath(string path, Func<string,
/// <param name="scriptTokens">Parsed tokens of <paramref name="scriptDefinition"/.></param>
/// <param name="skipVariableAnalysis">Whether variable analysis can be skipped (applicable if rules do not use variable analysis APIs).</param>
/// <returns></returns>
public List<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, out ScriptBlockAst scriptAst, out Token[] scriptTokens, bool skipVariableAnalysis = false)
public List<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, out ScriptBlockAst scriptAst, out Token[] scriptTokens, bool skipVariableAnalysis = false, bool emitSuppressionErrors = true)
{
scriptAst = null;
scriptTokens = null;
Expand Down Expand Up @@ -1490,7 +1490,7 @@ public List<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, o
}

// now, analyze the script definition
diagnosticRecords.AddRange(this.AnalyzeSyntaxTree(scriptAst, scriptTokens, null, skipVariableAnalysis));
diagnosticRecords.AddRange(this.AnalyzeSyntaxTree(scriptAst, scriptTokens, null, skipVariableAnalysis, emitSuppressionErrors));
return diagnosticRecords;
}

Expand Down Expand Up @@ -1549,11 +1549,11 @@ public EditableText Fix(EditableText text, Range range, bool skipParsing, out Ra
IEnumerable<DiagnosticRecord> records;
if (skipParsing && previousUnusedCorrections == 0)
{
records = AnalyzeSyntaxTree(scriptAst, scriptTokens, String.Empty, skipVariableAnalysis);
records = AnalyzeSyntaxTree(scriptAst, scriptTokens, String.Empty, skipVariableAnalysis, emitSuppressionErrors: false);
}
else
{
records = AnalyzeScriptDefinition(text.ToString(), out scriptAst, out scriptTokens, skipVariableAnalysis);
records = AnalyzeScriptDefinition(text.ToString(), out scriptAst, out scriptTokens, skipVariableAnalysis, emitSuppressionErrors: false);
}
var corrections = records
.Select(r => r.SuggestedCorrections)
Expand Down Expand Up @@ -1986,17 +1986,21 @@ bool IsRuleAllowed(IRule rule)
private Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
string ruleName,
Dictionary<string, List<RuleSuppression>> ruleSuppressions,
List<DiagnosticRecord> ruleDiagnosticRecords)
List<DiagnosticRecord> ruleDiagnosticRecords,
bool emitSuppressionErrors = true)
{
List<ErrorRecord> suppressRuleErrors;
var records = Helper.Instance.SuppressRule(
ruleName,
ruleSuppressions,
ruleDiagnosticRecords,
out suppressRuleErrors);
foreach (var error in suppressRuleErrors)
if (emitSuppressionErrors)
{
this.outputWriter.WriteError(error);
foreach (var error in suppressRuleErrors)
{
this.outputWriter.WriteError(error);
}
}
return records;
}
Expand All @@ -2014,13 +2018,15 @@ private Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
/// <returns>Returns a tuple of suppressed and diagnostic records</returns>
private Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
Dictionary<string, List<RuleSuppression>> ruleSuppressions,
DiagnosticRecord ruleDiagnosticRecord
DiagnosticRecord ruleDiagnosticRecord,
bool emitSuppressionErrors = true
)
{
return SuppressRule(
ruleDiagnosticRecord.RuleName,
ruleSuppressions,
new List<DiagnosticRecord> { ruleDiagnosticRecord });
new List<DiagnosticRecord> { ruleDiagnosticRecord },
emitSuppressionErrors);
}

/// <summary>
Expand All @@ -2037,7 +2043,8 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
ScriptBlockAst scriptAst,
Token[] scriptTokens,
string filePath,
bool skipVariableAnalysis = false)
bool skipVariableAnalysis = false,
bool emitSuppressionErrors = true)
{
Dictionary<string, List<RuleSuppression>> ruleSuppressions = new Dictionary<string,List<RuleSuppression>>();
ConcurrentBag<DiagnosticRecord> diagnostics = new ConcurrentBag<DiagnosticRecord>();
Expand Down Expand Up @@ -2117,7 +2124,10 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
ruleSuppressions,
ruleRecords,
out suppressRuleErrors);
result.AddRange(suppressRuleErrors);
if (emitSuppressionErrors)
{
result.AddRange(suppressRuleErrors);
}
foreach (var record in records.Item2)
{
diagnostics.Add(record);
Expand Down Expand Up @@ -2177,7 +2187,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
try
{
var ruleRecords = tokenRule.AnalyzeTokens(scriptTokens, filePath).ToList();
var records = SuppressRule(tokenRule.GetName(), ruleSuppressions, ruleRecords);
var records = SuppressRule(tokenRule.GetName(), ruleSuppressions, ruleRecords, emitSuppressionErrors);
foreach (var record in records.Item2)
{
diagnostics.Add(record);
Expand Down Expand Up @@ -2215,7 +2225,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
try
{
var ruleRecords = dscResourceRule.AnalyzeDSCClass(scriptAst, filePath).ToList();
var records = SuppressRule(dscResourceRule.GetName(), ruleSuppressions, ruleRecords);
var records = SuppressRule(dscResourceRule.GetName(), ruleSuppressions, ruleRecords, emitSuppressionErrors);
foreach (var record in records.Item2)
{
diagnostics.Add(record);
Expand All @@ -2234,7 +2244,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
}

// Check if the supplied artifact is indeed part of the DSC resource
if (!filePathIsNullOrWhiteSpace && Helper.Instance.IsDscResourceModule(filePath))
else if (!filePathIsNullOrWhiteSpace && Helper.Instance.IsDscResourceModule(filePath))
{
// Run all DSC Rules
foreach (IDSCResourceRule dscResourceRule in this.DSCResourceRules)
Expand All @@ -2248,7 +2258,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
try
{
var ruleRecords = dscResourceRule.AnalyzeDSCResource(scriptAst, filePath).ToList();
var records = SuppressRule(dscResourceRule.GetName(), ruleSuppressions, ruleRecords);
var records = SuppressRule(dscResourceRule.GetName(), ruleSuppressions, ruleRecords, emitSuppressionErrors);
foreach (var record in records.Item2)
{
diagnostics.Add(record);
Expand Down Expand Up @@ -2297,7 +2307,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(

foreach (var ruleRecord in this.GetExternalRecord(scriptAst, scriptTokens, exRules.ToArray(), filePath))
{
var records = SuppressRule(ruleSuppressions, ruleRecord);
var records = SuppressRule(ruleSuppressions, ruleRecord, emitSuppressionErrors);
foreach (var record in records.Item2)
{
diagnostics.Add(record);
Expand Down
24 changes: 24 additions & 0 deletions Tests/Engine/RuleSuppression.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,30 @@ function MyFunc
$suppErr.TargetObject.RuleSuppressionID | Should -BeExactly "banana"
}
}

It "Issues one unapplied suppression error when -Fix reanalyzes a file" {
$scriptPath = Join-Path $TestDrive 'SuppressionFix.ps1'
$script = @(
'function Test-Function1 {'
" [System.Diagnostics.CodeAnalysis.SuppressMessage('PSAvoidUsingWriteHost','NonExistentID123')]"
' param() ; Write-Host ''x'''
'}'
) -join "`n"

[System.IO.File]::WriteAllText($scriptPath, $script + "`n")

$diagnostics = Invoke-ScriptAnalyzer `
-Path $scriptPath `
-Fix `
-ErrorVariable fixErr `
-ErrorAction SilentlyContinue

$diagnostics | Should -HaveCount 1
$diagnostics[0].RuleName | Should -BeExactly 'PSAvoidUsingWriteHost'
$fixErr | Should -HaveCount 1
$fixErr[0].TargetObject.RuleName | Should -BeExactly 'PSAvoidUsingWriteHost'
$fixErr[0].TargetObject.RuleSuppressionID | Should -BeExactly 'NonExistentID123'
}
}

Context "RuleSuppressionID with named arguments" {
Expand Down
30 changes: 30 additions & 0 deletions Tests/Rules/UseDSCResourceFunctions.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,34 @@ Describe "StandardDSCFunctionsInClass" {
$noClassViolations.Count | Should -Be 0
}
}

Context "When a class-based DSC resource is also in DSC resource module layout" {
It "does not duplicate the unapplied suppression error" {
$resourceRoot = Join-Path $TestDrive 'DSCResources'
$resourceDir = Join-Path $resourceRoot 'MyRes'
$resourcePath = Join-Path $resourceDir 'MyRes.psm1'
$schemaPath = Join-Path $resourceDir 'MyRes.schema.mof'

New-Item -ItemType Directory -Path $resourceDir -Force | Out-Null
[System.IO.File]::WriteAllText($resourcePath, @'
[System.Diagnostics.CodeAnalysis.SuppressMessage('PSDSCStandardDSCFunctionsInResource', 'BadDscId', Scope='Class', Target='MyRes')]
[DscResource()]
class MyRes {
[DscProperty(Key)] [string] $Name
[MyRes] Get() { return $this }
}
'@.TrimStart() + "`n")
Set-Content -Path $schemaPath -Value ''

Invoke-ScriptAnalyzer `
-Path $resourcePath `
-ErrorVariable dscErr `
-ErrorAction SilentlyContinue |
Out-Null

$dscErr | Should -HaveCount 1
$dscErr[0].TargetObject.RuleName | Should -BeExactly $violationName
$dscErr[0].TargetObject.RuleSuppressionID | Should -BeExactly 'BadDscId'
}
}
}