From 929ae1415681a5e1cea9fa8869953a78a002f38f Mon Sep 17 00:00:00 2001 From: Matt McNabb Date: Tue, 10 Dec 2019 00:26:08 -0500 Subject: [PATCH 01/12] New rule - ReviewUnusedParameter --- RuleDocumentation/README.md | 1 + RuleDocumentation/ReviewUnusedParameter.md | 45 +++++++ Rules/ReviewUnusedParameter.cs | 130 +++++++++++++++++++ Rules/Strings.Designer.cs | 116 ++++++++++------- Rules/Strings.resx | 12 ++ Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 68 ++++++++++ 7 files changed, 329 insertions(+), 45 deletions(-) create mode 100644 RuleDocumentation/ReviewUnusedParameter.md create mode 100644 Rules/ReviewUnusedParameter.cs create mode 100644 Tests/Rules/ReviewUnusedParameter.tests.ps1 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..b6667a4b8 --- /dev/null +++ b/Rules/ReviewUnusedParameter.cs @@ -0,0 +1,130 @@ +// 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); + } + + var scriptBlockAsts = ast.FindAll(x => x is ScriptBlockAst, true); + if (scriptBlockAsts == null) + { + yield break; + } + + foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts) + { + // find all declared parameters + IEnumerable parameterAsts = scriptBlockAst.FindAll(a => a is ParameterAst, false); + + // list all variables + List variables = scriptBlockAst.FindAll(a => a is VariableExpressionAst, false) + .Cast() + .Select(v => v.VariablePath.ToString()) + .ToList(); + + foreach (ParameterAst parameterAst in parameterAsts) + { + // compare the list of variables to the parameter name + // there should be at least two matches of the variable name since the parameter declaration counts as one + int matchCount = variables + .Where(x => x == parameterAst.Name.VariablePath.ToString()) + .Count(); + if (matchCount > 1) + { + continue; + } + + // all bets are off if the script uses PSBoundParameters + if (variables.Contains("PSBoundParameters")) + { + 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 54cba38eb..0b69507d7 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -492,6 +492,42 @@ internal static string AvoidNullOrEmptyHelpMessageAttributeName { } } + /// + /// Looks up a localized string similar to Avoid overwriting built in cmdlets. + /// + internal static string AvoidOverwritingBuiltInCmdletsCommonName { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Do not overwrite the definition of a cmdlet that is included with PowerShell. + /// + internal static string AvoidOverwritingBuiltInCmdletsDescription { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to '{0}' is a cmdlet that is included with PowerShell (version {1}) whose definition should not be overridden. + /// + internal static string AvoidOverwritingBuiltInCmdletsError { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidOverwritingBuiltInCmdlets. + /// + internal static string AvoidOverwritingBuiltInCmdletsName { + get { + return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsName", resourceCulture); + } + } + /// /// Looks up a localized string similar to Avoid Using ShouldContinue Without Boolean Force Parameter. /// @@ -1815,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. /// @@ -2084,50 +2156,6 @@ internal static string UseCompatibleCmdletsName { return ResourceManager.GetString("UseCompatibleCmdletsName", resourceCulture); } } - - /// - /// Looks up a localized string similar to Avoid overwriting built in cmdlets. - /// - internal static string AvoidOverwritingBuiltInCmdletsCommonName - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsCommonName", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Avoid overwriting built in cmdlets. - /// - internal static string AvoidOverwritingBuiltInCmdletsDescription - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsDescription", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to '{0}' is a cmdlet that is included with PowerShell whose definition should not be overridden. - /// - internal static string AvoidOverwritingBuiltInCmdletsError - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsError", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to AvoidOverwritingBuiltInCmdlets. - /// - internal static string AvoidOverwritingBuiltInCmdletsName - { - get - { - return ResourceManager.GetString("AvoidOverwritingBuiltInCmdletsName", resourceCulture); - } - } /// /// Looks up a localized string similar to The command '{0}' is not available by default in PowerShell version '{1}' on platform '{2}'. diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 50362080f..545871430 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1104,4 +1104,16 @@ UseProcessBlockForPipelineCommand + + 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 + 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/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 new file mode 100644 index 000000000..c261d11a6 --- /dev/null +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -0,0 +1,68 @@ +Describe "ReviewUnusedParameter" { + BeforeAll { + $RuleName = 'PSReviewUnusedParameter' + $RuleSeverity = "Warning" + } + + Context "When there are violations" { + $Cases = @( + @{ + ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) $Param1}' + Name = "function with 1 unused parameter" + NumberOfViolations = 1 + } + @{ + ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) }' + Name = "function with 2 unused parameters" + NumberOfViolations = 2 + } + @{ + ScriptDefinition = '{ param ($Param1) }' + Name = "scriptblock with unused parameter" + NumberOfViolations = 1 + } + @{ + ScriptDefinition = '{ param ($Param1) }; { $Param1 }' + Name = "parameter with same name in different scope" + NumberOfViolations = 1 + } + ) + It "has 1 violation for " -TestCases $Cases { + param ($ScriptDefinition, $Name, $NumberOfViolations) + + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be $NumberOfViolations + $Violations.Severity | Select-Object -Unique | Should -Be $RuleSeverity + $Violations.RuleName | Select-Object -Unique | Should -Be $RuleName + } + } + + Context "When there are no violations" { + $Cases = @( + @{ + ScriptDefinition = 'function GoodFunc1 { param ($Param1, $Param2) $Param1; $Param2}' + Name = "function with 0 unused parameters" + } + @{ + ScriptDefinition = 'function GoodFunc1 { param ($Param1) $Splat = @{InputObject = $Param1}}' + Name = "function with splatted parameter" + } + ) + It "has no violations for function " -TestCases $Cases { + param ($ScriptDefinition, $Name) + + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violations when using PSBoundParameters" { + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition 'function Bound { param ($Param1) Get-Foo @PSBoundParameters }' -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violations when parameter is called in child scope" -Skip { + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition 'function Param { param ($Param1) function Child { $Param1 } }' -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + } +} From d6b38f0c7a81d39a092c55953052f1be69186667 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 3 Jan 2020 21:12:26 +0000 Subject: [PATCH 02/12] Fix test failures in AvoidAssignmentToAutomaticVariable by excluding the new rule as the parameter is not used in those tests --- Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index e2a363da5..e3972d721 100644 --- a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 +++ b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 @@ -52,14 +52,14 @@ Describe "AvoidAssignmentToAutomaticVariables" { It "Using Variable as parameter name in param block produces warning of Severity error" -TestCases $testCases_ReadOnlyVariables { param ($VariableName, $ExpectedSeverity) - [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" + [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" -ExcludeRule PSReviewUnusedParameter $warnings.Count | Should -Be 1 $warnings.Severity | Should -Be $ExpectedSeverity $warnings.RuleName | Should -Be $ruleName } 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)) From aaa4f41e390ea67895f544f6bd0a060da1b78585 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 3 Jan 2020 21:21:14 +0000 Subject: [PATCH 03/12] Fix test failures in other tests when parameters are not used by excluding rule --- Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 | 4 ++-- Tests/Rules/AvoidPositionalParameters.tests.ps1 | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 b/Tests/Rules/AvoidAssignmentToAutomaticVariable.tests.ps1 index e3972d721..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 @@ -52,7 +52,7 @@ Describe "AvoidAssignmentToAutomaticVariables" { It "Using Variable as parameter name in param block produces warning of Severity error" -TestCases $testCases_ReadOnlyVariables { param ($VariableName, $ExpectedSeverity) - [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" -ExcludeRule PSReviewUnusedParameter + [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" $warnings.Count | Should -Be 1 $warnings.Severity | Should -Be $ExpectedSeverity $warnings.RuleName | Should -Be $ruleName 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 } From f360fdca413d3f0741b493591143ba0bf444740a Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 3 Jan 2020 21:50:28 +0000 Subject: [PATCH 04/12] make linq query stop earlier for better performance, avoid redundant ToList() and minor style tweaks (Linq variable naming and trailing whitespace trim) --- Rules/ReviewUnusedParameter.cs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index b6667a4b8..8a592b7c3 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -28,7 +28,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) throw new ArgumentNullException(Strings.NullAstErrorMessage); } - var scriptBlockAsts = ast.FindAll(x => x is ScriptBlockAst, true); + var scriptBlockAsts = ast.FindAll(oneAst => oneAst is ScriptBlockAst, true); if (scriptBlockAsts == null) { yield break; @@ -37,22 +37,21 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts) { // find all declared parameters - IEnumerable parameterAsts = scriptBlockAst.FindAll(a => a is ParameterAst, false); + IEnumerable parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false); // list all variables - List variables = scriptBlockAst.FindAll(a => a is VariableExpressionAst, false) + IEnumerable variables = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) .Cast() - .Select(v => v.VariablePath.ToString()) - .ToList(); + .Select(variableExpressionAst => variableExpressionAst.VariablePath.ToString()); foreach (ParameterAst parameterAst in parameterAsts) { // compare the list of variables to the parameter name // there should be at least two matches of the variable name since the parameter declaration counts as one int matchCount = variables - .Where(x => x == parameterAst.Name.VariablePath.ToString()) - .Count(); - if (matchCount > 1) + .Where(variable => variable == parameterAst.Name.VariablePath.ToString()) + .Take(2).Count(); + if (matchCount == 2) { continue; } @@ -72,7 +71,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) parameterAst.Name.VariablePath.UserPath ); } - } + } } /// From cdbce04fdc5a4758f45d9d2ea367903bc1af7888 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 3 Jan 2020 21:52:30 +0000 Subject: [PATCH 05/12] format test file --- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index c261d11a6..4c15032dd 100644 --- a/Tests/Rules/ReviewUnusedParameter.tests.ps1 +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -17,8 +17,8 @@ Describe "ReviewUnusedParameter" { NumberOfViolations = 2 } @{ - ScriptDefinition = '{ param ($Param1) }' - Name = "scriptblock with unused parameter" + ScriptDefinition = '{ param ($Param1) }' + Name = "scriptblock with unused parameter" NumberOfViolations = 1 } @{ @@ -40,8 +40,8 @@ Describe "ReviewUnusedParameter" { Context "When there are no violations" { $Cases = @( @{ - ScriptDefinition = 'function GoodFunc1 { param ($Param1, $Param2) $Param1; $Param2}' - Name = "function with 0 unused parameters" + ScriptDefinition = 'function GoodFunc1 { param ($Param1, $Param2) $Param1; $Param2}' + Name = "function with 0 unused parameters" } @{ ScriptDefinition = 'function GoodFunc1 { param ($Param1) $Splat = @{InputObject = $Param1}}' From 938a2835ac8d3c8e5470642d923c3b85476b303c Mon Sep 17 00:00:00 2001 From: Matt McNabb Date: Sun, 5 Jan 2020 00:44:57 -0500 Subject: [PATCH 06/12] Style fixes for PR #1382 --- Rules/ReviewUnusedParameter.cs | 16 ++--- Rules/Strings.Designer.cs | 2 +- Rules/Strings.resx | 2 +- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 77 ++++++++++----------- 4 files changed, 47 insertions(+), 50 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 8a592b7c3..eb4fb0c88 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -28,7 +28,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) throw new ArgumentNullException(Strings.NullAstErrorMessage); } - var scriptBlockAsts = ast.FindAll(oneAst => oneAst is ScriptBlockAst, true); + IEnumerable scriptBlockAsts = ast.FindAll(oneAst => oneAst is ScriptBlockAst, true); if (scriptBlockAsts == null) { yield break; @@ -42,7 +42,13 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) // list all variables IEnumerable variables = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) .Cast() - .Select(variableExpressionAst => variableExpressionAst.VariablePath.ToString()); + .Select(variableExpressionAst => variableExpressionAst.VariablePath.UserPath); + + // all bets are off if the script uses PSBoundParameters + if (variables.Contains("PSBoundParameters")) + { + continue; + } foreach (ParameterAst parameterAst in parameterAsts) { @@ -56,12 +62,6 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) continue; } - // all bets are off if the script uses PSBoundParameters - if (variables.Contains("PSBoundParameters")) - { - continue; - } - yield return new DiagnosticRecord( string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterError, parameterAst.Name.VariablePath.UserPath), parameterAst.Name.Extent, diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 0b69507d7..c1da57947 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1870,7 +1870,7 @@ internal static string ReviewUnusedParameterDescription { } /// - /// Looks up a localized string similar to The parameter {0} has been declared but not used. . + /// Looks up a localized string similar to The parameter '{0}' has been declared but not used. . /// internal static string ReviewUnusedParameterError { get { diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 545871430..2f64a1933 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1111,7 +1111,7 @@ 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. + The parameter '{0}' has been declared but not used. ReviewUnusedParameter diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index 4c15032dd..03e23f612 100644 --- a/Tests/Rules/ReviewUnusedParameter.tests.ps1 +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -5,63 +5,60 @@ Describe "ReviewUnusedParameter" { } Context "When there are violations" { - $Cases = @( - @{ - ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) $Param1}' - Name = "function with 1 unused parameter" - NumberOfViolations = 1 - } - @{ - ScriptDefinition = 'function BadFunc1 { param ($Param1, $Param2) }' - Name = "function with 2 unused parameters" - NumberOfViolations = 2 - } - @{ - ScriptDefinition = '{ param ($Param1) }' - Name = "scriptblock with unused parameter" - NumberOfViolations = 1 - } - @{ - ScriptDefinition = '{ param ($Param1) }; { $Param1 }' - Name = "parameter with same name in different scope" - NumberOfViolations = 1 - } - ) - It "has 1 violation for " -TestCases $Cases { - param ($ScriptDefinition, $Name, $NumberOfViolations) + 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.Count | Should -Be $NumberOfViolations $Violations.Severity | Select-Object -Unique | Should -Be $RuleSeverity $Violations.RuleName | Select-Object -Unique | Should -Be $RuleName } } Context "When there are no violations" { - $Cases = @( - @{ - ScriptDefinition = 'function GoodFunc1 { param ($Param1, $Param2) $Param1; $Param2}' - Name = "function with 0 unused parameters" - } - @{ - ScriptDefinition = 'function GoodFunc1 { param ($Param1) $Splat = @{InputObject = $Param1}}' - Name = "function with splatted parameter" - } - ) - It "has no violations for function " -TestCases $Cases { - param ($ScriptDefinition, $Name) + 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" { - $Violations = Invoke-ScriptAnalyzer -ScriptDefinition 'function Bound { param ($Param1) Get-Foo @PSBoundParameters }' -IncludeRule $RuleName + $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 { - $Violations = Invoke-ScriptAnalyzer -ScriptDefinition 'function Param { param ($Param1) function Child { $Param1 } }' -IncludeRule $RuleName + It "has no violations when parameter is called in child scope" -skip { + $ScriptDefinition = 'function Param { param ($Param1) function Child { $Param1 } }' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName $Violations.Count | Should -Be 0 } } From 67b4846f0212bae4b4f08a86426da10143c119c4 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 13 Jan 2020 09:45:41 +0000 Subject: [PATCH 07/12] Fix merge error by re-adding original change --- Rules/Strings.resx | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Rules/Strings.resx b/Rules/Strings.resx index c2bf6c943..1a9f996ee 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 From 34379ab1f24543481e7e92be29795a98ceb057ba Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 13 Jan 2020 09:46:11 +0000 Subject: [PATCH 08/12] whitespace tidy --- Rules/Strings.resx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 1a9f996ee..0361517b2 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -1107,7 +1107,7 @@ Use only 1 whitespace between parameter names or values. - + ReviewUnusedParameter From e8d56c151b97cb9f67244fcfa8525b0f0f7150fc Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 13 Jan 2020 23:24:45 +0000 Subject: [PATCH 09/12] Use OrderBy to get a dictionary of the variable Count for better performance --- Rules/ReviewUnusedParameter.cs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index eb4fb0c88..0ff928d18 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -40,24 +40,22 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) IEnumerable parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false); // list all variables - IEnumerable variables = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) - .Cast() - .Select(variableExpressionAst => variableExpressionAst.VariablePath.UserPath); + IDictionary variableCount = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) + .Select(variableExpressionAst => ((VariableExpressionAst)variableExpressionAst).VariablePath.UserPath) + .GroupBy(variableName => variableName) + .ToDictionary(variableName => variableName.Key, variableName => variableName.Count()); // all bets are off if the script uses PSBoundParameters - if (variables.Contains("PSBoundParameters")) + if (variableCount.ContainsKey("PSBoundParameters")) { continue; } foreach (ParameterAst parameterAst in parameterAsts) { - // compare the list of variables to the parameter name - // there should be at least two matches of the variable name since the parameter declaration counts as one - int matchCount = variables - .Where(variable => variable == parameterAst.Name.VariablePath.ToString()) - .Take(2).Count(); - if (matchCount == 2) + // 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; } From 9f61f2eab16818df510b176ffbff98ef78b91df5 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 13 Jan 2020 23:33:33 +0000 Subject: [PATCH 10/12] case insensitive dictionary and add test case --- Rules/ReviewUnusedParameter.cs | 2 +- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 0ff928d18..687135acb 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -43,7 +43,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) IDictionary variableCount = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) .Select(variableExpressionAst => ((VariableExpressionAst)variableExpressionAst).VariablePath.UserPath) .GroupBy(variableName => variableName) - .ToDictionary(variableName => variableName.Key, variableName => variableName.Count()); + .ToDictionary(variableName => variableName.Key, variableName => variableName.Count(), StringComparer.OrdinalIgnoreCase); // all bets are off if the script uses PSBoundParameters if (variableCount.ContainsKey("PSBoundParameters")) diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index 03e23f612..f5d6004cc 100644 --- a/Tests/Rules/ReviewUnusedParameter.tests.ps1 +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -57,7 +57,13 @@ Describe "ReviewUnusedParameter" { } It "has no violations when parameter is called in child scope" -skip { - $ScriptDefinition = 'function Param { param ($Param1) function Child { $Param1 } }' + $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 } From 00d882d4fcc57ce05952b75cc25d8ff8311f19f7 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 14 Jan 2020 18:29:00 +0000 Subject: [PATCH 11/12] re-trigger ci From 8c06a4177754ca4a27bcb9a7bc696eb5cdcf55f3 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 14 Jan 2020 18:31:38 +0000 Subject: [PATCH 12/12] case insensitive grouping --- Rules/ReviewUnusedParameter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 687135acb..5a06500d8 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -42,7 +42,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) // list all variables IDictionary variableCount = scriptBlockAst.FindAll(oneAst => oneAst is VariableExpressionAst, false) .Select(variableExpressionAst => ((VariableExpressionAst)variableExpressionAst).VariablePath.UserPath) - .GroupBy(variableName => variableName) + .GroupBy(variableName => variableName, StringComparer.OrdinalIgnoreCase) .ToDictionary(variableName => variableName.Key, variableName => variableName.Count(), StringComparer.OrdinalIgnoreCase); // all bets are off if the script uses PSBoundParameters