ReviewUnusedParameter does not capture parameter usage within a scriptblock

[bergmeister]

Steps to reproduce

Run Invoke-ScriptAnalyzer against the following with the new 1.19.0 release.

function foo {
    Param(
        $MyParameter
    )

    Get-Item| ForEach-Object { Get-ChildItem $MyParameter }
}

Expected behavior

No rule violations.

Actual behavior

The new ReviewUnusedParameter rule doesn't notice the usage. I suspect this is similar to the limitation of the AvoidUsingCmdletAliases rule though. Not sure if we should relax the ReviewUnusedParameter rule in this case to search nested scriptblocks inside a function scope. cc @mattmcnabb @rjmholt @JamesWTruher

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSReviewUnusedParameter             Warning      test.ps1   4     The parameter 'MyParameter' has been declared but not used.

If an unexpected error was thrown then please report the full error details using e.g. $error[0] | Select-Object *

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      7.1.0-preview.2
PSEdition                      Core
GitCommitId                    7.1.0-preview.2
OS                             Microsoft Windows 10.0.18363
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.19.0

[bergmeister]

@mattmcnabb @rjmholt It seems we actually discussed this in the PR review with divided opinions: https://github.com/PowerShell/PSScriptAnalyzer/pull/1382#discussion_r366098223 Should we re-visit this decision? Maybe a Strict configuration option on the rule might be the best solution as the community has shown anger at false positives of UseDeclaredVarsMoreThanAssignments in the past and in the case of parameter usage, I don't think we need to be that rigorous. I don't have a strong preference whether a hypothetical Strict config option would be on or off by default (PSSA might even choose different default compared to the PowerShell extension of VS Code that defines its own default settings anyway)

[kmbn]

This caused some of my CI pipelines to suddenly fail today. Since I hadn't pinned the version of PSScriptAnalyzer that was used in the pipelines, and since I hadn't updated to 1.19.0 locally, I thought something was wrong with the pipeline or that I'd inadvertently introduced an error to our scripts.

As a user I'd expect to receive a warning only if there really is an issue. I like the idea of the rule, but I'd rather forgo it entirely than have to add workarounds to our scripts or toggle Strict mode in certain cases. If a Strict config were to be added, I'd expect it to be off by default so that one would only (possibly) see false positive if one deliberately turned on the rule.

In case I'm not understanding what the Strict config would do, the behavior I'd expect as a user is: 1) by default, don't check for unused parameters at all, 2) require the user to explicitly enable the rule and 3) indicate that the rule may yield false positives (I would have appreciated a mention of that directly in the warning message).

(This is my first time commenting on a PSScriptAnalyzer issue or PR. -Even though this new rule has been negative for me, I want to thank you for your work on this tool--it's been a great help not only for ensuring a clear and consistent style for our scripts but also for teaching how to use PowerShell.)

[rjmholt]

We can theoretically know in this case that the variable will be used; ForEach-Object immediately calls the scriptblock it's passed, so the variable is inherited.

However, this is going to be undecidable in general, since I can write a program like this:

function New-ScriptBlock
{
   param($x)

   { "`$x: $x" }
}

$sb = New-ScriptBlock -x 'Hi'
& $sb

We can solve the common case problem for commands that pass and invoke scriptblocks, but the blocker there is parameter binding; to properly resolve when a scriptblock corresponds to a parameter that's going to be invoked immediately, we really need a general purpose way to decide which argument corresponds to which parameter.

That's where I got to here; it's not just that we need to solve it for ForEach-Object, but also the -Variable commands and a few others beyond that

[rjmholt]

I think for now it's ok to search nested scriptblocks though

[bergmeister]

There is also this proposal in PowerShell to help PSSA: https://github.com/PowerShell/PowerShell/issues/12287

[bergmeister]

@kmbn The idea behind Strict would be to only search in the current scope, which can lead to false positives like this one. If Strict is off (which the default should probably be), then it would search all child scopes and therefore the likelihood of a false positive is very small. Technically, the way PowerShell scoping works, one doesn't have to pass all variables to a called function but I think it's considered a best practice to explicitly pass all variables through, hence why I'd still leave the rule enabled by default but have Strict off by default. Would you agree on that?

[rjmholt]

There is also this proposal in PowerShell to help PSSA: PowerShell/PowerShell#12287

Yeah, this proposal would help in the cases we don't know about, but most of the time people use ForEach-Object and we already know. The hard part for us is not knowing the common commands that do this, but being able to perform the analysis once we know.

In this case, the simple solution is to also look in the child scriptblock. The better solution is to search the child scriptblock when the command is ForEach-Object or Where-Object

[JPRuskin]

In this case, the simple solution is to also look in the child scriptblock. The better solution is to search the child scriptblock when the command is ForEach-Object or Where-Object

To add to the list, we're also seeing this fail with Invoke-Command -ScriptBlock {} (though this using $using:varName), and with usage via @PSBoundParameters, which is frustrating. I'm unsure what to suggest, though.

[bergmeister]

@JPRuskin The rule looks only in the current scope at the moment, which is something we should definitely improve as per above comments. Thanks for the suggestion to also scan for $using: since this would tell PSSA implicitly that the scope is valid. Making it recognize the usage when using splatted parameters might be trickier though. For the moment, I suggest to suppress for the parameter name specifically (or completely disable the rule if it is too much of a pain.

 [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'ReplaceWithParameterName',
        Justification = 'False positive as rule does not scan child scopes')]

[plastikfan]

I have found another way in which this rule incorrectly fires which is related to this issue:

Consider this:

function get-DoesContainScheme {
  [OutputType([boolean])]
  param(
    [Parameter()]
    [string]$SchemeName,

    [Parameter()]
    [object[]]$Schemes
  )
  # $null = $SchemeName;
  $found = $Schemes | Where-Object { $_.name -eq $SchemeName };

  return ($null -ne $found);
}

Reports:

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSReviewUnusedParameter             Warning      get-DoesCo 23    The parameter 'SchemeName' has been declared but not    
                                                 ntainSchem       used.
                                                 e.ps1

❗ : note the commented out $null statement. When this is uncommented, the rule no longer fires.

... and for completeness:

λ $PSVersionTable

Name                           Value
----                           -----
PSVersion                      7.0.1
PSEdition                      Core
GitCommitId                    7.0.1
OS                             Microsoft Windows 10.0.19041
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

[VOVELEE]

This issue is also observed in Pester tests when you directly use Parameter in "It" statement.

[joeskeen]

@bergmeister I have tried suppressing the warning as you suggested:

        [Parameter(Mandatory=$false, HelpMessage="The SystemAttributeValue.ObjectId property (Required)")]
        [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', '$ObjectId', Justification = 'false positive')]
        [int] $ObjectId

But I am still getting the PSScriptAnalyzer warning. What am I doing wrong? Do I need to suppress it at the cmdlet level for the specific parameter?, ie

function Assert-ValidSystemAttributeValue {
    [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', '$ObjectId', Justification = 'false positive')]
    [CmdletBinding()]
    param(

I dunno, I've tried several things and none of them are working, and I'm not seeing any real documentation on it.

[mrboring]

I think I have another example of this issue in Pester tests. The code below is based on actual code, but simplified as much as possible.

Expected behavior

No PSReviewUnusedParameter issues:

Actual behavior

A PSReviewUnusedParameter issue for $ParamA:

Environment data

> $PSVersionTable


Name                           Value
----                           -----
PSVersion                      7.1.0
PSEdition                      Core
GitCommitId                    7.1.0
OS                             Microsoft Windows 10.0.19042
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }

1.19.1
1.18.3
1.18.2
1.18.1
1.18.0
1.19.1

[stack-mriso]

I'm also seeing this for .foreach loops. I'm not seeing it happening for standard foreach() loops, however. I assume it has to do with a .foreach being a script block and foreach() not being a script block.

[fsackur]

Possibly related? #1775

[bergmeister]

Possibly related? #1775

No, I replied in #1775

[michielvoo]

I have tried suppressing the warning as you suggested:

[Parameter(Mandatory=$false, HelpMessage="The SystemAttributeValue.ObjectId property (Required)")]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', '$ObjectId', Justification = 'false positive')]

But I am still getting the PSScriptAnalyzer warning. What am I doing wrong?

To suppress PSReviewUnusedParameter for a specific parameter you should set SuppressMessageAttribute's checkId to the name of the parameter without the $ prefix.

[bergmeister]

thanks @michielvoo for stepping in, you are correct :-)

[robinmalik]

Sadly (even without the $) I'm unable to get this to work.

[Parameter(Mandatory = $False)]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'SkipModules', Justification = 'false positive')]
[Array]$SkipModules

Within the script: $Modules = Get-InstalledModule | Where-Object { $_.InstalledLocation -like "$FilterPath*" -and $_.Name -notin $SkipModules }

As a workaround I just added a verbose statement in that uses the parameter 😁

[BenedekFarkas]

@robinmalik the suppression example you wrote above works as-is, but with the attribute attached to the param block, not the parameter. Thanks for the tip though, because we had this rule disabled for our scripts, but this will be a great improvement!

[robinmalik]

@robinmalik the suppression example you wrote above works as-is, but with the attribute attached to the param block, not the parameter. Thanks for the tip though, because we had this rule disabled for our scripts, but this will be a great improvement!

Amazing, thanks for the clarification there. I can confirm it works now :) For anyone else getting stuck like I did:

[CmdletBinding(SupportsShouldProcess)]
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'SkipModules', Justification = 'false positive')]
param(
   [Array]$SkipModules
   ...
)

[robinmalik]

I've noticed that PSScriptAnalyzer doesn't appear to detect conditions where the use of the variable within the script is like so: if($LogFile) {}. Apologies if this has already been reported. At least it can be suppressed though.

[bergmeister]

@robinmalik I do not see the behaviour that you see, the following does not report a warning

function Test {
    Param (
        [String]$a
    )
    if ($a) {
        return
    }
}

[robinmalik]

@bergmeister Thanks, I can't recall what scripts triggered this originally (possibly my mistake) but I'm also unable to see that behaviour with your example given. If I do come across the issue again I'll report back, but disregard it for now :)

[iRon7]

First of all, using the workaround:

[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'ReplaceWithParameterName',   Justification = 'False positive as rule does not scan child scopes')]

Is the correct way to suppress this false positive.

But in case it concerns a lot of parameters, you might also use this dirty workaround:

function foo {
    Param(
        $MyParameter1,
        $MyParameter2,
        $MyParameter3
    )
    $Null = $MyParameter1,  $MyParameter2,  $MyParameter3 # Prevent PSReviewUnusedParameter false positive

    Get-Item| ForEach-Object { Get-ChildItem $MyParameter1 }
}

Note that whenever this request #1894 would be implemented, it won't capture this dirty workaround

[joeskeen]

@mrboring : I think I have another example of this issue in Pester tests. The code below is based on actual code, but simplified as much as possible.

FWIW, I have avoided this problem in Pester tests by making any variable that will be used in multiple ScriptBlocks be $script: scoped. This ensures that the variables transcend the ScriptBlock boundary and can be accessed anywhere.

Just be careful that anything you dot-source doesn't also use a script-scoped variable of the same name or you will cause problems for yourself.

Before (analyzer warnings)

After (no warnings)

[FriedrichWeinmann]

Heya, as this rule has been bugging me for some time now, I've done a quick PR for a proposed solution, traversing into the scriptblocks passed to whitelisted commands (like Where-Object). Of course configurable to extend the list as the user prefers: https://github.com/PowerShell/PSScriptAnalyzer/pull/1921

Any thoughts on that approach?

[Hrxn]

@FriedrichWeinmann The only problem with this suggestion I'm seeing for now is that a whitelisting based approach does not fix the issue where PSSA does trigger this rule for an "unused" parameter that is simply in a sub-scope regardless from some specific command usage, i.e. any locally defined function, for example.

[iRon7]

@FriedrichWeinmann

Heya, as this rule has been bugging me for some time now

Same for me and we are apparently not the only one (looking to the visibility of this issue and the duplicates). I suspect that everybody working with PSScriptAnalyzer will run into this bug and spends some troubleshoot and resolve it. (as for #1163)

Any thoughts on that approach?

false positives are worse than false negatives (knowing that is the default behavior without PSScriptAnalyzer or specific rule). From that view, I think it is better to approach this wit Blacklisted constrains. Meaning:

• Main condition: A warning should be returned If the concerned parameter (e.g. $MyParameter) is nowhere found in the processing blocks of the script. unless (blacklisted constrains exceptions):
  • The variable is (re)assigned ($MyParameter =, rather than retrieved)
  • The variable out of scope, where I even have some doubts whether the concerned warning applies, as something like:

param(
    [String]$String1,
    [String]$String2,
    [String]$MatchCase
)
function Helper ($String1, $String2) {
    if ($MatchCase) { $String1 -ceq $String2 } else { $String1 -eq $String2 }
}
Helper $String1 $String2

might not indeed be a good practice, but the reason for this is not because "The parameter 'MatchCase' has been declared but not used." but because "'MatchCase' is not defined in the current scope".

Anyways, I think that these two rules (PSUseDeclaredVarsMoreThanAssignments and PSReviewUnusedParameter) are just overcomplicating the actual issue were it could just be: don't warn if the concerned variable can be found back (anywhere/later) in the script (even this might not capture everything, it is better than a false-possitive)

[alexchandel]

Please fix this bug. It is not an "enhancement." Using a variable in a script block = "using a variable."

Whether the script block is actually invoked is irrelevant. "Using" a parameter by referencing it from an unused script block is like "using" it by storing it in an unused hashtable. The question is no longer whether the parameter has been used, but whether the new value is used; the original parameter should no longer be linted PSReviewUnusedParameter.

Here's another replication case:

function Invoke-In {
    <#
    .SYNOPSIS
        Runs a block in a pushed directory, popping it afterwards.
    #>
    param (
        [Parameter(Mandatory=$True, Position=0)]
        [ValidateScript({Test-Path -PathType Container -LiteralPath $_})]
        [string]$Directory,
        [Parameter(Mandatory=$True, Position=1)]
        [ScriptBlock]$CodeBlock
    )
    pushd $Directory
    try {
        return & $CodeBlock
    } finally {
        popd
    }
}

function Build-Proj {
    param (
        [string] $Platform
    )
    Invoke-In vp3d {
        echo MSBuild -noLogo Proj.vcxproj -p:Configuration=Release -p:Platform=$Platform -consoleLoggerParameters:ForceConsoleColor
    }
}

If $CodeBlock were never actually invoked, that's a problem of Invoke-In, not Build-Proj!