Enhance suppression error handling in ScriptAnalyzer and add tests for unapplied suppression errors

Author: liamjpeters

Engine/ScriptAnalyzer.cs

@@ -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;
@@ -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;
         }
 
@@ -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)
@@ -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;
         }
@@ -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>
@@ -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>();
@@ -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);
@@ -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);
@@ -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);
@@ -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)
@@ -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);
@@ -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);

Tests/Engine/RuleSuppression.tests.ps1

@@ -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" {

Tests/Rules/UseDSCResourceFunctions.tests.ps1

@@ -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'
+        }
+    }
 }