diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 4adf407ad..c86e2eb55 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -602,91 +602,52 @@ public bool HasSplattedVariable(CommandAst cmdAst) } /// - /// Given a commandast, checks whether positional parameters are used or not. + /// Given a commandast, checks if the command is a Cmdlet. /// /// - /// only return true if more than three positional parameters are used /// - 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 switchParams = null; + var commandInfo = GetCommandInfo(cmdAst.GetCommandName()); + return (commandInfo != null && commandInfo.CommandType == System.Management.Automation.CommandTypes.Cmdlet); + } + /// + /// Given a commandast, checks whether positional parameters are used or not. + /// + /// + /// only return true if more than two positional parameters are used + /// + 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(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; } diff --git a/Rules/AvoidPositionalParameters.cs b/Rules/AvoidPositionalParameters.cs index de530ab78..0fa54845d 100644 --- a/Rules/AvoidPositionalParameters.cs +++ b/Rules/AvoidPositionalParameters.cs @@ -27,6 +27,19 @@ public IEnumerable 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 functionDefinitionAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true); + HashSet declaredFunctionNames = new HashSet(); + + foreach (FunctionDefinitionAst functionDefinitionAst in functionDefinitionAsts) + { + if (string.IsNullOrEmpty(functionDefinitionAst.Name)) + { + continue; + } + declaredFunctionNames.Add(functionDefinitionAst.Name); + } + // Finds all CommandAsts. IEnumerable foundAsts = ast.FindAll(testAst => testAst is CommandAst, true); @@ -39,8 +52,8 @@ public IEnumerable 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; diff --git a/Rules/UseCmdletCorrectly.cs b/Rules/UseCmdletCorrectly.cs index fae21cfad..8138e6dec 100644 --- a/Rules/UseCmdletCorrectly.cs +++ b/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; } diff --git a/Tests/Rules/AvoidPositionalParameters.tests.ps1 b/Tests/Rules/AvoidPositionalParameters.tests.ps1 index 8e95ca008..1e0000a54 100644 --- a/Tests/Rules/AvoidPositionalParameters.tests.ps1 +++ b/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 + } + } }