diff --git a/Rules/AvoidUninitializedVariable.cs b/Rules/AvoidUninitializedVariable.cs index 2872f0e56..7a0031740 100644 --- a/Rules/AvoidUninitializedVariable.cs +++ b/Rules/AvoidUninitializedVariable.cs @@ -52,12 +52,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) IEnumerable funcAsts = ast.FindAll(item => item is FunctionDefinitionAst, true); IEnumerable funcMemberAsts = ast.FindAll(item => item is FunctionMemberAst, true); - - // Checks whether this is a dsc resource file (we don't raise this rule for get, set and test-target resource - bool isDscResourceFile = Helper.Instance.IsDscResourceModule(fileName); - - List targetResourcesFunctions = new List( new string[] { "get-targetresource", "set-targetresource", "test-targetresource" }); - + foreach (FunctionDefinitionAst funcAst in funcAsts) { // Finds all VariableExpressionAst. @@ -65,13 +60,16 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) HashSet paramVariables = new HashSet(); - if (isDscResourceFile && targetResourcesFunctions.Contains(funcAst.Name, StringComparer.OrdinalIgnoreCase)) + // don't raise the rules for variables in the param block. + if (funcAst.Body != null && funcAst.Body.ParamBlock != 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) - { - paramVariables.UnionWith(funcAst.Body.ParamBlock.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath)); - } + paramVariables.UnionWith(funcAst.Body.ParamBlock.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath)); + } + + //don't raise the rules for parameters outside the param block + if(funcAst.Parameters != null) + { + paramVariables.UnionWith(funcAst.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath)); } // Iterates all VariableExpressionAst and check the command name. diff --git a/Rules/ProvideDefaultParameterValue.cs b/Rules/ProvideDefaultParameterValue.cs new file mode 100644 index 000000000..b10c2a70b --- /dev/null +++ b/Rules/ProvideDefaultParameterValue.cs @@ -0,0 +1,140 @@ +// +// Copyright (c) Microsoft Corporation. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// + +using System; +using System.Linq; +using System.Collections.Generic; +using System.Management.Automation.Language; +using Microsoft.Windows.Powershell.ScriptAnalyzer.Generic; +using System.ComponentModel.Composition; +using System.Globalization; + +namespace Microsoft.Windows.Powershell.ScriptAnalyzer.BuiltinRules +{ + /// + /// ProvideDefaultParameterValue: Check if any uninitialized variable is used. + /// + [Export(typeof(IScriptRule))] + public class ProvideDefaultParameterValue : IScriptRule + { + /// + /// AnalyzeScript: Check if any uninitialized variable is used. + /// + 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); + + // Checks whether this is a dsc resource file (we don't raise this rule for get, set and test-target resource + bool isDscResourceFile = 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)) + { + // don't raise the rules for variables in the param block. + if (funcAst.Body != null && funcAst.Body.ParamBlock != null && funcAst.Body.ParamBlock.Parameters != null) + { + dscVariables.UnionWith(funcAst.Body.ParamBlock.Parameters.Select(paramAst => paramAst.Name.VariablePath.UserPath)); + } + } + // 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)) + { + 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 (funcAst.Parameters != null) + { + foreach (var paramAst in funcAst.Parameters) + { + if (Helper.Instance.IsUninitialized(paramAst.Name, funcAst) && !dscVariables.Contains(paramAst.Name.VariablePath.UserPath)) + { + 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); + } + } + } + } + } + + /// + /// GetName: Retrieves the name of this rule. + /// + /// The name of this rule + public string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ProvideDefaultParameterValueName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.ProvideDefaultParameterValueDescription); + } + + /// + /// Method: Retrieves the type of the rule: builtin, managed or module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + + /// + /// GetSeverity: Retrieves the severity of the rule: error, warning of information. + /// + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// Method: Retrieves the module/assembly name the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } +} diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index 7401ac269..9f553cb06 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -59,6 +59,7 @@ + diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 641440213..3f8180b6c 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ // // This code was generated by a tool. -// Runtime Version:4.0.30319.34014 +// Runtime Version:4.0.30319.34209 // // Changes to this file may cause incorrect behavior and will be lost if // the code is regenerated. @@ -1104,6 +1104,42 @@ 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 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 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 Verbose. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index b26e4d6fa..da0010853 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -714,4 +714,16 @@ No examples found for resource '{0}' + + Default Parameter Values + + + Parameters must have a default value. To fix a violation of this rule, please specify a default value for all parameters + + + 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 + + + ProvideDefaultParameterValue + \ No newline at end of file diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index f5303a2f4..ad4c411b2 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -136,12 +136,12 @@ Describe "Test IncludeRule" { Context "IncludeRule supports wild card" { It "includes 1 wildcard rule"{ $includeWildcard = Invoke-ScriptAnalyzer $directory\..\Rules\BadCmdlet.ps1 -IncludeRule $avoidRules - $includeWildcard.Count | Should be 5 + $includeWildcard.Count | Should be 3 } it "includes 2 wildcardrules" { $includeWildcard = Invoke-ScriptAnalyzer $directory\..\Rules\BadCmdlet.ps1 -IncludeRule $avoidRules, $useRules - $includeWildcard.Count | Should be 9 + $includeWildcard.Count | Should be 7 } } } diff --git a/Tests/Engine/RuleSuppression.ps1 b/Tests/Engine/RuleSuppression.ps1 index 45ae97bd3..44fb3e867 100644 --- a/Tests/Engine/RuleSuppression.ps1 +++ b/Tests/Engine/RuleSuppression.ps1 @@ -6,7 +6,7 @@ Param( function SuppressMe () { [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideVerboseMessage")] - [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUninitializedVariable", "unused1")] + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSProvideDefaultParameterValue", "unused1")] Param([string]$unUsed1, [int] $unUsed2) { Write-Host "I do nothing" diff --git a/Tests/Engine/RuleSuppression.tests.ps1 b/Tests/Engine/RuleSuppression.tests.ps1 index 15a026025..bb6ff4a77 100644 --- a/Tests/Engine/RuleSuppression.tests.ps1 +++ b/Tests/Engine/RuleSuppression.tests.ps1 @@ -33,7 +33,7 @@ Describe "RuleSuppressionWithoutScope" { Context "RuleSuppressionID" { It "Only suppress violations for that ID" { - $suppression = $violations | Where-Object {$_.RuleName -eq "PSAvoidUninitializedVariable" } + $suppression = $violations | Where-Object {$_.RuleName -eq "PSProvideDefaultParameterValue" } $suppression.Count | Should Be 1 } } diff --git a/Tests/Rules/ProvideDefaultParameterValue.ps1 b/Tests/Rules/ProvideDefaultParameterValue.ps1 new file mode 100644 index 000000000..706ba9e6c --- /dev/null +++ b/Tests/Rules/ProvideDefaultParameterValue.ps1 @@ -0,0 +1,20 @@ +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 new file mode 100644 index 000000000..c731d7974 --- /dev/null +++ b/Tests/Rules/ProvideDefaultParameterValue.tests.ps1 @@ -0,0 +1,24 @@ +Import-Module PSScriptAnalyzer +$violationName = "PSProvideDefaultParameterValue" +$violationMessage = "Parameter 'Param2' 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 diff --git a/Tests/Rules/ProvideDefaultParameterValueNoViolations.ps1 b/Tests/Rules/ProvideDefaultParameterValueNoViolations.ps1 new file mode 100644 index 000000000..a42931454 --- /dev/null +++ b/Tests/Rules/ProvideDefaultParameterValueNoViolations.ps1 @@ -0,0 +1,19 @@ +function GoodFunc +{ + param( + [Parameter(Mandatory=$true)] + [ValidateNotNullOrEmpty()] + [string] + $Param1, + [Parameter(Mandatory=$false)] + [ValidateNotNullOrEmpty()] + [string] + $Param2=$null + ) + $Param1 +} + +function GoodFunc2($Param1 = $null) +{ + $Param1 +} \ No newline at end of file