Allow for suppression of custom rules (#1145)

* Fix rule name lookup mismatch for custom rules to allow for suppresion and unify code

* Remove note that custom rules suppression are not supported

* fix CustomizedRule.tests.ps1

* do not change GetName method and add GetFullName method instead to avoid breaking existing functionality, therefore also revert the last change in test file

* Add test and fix dummy test rule to return script extent. TODO: Fix suppressing custom rules when there is no Extent supplied

* Make test custom rule return null extent again by fixing the Engine to be able to cope with it

Author: bergmeister

Engine/Generic/ExternalRule.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using System.Globalization;
+
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic
 {
     internal class ExternalRule : IExternalRule
@@ -20,6 +22,11 @@ public string GetName()
             return this.name;
         }
 
+        public string GetFullName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, "{0}\\{1}", this.GetSourceName(), this.name);
+        }
+
         public string GetCommonName()
         {
             return this.commonName;

Engine/Generic/IExternalRule.cs

@@ -8,6 +8,12 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic
     /// </summary>
     internal interface IExternalRule : IRule
     {
+        /// <summary>
+        /// Adds module name (source name) to handle ducplicate function names in different modules.
+        /// </summary>
+        /// <returns></returns>
+        string GetFullName();
+
         /// <summary>
         /// GetParameter: Retrieves AstType parameter
         /// </summary>

Engine/Generic/RuleSuppression.cs

@@ -64,7 +64,7 @@ public string RuleName
                     && (ScriptAnalyzer.Instance.TokenRules != null
                         && ScriptAnalyzer.Instance.TokenRules.Count(item => String.Equals(item.GetName(), _ruleName, StringComparison.OrdinalIgnoreCase)) == 0)
                     && (ScriptAnalyzer.Instance.ExternalRules != null
-                        && ScriptAnalyzer.Instance.ExternalRules.Count(item => String.Equals(item.GetName(), _ruleName, StringComparison.OrdinalIgnoreCase)) == 0)
+                        && ScriptAnalyzer.Instance.ExternalRules.Count(item => String.Equals(item.GetFullName(), _ruleName, StringComparison.OrdinalIgnoreCase)) == 0)
                     && (ScriptAnalyzer.Instance.DSCResourceRules != null
                         && ScriptAnalyzer.Instance.DSCResourceRules.Count(item => String.Equals(item.GetName(), _ruleName, StringComparison.OrdinalIgnoreCase)) == 0))
                 {

Engine/Helper.cs

@@ -1420,7 +1420,7 @@ public Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
                 while (startRecord < diagnostics.Count
                     // && diagnostics[startRecord].Extent.StartOffset < ruleSuppression.StartOffset)
                     // && diagnostics[startRecord].Extent.StartLineNumber < ruleSuppression.st)
-                    && offsetArr[startRecord].Item1 < ruleSuppression.StartOffset)
+                    && offsetArr[startRecord] != null && offsetArr[startRecord].Item1 < ruleSuppression.StartOffset)
                 {
                     startRecord += 1;
                 }
@@ -1434,7 +1434,7 @@ public Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(
                     var curOffset = offsetArr[recordIndex];
 
                     //if (record.Extent.EndOffset > ruleSuppression.EndOffset)
-                    if (curOffset.Item2 > ruleSuppression.EndOffset)
+                    if (curOffset != null && curOffset.Item2 > ruleSuppression.EndOffset)
                     {
                         break;
                     }
@@ -1490,6 +1490,10 @@ private Tuple<int,int>[] GetOffsetArray(List<DiagnosticRecord> diagnostics)
             for (int k = 0; k < diagnostics.Count; k++)
             {
                 var ext = diagnostics[k].Extent;
+                if (ext == null)
+                {
+                    continue;
+                }
                 if (ext.StartOffset == 0 && ext.EndOffset == 0)
                 {
                     // check if line and column number correspond to 0 offsets

Engine/ScriptAnalyzer.cs

@@ -1204,9 +1204,7 @@ internal IEnumerable<DiagnosticRecord> GetExternalRecord(Ast ast, Token[] token,
 
                         // Adds command to run external analyzer rule, like
                         // Measure-CurlyBracket -ScriptBlockAst $ScriptBlockAst
-                        // Adds module name (source name) to handle ducplicate function names in different modules.
-                        string ruleName = string.Format("{0}\\{1}", rule.GetSourceName(), rule.GetName());
-                        posh.Commands.AddCommand(ruleName);
+                        posh.Commands.AddCommand(rule.GetFullName());
                         posh.Commands.AddParameter(rule.GetParameter(), token);
 
                         // Merges results because external analyzer rules may throw exceptions.
@@ -1234,9 +1232,7 @@ internal IEnumerable<DiagnosticRecord> GetExternalRecord(Ast ast, Token[] token,
 
                             // Adds command to run external analyzer rule, like
                             // Measure-CurlyBracket -ScriptBlockAst $ScriptBlockAst
-                            // Adds module name (source name) to handle ducplicate function names in different modules.
-                            string ruleName = string.Format("{0}\\{1}", rule.GetSourceName(), rule.GetName());
-                            posh.Commands.AddCommand(ruleName);
+                            posh.Commands.AddCommand(rule.GetFullName());
                             posh.Commands.AddParameter(rule.GetParameter(), childAst);
 
                             // Merges results because external analyzer rules may throw exceptions.
@@ -2276,7 +2272,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeSyntaxTree(
                 {
                     if (IsRuleAllowed(exRule))
                     {
-                        string ruleName = string.Format(CultureInfo.CurrentCulture, "{0}\\{1}", exRule.GetSourceName(), exRule.GetName());
+                        string ruleName = exRule.GetFullName();
                         this.outputWriter.WriteVerbose(string.Format(CultureInfo.CurrentCulture, Strings.VerboseRunningMessage, ruleName));
 
                         // Ensure that any unhandled errors from Rules are converted to non-terminating errors

README.md

@@ -307,8 +307,6 @@ Suppress violation in `start-bar`, `start-baz` and `start-bam` but not in `start
 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)

Tests/Engine/CustomizedRule.tests.ps1

@@ -160,6 +160,17 @@ Describe "Test importing correct customized rules" {
             $violations[0].SuggestedCorrections.Text   | Should -Be 'text'
             $violations[0].SuggestedCorrections.File   | Should -Be 'filePath'
             $violations[0].SuggestedCorrections.Description   | Should -Be 'description'
+		}
+
+        It "can suppress custom rule" {
+			$script = "[Diagnostics.CodeAnalysis.SuppressMessageAttribute('samplerule\$measure','')]Param()"
+			$testScriptPath = "TestDrive:\SuppressedCustomRule.ps1"
+			Set-Content -Path $testScriptPath -Value $script
+
+            $customizedRulePath = Invoke-ScriptAnalyzer -Path $testScriptPath -CustomizedRulePath $directory\samplerule\samplerule.psm1 |
+				Where-Object { $_.Message -eq $message }
+
+            $customizedRulePath.Count | Should -Be 0
         }
 
         if (!$testingLibraryUsage)

Tests/Engine/samplerule/samplerule.psm1

@@ -27,13 +27,16 @@ function Measure-RequiresRunAsAdministrator
         [ValidateNotNullOrEmpty()]
         [System.Management.Automation.Language.ScriptBlockAst]
         $testAst
-    )        
+    )
+
+    # The implementation is mocked out for testing purposes only and many properties are deliberately set to null to test if PSSA can cope with it
     $l=(new-object System.Collections.ObjectModel.Collection["Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent"])
     $c = (new-object Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.CorrectionExtent 1,2,3,4,'text','filePath','description')
     $l.Add($c)
+    $extent = $null
     $dr = New-Object `
             -Typename "Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticRecord" `
-            -ArgumentList "This is help",$ast.Extent,$PSCmdlet.MyInvocation.InvocationName,Warning,$null,$null,$l     
+            -ArgumentList "This is help",$extent,$PSCmdlet.MyInvocation.InvocationName,Warning,$null,$null,$l
         return $dr
 }
 Export-ModuleMember -Function Measure*