From b09c4e810feb2894c9ec9ffae2a8865211db2ccd Mon Sep 17 00:00:00 2001 From: Liam Peters Date: Mon, 3 Mar 2025 20:49:33 +0000 Subject: [PATCH 1/5] Check for ValueFromPipeline --- Rules/ReviewUnusedParameter.cs | 20 ++++++++++++ Tests/Rules/ReviewUnusedParameter.tests.ps1 | 36 +++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index f13584fed..99e8af233 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Management.Automation.Language; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Extensions; #if !CORECLR using System.ComponentModel.Composition; #endif @@ -102,6 +103,25 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) foreach (ParameterAst parameterAst in parameterAsts) { + // Check if the parameter has the ValueFromPipeline attribute + NamedAttributeArgumentAst valueFromPipeline = (NamedAttributeArgumentAst)parameterAst.Find( + valFromPipelineAst => valFromPipelineAst is NamedAttributeArgumentAst namedAttrib && string.Equals( + namedAttrib.ArgumentName, "ValueFromPipeline", + StringComparison.OrdinalIgnoreCase + ), + false + ); + // If the parameter has the ValueFromPipeline attribute, check for usages of $_ or $PSItem + if (valueFromPipeline?.GetValue() == true) + { + variableCount.TryGetValue("_", out int underscoreVariableCount); + variableCount.TryGetValue("psitem", out int psitemVariableCount); + if (underscoreVariableCount > 0 || psitemVariableCount > 0) + { + continue; + } + } + // 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) diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index 59d8b160d..0be91ef4d 100644 --- a/Tests/Rules/ReviewUnusedParameter.tests.ps1 +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -20,6 +20,18 @@ Describe "ReviewUnusedParameter" { $Violations.Count | Should -Be 2 } + It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$_ usage" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) $_}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 1 + } + + It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$PSItem usage" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) $PSItem}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 1 + } + It "has 1 violation - scriptblock with 1 unused parameter" { $ScriptDefinition = '{ param ($Param1) }' $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName @@ -59,6 +71,30 @@ Describe "ReviewUnusedParameter" { $Violations.Count | Should -Be 0 } + It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$_ usage" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $_}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$PSItem usage" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $PSItem}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$_ usage" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) $_}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 0 + } + + It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$PSItem usage" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) $PSItem}' + $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 From a379e1fe41c308d2931637359a0d3b6ffe9816ab Mon Sep 17 00:00:00 2001 From: Liam Peters Date: Tue, 4 Mar 2025 09:33:09 +0000 Subject: [PATCH 2/5] Check whether each scriptblock being analysed has a process block that directly contains variable usage of $_ or $PSItem. Then when we encounted a parameter with ValueFromPipeline set, we consider whether we saw usage within a process block by automatic variable. --- Rules/ReviewUnusedParameter.cs | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 99e8af233..afacc3d2f 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -98,6 +98,19 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) // find all declared parameters IEnumerable parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false); + // does the scriptblock have a process block where either $PSItem or $_ is referenced + bool hasProcessBlockWithPSItemOrUnderscore = false; + if (scriptBlockAst.ProcessBlock != null) + { + IDictionary processBlockVariableCount = GetVariableCount(scriptBlockAst.ProcessBlock); + processBlockVariableCount.TryGetValue("_", out int underscoreVariableCount); + processBlockVariableCount.TryGetValue("psitem", out int psitemVariableCount); + if (underscoreVariableCount > 0 || psitemVariableCount > 0) + { + hasProcessBlockWithPSItemOrUnderscore = true; + } + } + // list all variables IDictionary variableCount = GetVariableCount(scriptBlockAst); @@ -108,18 +121,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) valFromPipelineAst => valFromPipelineAst is NamedAttributeArgumentAst namedAttrib && string.Equals( namedAttrib.ArgumentName, "ValueFromPipeline", StringComparison.OrdinalIgnoreCase - ), + ), false ); - // If the parameter has the ValueFromPipeline attribute, check for usages of $_ or $PSItem - if (valueFromPipeline?.GetValue() == true) + // If the parameter has the ValueFromPipeline attribute and the scriptblock has a process block with + // $_ or $PSItem usage, then the parameter is considered used + if (valueFromPipeline?.GetValue() == true && hasProcessBlockWithPSItemOrUnderscore) { - variableCount.TryGetValue("_", out int underscoreVariableCount); - variableCount.TryGetValue("psitem", out int psitemVariableCount); - if (underscoreVariableCount > 0 || psitemVariableCount > 0) - { - continue; - } + continue; } // there should be at least two usages of the variable since the parameter declaration counts as one @@ -240,7 +249,7 @@ public string GetSourceName() /// 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) + IDictionary GetVariableCount(Ast ast, Dictionary data = null) { Dictionary content = data; if (null == data) From 06141fc29f8f9b07266e815a83309198caa8854d Mon Sep 17 00:00:00 2001 From: Liam Peters Date: Tue, 4 Mar 2025 09:33:34 +0000 Subject: [PATCH 3/5] Amend tests to consider process block --- Tests/Rules/ReviewUnusedParameter.tests.ps1 | 36 ++++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/Tests/Rules/ReviewUnusedParameter.tests.ps1 b/Tests/Rules/ReviewUnusedParameter.tests.ps1 index 0be91ef4d..9e4202dcf 100644 --- a/Tests/Rules/ReviewUnusedParameter.tests.ps1 +++ b/Tests/Rules/ReviewUnusedParameter.tests.ps1 @@ -20,14 +20,26 @@ Describe "ReviewUnusedParameter" { $Violations.Count | Should -Be 2 } - It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$_ usage" { - $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) $_}' + It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$_ usage inside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) process {$_}}' $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName $Violations.Count | Should -Be 1 } - It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$PSItem usage" { - $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) $PSItem}' + It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$PSItem usage inside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) process {$PSItem}}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 1 + } + + It "has 1 violation - function with 1 parameter with ValueFromPipeline set to true and `$_ usage outside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $_}' + $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName + $Violations.Count | Should -Be 1 + } + + It "has 1 violation - function with 1 parameter with ValueFromPipeline set to true and `$PSItem usage outside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $PSItem}' $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName $Violations.Count | Should -Be 1 } @@ -71,26 +83,26 @@ Describe "ReviewUnusedParameter" { $Violations.Count | Should -Be 0 } - It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$_ usage" { - $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $_}' + It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$_ usage inside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) process {$_}}' $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName $Violations.Count | Should -Be 0 } - It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$PSItem usage" { - $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $PSItem}' + It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$PSItem usage inside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) process {$PSItem}}' $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName $Violations.Count | Should -Be 0 } - It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$_ usage" { - $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) $_}' + It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$_ usage inside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) process{$_}}' $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName $Violations.Count | Should -Be 0 } - It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$PSItem usage" { - $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) $PSItem}' + It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$PSItem usage inside process block" { + $ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) process{$PSItem}}' $Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName $Violations.Count | Should -Be 0 } From 8258436b316fa6954093d5f1e1df932e642ac65a Mon Sep 17 00:00:00 2001 From: Liam Peters Date: Wed, 5 Mar 2025 07:46:00 +0000 Subject: [PATCH 4/5] Update Rules/ReviewUnusedParameter.cs Co-authored-by: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> --- Rules/ReviewUnusedParameter.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index afacc3d2f..79c72885d 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -126,7 +126,8 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) ); // If the parameter has the ValueFromPipeline attribute and the scriptblock has a process block with // $_ or $PSItem usage, then the parameter is considered used - if (valueFromPipeline?.GetValue() == true && hasProcessBlockWithPSItemOrUnderscore) + if (valueFromPipeline?.GetValue() && hasProcessBlockWithPSItemOrUnderscore) + { continue; } From d83b1d940315e88cfb0f08576fd68089d7a24068 Mon Sep 17 00:00:00 2001 From: Liam Peters Date: Mon, 10 Mar 2025 19:30:00 +0000 Subject: [PATCH 5/5] Update Rules/ReviewUnusedParameter.cs --- Rules/ReviewUnusedParameter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Rules/ReviewUnusedParameter.cs b/Rules/ReviewUnusedParameter.cs index 79c72885d..9b727e7fc 100644 --- a/Rules/ReviewUnusedParameter.cs +++ b/Rules/ReviewUnusedParameter.cs @@ -126,7 +126,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) ); // If the parameter has the ValueFromPipeline attribute and the scriptblock has a process block with // $_ or $PSItem usage, then the parameter is considered used - if (valueFromPipeline?.GetValue() && hasProcessBlockWithPSItemOrUnderscore) + if (valueFromPipeline != null && valueFromPipeline.GetValue() && hasProcessBlockWithPSItemOrUnderscore) { continue;