New rule - ReviewUnusedParameter (#1382)

* New rule - ReviewUnusedParameter

* Fix test failures in AvoidAssignmentToAutomaticVariable by excluding the new rule as the parameter is not used in those tests

* Fix test failures in other tests when parameters are not used by excluding rule

* make linq query stop earlier for better performance, avoid redundant ToList() and minor style tweaks (Linq variable naming and trailing whitespace trim)

* format test file

* Style fixes for PR #1382

* Fix merge error by re-adding original change

* whitespace tidy

* Use OrderBy to get a dictionary of the variable Count for better performance

* case insensitive dictionary and add test case

* re-trigger ci

* case insensitive grouping

Co-authored-by: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>

Author: mattmcnabb

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 | |

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
+}
+```

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
+{
+    /// <summary>
+    /// ReviewUnusedParameter: Check that all declared parameters are used in the script body.
+    /// </summary>
+#if !CORECLR
+[Export(typeof(IScriptRule))]
+#endif
+    public class ReviewUnusedParameter : IScriptRule
+    {
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null)
+            {
+                throw new ArgumentNullException(Strings.NullAstErrorMessage);
+            }
+
+            IEnumerable<Ast> scriptBlockAsts = ast.FindAll(oneAst => oneAst is ScriptBlockAst, true);
+            if (scriptBlockAsts == null)
+            {
+                yield break;
+            }
+
+            foreach (ScriptBlockAst scriptBlockAst in scriptBlockAsts)
+            {
+                // find all declared parameters
+                IEnumerable<Ast> parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false);
+
+                // list all variables
+                IDictionary<string, int> 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
+                    );
+                }
+            }
+        }
+
+        /// <summary>
+        /// GetName: Retrieves the name of this rule.
+        /// </summary>
+        /// <returns>The name of this rule</returns>
+        public string GetName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.ReviewUnusedParameterName);
+        }
+
+        /// <summary>
+        /// GetCommonName: Retrieves the common name of this rule.
+        /// </summary>
+        /// <returns>The common name of this rule</returns>
+        public string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterCommonName);
+        }
+
+        /// <summary>
+        /// GetDescription: Retrieves the description of this rule.
+        /// </summary>
+        /// <returns>The description of this rule</returns>
+        public string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.ReviewUnusedParameterDescription);
+        }
+
+        /// <summary>
+        /// GetSourceType: Retrieves the type of the rule, builtin, managed or module.
+        /// </summary>
+        public SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+
+        /// <summary>
+        /// GetSeverity: Retrieves the severity of the rule: error, warning of information.
+        /// </summary>
+        /// <returns></returns>
+        public RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Warning;
+        }
+
+        /// <summary>
+        /// GetSourceName: Retrieves the module/assembly name the rule is from.
+        /// </summary>
+        public string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+    }
+}

Rules/Strings.Designer.cs

@@ -1107,4 +1107,16 @@
   <data name="UseConsistentWhitespaceErrorSpaceBetweenParameter" xml:space="preserve">
     <value>Use only 1 whitespace between parameter names or values.</value>
   </data>
+  <data name="ReviewUnusedParameterCommonName" xml:space="preserve">
+    <value>ReviewUnusedParameter</value>
+  </data>
+  <data name="ReviewUnusedParameterDescription" xml:space="preserve">
+    <value>Ensure all parameters are used within the same script, scriptblock, or function where they are declared.</value>	
+  </data>	
+  <data name="ReviewUnusedParameterError" xml:space="preserve">	
+    <value>The parameter '{0}' has been declared but not used. </value>	
+  </data>	
+  <data name="ReviewUnusedParameterName" xml:space="preserve">	
+    <value>ReviewUnusedParameter</value>	
+  </data>
 </root>

Rules/Strings.resx

@@ -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

Tests/Engine/GetScriptAnalyzerRule.tests.ps1

@@ -43,7 +43,7 @@ Describe "AvoidAssignmentToAutomaticVariables" {
         It "Using Variable <VariableName> 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))

Tests/Rules/AvoidAssignmentToAutomaticVariable.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
         }

Tests/Rules/AvoidPositionalParameters.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
+        }
+    }
+}