diff --git a/Rules/AvoidDefaultValueForMandatoryParameter.cs b/Rules/AvoidDefaultValueForMandatoryParameter.cs index f3d66d973..bad9e4e28 100644 --- a/Rules/AvoidDefaultValueForMandatoryParameter.cs +++ b/Rules/AvoidDefaultValueForMandatoryParameter.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Management.Automation.Language; #if !CORECLR using System.ComponentModel.Composition; @@ -27,59 +28,107 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage); - // Finds all functionAst - IEnumerable functionAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true); - - foreach (FunctionDefinitionAst funcAst in functionAsts) + // Find all ParameterAst which are children of a ParamBlockAst. This + // doesn't pick up where they appear as children of a + // FunctionDefinitionAst. i.e. + // + // function foo ($a,$b){} -> $a and $b are `ParameterAst` + // + // Include only parameters which have a default value (as without + // one this rule would never alert) + // Include only parameters where ALL parameter attributes have the + // mandatory named argument set to true (implicitly or explicitly) + + var mandatoryParametersWithDefaultValues = + ast.FindAll(testAst => testAst is ParamBlockAst, true) + .Cast() + .Where(pb => pb.Parameters?.Count > 0) + .SelectMany(pb => pb.Parameters) + .Where(paramAst => + paramAst.DefaultValue != null && + HasMandatoryInAllParameterAttributes( + paramAst, + StringComparer.OrdinalIgnoreCase + ) + ); + + // Report diagnostics for each parameter that violates the rule + foreach (var parameter in mandatoryParametersWithDefaultValues) { - if (funcAst.Body != null && funcAst.Body.ParamBlock != null - && funcAst.Body.ParamBlock.Attributes != null && funcAst.Body.ParamBlock.Parameters != null) - { - foreach (var paramAst in funcAst.Body.ParamBlock.Parameters) - { - bool mandatory = false; - - // check that param is mandatory - foreach (var paramAstAttribute in paramAst.Attributes) - { - if (paramAstAttribute is AttributeAst) - { - var namedArguments = (paramAstAttribute as AttributeAst).NamedArguments; - if (namedArguments != null) - { - foreach (NamedAttributeArgumentAst namedArgument in namedArguments) - { - if (String.Equals(namedArgument.ArgumentName, "mandatory", StringComparison.OrdinalIgnoreCase)) - { - // 3 cases: [Parameter(Mandatory)], [Parameter(Mandatory=$true)] and [Parameter(Mandatory=value)] where value is not equal to 0. - if (namedArgument.ExpressionOmitted - || (String.Equals(namedArgument.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase)) - || (int.TryParse(namedArgument.Argument.Extent.Text, out int mandatoryValue) && mandatoryValue != 0)) - { - mandatory = true; - break; - } - } - } - } - } - } - - if (!mandatory) - { - break; - } - - if (paramAst.DefaultValue != null) - { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterError, paramAst.Name.VariablePath.UserPath), - paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName, paramAst.Name.VariablePath.UserPath); - } - } - } + yield return new DiagnosticRecord( + string.Format( + CultureInfo.CurrentCulture, + Strings.AvoidDefaultValueForMandatoryParameterError, + parameter.Name.VariablePath.UserPath + ), + parameter.Name.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + parameter.Name.VariablePath.UserPath + ); } } + /// + /// Determines if a parameter is mandatory in all of its Parameter attributes. + /// A parameter may have multiple [Parameter] attributes for different parameter sets. + /// This method returns true only if ALL [Parameter] attributes have Mandatory=true. + /// + /// The parameter AST to examine + /// String comparer for case-insensitive attribute name matching + /// + /// True if the parameter has at least one [Parameter] attribute and ALL of them + /// have the Mandatory named argument set to true (explicitly or implicitly). + /// False if the parameter has no [Parameter] attributes or if any [Parameter] + /// attribute does not have Mandatory=true. + /// + private static bool HasMandatoryInAllParameterAttributes(ParameterAst paramAst, StringComparer comparer) + { + var parameterAttributes = paramAst.Attributes.OfType() + .Where(attr => IsParameterAttribute(attr.TypeName?.Name, comparer)) + .ToList(); + + return parameterAttributes.Count > 0 && + parameterAttributes.All(attr => HasMandatoryArgument(attr, comparer)); + } + + /// + /// Determines if an attribute type name represents a PowerShell Parameter attribute. + /// Checks for both the short form "Parameter" and full form "ParameterAttribute". + /// + /// The attribute type name to check + /// String comparer for case-insensitive matching + /// + /// True if the type name is "Parameter" or "ParameterAttribute" (case-insensitive). + /// False otherwise. + /// + private static bool IsParameterAttribute(string typeName, StringComparer comparer) + { + return comparer.Equals(typeName, "parameter"); + } + + /// + /// Determines if a Parameter attribute has the Mandatory named argument set to true. + /// Handles both explicit (Mandatory=$true) and implicit (Mandatory) cases. + /// Uses the Helper.Instance.GetNamedArgumentAttributeValue method to evaluate + /// the mandatory argument value. + /// + /// The Parameter attribute AST to examine + /// String comparer for case-insensitive argument name matching + /// + /// True if the attribute has a "Mandatory" named argument that evaluates to true. + /// False if there is no "Mandatory" argument or if it evaluates to false. + /// + private static bool HasMandatoryArgument(AttributeAst attr, StringComparer comparer) + { + return attr.NamedArguments?.OfType() + .Any(namedArg => + comparer.Equals(namedArg?.ArgumentName, "mandatory") && + Helper.Instance.GetNamedArgumentAttributeValue(namedArg) + ) == true; + } + /// /// GetName: Retrieves the name of this rule. /// @@ -134,6 +183,3 @@ public string GetSourceName() } } - - - diff --git a/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1 b/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1 index a087a97f4..32a62f9be 100644 --- a/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1 +++ b/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1 @@ -6,37 +6,176 @@ BeforeAll { } Describe "AvoidDefaultValueForMandatoryParameter" { - Context "When there are violations" { - It "has 1 provide default value for mandatory parameter violation with CmdletBinding" { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ [CmdletBinding()]Param([Parameter(Mandatory)]$Param1=''defaultValue'') }' | - Where-Object { $_.RuleName -eq $ruleName } + + Context "Basic mandatory parameter violations" { + It "should flag mandatory parameter with default value (implicit)" { + $script = 'Function Test { Param([Parameter(Mandatory)]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 1 + } + + It "should flag mandatory parameter with default value (explicit true)" { + $script = 'Function Test { Param([Parameter(Mandatory=$true)]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 1 + } + + It "should flag mandatory parameter with default value (numeric true)" { + $script = 'Function Test { Param([Parameter(Mandatory=1)]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 1 + } + } + + Context "Parameter sets (multiple Parameter attributes)" { + It "should NOT flag parameter mandatory in some but not all parameter sets" { + $script = @' +Function Test { + Param( + [Parameter(Mandatory, ParameterSetName='Set1')] + [Parameter(ParameterSetName='Set2')] + $Param = 'default' + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + + It "should flag parameter mandatory in ALL parameter sets" { + $script = @' +Function Test { + Param( + [Parameter(Mandatory, ParameterSetName='Set1')] + [Parameter(Mandatory, ParameterSetName='Set2')] + $Param = 'default' + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } $violations.Count | Should -Be 1 } - It "has 1 provide default value for mandatory=$true parameter violation without CmdletBinding" { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$true)]$Param1=''defaultValue'') }' | - Where-Object { $_.RuleName -eq $ruleName } + It "should handle mixed mandatory/non-mandatory in multiple parameter sets" { + $script = @' +Function Test { + Param( + [Parameter(Mandatory=$true, ParameterSetName='Set1')] + [Parameter(Mandatory=$false, ParameterSetName='Set2')] + [Parameter(ParameterSetName='Set3')] + $Param = 'default' + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + } + + Context "Script-level param blocks" { + It "should flag mandatory parameters with defaults in script-level param blocks" { + $script = @' +Param( + [Parameter(Mandatory)] + $ScriptParam = 'default' +) +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } $violations.Count | Should -Be 1 } - It "returns violations when the parameter is specified as mandatory=1 and has a default value" { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=1)]$Param1=''defaultValue'') }' | - Where-Object { $_.RuleName -eq $ruleName } + It "should NOT flag non-mandatory parameters in script-level param blocks" { + $script = @' +Param( + [Parameter()] + $ScriptParam = 'default' +) +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + } + + Context "Non-Parameter attributes" { + It "should NOT flag non-Parameter attributes with Mandatory property" { + $script = 'Function Test { Param([MyCustomAttribute(Mandatory)]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + + It "should NOT flag parameters with only validation attributes" { + $script = 'Function Test { Param([ValidateNotNull()]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + } + + Context "Valid scenarios (no violations)" { + It "should NOT flag mandatory parameters without default values" { + $script = 'Function Test { Param([Parameter(Mandatory)]$Param) }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + + It "should NOT flag non-mandatory parameters with default values" { + $script = 'Function Test { Param([Parameter(Mandatory=$false)]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + + It "should NOT flag parameters without Parameter attributes" { + $script = 'Function Test { Param($Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + + It "should NOT flag mandatory=0 parameters" { + $script = 'Function Test { Param([Parameter(Mandatory=0)]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 0 + } + } + + Context "Complex scenarios" { + It "should handle multiple parameters with mixed violations" { + $script = @' +Function Test { + Param( + [Parameter(Mandatory)]$BadParam = "default", + [Parameter()]$GoodParam = "default", + [Parameter(Mandatory)]$AnotherBadParam = "default" + ) +} +'@ + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 2 + } + + It "should work with CmdletBinding" { + $script = 'Function Test { [CmdletBinding()]Param([Parameter(Mandatory)]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } $violations.Count | Should -Be 1 } } - Context "When there are no violations" { - It "has 1 provide default value for mandatory parameter violation" { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$false)]$Param1=''val1'', [Parameter(Mandatory)]$Param2=''val2'', $Param3=''val3'') }' | - Where-Object { $_.RuleName -eq $ruleName } + Context "Edge cases" { + It "should handle empty param blocks gracefully" { + $script = 'Function Test { Param() }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } $violations.Count | Should -Be 0 } - It "returns no violations when the parameter is specified as mandatory=0 and has a default value" { - $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=0)]$Param1=''val1'') }' | - Where-Object { $_.RuleName -eq $ruleName } + It "should handle null/empty default values" { + $script = 'Function Test { Param([Parameter(Mandatory)]$Param = $null) }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } + $violations.Count | Should -Be 1 + } + + It "should handle parameters with multiple non-Parameter attributes" { + $script = 'Function Test { Param([ValidateNotNull()][Alias("P")]$Param = "default") }' + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName } $violations.Count | Should -Be 0 } } + }