Skip to content

PSReviewUnusedParameter false positive for ValueFromPipeline #2072

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 13, 2025
Merged

PSReviewUnusedParameter false positive for ValueFromPipeline #2072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b09c4e8
Check for ValueFromPipeline
liamjpeters Mar 3, 2025
a379e1f
Check whether each scriptblock being analysed has a process block tha…
liamjpeters Mar 4, 2025
06141fc
Amend tests to consider process block
liamjpeters Mar 4, 2025
8258436
Update Rules/ReviewUnusedParameter.cs
liamjpeters Mar 5, 2025
d83b1d9
Update Rules/ReviewUnusedParameter.cs
liamjpeters Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion Rules/ReviewUnusedParameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -97,11 +98,40 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
// find all declared parameters
IEnumerable<Ast> 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<string, int> 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<string, int> variableCount = GetVariableCount(scriptBlockAst);

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 and the scriptblock has a process block with
// $_ or $PSItem usage, then the parameter is considered used
if (valueFromPipeline != null && valueFromPipeline.GetValue() && hasProcessBlockWithPSItemOrUnderscore)

{
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)
Expand Down Expand Up @@ -220,7 +250,7 @@ public string GetSourceName()
/// <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)
IDictionary<string, int> GetVariableCount(Ast ast, Dictionary<string, int> data = null)
{
Dictionary<string, int> content = data;
if (null == data)
Expand Down
48 changes: 48 additions & 0 deletions Tests/Rules/ReviewUnusedParameter.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,30 @@ Describe "ReviewUnusedParameter" {
$Violations.Count | Should -Be 2
}

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

It "has 1 violation - scriptblock with 1 unused parameter" {
$ScriptDefinition = '{ param ($Param1) }'
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
Expand Down Expand Up @@ -59,6 +83,30 @@ Describe "ReviewUnusedParameter" {
$Violations.Count | Should -Be 0
}

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 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 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 inside process block" {
$ScriptDefinition = 'function BadFunc1 { param ([Parameter(ValueFromPipeline)] $Param1) process{$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
Expand Down