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

Engine/ScriptAnalyzer.cs

@@ -1447,10 +1447,24 @@ public IEnumerable<DiagnosticRecord> AnalyzeAndFixPath(string path, Func<string,
         /// </summary>
         /// <param name="scriptDefinition">The script to be analyzed.</param>
         /// <param name="scriptAst">Parsed AST of <paramref name="scriptDefinition"/>.</param>
-        /// <param name="scriptTokens">Parsed tokens of <paramref name="scriptDefinition"/.></param>
+        /// <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)
+        {
+            return AnalyzeScriptDefinition(scriptDefinition, out scriptAst, out scriptTokens, skipVariableAnalysis, emitSuppressionErrors: true);
+        }
+
+        /// <summary>
+        /// Analyzes a script definition in the form of a string input.
+        /// </summary>
+        /// <param name="scriptDefinition">The script to be analysed.</param>
+        /// <param name="scriptAst">Parsed AST of <paramref name="scriptDefinition"/>.</param>
+        /// <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>
+        /// <param name="emitSuppressionErrors">Whether to emit errors for unapplied rule suppression IDs.</param>
+        /// <returns>A list of diagnostics found by rules.</returns>
+        public List<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, out ScriptBlockAst scriptAst, out Token[] scriptTokens, bool skipVariableAnalysis, bool emitSuppressionErrors)
         {
             scriptAst = null;
             scriptTokens = null;
@@ -1490,7 +1504,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 +1563,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 +2000,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 +2032,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>
@@ -2038,6 +2058,27 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
             Token[] scriptTokens,
             string filePath,
             bool skipVariableAnalysis = false)
+        {
+            return AnalyzeSyntaxTree(scriptAst, scriptTokens, filePath, skipVariableAnalysis, emitSuppressionErrors: true);
+        }
+
+        /// <summary>
+        /// Analyzes the syntax tree of a script file that has already been parsed.
+        /// </summary>
+        /// <param name="scriptAst">The ScriptBlockAst from the parsed script.</param>
+        /// <param name="scriptTokens">The tokens found in the script.</param>
+        /// <param name="filePath">The path to the file that was parsed.
+        /// If AnalyzeSyntaxTree is called from an AST obtained via ParseInput, this field will be String.Empty.
+        /// </param>
+        /// <param name="skipVariableAnalysis">Whether to skip variable analysis.</param>
+        /// <param name="emitSuppressionErrors">Whether to emit errors for unapplied rule suppression IDs.</param>
+        /// <returns>An enumeration of DiagnosticRecords found by rules.</returns>
+        public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
+            ScriptBlockAst scriptAst,
+            Token[] scriptTokens,
+            string filePath,
+            bool skipVariableAnalysis,
+            bool emitSuppressionErrors)
         {
             Dictionary<string, List<RuleSuppression>> ruleSuppressions = new Dictionary<string,List<RuleSuppression>>();
             ConcurrentBag<DiagnosticRecord> diagnostics = new ConcurrentBag<DiagnosticRecord>();
@@ -2117,7 +2158,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 +2221,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 +2259,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 +2278,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 +2292,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 +2341,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" -Skip:$testingLibraryUsage {
+            $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'
+        }
+    }
 }