Skip to content

Commit ef6d8a6

Browse files
author
Quoc Truong
committed
Change Provide Default Parameter Value to Avoid Default Value For Mandatory Parameter
1 parent b5c991b commit ef6d8a6

9 files changed

+153
-125
lines changed

Rules/ProvideDefaultParameterValue.cs renamed to Rules/AvoidDefaultValueForMandatoryParameter.cs

Lines changed: 42 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -13,18 +13,19 @@
1313
using System;
1414
using System.Linq;
1515
using System.Collections.Generic;
16+
using System.Management.Automation;
1617
using System.Management.Automation.Language;
17-
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
1818
using System.ComponentModel.Composition;
1919
using System.Globalization;
20+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
2021

2122
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
2223
{
2324
/// <summary>
2425
/// ProvideDefaultParameterValue: Check if any uninitialized variable is used.
2526
/// </summary>
2627
[Export(typeof(IScriptRule))]
27-
public class ProvideDefaultParameterValue : IScriptRule
28+
public class AvoidDefaultValueForMandatoryParameter : IScriptRule
2829
{
2930
/// <summary>
3031
/// AnalyzeScript: Check if any uninitialized variable is used.
@@ -36,48 +37,53 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
3637
// Finds all functionAst
3738
IEnumerable<Ast> functionAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true);
3839

39-
// Checks whether this is a dsc resource file (we don't raise this rule for get, set and test-target resource
40-
bool isDscResourceFile = !String.IsNullOrWhiteSpace(fileName) && Helper.Instance.IsDscResourceModule(fileName);
41-
42-
List<string> targetResourcesFunctions = new List<string>(new string[] { "get-targetresource", "set-targetresource", "test-targetresource" });
43-
44-
4540
foreach (FunctionDefinitionAst funcAst in functionAsts)
4641
{
47-
// Finds all ParamAsts.
48-
IEnumerable<Ast> varAsts = funcAst.FindAll(testAst => testAst is VariableExpressionAst, true);
49-
50-
// Iterrates all ParamAsts and check if their names are on the list.
51-
52-
HashSet<string> dscVariables = new HashSet<string>();
53-
if (isDscResourceFile && targetResourcesFunctions.Contains(funcAst.Name, StringComparer.OrdinalIgnoreCase))
42+
if (funcAst.Body != null && funcAst.Body.ParamBlock != null
43+
&& funcAst.Body.ParamBlock.Attributes != null && funcAst.Body.ParamBlock.Parameters != null)
5444
{
55-
// don't raise the rules for variables in the param block.
56-
if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null)
45+
// only raise this rule for function with cmdletbinding
46+
if (!funcAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute)))
5747
{
58-
dscVariables.UnionWith(funcAst.Body.ParamBlock.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath));
48+
continue;
5949
}
60-
}
61-
// only raise the rules for variables in the param block.
62-
if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null)
63-
{
50+
6451
foreach (var paramAst in funcAst.Body.ParamBlock.Parameters)
6552
{
66-
if (Helper.Instance.IsUninitialized(paramAst.Name, funcAst) && !dscVariables.Contains(paramAst.Name.VariablePath.UserPath))
53+
bool mandatory = false;
54+
55+
// check that param is mandatory
56+
foreach (var paramAstAttribute in paramAst.Attributes)
6757
{
68-
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueError, paramAst.Name.VariablePath.UserPath),
69-
paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName, paramAst.Name.VariablePath.UserPath);
58+
if (paramAstAttribute is AttributeAst)
59+
{
60+
var namedArguments = (paramAstAttribute as AttributeAst).NamedArguments;
61+
if (namedArguments != null)
62+
{
63+
foreach (NamedAttributeArgumentAst namedArgument in namedArguments)
64+
{
65+
if (String.Equals(namedArgument.ArgumentName, "mandatory", StringComparison.OrdinalIgnoreCase))
66+
{
67+
// 2 cases: [Parameter(Mandatory)] and [Parameter(Mandatory=$true)]
68+
if (namedArgument.ExpressionOmitted || (!namedArgument.ExpressionOmitted && String.Equals(namedArgument.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase)))
69+
{
70+
mandatory = true;
71+
break;
72+
}
73+
}
74+
}
75+
}
76+
}
7077
}
71-
}
72-
}
7378

74-
if (funcAst.Parameters != null)
75-
{
76-
foreach (var paramAst in funcAst.Parameters)
77-
{
78-
if (Helper.Instance.IsUninitialized(paramAst.Name, funcAst) && !dscVariables.Contains(paramAst.Name.VariablePath.UserPath))
79+
if (!mandatory)
80+
{
81+
break;
82+
}
83+
84+
if (paramAst.DefaultValue != null)
7985
{
80-
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueError, paramAst.Name.VariablePath.UserPath),
86+
yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterError, paramAst.Name.VariablePath.UserPath),
8187
paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName, paramAst.Name.VariablePath.UserPath);
8288
}
8389
}
@@ -91,7 +97,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
9197
/// <returns>The name of this rule</returns>
9298
public string GetName()
9399
{
94-
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ProvideDefaultParameterValueName);
100+
return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidDefaultValueForMandatoryParameterName);
95101
}
96102

97103
/// <summary>
@@ -100,7 +106,7 @@ public string GetName()
100106
/// <returns>The common name of this rule</returns>
101107
public string GetCommonName()
102108
{
103-
return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueCommonName);
109+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterCommonName);
104110
}
105111

106112
/// <summary>
@@ -109,7 +115,7 @@ public string GetCommonName()
109115
/// <returns>The description of this rule</returns>
110116
public string GetDescription()
111117
{
112-
return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueDescription);
118+
return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterDescription);
113119
}
114120

115121
/// <summary>

‎Rules/ScriptAnalyzerBuiltinRules.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@
5959
<Compile Include="AvoidReservedParams.cs" />
6060
<Compile Include="AvoidShouldContinueWithoutForce.cs" />
6161
<Compile Include="AvoidUsingDeprecatedManifestFields.cs" />
62-
<Compile Include="ProvideDefaultParameterValue.cs" />
62+
<Compile Include="AvoidDefaultValueForMandatoryParameter.cs" />
6363
<Compile Include="AvoidUsernameAndPasswordParams.cs" />
6464
<Compile Include="AvoidUsingComputerNameHardcoded.cs" />
6565
<Compile Include="AvoidUsingConvertToSecureStringWithPlainText.cs" />

‎Rules/Strings.Designer.cs‎

Lines changed: 36 additions & 36 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Rules/Strings.resx‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -711,17 +711,17 @@
711711
<data name="DscExamplesPresentNoExamplesError" xml:space="preserve">
712712
<value>No examples found for resource '{0}'</value>
713713
</data>
714-
<data name="ProvideDefaultParameterValueCommonName" xml:space="preserve">
715-
<value>Default Parameter Values</value>
714+
<data name="AvoidDefaultValueForMandatoryParameterCommonName" xml:space="preserve">
715+
<value>Avoid Default Value For Mandatory Parameter</value>
716716
</data>
717-
<data name="ProvideDefaultParameterValueDescription" xml:space="preserve">
718-
<value>Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters</value>
717+
<data name="AvoidDefaultValueForMandatoryParameterDescription" xml:space="preserve">
718+
<value>Mandatory parameter should not be initialized with a default value in the param block because this value will be ignored.. To fix a violation of this rule, please avoid initializing a value for the mandatory parameter in the param block.</value>
719719
</data>
720-
<data name="ProvideDefaultParameterValueError" xml:space="preserve">
721-
<value>Parameter '{0}' is not initialized. Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters</value>
720+
<data name="AvoidDefaultValueForMandatoryParameterError" xml:space="preserve">
721+
<value>Mandatory Parameter '{0}' is initialized in the Param block. To fix a violation of this rule, please leave it unintialized.</value>
722722
</data>
723-
<data name="ProvideDefaultParameterValueName" xml:space="preserve">
724-
<value>ProvideDefaultParameterValue</value>
723+
<data name="AvoidDefaultValueForMandatoryParameterName" xml:space="preserve">
724+
<value>AvoidDefaultValueForMandatoryParameter</value>
725725
</data>
726726
<data name="AvoidUsingDeprecatedManifestFieldsCommonName" xml:space="preserve">
727727
<value>Avoid Using Deprecated Manifest Fields</value>

‎Tests/Rules/AvoidDefaultValueForMandatoryParameter.ps1‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
function BadFunc
2+
{
3+
[CmdletBinding()]
4+
param(
5+
# this one has default value
6+
[Parameter(Mandatory=$true)]
7+
[ValidateNotNullOrEmpty()]
8+
[string]
9+
$Param1="String",
10+
# this parameter has no default value
11+
[Parameter(Mandatory=$false)]
12+
[ValidateNotNullOrEmpty()]
13+
[string]
14+
$Param2
15+
)
16+
$Param1
17+
$Param1 = "test"
18+
}
19+
20+
function GoodFunc1($Param1)
21+
{
22+
$Param1
23+
}
24+
25+
# same as BadFunc but this one has no cmdletbinding
26+
function GoodFunc2
27+
{
28+
param(
29+
# this one has default value
30+
[Parameter(Mandatory=$true)]
31+
[ValidateNotNullOrEmpty()]
32+
[string]
33+
$Param1="String",
34+
# this parameter has no default value
35+
[Parameter(Mandatory=$false)]
36+
[ValidateNotNullOrEmpty()]
37+
[string]
38+
$Param2
39+
)
40+
$Param1
41+
$Param1 = "test"
42+
}

‎Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
Import-Module PSScriptAnalyzer
2+
$violationName = "PSAvoidDefaultValueForMandatoryParameter"
3+
$violationMessage = "Mandatory Parameter 'Param1' is initialized in the Param block. To fix a violation of this rule, please leave it unintialized."
4+
$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
5+
$violations = Invoke-ScriptAnalyzer "$directory\AvoidDefaultValueForMandatoryParameter.ps1" | Where-Object {$_.RuleName -match $violationName}
6+
$noViolations = Invoke-ScriptAnalyzer "$directory\AvoidDefaultValueForMandatoryParameterNoViolations.ps1"
7+
8+
Describe "AvoidDefaultValueForMandatoryParameter" {
9+
Context "When there are violations" {
10+
It "has 1 provide default value for mandatory parameter violation" {
11+
$violations.Count | Should Be 1
12+
}
13+
14+
It "has the correct description message" {
15+
$violations[0].Message | Should Match $violationMessage
16+
}
17+
}
18+
19+
Context "When there are no violations" {
20+
It "returns no violations" {
21+
$noViolations.Count | Should Be 0
22+
}
23+
}
24+
}

Tests/Rules/ProvideDefaultParameterValueNoViolations.ps1 renamed to Tests/Rules/AvoidDefaultValueForMandatoryParameterNoViolations.ps1

File renamed without changes.

‎Tests/Rules/ProvideDefaultParameterValue.ps1‎

Lines changed: 0 additions & 20 deletions
This file was deleted.

‎Tests/Rules/ProvideDefaultParameterValue.tests.ps1‎

Lines changed: 0 additions & 24 deletions
This file was deleted.

0 commit comments

Comments
 (0)