Trigger AvoidPositionalParameters rule for function defined and called inside a script. (#963)

* Trigger AvoidPositionalParameters rule for function defined and called inside a sript given as argument to Script Analyzer.

* Get rid of debug print.

* Tests fpr PSAvoidPOsitionalParameters rule.

* Tests for AvoidPositionalParameters rule fixes.

* Counting positional parameters fix.

* Code cleanup for AvoidPositionalParameter rule fix.

* Trigger AvoidPositionalParameters rule for function defined and called inside a sript given as argument to Script Analyzer.

* Tests fpr PSAvoidPOsitionalParameters rule.

* Counting positional parameters fix.

* Tests fixes for AvoidPositionalParameters rule.

* Code cleanup

Author: kalgiz

Engine/Helper.cs

@@ -602,91 +602,52 @@ public bool HasSplattedVariable(CommandAst cmdAst)
         }
 
         /// <summary>
-        /// Given a commandast, checks whether positional parameters are used or not.
+        /// Given a commandast, checks if the command is a Cmdlet.
         /// </summary>
         /// <param name="cmdAst"></param>
-        /// <param name="moreThanThreePositional">only return true if more than three positional parameters are used</param>
         /// <returns></returns>
-        public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositional = false)
-        {
+        public bool IsCmdlet(CommandAst cmdAst) {
             if (cmdAst == null)
             {
                 return false;
             }
 
-            var commandInfo = GetCommandInfoLegacy(cmdAst.GetCommandName());
-            if (commandInfo == null || (commandInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet))
-            {
-                return false;
-            }
-
-            IEnumerable<ParameterMetadata> switchParams = null;
+            var commandInfo = GetCommandInfo(cmdAst.GetCommandName());
+            return (commandInfo != null && commandInfo.CommandType == System.Management.Automation.CommandTypes.Cmdlet);
+        }
 
+        /// <summary>
+        /// Given a commandast, checks whether positional parameters are used or not.
+        /// </summary>
+        /// <param name="cmdAst"></param>
+        /// <param name="moreThanTwoPositional">only return true if more than two positional parameters are used</param>
+        /// <returns></returns>
+        public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanTwoPositional = false)
+        {
             if (HasSplattedVariable(cmdAst))
             {
                 return false;
             }
 
-            if (commandInfo != null && commandInfo.CommandType == System.Management.Automation.CommandTypes.Cmdlet)
-            {
-                try
-                {
-                    switchParams = commandInfo.Parameters.Values.Where<ParameterMetadata>(pm => pm.SwitchParameter);
-                }
-                catch (Exception)
-                {
-                    switchParams = null;
-                }
-            }
-
-            int parameters = 0;
             // Because of the way we count, we will also count the cmdlet as an argument so we have to -1
-            int arguments = -1;
-
-            foreach (CommandElementAst ceAst in cmdAst.CommandElements)
-            {
-                var cmdParamAst = ceAst as CommandParameterAst;
-                if (cmdParamAst != null)
-                {
-                    // Skip if it's a switch parameter
-                    if (switchParams != null &&
-                        switchParams.Any(
-                            pm => String.Equals(
-                                pm.Name,
-                                cmdParamAst.ParameterName, StringComparison.OrdinalIgnoreCase)))
-                    {
-                        continue;
-                    }
-
-                    parameters += 1;
-
-                    if (cmdParamAst.Argument != null)
-                    {
-                        arguments += 1;
-                    }
+            int argumentsWithoutProcedingParameters = 0;
 
-                }
-                else
+            var commandElementCollection = cmdAst.CommandElements;
+            for (int i = 1; i < commandElementCollection.Count(); i++) {
+                if (!(commandElementCollection[i] is CommandParameterAst) && !(commandElementCollection[i-1] is CommandParameterAst))
                 {
-                    arguments += 1;
+                    argumentsWithoutProcedingParameters++;
                 }
             }
 
             // if not the first element in a pipeline, increase the number of arguments by 1
             PipelineAst parent = cmdAst.Parent as PipelineAst;
-
             if (parent != null && parent.PipelineElements.Count > 1 && parent.PipelineElements[0] != cmdAst)
             {
-                arguments += 1;
-            }
-
-            // if we are only checking for 3 or more positional parameters, check that arguments < parameters + 3
-            if (moreThanThreePositional && (arguments - parameters) < 3)
-            {
-                return false;
+                argumentsWithoutProcedingParameters++;
             }
 
-            return arguments > parameters;
+            return moreThanTwoPositional ? argumentsWithoutProcedingParameters > 2 : argumentsWithoutProcedingParameters > 0;
         }
 
 

Rules/AvoidPositionalParameters.cs

@@ -27,6 +27,19 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         {
             if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
 
+            // Find all function definitions in the script and add them to the set.
+            IEnumerable<Ast> functionDefinitionAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true);
+            HashSet<String> declaredFunctionNames = new HashSet<String>();
+
+            foreach (FunctionDefinitionAst functionDefinitionAst in functionDefinitionAsts)
+            {
+                if (string.IsNullOrEmpty(functionDefinitionAst.Name))
+                {
+                    continue;
+                }
+                declaredFunctionNames.Add(functionDefinitionAst.Name);
+            }
+
             // Finds all CommandAsts.
             IEnumerable<Ast> foundAsts = ast.FindAll(testAst => testAst is CommandAst, true);
 
@@ -39,8 +52,8 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                 // MSDN: CommandAst.GetCommandName Method
                 if (cmdAst.GetCommandName() == null) continue;
 
-                if (Helper.Instance.GetCommandInfoLegacy(cmdAst.GetCommandName()) != null
-                    && Helper.Instance.PositionalParameterUsed(cmdAst, true))
+                if ((Helper.Instance.IsCmdlet(cmdAst) || declaredFunctionNames.Contains(cmdAst.GetCommandName())) &&
+                    (Helper.Instance.PositionalParameterUsed(cmdAst, true)))
                 {
                     PipelineAst parent = cmdAst.Parent as PipelineAst;
 

Rules/UseCmdletCorrectly.cs

@@ -129,7 +129,7 @@ private bool MandatoryParameterExists(CommandAst cmdAst)
                 return true;
             }
 
-            if (mandParams.Count() == 0 || Helper.Instance.PositionalParameterUsed(cmdAst))
+            if (mandParams.Count == 0 || (Helper.Instance.IsCmdlet(cmdAst) && Helper.Instance.PositionalParameterUsed(cmdAst)))
             {
                 returnValue = true;
             }

Tests/Rules/AvoidPositionalParameters.tests.ps1

@@ -21,9 +21,26 @@ Describe "AvoidPositionalParameters" {
         It "returns no violations" {
             $noViolations.Count | Should -Be 0
         }
-        
+
         It "returns no violations for DSC configuration" {
             $noViolationsDSC.Count | Should -Be 0
         }
     }
+
+    Context "Function defined and called in script, which has 3 or more positional parameters triggers rule." {
+        It "returns avoid positional parameters violation" {
+            $sb=
+            {
+                Function Foo {
+                param(
+                    [Parameter(Mandatory=$true,Position=1)] $A,
+                    [Parameter(Position=2)]$B,
+                    [Parameter(Position=3)]$C)
+                }
+                Foo "a" "b" "c"}
+            $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "$sb"
+            $warnings.Count | Should -Be 1
+            $warnings.RuleName | Should -BeExactly $violationName
+        }
+    }
 }