Skip to content

Commit dc55078

Browse files
liamjpetersliamjpeters
authored
PSAvoidDefaultValueForMandatoryParameter: Fix param block and parameter set handling (#2121)
* Updated rule * Remove no-longer relevant comment * Inline unnecessary function calls * Tidy up HasMandatoryInAllParameterAttributes
1 parent 08bb38b commit dc55078

File tree

2 files changed

+220
-69
lines changed

2 files changed

+220
-69
lines changed

‎Rules/AvoidDefaultValueForMandatoryParameter.cs‎

Lines changed: 64 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System;
55
using System.Collections.Generic;
6+
using System.Linq;
67
using System.Management.Automation.Language;
78
#if !CORECLR
89
using System.ComponentModel.Composition;
@@ -27,59 +28,73 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
2728
{
2829
if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
2930

30-
// Finds all functionAst
31-
IEnumerable<Ast> functionAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true);
32-
33-
foreach (FunctionDefinitionAst funcAst in functionAsts)
31+
// Find all ParameterAst which are children of a ParamBlockAst. This
32+
// doesn't pick up where they appear as children of a
33+
// FunctionDefinitionAst. i.e.
34+
//
35+
// function foo ($a,$b){} -> $a and $b are `ParameterAst`
36+
//
37+
// Include only parameters which have a default value (as without
38+
// one this rule would never alert)
39+
// Include only parameters where ALL parameter attributes have the
40+
// mandatory named argument set to true (implicitly or explicitly)
41+
42+
var mandatoryParametersWithDefaultValues =
43+
ast.FindAll(testAst => testAst is ParamBlockAst, true)
44+
.Cast<ParamBlockAst>()
45+
.Where(pb => pb.Parameters?.Count > 0)
46+
.SelectMany(pb => pb.Parameters)
47+
.Where(paramAst =>
48+
paramAst.DefaultValue != null &&
49+
HasMandatoryInAllParameterAttributes(paramAst)
50+
);
51+
52+
// Report diagnostics for each parameter that violates the rule
53+
foreach (var parameter in mandatoryParametersWithDefaultValues)
3454
{
35-
if (funcAst.Body != null && funcAst.Body.ParamBlock != null
36-
&& funcAst.Body.ParamBlock.Attributes != null && funcAst.Body.ParamBlock.Parameters != null)
37-
{
38-
foreach (var paramAst in funcAst.Body.ParamBlock.Parameters)
39-
{
40-
bool mandatory = false;
41-
42-
// check that param is mandatory
43-
foreach (var paramAstAttribute in paramAst.Attributes)
44-
{
45-
if (paramAstAttribute is AttributeAst)
46-
{
47-
var namedArguments = (paramAstAttribute as AttributeAst).NamedArguments;
48-
if (namedArguments != null)
49-
{
50-
foreach (NamedAttributeArgumentAst namedArgument in namedArguments)
51-
{
52-
if (String.Equals(namedArgument.ArgumentName, "mandatory", StringComparison.OrdinalIgnoreCase))
53-
{
54-
// 3 cases: [Parameter(Mandatory)], [Parameter(Mandatory=$true)] and [Parameter(Mandatory=value)] where value is not equal to 0.
55-
if (namedArgument.ExpressionOmitted
56-
|| (String.Equals(namedArgument.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase))
57-
|| (int.TryParse(namedArgument.Argument.Extent.Text, out int mandatoryValue) && mandatoryValue != 0))
58-
{
59-
mandatory = true;
60-
break;
61-
}
62-
}
63-
}
64-
}
65-
}
66-
}
67-
68-
if (!mandatory)
69-
{
70-
break;
71-
}
72-
73-
if (paramAst.DefaultValue != null)
74-
{
75-
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterError, paramAst.Name.VariablePath.UserPath),
76-
paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName, paramAst.Name.VariablePath.UserPath);
77-
}
78-
}
79-
}
55+
yield return new DiagnosticRecord(
56+
string.Format(
57+
CultureInfo.CurrentCulture,
58+
Strings.AvoidDefaultValueForMandatoryParameterError,
59+
parameter.Name.VariablePath.UserPath
60+
),
61+
parameter.Name.Extent,
62+
GetName(),
63+
DiagnosticSeverity.Warning,
64+
fileName,
65+
parameter.Name.VariablePath.UserPath
66+
);
8067
}
8168
}
8269

70+
/// <summary>
71+
/// Determines if a parameter is mandatory in all of its Parameter attributes.
72+
/// A parameter may have multiple [Parameter] attributes for different parameter sets.
73+
/// This method returns true only if ALL [Parameter] attributes have Mandatory=true.
74+
/// </summary>
75+
/// <param name="paramAst">The parameter AST to examine</param>
76+
/// <param name="comparer">String comparer for case-insensitive attribute name matching</param>
77+
/// <returns>
78+
/// True if the parameter has at least one [Parameter] attribute and ALL of them
79+
/// have the Mandatory named argument set to true (explicitly or implicitly).
80+
/// False if the parameter has no [Parameter] attributes or if any [Parameter]
81+
/// attribute does not have Mandatory=true.
82+
/// </returns>
83+
private static bool HasMandatoryInAllParameterAttributes(ParameterAst paramAst)
84+
{
85+
var parameterAttributes = paramAst.Attributes.OfType<AttributeAst>()
86+
.Where(attr => string.Equals(attr.TypeName?.Name, "parameter", StringComparison.OrdinalIgnoreCase));
87+
88+
return parameterAttributes.Any() &&
89+
parameterAttributes.All(attr =>
90+
attr.NamedArguments.OfType<NamedAttributeArgumentAst>()
91+
.Any(namedArg =>
92+
string.Equals(namedArg.ArgumentName, "mandatory", StringComparison.OrdinalIgnoreCase) &&
93+
Helper.Instance.GetNamedArgumentAttributeValue(namedArg)
94+
)
95+
);
96+
}
97+
8398
/// <summary>
8499
/// GetName: Retrieves the name of this rule.
85100
/// </summary>
@@ -134,6 +149,3 @@ public string GetSourceName()
134149
}
135150
}
136151

137-
138-
139-

‎Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1‎

Lines changed: 156 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,37 +6,176 @@ BeforeAll {
66
}
77

88
Describe "AvoidDefaultValueForMandatoryParameter" {
9-
Context "When there are violations" {
10-
It "has 1 provide default value for mandatory parameter violation with CmdletBinding" {
11-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ [CmdletBinding()]Param([Parameter(Mandatory)]$Param1=''defaultValue'') }' |
12-
Where-Object { $_.RuleName -eq $ruleName }
9+
10+
Context "Basic mandatory parameter violations" {
11+
It "should flag mandatory parameter with default value (implicit)" {
12+
$script = 'Function Test { Param([Parameter(Mandatory)]$Param = "default") }'
13+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
14+
$violations.Count | Should -Be 1
15+
}
16+
17+
It "should flag mandatory parameter with default value (explicit true)" {
18+
$script = 'Function Test { Param([Parameter(Mandatory=$true)]$Param = "default") }'
19+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
20+
$violations.Count | Should -Be 1
21+
}
22+
23+
It "should flag mandatory parameter with default value (numeric true)" {
24+
$script = 'Function Test { Param([Parameter(Mandatory=1)]$Param = "default") }'
25+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
26+
$violations.Count | Should -Be 1
27+
}
28+
}
29+
30+
Context "Parameter sets (multiple Parameter attributes)" {
31+
It "should NOT flag parameter mandatory in some but not all parameter sets" {
32+
$script = @'
33+
Function Test {
34+
Param(
35+
[Parameter(Mandatory, ParameterSetName='Set1')]
36+
[Parameter(ParameterSetName='Set2')]
37+
$Param = 'default'
38+
)
39+
}
40+
'@
41+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
42+
$violations.Count | Should -Be 0
43+
}
44+
45+
It "should flag parameter mandatory in ALL parameter sets" {
46+
$script = @'
47+
Function Test {
48+
Param(
49+
[Parameter(Mandatory, ParameterSetName='Set1')]
50+
[Parameter(Mandatory, ParameterSetName='Set2')]
51+
$Param = 'default'
52+
)
53+
}
54+
'@
55+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
1356
$violations.Count | Should -Be 1
1457
}
1558

16-
It "has 1 provide default value for mandatory=$true parameter violation without CmdletBinding" {
17-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$true)]$Param1=''defaultValue'') }' |
18-
Where-Object { $_.RuleName -eq $ruleName }
59+
It "should handle mixed mandatory/non-mandatory in multiple parameter sets" {
60+
$script = @'
61+
Function Test {
62+
Param(
63+
[Parameter(Mandatory=$true, ParameterSetName='Set1')]
64+
[Parameter(Mandatory=$false, ParameterSetName='Set2')]
65+
[Parameter(ParameterSetName='Set3')]
66+
$Param = 'default'
67+
)
68+
}
69+
'@
70+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
71+
$violations.Count | Should -Be 0
72+
}
73+
}
74+
75+
Context "Script-level param blocks" {
76+
It "should flag mandatory parameters with defaults in script-level param blocks" {
77+
$script = @'
78+
Param(
79+
[Parameter(Mandatory)]
80+
$ScriptParam = 'default'
81+
)
82+
'@
83+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
1984
$violations.Count | Should -Be 1
2085
}
2186

22-
It "returns violations when the parameter is specified as mandatory=1 and has a default value" {
23-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=1)]$Param1=''defaultValue'') }' |
24-
Where-Object { $_.RuleName -eq $ruleName }
87+
It "should NOT flag non-mandatory parameters in script-level param blocks" {
88+
$script = @'
89+
Param(
90+
[Parameter()]
91+
$ScriptParam = 'default'
92+
)
93+
'@
94+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
95+
$violations.Count | Should -Be 0
96+
}
97+
}
98+
99+
Context "Non-Parameter attributes" {
100+
It "should NOT flag non-Parameter attributes with Mandatory property" {
101+
$script = 'Function Test { Param([MyCustomAttribute(Mandatory)]$Param = "default") }'
102+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
103+
$violations.Count | Should -Be 0
104+
}
105+
106+
It "should NOT flag parameters with only validation attributes" {
107+
$script = 'Function Test { Param([ValidateNotNull()]$Param = "default") }'
108+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
109+
$violations.Count | Should -Be 0
110+
}
111+
}
112+
113+
Context "Valid scenarios (no violations)" {
114+
It "should NOT flag mandatory parameters without default values" {
115+
$script = 'Function Test { Param([Parameter(Mandatory)]$Param) }'
116+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
117+
$violations.Count | Should -Be 0
118+
}
119+
120+
It "should NOT flag non-mandatory parameters with default values" {
121+
$script = 'Function Test { Param([Parameter(Mandatory=$false)]$Param = "default") }'
122+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
123+
$violations.Count | Should -Be 0
124+
}
125+
126+
It "should NOT flag parameters without Parameter attributes" {
127+
$script = 'Function Test { Param($Param = "default") }'
128+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
129+
$violations.Count | Should -Be 0
130+
}
131+
132+
It "should NOT flag mandatory=0 parameters" {
133+
$script = 'Function Test { Param([Parameter(Mandatory=0)]$Param = "default") }'
134+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
135+
$violations.Count | Should -Be 0
136+
}
137+
}
138+
139+
Context "Complex scenarios" {
140+
It "should handle multiple parameters with mixed violations" {
141+
$script = @'
142+
Function Test {
143+
Param(
144+
[Parameter(Mandatory)]$BadParam = "default",
145+
[Parameter()]$GoodParam = "default",
146+
[Parameter(Mandatory)]$AnotherBadParam = "default"
147+
)
148+
}
149+
'@
150+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
151+
$violations.Count | Should -Be 2
152+
}
153+
154+
It "should work with CmdletBinding" {
155+
$script = 'Function Test { [CmdletBinding()]Param([Parameter(Mandatory)]$Param = "default") }'
156+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
25157
$violations.Count | Should -Be 1
26158
}
27159
}
28160

29-
Context "When there are no violations" {
30-
It "has 1 provide default value for mandatory parameter violation" {
31-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$false)]$Param1=''val1'', [Parameter(Mandatory)]$Param2=''val2'', $Param3=''val3'') }' |
32-
Where-Object { $_.RuleName -eq $ruleName }
161+
Context "Edge cases" {
162+
It "should handle empty param blocks gracefully" {
163+
$script = 'Function Test { Param() }'
164+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
33165
$violations.Count | Should -Be 0
34166
}
35167

36-
It "returns no violations when the parameter is specified as mandatory=0 and has a default value" {
37-
$violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=0)]$Param1=''val1'') }' |
38-
Where-Object { $_.RuleName -eq $ruleName }
168+
It "should handle null/empty default values" {
169+
$script = 'Function Test { Param([Parameter(Mandatory)]$Param = $null) }'
170+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
171+
$violations.Count | Should -Be 1
172+
}
173+
174+
It "should handle parameters with multiple non-Parameter attributes" {
175+
$script = 'Function Test { Param([ValidateNotNull()][Alias("P")]$Param = "default") }'
176+
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $script | Where-Object { $_.RuleName -eq $ruleName }
39177
$violations.Count | Should -Be 0
40178
}
41179
}
180+
42181
}

0 commit comments

Comments
 (0)