diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md index 3372326a2..78a0ab2e4 100644 --- a/RuleDocumentation/README.md +++ b/RuleDocumentation/README.md @@ -43,6 +43,7 @@ |[ProvideCommentHelp](./ProvideCommentHelp.md) | Information | Yes | |[ReservedCmdletChar](./ReservedCmdletChar.md) | Error | | |[ReservedParams](./ReservedParams.md) | Error | | +|[ReviewUnusedParameter](./ReviewUnusedParameter.md) | Warning | | |[ShouldProcess](./ShouldProcess.md) | Error | | |[UseApprovedVerbs](./UseApprovedVerbs.md) | Warning | | |[UseBOMForUnicodeEncodedFile](./UseBOMForUnicodeEncodedFile.md) | Warning | | diff --git a/RuleDocumentation/ReviewUnusedParameter.md b/RuleDocumentation/ReviewUnusedParameter.md new file mode 100644 index 000000000..82af88c62 --- /dev/null +++ b/RuleDocumentation/ReviewUnusedParameter.md @@ -0,0 +1,45 @@ +# ReviewUnusedParameter + +**Severity Level: Warning** + +## Description + +This rule identifies parameters declared in a script, scriptblock, or function scope that have not been used in that scope. + +## How + +Consider removing the unused parameter. + +## Example + +### Wrong + +``` PowerShell +function Test-Parameter +{ + Param ( + $Parameter1, + + # this parameter is never called in the function + $Parameter2 + ) + + Get-Something $Parameter1 +} +``` + +### Correct + +``` PowerShell +function Test-Parameter +{ + Param ( + $Parameter1, + + # now this parameter is being called in the same scope + $Parameter2 + ) + + Get-Something $Parameter1 $Parameter2 +} +``` diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs new file mode 100644 index 000000000..5a06500d8 --- /dev/null +++ b/Rules/ReviewUnusedParameter.cs @@ -0,0 +1,127 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Management.Automation.Language; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// ReviewUnusedParameter: Check that all declared parameters are used in the script body. + /// +#if !CORECLR +[Export(typeof(IScriptRule))] +#endif + public class ReviewUnusedParameter : IScriptRule + { + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException(Strings.NullAstErrorMessage); + } + + IEnumerable scriptBlockAsts = ast.FindAll(oneAst => oneAst is ScriptBlockAst, true); + if (scriptBlockAsts == null) + { + yield break; + } + + foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts) + { + // find all declared parameters + IEnumerable parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false); + + // list all variables + IDictionary variableCount = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) + .Select(variableExpressionAst => ((VariableExpressionAst)variableExpressionAst).VariablePath.UserPath) + .GroupBy(variableName => variableName, StringComparer.OrdinalIgnoreCase) + .ToDictionary(variableName => variableName.Key, variableName => variableName.Count(), StringComparer.OrdinalIgnoreCase); + + // all bets are off if the script uses PSBoundParameters + if (variableCount.ContainsKey("PSBoundParameters")) + { + continue; + } + + foreach (ParameterAst parameterAst in parameterAsts) + { + // there should be at least two usages of the variable since the parameter declaration counts as one + variableCount.TryGetValue(parameterAst.Name.VariablePath.UserPath, out int variableUsageCount); + if (variableUsageCount >= 2) + { + continue; + } + + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterError, parameterAst.Name.VariablePath.UserPath), + parameterAst.Name.Extent, + GetName(), + DiagnosticSeverity.Warning, + fileName, + parameterAst.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.ReviewUnusedParameterName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterDescription); + } + + /// + /// GetSourceType: 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; + } + + /// + /// GetSourceName: Retrieves the module/assembly name the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } +} diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 8fbcbc368..1009fb128 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1851,6 +1851,42 @@ internal static string ReturnCorrectTypesForSetTargetResourceFunctionsDSCError { } } + /// + /// Looks up a localized string similar to ReviewUnusedParameter. + /// + internal static string ReviewUnusedParameterCommonName { + get { + return ResourceManager.GetString("ReviewUnusedParameterCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Ensure all parameters are used within the same script, scriptblock, or function where they are declared.. + /// + internal static string ReviewUnusedParameterDescription { + get { + return ResourceManager.GetString("ReviewUnusedParameterDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The parameter '{0}' has been declared but not used. . + /// + internal static string ReviewUnusedParameterError { + get { + return ResourceManager.GetString("ReviewUnusedParameterError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to ReviewUnusedParameter. + /// + internal static string ReviewUnusedParameterName { + get { + return ResourceManager.GetString("ReviewUnusedParameterName", resourceCulture); + } + } + /// /// Looks up a localized string similar to ScriptDefinition. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index c2bf6c943..0361517b2 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1107,4 +1107,16 @@ Use only 1 whitespace between parameter names or values. + + ReviewUnusedParameter + + + Ensure all parameters are used within the same script, scriptblock, or function where they are declared. + + + The parameter '{0}' has been declared but not used. + + + ReviewUnusedParameter + \ No newline at end of file diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index be42ced25..404998d43 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -59,7 +59,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 62 + $expectedNumRules = 63 if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4)) { # for PSv3 PSAvoidGlobalAliases is not shipped because diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index e2a363da5..fb85260d1 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -43,7 +43,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { It "Using Variable as parameter name produces warning of Severity error" -TestCases $testCases_ReadOnlyVariables { param ($VariableName, $ExpectedSeverity) - [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}" + [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo{Param(`$$VariableName)}" -ExcludeRule PSReviewUnusedParameter $warnings.Count | Should -Be 1 $warnings.Severity | Should -Be $ExpectedSeverity $warnings.RuleName | Should -Be $ruleName @@ -59,7 +59,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { } It "Does not flag parameter attributes" { - [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'function foo{Param([Parameter(Mandatory=$true)]$param1)}' + [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'function foo{Param([Parameter(Mandatory=$true)]$param1)}' -ExcludeRule PSReviewUnusedParameter $warnings.Count | Should -Be 0 } @@ -86,7 +86,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { Set-Variable -Name $VariableName -Value 'foo' continue } - + # Setting the $Error variable has the side effect of the ErrorVariable to contain only the exception message string, therefore exclude this case. # For the library test in WMF 4, assigning a value $PSEdition does not seem to throw an error, therefore this special case is excluded as well. if ($VariableName -ne 'Error' -and ($VariableName -ne 'PSEdition' -and $PSVersionTable.PSVersion.Major -ne 4)) diff --git a/Tests/Rules/AvoidPositionalParameters.tests.ps1 b/Tests/Rules/AvoidPositionalParameters.tests.ps1 index ebc11b144..ded53c954 100644 --- a/Tests/Rules/AvoidPositionalParameters.tests.ps1 +++ b/Tests/Rules/AvoidPositionalParameters.tests.ps1 @@ -45,7 +45,7 @@ Describe "AvoidPositionalParameters" { [Parameter(Position=3)]$C) } Foo "a" "b" "c"} - $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "$sb" + $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "$sb" -ExcludeRule PSReviewUnusedParameter $warnings.Count | Should -Be 1 $warnings.RuleName | Should -BeExactly $violationName } diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 new file mode 100644 index 000000000..f5d6004cc --- /dev/null +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -0,0 +1,71 @@ +Describe "ReviewUnusedParameter" { + BeforeAll { + $RuleName = 'PSReviewUnusedParameter' + $RuleSeverity = "Warning" + } + + Context "When there are violations" { + It "has 1 violation - function with 1 unused parameter" { + $ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) $Param1}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 1 + } + + It "has 2 violations - function with 2 unused parameters" { + $ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) }' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 2 + } + + It "has 1 violation - scriptblock with 1 unused parameter" { + $ScriptDefinition = '{ param ($Param1) }' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 1 + } + + It "doesn't traverse scriptblock scope" { + $ScriptDefinition = '{ param ($Param1) }; { $Param1 }' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 1 + } + + It "violations have correct rule and severity" { + $ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) $Param1}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Severity | Select-Object -Unique | Should -Be $RuleSeverity + $Violations.RuleName | Select-Object -Unique | Should -Be $RuleName + } + } + + Context "When there are no violations" { + It "has no violations - function that uses all parameters" { + $ScriptDefinition = 'function GoodFunc1 { param ($Param1, $Param2) $Param1; $Param2}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violations - function with splatting" { + $ScriptDefinition = 'function GoodFunc1 { param ($Param1) $Splat = @{InputObject = $Param1}}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violations when using PSBoundParameters" { + $ScriptDefinition = 'function Bound { param ($Param1) Get-Foo @PSBoundParameters }' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violations when parameter is called in child scope" -skip { + $ScriptDefinition = 'function foo { param ($Param1) function Child { $Param1 } }' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violations when case of parameter and variable usage do not match" -skip { + $ScriptDefinition = 'function foo { param ($Param1, $param2) $param1; $Param2}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + } +}