PSAvoidUsingPositionalParameters: Do not warn on AZ CLI (#1846)

* PSAvoidUsingPositionalParameters: Do not warn on AZ CLI

* Add CommandAllowList configuration and make it case insensitive

* fix failing test on Linux as az is not a script on linux and make test not depend on AZ CLI

Author: bergmeister

Rules/AvoidPositionalParameters.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Management.Automation.Language;
 using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+using System.Linq;
 #if !CORECLR
 using System.ComponentModel.Composition;
 #endif
@@ -18,12 +19,20 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 #if !CORECLR
 [Export(typeof(IScriptRule))]
 #endif
-    public class AvoidPositionalParameters : IScriptRule
+    public class AvoidPositionalParameters : ConfigurableRule
     {
+        [ConfigurableRuleProperty(defaultValue: new string[] { "az" })]
+        public string[] CommandAllowList { get; set; }
+
+        public AvoidPositionalParameters()
+        {
+            Enable = true; // keep it enabled by default, user can still override this with settings
+        }
+
         /// <summary>
         /// AnalyzeScript: Analyze the ast to check that positional parameters are not used.
         /// </summary>
-        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         {
             if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
 
@@ -57,20 +66,24 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                 {
                     PipelineAst parent = cmdAst.Parent as PipelineAst;
 
+                    string commandName = cmdAst.GetCommandName();
                     if (parent != null && parent.PipelineElements.Count > 1)
                     {
                         // raise if it's the first element in pipeline. otherwise no.
-                        if (parent.PipelineElements[0] == cmdAst)
+                        if (parent.PipelineElements[0] == cmdAst && !CommandAllowList.Contains(commandName, StringComparer.OrdinalIgnoreCase))
                         {
-                            yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, cmdAst.GetCommandName()),
-                                cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, cmdAst.GetCommandName());
+                            yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, commandName),
+                                cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, commandName);
                         }
                     }
                     // not in pipeline so just raise it normally
                     else
                     {
-                        yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, cmdAst.GetCommandName()),
-                            cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, cmdAst.GetCommandName());
+                        if (!CommandAllowList.Contains(commandName, StringComparer.OrdinalIgnoreCase))
+                        {
+                            yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, commandName),
+                                cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, commandName);
+                        }
                     }
                 }
             }
@@ -80,7 +93,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         /// GetName: Retrieves the name of this rule.
         /// </summary>
         /// <returns>The name of this rule</returns>
-        public string GetName()
+        public override string GetName()
         {
             return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidUsingPositionalParametersName);
         }
@@ -89,7 +102,7 @@ public string GetName()
         /// GetCommonName: Retrieves the common name of this rule.
         /// </summary>
         /// <returns>The common name of this rule</returns>
-        public string GetCommonName()
+        public override string GetCommonName()
         {
             return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersCommonName);
         }
@@ -98,15 +111,15 @@ public string GetCommonName()
         /// GetDescription: Retrieves the description of this rule.
         /// </summary>
         /// <returns>The description of this rule</returns>
-        public string GetDescription()
+        public override string GetDescription()
         {
             return string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersDescription);
         }
 
         /// <summary>
         /// Method: Retrieves the type of the rule: builtin, managed or module.
         /// </summary>
-        public SourceType GetSourceType()
+        public override SourceType GetSourceType()
         {
             return SourceType.Builtin;
         }
@@ -115,15 +128,15 @@ public SourceType GetSourceType()
         /// GetSeverity: Retrieves the severity of the rule: error, warning of information.
         /// </summary>
         /// <returns></returns>
-        public RuleSeverity GetSeverity()
+        public override RuleSeverity GetSeverity()
         {
             return RuleSeverity.Information;
         }
 
         /// <summary>
         /// Method: Retrieves the module/assembly name the rule is from.
         /// </summary>
-        public string GetSourceName()
+        public override string GetSourceName()
         {
             return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
         }

Tests/Rules/AvoidPositionalParameters.tests.ps1

@@ -26,6 +26,14 @@ Describe "AvoidPositionalParameters" {
             $violations.RuleName | Should -Contain 'PSAvoidUsingCmdletAliases'
         }
 
+        It "returns violations for command that is not in allow list of settings" {
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Join-Path a b c d' -Settings @{
+                IncludeRules = @('PSAvoidUsingPositionalParameters')
+                Rules        = @{ PSAvoidUsingPositionalParameters = @{ CommandAllowList = 'Test-Path' } }
+            }
+            $violations.Count | Should -Be 1
+            $violations.RuleName | Should -Be 'PSAvoidUsingPositionalParameters'
+        }
     }
 
     Context "When there are no violations" {
@@ -36,6 +44,17 @@ Describe "AvoidPositionalParameters" {
         It "returns no violations for DSC configuration" {
             $noViolationsDSC.Count | Should -Be 0
         }
+
+        It "returns no violations for AZ CLI by default" {
+            Invoke-ScriptAnalyzer -ScriptDefinition 'az group deployment list' | Should -BeNullOrEmpty
+        }
+
+        It "returns no violations for command from allow list defined in settings and is case invariant" {
+            Invoke-ScriptAnalyzer -ScriptDefinition 'join-patH a b c' -Settings @{
+                IncludeRules = @('PSAvoidUsingPositionalParameters')
+                Rules        = @{ PSAvoidUsingPositionalParameters = @{ CommandAllowList = 'az', 'Join-Path' } }
+            } | Should -BeNullOrEmpty
+        }
     }
 
     Context "Function defined and called in script, which has 3 or more positional parameters triggers rule." {

docs/Rules/AvoidUsingPositionalParameters.md

@@ -20,6 +20,27 @@ rule from being too noisy, this rule gets only triggered when there are 3 or mor
 supplied. A simple example where the risk of using positional parameters is negligible, is
 `Test-Path $Path`.
 
+## Configuration
+
+```powershell
+Rules = @{
+    AvoidUsingPositionalParameters = @{
+        CommandAllowList = 'az', 'Join-Path'
+        Enable           = $true
+    }
+}
+```
+
+### Parameters
+
+#### AvoidUsingPositionalParameters: string[] (Default value is 'az')
+
+Commands to be excluded from this rule. `az` is excluded by default because starting with version 2.40.0 the entrypoint of the AZ CLI became an `az.ps1` script but this script does not have any named parameters and just passes them on using `$args` as is to the Python process that it starts, therefore it is still a CLI and not a PowerShell command.
+
+#### Enable: bool (Default value is `$true`)
+
+Enable or disable the rule during ScriptAnalyzer invocation.
+
 ## How
 
 Use full parameter names when calling commands.