PSReviewUnusedParameter: Add CommandsToTraverse option (#1921)

* Added command traversal option Explicitly included Where-Object and ForEach-Object scriptblocks to also be searched for variable use * Command traversal check no longer case sensitive * Extended tests for selective command traversal * Rename setting to CommandsToTraverse * Added docs for new configuration: CommandsToTraverse
PowerShell · Feb 13, 2024 · c06e005 · c06e005
1 parent 6cb66c1
commit c06e005
Show file tree

Hide file tree

Showing 3 changed files with 130 additions and 5 deletions.
diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs
@@ -21,8 +21,60 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 #endif
     public class ReviewUnusedParameter : IScriptRule
     {
+        private readonly string TraverseArgName = "CommandsToTraverse";
+        public List<string> TraverseCommands { get; private set; }
+
+        /// <summary>
+        /// Configure the rule.
+        ///
+        /// Sets the list of commands to traverse of this rule
+        /// </summary>
+        private void SetProperties()
+        {
+            TraverseCommands = new List<string>() { "Where-Object", "ForEach-Object" };
+
+            Dictionary<string, object> ruleArgs = Helper.Instance.GetRuleArguments(GetName());
+            if (ruleArgs == null)
+            {
+                return;
+            }
+
+            if (!ruleArgs.TryGetValue(TraverseArgName, out object obj))
+            {
+                return;
+            }
+            IEnumerable<string> commands = obj as IEnumerable<string>;
+            if (commands == null)
+            {
+                // try with enumerable objects
+                var enumerableObjs = obj as IEnumerable<object>;
+                if (enumerableObjs == null)
+                {
+                    return;
+                }
+                foreach (var x in enumerableObjs)
+                {
+                    var y = x as string;
+                    if (y == null)
+                    {
+                        return;
+                    }
+                    else
+                    {
+                        TraverseCommands.Add(y);
+                    }
+                }
+            }
+            else
+            {
+                TraverseCommands.AddRange(commands);
+            }
+        }
+
         public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
         {
+            SetProperties();
+
             if (ast == null)
             {
                 throw new ArgumentNullException(Strings.NullAstErrorMessage);
@@ -46,10 +98,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                 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);
+                IDictionary<string, int> variableCount = GetVariableCount(scriptBlockAst);
 
                 foreach (ParameterAst parameterAst in parameterAsts)
                 {
@@ -164,5 +213,39 @@ public string GetSourceName()
         {
             return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
         }
+
+        /// <summary>
+        /// Returns a dictionary including all variables in the scriptblock and their count
+        /// </summary>
+        /// <param name="ast">The scriptblock ast to scan</param>
+        /// <param name="data">Previously generated data. New findings are added to any existing dictionary if present</param>
+        /// <returns>a dictionary including all variables in the scriptblock and their count</returns>
+        IDictionary<string, int> GetVariableCount(ScriptBlockAst ast, Dictionary<string, int> data = null)
+        {
+            Dictionary<string, int> content = data;
+            if (null == data)
+                content = new Dictionary<string, int>(StringComparer.OrdinalIgnoreCase);
+            IDictionary<string, int> result = ast.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);
+
+            foreach (string key in result.Keys)
+            {
+                if (content.ContainsKey(key))
+                    content[key] = content[key] + result[key];
+                else
+                    content[key] = result[key];
+            }
+
+            IEnumerable<Ast> foundScriptBlocks = ast.FindAll(oneAst => oneAst is ScriptBlockExpressionAst, false)
+                .Where(oneAst => oneAst?.Parent is CommandAst && ((CommandAst)oneAst.Parent).CommandElements[0] is StringConstantExpressionAst && TraverseCommands.Contains(((StringConstantExpressionAst)((CommandAst)oneAst.Parent).CommandElements[0]).Value, StringComparer.OrdinalIgnoreCase))
+                .Select(oneAst => ((ScriptBlockExpressionAst)oneAst).ScriptBlock);
+            foreach (Ast astItem in foundScriptBlocks)
+                if (astItem != ast)
+                    GetVariableCount((ScriptBlockAst)astItem, content);
+
+            return content;
+        }
     }
 }
diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1
@@ -32,6 +32,12 @@ Describe "ReviewUnusedParameter" {
             $Violations.Count | Should -Be 1
         }
 
+		It "doesn't traverse scriptblock scope for a random command" {
+            $ScriptDefinition = '{ param ($Param1) 1..3 | Invoke-Parallel { $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
@@ -81,6 +87,24 @@ Describe "ReviewUnusedParameter" {
             $ScriptDefinition = 'function foo { param ($Param1, $param2) $param1; $Param2}'
             $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
             $Violations.Count | Should -Be 0
+        }
+
+		It "does traverse scriptblock scope for Foreach-Object" {
+            $ScriptDefinition = '{ param ($Param1) 1..3 | ForEach-Object { $Param1 }}'
+            $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
+            $Violations.Count | Should -Be 0
+        }
+
+		It "does traverse scriptblock scope for commands added to the traversal list" {
+            $ScriptDefinition = '{ param ($Param1) Invoke-PSFProtectedCommand { $Param1 } }'
+            $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName -Settings @{
+				Rules = @{
+					PSReviewUnusedParameter = @{
+						CommandsToTraverse = @('Invoke-PSFProtectedCommand')
+					}
+				}
+			}
+            $Violations.Count | Should -Be 0
         }
     }
-}
+}
diff --git a/docs/Rules/ReviewUnusedParameter.md b/docs/Rules/ReviewUnusedParameter.md
@@ -14,6 +14,24 @@ title: ReviewUnusedParameter
 This rule identifies parameters declared in a script, scriptblock, or function scope that have not
 been used in that scope.
 
+## Configuration settings
+
+|Configuration key|Meaning|Accepted values|Mandatory|Example|
+|---|---|---|---|---|
+|CommandsToTraverse|By default, this command will not consider child scopes other than scriptblocks provided to Where-Object or ForEach-Object. This setting allows you to add additional commands that accept scriptblocks that this rule should traverse into.|string[]: list of commands whose scriptblock to traverse.|`@('Invoke-PSFProtectedCommand')`|
+
+```powershell
+@{
+    Rules = @{
+        ReviewUnusedParameter = @{
+            CommandsToTraverse = @(
+                'Invoke-PSFProtectedCommand'
+            )
+        }
+    }
+}
+```
+
 ## How
 
 Consider removing the unused parameter.