From 7dfcab17072f3f336744a0ca04d2b4841a0dbfc6 Mon Sep 17 00:00:00 2001 From: Friedrich Weinmann Date: Thu, 1 Jun 2023 10:38:49 +0200 Subject: [PATCH 1/5] Added command traversal option Explicitly included Where-Object and ForEach-Object scriptblocks to also be searched for variable use --- Rules/ReviewUnusedParameter.cs | 91 ++++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 4 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index ffaaa1334..385558fe0 100644 --- 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 = "traverseList"; + public List TraverseCommands { get; private set; } + + /// + /// Configure the rule. + /// + /// Sets the list of commands to traverse of this rule + /// + private void SetProperties() + { + TraverseCommands = new List() { "Where-Object", "ForEach-Object" }; + + Dictionary ruleArgs = Helper.Instance.GetRuleArguments(GetName()); + if (ruleArgs == null) + { + return; + } + + if (!ruleArgs.TryGetValue(TraverseArgName, out object obj)) + { + return; + } + IEnumerable commands = obj as IEnumerable; + if (commands == null) + { + // try with enumerable objects + var enumerableObjs = obj as IEnumerable; + 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 AnalyzeScript(Ast ast, string fileName) { + SetProperties(); + if (ast == null) { throw new ArgumentNullException(Strings.NullAstErrorMessage); @@ -46,10 +98,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) 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); + IDictionary variableCount = GetVariableCount(scriptBlockAst); foreach (ParameterAst parameterAst in parameterAsts) { @@ -164,5 +213,39 @@ public string GetSourceName() { return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); } + + /// + /// Returns a dictionary including all variables in the scriptblock and their count + /// + /// The scriptblock ast to scan + /// Previously generated data. New findings are added to any existing dictionary if present + /// a dictionary including all variables in the scriptblock and their count + IDictionary GetVariableCount(ScriptBlockAst ast, Dictionary data = null) + { + Dictionary content = data; + if (null == data) + content = new Dictionary(StringComparer.OrdinalIgnoreCase); + IDictionary 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 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)) + .Select(oneAst => ((ScriptBlockExpressionAst)oneAst).ScriptBlock); + foreach (Ast astItem in foundScriptBlocks) + if (astItem != ast) + GetVariableCount((ScriptBlockAst)astItem, content); + + return content; + } } } From a68ba4f22506164bc0ada08a31c92ea79b10f19e Mon Sep 17 00:00:00 2001 From: FriedrichWeinmann Date: Wed, 3 Jan 2024 12:34:04 +0100 Subject: [PATCH 2/5] Command traversal check no longer case sensitive --- Rules/ReviewUnusedParameter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 385558fe0..3fd4359fa 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -239,7 +239,7 @@ IDictionary GetVariableCount(ScriptBlockAst ast, Dictionary 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)) + .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) From e6fc7579ea7d6f001589fb0a6858d5afe36652e7 Mon Sep 17 00:00:00 2001 From: FriedrichWeinmann Date: Wed, 3 Jan 2024 12:34:40 +0100 Subject: [PATCH 3/5] Extended tests for selective command traversal --- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 26 ++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index 0249f9743..0e33b54da 100644 --- 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 = @{ + traverseList = @('Invoke-PSFProtectedCommand') + } + } + } + $Violations.Count | Should -Be 0 } } -} +} \ No newline at end of file From 12f1c65e78992f3bc46ff69684be69b01b28c19d Mon Sep 17 00:00:00 2001 From: Friedrich Weinmann Date: Mon, 5 Feb 2024 14:45:18 +0100 Subject: [PATCH 4/5] Rename setting to CommandsToTraverse --- Rules/ReviewUnusedParameter.cs | 2 +- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 3fd4359fa..f13584fed 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -21,7 +21,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public class ReviewUnusedParameter : IScriptRule { - private readonly string TraverseArgName = "traverseList"; + private readonly string TraverseArgName = "CommandsToTraverse"; public List TraverseCommands { get; private set; } /// diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index 0e33b54da..59d8b160d 100644 --- a/Tests/Rules/ReviewUnusedParameter.tests.ps1 +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -100,7 +100,7 @@ Describe "ReviewUnusedParameter" { $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName -Settings @{ Rules = @{ PSReviewUnusedParameter = @{ - traverseList = @('Invoke-PSFProtectedCommand') + CommandsToTraverse = @('Invoke-PSFProtectedCommand') } } } From 81b817966a8f550f9db900b44d26fbebdde2627c Mon Sep 17 00:00:00 2001 From: Friedrich Weinmann Date: Mon, 5 Feb 2024 14:45:43 +0100 Subject: [PATCH 5/5] Added docs for new configuration: CommandsToTraverse --- docs/Rules/ReviewUnusedParameter.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/Rules/ReviewUnusedParameter.md b/docs/Rules/ReviewUnusedParameter.md index 309bcd4d2..9c488367d 100644 --- 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.