diff --git a/Rules/ProvideDefaultParameterValue.cs b/Rules/AvoidDefaultValueForMandatoryParameter.cs similarity index 59% rename from Rules/ProvideDefaultParameterValue.cs rename to Rules/AvoidDefaultValueForMandatoryParameter.cs index c67c14133..5db8bfe6f 100644 --- a/Rules/ProvideDefaultParameterValue.cs +++ b/Rules/AvoidDefaultValueForMandatoryParameter.cs @@ -13,10 +13,11 @@ using System; using System.Linq; using System.Collections.Generic; +using System.Management.Automation; using System.Management.Automation.Language; -using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; using System.ComponentModel.Composition; using System.Globalization; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -24,7 +25,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules /// ProvideDefaultParameterValue: Check if any uninitialized variable is used. /// [Export(typeof(IScriptRule))] - public class ProvideDefaultParameterValue : IScriptRule + public class AvoidDefaultValueForMandatoryParameter : IScriptRule { /// /// AnalyzeScript: Check if any uninitialized variable is used. @@ -36,48 +37,53 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) // Finds all functionAst IEnumerable functionAsts = ast.FindAll(testAst => testAst is FunctionDefinitionAst, true); - // Checks whether this is a dsc resource file (we don't raise this rule for get, set and test-target resource - bool isDscResourceFile = !String.IsNullOrWhiteSpace(fileName) && Helper.Instance.IsDscResourceModule(fileName); - - List targetResourcesFunctions = new List(new string[] { "get-targetresource", "set-targetresource", "test-targetresource" }); - - foreach (FunctionDefinitionAst funcAst in functionAsts) { - // Finds all ParamAsts. - IEnumerable varAsts = funcAst.FindAll(testAst => testAst is VariableExpressionAst, true); - - // Iterrates all ParamAsts and check if their names are on the list. - - HashSet dscVariables = new HashSet(); - if (isDscResourceFile && targetResourcesFunctions.Contains(funcAst.Name, StringComparer.OrdinalIgnoreCase)) + if (funcAst.Body != null && funcAst.Body.ParamBlock != null + && funcAst.Body.ParamBlock.Attributes != null && funcAst.Body.ParamBlock.Parameters != null) { - // don't raise the rules for variables in the param block. - if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null) + // only raise this rule for function with cmdletbinding + if (!funcAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute))) { - dscVariables.UnionWith(funcAst.Body.ParamBlock.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath)); + continue; } - } - // only raise the rules for variables in the param block. - if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null) - { + foreach (var paramAst in funcAst.Body.ParamBlock.Parameters) { - if (Helper.Instance.IsUninitialized(paramAst.Name, funcAst) && !dscVariables.Contains(paramAst.Name.VariablePath.UserPath)) + bool mandatory = false; + + // check that param is mandatory + foreach (var paramAstAttribute in paramAst.Attributes) { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueError, paramAst.Name.VariablePath.UserPath), - paramAst.Name.Extent, GetName(), DiagnosticSeverity.Warning, fileName, paramAst.Name.VariablePath.UserPath); + 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)) + { + // 2 cases: [Parameter(Mandatory)] and [Parameter(Mandatory=$true)] + if (namedArgument.ExpressionOmitted || (!namedArgument.ExpressionOmitted && String.Equals(namedArgument.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase))) + { + mandatory = true; + break; + } + } + } + } + } } - } - } - if (funcAst.Parameters != null) - { - foreach (var paramAst in funcAst.Parameters) - { - if (Helper.Instance.IsUninitialized(paramAst.Name, funcAst) && !dscVariables.Contains(paramAst.Name.VariablePath.UserPath)) + if (!mandatory) + { + break; + } + + if (paramAst.DefaultValue != null) { - yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueError, paramAst.Name.VariablePath.UserPath), + 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); } } @@ -91,7 +97,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// The name of this rule public string GetName() { - return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ProvideDefaultParameterValueName); + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.AvoidDefaultValueForMandatoryParameterName); } /// @@ -100,7 +106,7 @@ public string GetName() /// The common name of this rule public string GetCommonName() { - return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueCommonName); + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterCommonName); } /// @@ -109,7 +115,7 @@ public string GetCommonName() /// The description of this rule public string GetDescription() { - return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueDescription); + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidDefaultValueForMandatoryParameterDescription); } /// diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index 6f8058d4b..48154170e 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -59,7 +59,7 @@ - + diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 9128ebedd..448fb0efe 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -96,6 +96,42 @@ internal static string AvoidComputerNameHardcodedName { } } + /// + /// Looks up a localized string similar to Avoid Default Value For Mandatory Parameter. + /// + internal static string AvoidDefaultValueForMandatoryParameterCommonName { + get { + return ResourceManager.GetString("AvoidDefaultValueForMandatoryParameterCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to 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.. + /// + internal static string AvoidDefaultValueForMandatoryParameterDescription { + get { + return ResourceManager.GetString("AvoidDefaultValueForMandatoryParameterDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Mandatory Parameter '{0}' is initialized in the Param block. To fix a violation of this rule, please leave it unintialized.. + /// + internal static string AvoidDefaultValueForMandatoryParameterError { + get { + return ResourceManager.GetString("AvoidDefaultValueForMandatoryParameterError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidDefaultValueForMandatoryParameter. + /// + internal static string AvoidDefaultValueForMandatoryParameterName { + get { + return ResourceManager.GetString("AvoidDefaultValueForMandatoryParameterName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Switch Parameters Should Not Default To True. /// @@ -1176,42 +1212,6 @@ internal static string ProvideCommentHelpName { } } - /// - /// Looks up a localized string similar to Default Parameter Values. - /// - internal static string ProvideDefaultParameterValueCommonName { - get { - return ResourceManager.GetString("ProvideDefaultParameterValueCommonName", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters. - /// - internal static string ProvideDefaultParameterValueDescription { - get { - return ResourceManager.GetString("ProvideDefaultParameterValueDescription", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to 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. - /// - internal static string ProvideDefaultParameterValueError { - get { - return ResourceManager.GetString("ProvideDefaultParameterValueError", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to ProvideDefaultParameterValue. - /// - internal static string ProvideDefaultParameterValueName { - get { - return ResourceManager.GetString("ProvideDefaultParameterValueName", resourceCulture); - } - } - /// /// Looks up a localized string similar to Reserved Cmdlet Chars. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 2679b9b34..f297f16b0 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -711,17 +711,17 @@ No examples found for resource '{0}' - - Default Parameter Values + + Avoid Default Value For Mandatory Parameter - - Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters + + 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. - - 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 + + Mandatory Parameter '{0}' is initialized in the Param block. To fix a violation of this rule, please leave it unintialized. - - ProvideDefaultParameterValue + + AvoidDefaultValueForMandatoryParameter Avoid Using Deprecated Manifest Fields diff --git a/Tests/Engine/RuleSuppression.ps1 b/Tests/Engine/RuleSuppression.ps1 index b63aef84c..02651b0ee 100644 --- a/Tests/Engine/RuleSuppression.ps1 +++ b/Tests/Engine/RuleSuppression.ps1 @@ -6,8 +6,17 @@ Param( function SuppressMe () { [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideCommentHelp")] - [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "unused1")] - Param([string]$unUsed1, [int] $unUsed2) + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "unused1")] + [CmdletBinding()] + Param( + [Parameter(Mandatory=$true)] + [string] + $unUsed1="unused", + + [Parameter(Mandatory=$true)] + [int] + $unUsed2=3 + ) { Write-Host "I do nothing" } @@ -16,9 +25,18 @@ function SuppressMe () function SuppressTwoVariables() { - [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "b")] - [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "a")] - Param([string]$a, [int]$b) + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "b")] + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidDefaultValueForMandatoryParameter", "a")] + [CmdletBinding()] + Param( + [Parameter(Mandatory=$true)] + [string] + $a="unused", + + [Parameter(Mandatory=$true)] + [int] + $b=3 + ) { } } diff --git a/Tests/Engine/RuleSuppression.tests.ps1 b/Tests/Engine/RuleSuppression.tests.ps1 index 35a4f15c7..555b770c5 100644 --- a/Tests/Engine/RuleSuppression.tests.ps1 +++ b/Tests/Engine/RuleSuppression.tests.ps1 @@ -49,9 +49,9 @@ Describe "RuleSuppressionWithoutScope" { Context "RuleSuppressionID" { It "Only suppress violations for that ID" { - $suppression = $violations | Where-Object {$_.RuleName -eq "PSProvideDefaultParameterValue" } + $suppression = $violations | Where-Object {$_.RuleName -eq "PSAvoidDefaultValueForMandatoryParameter" } $suppression.Count | Should Be 1 - $suppression = $violationsUsingScriptDefinition | Where-Object {$_.RuleName -eq "PSProvideDefaultParameterValue" } + $suppression = $violationsUsingScriptDefinition | Where-Object {$_.RuleName -eq "PSAvoidDefaultValueForMandatoryParameter" } $suppression.Count | Should Be 1 } } diff --git a/Tests/Rules/AvoidDefaultValueForMandatoryParameter.ps1 b/Tests/Rules/AvoidDefaultValueForMandatoryParameter.ps1 new file mode 100644 index 000000000..d1cccd86b --- /dev/null +++ b/Tests/Rules/AvoidDefaultValueForMandatoryParameter.ps1 @@ -0,0 +1,42 @@ +function BadFunc +{ + [CmdletBinding()] + param( + # this one has default value + [Parameter(Mandatory=$true)] + [ValidateNotNullOrEmpty()] + [string] + $Param1="String", + # this parameter has no default value + [Parameter(Mandatory=$false)] + [ValidateNotNullOrEmpty()] + [string] + $Param2 + ) + $Param1 + $Param1 = "test" +} + +function GoodFunc1($Param1) +{ + $Param1 +} + +# same as BadFunc but this one has no cmdletbinding +function GoodFunc2 +{ + param( + # this one has default value + [Parameter(Mandatory=$true)] + [ValidateNotNullOrEmpty()] + [string] + $Param1="String", + # this parameter has no default value + [Parameter(Mandatory=$false)] + [ValidateNotNullOrEmpty()] + [string] + $Param2 + ) + $Param1 + $Param1 = "test" +} \ No newline at end of file diff --git a/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1 b/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1 new file mode 100644 index 000000000..f29a98699 --- /dev/null +++ b/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1 @@ -0,0 +1,24 @@ +Import-Module PSScriptAnalyzer +$violationName = "PSAvoidDefaultValueForMandatoryParameter" +$violationMessage = "Mandatory Parameter 'Param1' is initialized in the Param block. To fix a violation of this rule, please leave it unintialized." +$directory = Split-Path -Parent $MyInvocation.MyCommand.Path +$violations = Invoke-ScriptAnalyzer "$directory\AvoidDefaultValueForMandatoryParameter.ps1" | Where-Object {$_.RuleName -match $violationName} +$noViolations = Invoke-ScriptAnalyzer "$directory\AvoidDefaultValueForMandatoryParameterNoViolations.ps1" + +Describe "AvoidDefaultValueForMandatoryParameter" { + Context "When there are violations" { + It "has 1 provide default value for mandatory parameter violation" { + $violations.Count | Should Be 1 + } + + It "has the correct description message" { + $violations[0].Message | Should Match $violationMessage + } + } + + Context "When there are no violations" { + It "returns no violations" { + $noViolations.Count | Should Be 0 + } + } +} \ No newline at end of file diff --git a/Tests/Rules/ProvideDefaultParameterValueNoViolations.ps1 b/Tests/Rules/AvoidDefaultValueForMandatoryParameterNoViolations.ps1 similarity index 100% rename from Tests/Rules/ProvideDefaultParameterValueNoViolations.ps1 rename to Tests/Rules/AvoidDefaultValueForMandatoryParameterNoViolations.ps1 diff --git a/Tests/Rules/ProvideDefaultParameterValue.ps1 b/Tests/Rules/ProvideDefaultParameterValue.ps1 deleted file mode 100644 index 706ba9e6c..000000000 --- a/Tests/Rules/ProvideDefaultParameterValue.ps1 +++ /dev/null @@ -1,20 +0,0 @@ -function BadFunc -{ - param( - [Parameter(Mandatory=$true)] - [ValidateNotNullOrEmpty()] - [string] - $Param1, - [Parameter(Mandatory=$false)] - [ValidateNotNullOrEmpty()] - [string] - $Param2 - ) - $Param1 - $Param1 = "test" -} - -function BadFunc2($Param1) -{ - $Param1 -} \ No newline at end of file diff --git a/Tests/Rules/ProvideDefaultParameterValue.tests.ps1 b/Tests/Rules/ProvideDefaultParameterValue.tests.ps1 deleted file mode 100644 index 5289fdc72..000000000 --- a/Tests/Rules/ProvideDefaultParameterValue.tests.ps1 +++ /dev/null @@ -1,24 +0,0 @@ -Import-Module PSScriptAnalyzer -$violationName = "PSProvideDefaultParameterValue" -$violationMessage = "Parameter 'Param1' is not initialized. Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters" -$directory = Split-Path -Parent $MyInvocation.MyCommand.Path -$violations = Invoke-ScriptAnalyzer $directory\ProvideDefaultParameterValue.ps1 | Where-Object {$_.RuleName -match $violationName} -$noViolations = Invoke-ScriptAnalyzer $directory\ProvideDefaultParameterValueNoViolations.ps1 - -Describe "ProvideDefaultParameters" { - Context "When there are violations" { - It "has 2 provide default parameter value violation" { - $violations.Count | Should Be 2 - } - - It "has the correct description message" { - $violations[0].Message | Should Match $violationMessage - } - } - - Context "When there are no violations" { - It "returns no violations" { - $noViolations.Count | Should Be 0 - } - } -} \ No newline at end of file