Skip to content

Commit 1d394ee

Browse files
liamjpetersandyleejordan
liamjpeters
and
andyleejordan
authored
PSReviewUnusedParameter false positive for ValueFromPipeline (#2072)
* Check for ValueFromPipeline * 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. * Amend tests to consider process block * Update Rules/ReviewUnusedParameter.cs Co-authored-by: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> * Update Rules/ReviewUnusedParameter.cs --------- Co-authored-by: Andy Jordan <2226434+andyleejordan@users.noreply.github.com>
1 parent d4fbdc3 commit 1d394ee

2 files changed

Lines changed: 79 additions & 1 deletion

File tree

‎Rules/ReviewUnusedParameter.cs‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Linq;
77
using System.Management.Automation.Language;
88
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
9+
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Extensions;
910
#if !CORECLR
1011
using System.ComponentModel.Composition;
1112
#endif
@@ -97,11 +98,40 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
9798
// find all declared parameters
9899
IEnumerable<Ast> parameterAsts = scriptBlockAst.FindAll(oneAst => oneAst is ParameterAst, false);
99100

101+
// does the scriptblock have a process block where either $PSItem or $_ is referenced
102+
bool hasProcessBlockWithPSItemOrUnderscore = false;
103+
if (scriptBlockAst.ProcessBlock != null)
104+
{
105+
IDictionary<string, int> processBlockVariableCount = GetVariableCount(scriptBlockAst.ProcessBlock);
106+
processBlockVariableCount.TryGetValue("_", out int underscoreVariableCount);
107+
processBlockVariableCount.TryGetValue("psitem", out int psitemVariableCount);
108+
if (underscoreVariableCount > 0 || psitemVariableCount > 0)
109+
{
110+
hasProcessBlockWithPSItemOrUnderscore = true;
111+
}
112+
}
113+
100114
// list all variables
101115
IDictionary<string, int> variableCount = GetVariableCount(scriptBlockAst);
102116

103117
foreach (ParameterAst parameterAst in parameterAsts)
104118
{
119+
// Check if the parameter has the ValueFromPipeline attribute
120+
NamedAttributeArgumentAst valueFromPipeline = (NamedAttributeArgumentAst)parameterAst.Find(
121+
valFromPipelineAst => valFromPipelineAst is NamedAttributeArgumentAst namedAttrib && string.Equals(
122+
namedAttrib.ArgumentName, "ValueFromPipeline",
123+
StringComparison.OrdinalIgnoreCase
124+
),
125+
false
126+
);
127+
// If the parameter has the ValueFromPipeline attribute and the scriptblock has a process block with
128+
// $_ or $PSItem usage, then the parameter is considered used
129+
if (valueFromPipeline != null && valueFromPipeline.GetValue() && hasProcessBlockWithPSItemOrUnderscore)
130+
131+
{
132+
continue;
133+
}
134+
105135
// there should be at least two usages of the variable since the parameter declaration counts as one
106136
variableCount.TryGetValue(parameterAst.Name.VariablePath.UserPath, out int variableUsageCount);
107137
if (variableUsageCount >= 2)
@@ -220,7 +250,7 @@ public string GetSourceName()
220250
/// <param name="ast">The scriptblock ast to scan</param>
221251
/// <param name="data">Previously generated data. New findings are added to any existing dictionary if present</param>
222252
/// <returns>a dictionary including all variables in the scriptblock and their count</returns>
223-
IDictionary<string, int> GetVariableCount(ScriptBlockAst ast, Dictionary<string, int> data = null)
253+
IDictionary<string, int> GetVariableCount(Ast ast, Dictionary<string, int> data = null)
224254
{
225255
Dictionary<string, int> content = data;
226256
if (null == data)

‎Tests/Rules/ReviewUnusedParameter.tests.ps1‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,30 @@ Describe "ReviewUnusedParameter" {
2020
$Violations.Count | Should -Be 2
2121
}
2222

23+
It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$_ usage inside process block" {
24+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) process {$_}}'
25+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
26+
$Violations.Count | Should -Be 1
27+
}
28+
29+
It "has 1 violation - function with 1 parameter with ValueFromPipeline set to false and `$PSItem usage inside process block" {
30+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $false)] $Param1) process {$PSItem}}'
31+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
32+
$Violations.Count | Should -Be 1
33+
}
34+
35+
It "has 1 violation - function with 1 parameter with ValueFromPipeline set to true and `$_ usage outside process block" {
36+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $_}'
37+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
38+
$Violations.Count | Should -Be 1
39+
}
40+
41+
It "has 1 violation - function with 1 parameter with ValueFromPipeline set to true and `$PSItem usage outside process block" {
42+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) $PSItem}'
43+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
44+
$Violations.Count | Should -Be 1
45+
}
46+
2347
It "has 1 violation - scriptblock with 1 unused parameter" {
2448
$ScriptDefinition = '{ param ($Param1) }'
2549
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
@@ -59,6 +83,30 @@ Describe "ReviewUnusedParameter" {
5983
$Violations.Count | Should -Be 0
6084
}
6185

86+
It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$_ usage inside process block" {
87+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) process {$_}}'
88+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
89+
$Violations.Count | Should -Be 0
90+
}
91+
92+
It "has no violation - function with 1 parameter with ValueFromPipeline explictly set to true and `$PSItem usage inside process block" {
93+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline = $true)] $Param1) process {$PSItem}}'
94+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
95+
$Violations.Count | Should -Be 0
96+
}
97+
98+
It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$_ usage inside process block" {
99+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) process{$_}}'
100+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
101+
$Violations.Count | Should -Be 0
102+
}
103+
104+
It "has no violation - function with 1 parameter with ValueFromPipeline implicitly set to true and `$PSItem usage inside process block" {
105+
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) process{$PSItem}}'
106+
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
107+
$Violations.Count | Should -Be 0
108+
}
109+
62110
It "has no violations when using PSBoundParameters" {
63111
$ScriptDefinition = 'function Bound { param ($Param1) Get-Foo @PSBoundParameters }'
64112
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName

0 commit comments

Comments
 (0)