Skip to content

Commit 1abe00e

Browse files
liamjpetersliamjpeters
committed
Updated rule
1 parent ea70855 commit 1abe00e

File tree

2 files changed

+252
-68
lines changed

2 files changed

+252
-68
lines changed

‎Rules/AvoidDefaultValueForMandatoryParameter.cs

Lines changed: 96 additions & 51 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,57 +28,105 @@ 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) // Add null safety
46+
.SelectMany(pb => pb.Parameters)
47+
.Where(paramAst =>
48+
paramAst.DefaultValue != null &&
49+
HasMandatoryInAllParameterAttributes(
50+
paramAst,
51+
StringComparer.OrdinalIgnoreCase
52+
)
53+
);
54+
55+
// Report diagnostics for each parameter that violates the rule
56+
foreach (var parameter in mandatoryParametersWithDefaultValues)
3457
{
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;
58+
yield return new DiagnosticRecord(
59+
string.Format(
60+
CultureInfo.CurrentCulture,
61+
Strings.AvoidDefaultValueForMandatoryParameterError,
62+
parameter.Name.VariablePath.UserPath
63+
),
64+
parameter.Name.Extent,
65+
GetName(),
66+
DiagnosticSeverity.Warning,
67+
fileName,
68+
parameter.Name.VariablePath.UserPath
69+
);
70+
}
71+
}
4172

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-
}
73+
/// <summary>
74+
/// Determines if a parameter is mandatory in all of its Parameter attributes.
75+
/// A parameter may have multiple [Parameter] attributes for different parameter sets.
76+
/// This method returns true only if ALL [Parameter] attributes have Mandatory=true.
77+
/// </summary>
78+
/// <param name="paramAst">The parameter AST to examine</param>
79+
/// <param name="comparer">String comparer for case-insensitive attribute name matching</param>
80+
/// <returns>
81+
/// True if the parameter has at least one [Parameter] attribute and ALL of them
82+
/// have the Mandatory named argument set to true (explicitly or implicitly).
83+
/// False if the parameter has no [Parameter] attributes or if any [Parameter]
84+
/// attribute does not have Mandatory=true.
85+
/// </returns>
86+
private static bool HasMandatoryInAllParameterAttributes(ParameterAst paramAst, StringComparer comparer)
87+
{
88+
var parameterAttributes = paramAst.Attributes.OfType<AttributeAst>()
89+
.Where(attr => IsParameterAttribute(attr.TypeName?.Name, comparer))
90+
.ToList();
6791

68-
if (!mandatory)
69-
{
70-
break;
71-
}
92+
return parameterAttributes.Count > 0 &&
93+
parameterAttributes.All(attr => HasMandatoryArgument(attr, comparer));
94+
}
7295

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-
}
80-
}
96+
/// <summary>
97+
/// Determines if an attribute type name represents a PowerShell Parameter attribute.
98+
/// Checks for both the short form "Parameter" and full form "ParameterAttribute".
99+
/// </summary>
100+
/// <param name="typeName">The attribute type name to check</param>
101+
/// <param name="comparer">String comparer for case-insensitive matching</param>
102+
/// <returns>
103+
/// True if the type name is "Parameter" or "ParameterAttribute" (case-insensitive).
104+
/// False otherwise.
105+
/// </returns>
106+
private static bool IsParameterAttribute(string typeName, StringComparer comparer)
107+
{
108+
return comparer.Equals(typeName, "parameter");
109+
}
110+
111+
/// <summary>
112+
/// Determines if a Parameter attribute has the Mandatory named argument set to true.
113+
/// Handles both explicit (Mandatory=$true) and implicit (Mandatory) cases.
114+
/// Uses the Helper.Instance.GetNamedArgumentAttributeValue method to evaluate
115+
/// the mandatory argument value.
116+
/// </summary>
117+
/// <param name="attr">The Parameter attribute AST to examine</param>
118+
/// <param name="comparer">String comparer for case-insensitive argument name matching</param>
119+
/// <returns>
120+
/// True if the attribute has a "Mandatory" named argument that evaluates to true.
121+
/// False if there is no "Mandatory" argument or if it evaluates to false.
122+
/// </returns>
123+
private static bool HasMandatoryArgument(AttributeAst attr, StringComparer comparer)
124+
{
125+
return attr.NamedArguments?.OfType<NamedAttributeArgumentAst>()
126+
.Any(namedArg =>
127+
comparer.Equals(namedArg?.ArgumentName, "mandatory") &&
128+
Helper.Instance.GetNamedArgumentAttributeValue(namedArg)
129+
) == true;
81130
}
82131

83132
/// <summary>
@@ -133,7 +182,3 @@ public string GetSourceName()
133182
}
134183
}
135184
}
136-
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)