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: #1382 (comment)
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: PowerShell/PowerShell#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.

[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:
#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!

[muvijay]

Any progress over here, issue reported 3 years back and still no fix?

[Hrxn]

@muvijay There has been progress on this, actually.

Although https://github.com/PowerShell/PSScriptAnalyzer/blob/master/CHANGELOG.MD has not been updated, apparently. It's still on 1.21.0. But it's mentioned on the Releases page for 1.22.0

• PSReviewUnusedParameter: Add CommandsToTraverse option by FriedrichWeinmann in PSReviewUnusedParameter: Add CommandsToTraverse option #1921

Reusing my own old testcase here, this does not work and still triggers a PSReviewUnusedParameter violation for $DemoSwitch

param(
	[Parameter(Position = 0)]
	[string] $ExampleInput = 'I am just a simple DEMO string',

	[switch] $Lower,
	[switch] $Upper,
	[switch] $Variant,

	[switch] $DemoSwitch
)

function Assert-Parameter ([string] $In) {
	if ($DemoSwitch) {
		return $In.Replace('string', 'string, DemoSwitch is SET! SUCCESS!')
	} else {
		return $In.Replace('string', 'string, DemoSwitch is NOT SET! ("default" operation)')
	}
}

if ($Variant) {
	Write-Output (Assert-Parameter -In $ExampleInput)
} elseif ($Lower) {
	Write-Output $ExampleInput.ToLower()
} elseif ($Upper) {
	Write-Output $ExampleInput.ToUpper()
} else {
	Write-Output $ExampleInput
}

To be fair, my custom function here is used inside parentheses as a parameter to Write-Output, but if I slightly change the testcase to:

param(
	[Parameter(Position = 0)]
	[string] $ExampleInput = 'I am just a simple DEMO string',

	[switch] $Lower,
	[switch] $Upper,
	[switch] $Variant,

	[switch] $DemoSwitch
)

function Assert-Parameter ([string] $In) {
	if ($DemoSwitch) {
		return $In.Replace('string', 'string, DemoSwitch is SET! SUCCESS!')
	} else {
		return $In.Replace('string', 'string, DemoSwitch is NOT SET! ("default" operation)')
	}
}

if ($Variant) {
	$Result = Assert-Parameter -In $ExampleInput
	Write-Output $Result
} elseif ($Lower) {
	Write-Output $ExampleInput.ToLower()
} elseif ($Upper) {
	Write-Output $ExampleInput.ToUpper()
} else {
	Write-Output $ExampleInput
}

It still triggers the PSReviewUnusedParameter violation for $DemoSwitch..

I have this in Rules = @{..} in my settings file for Invoke-ScriptAnalyzer:

        PSReviewUnusedParameter = @{
            CommandsToTraverse = @(
                'Assert-Parameter'
            )
        }

Seems like it does not do what I think it is supposed to do here...

Tagging @FriedrichWeinmann here, maybe he can weigh in?

[FriedrichWeinmann]

Heya :)
The configuration is intended for scriptblocks you provide to a command. Not for the command code itself.
With that configuration you could then do something like this without a complaint:

Assert-Parameter { if ($DemoSwitch) { "Yay" } }

I am fundamentally opposed to scope boundary violations, so I did not even consider implementing looking into child-functions for this, sorry. Honestly, I'm also against ever implementing it that way, in order to not encourage people to do so.

[Hrxn]

Hey, thanks for weighing in so quickly! 😄

Okay, I see what you mean by scriptblock only. But I'm a but confused, why would you describe this as a scope boundary violation? I mean, it's still a scope issue, this should be clear.
Assert-Parameter here is a local function and it creates its own scope, true, so far we're all on the same page.

I still don't see the fundamental difference here with regard to ForEach-Object and Where-Object though, besides from pipeline usage?

Also, what exactly would you intent to discourage here?
Do you consider my example testcase here to be non-idiomatic or conceptually suboptimal or something?
I'd like to understand that better, to be honest.

[FriedrichWeinmann]

The scriptblocks provided to ForEach-Object & Where-Object do not become part of the code implementing the command - they are code provided to the commands, which the commands then execute for the caller (script). The scriptblocks are not executed within the context of those commands.

Pipeline usage doesn't really factor in here.

Scope Boundary Violation

There is a commonly propagated best practice where functions should not directly access variables from their parent scopes - all information a command needs should be provided via parameter, all results returned via output.
There is no technical issue with access variables directly from the parent command, so it's not like you are hurting your code with something like this:

function Get-Test {
  $Test
}
$Test = 42
Get-Test

But!!

There is one problem with that approach: It's really hard to track, where the information came from and where it's going to.
Imagine later on you update the script and decide to rename $Test to a name better representing the content, but forget to rename it in the nested function, since you forgot all about that?

Also, imagine your helper function turns out useful and you want to copy it over to another command/script. Now you need to remember exactly about that variable you used or figure out some other way to track this. If instead you had made it a parameter, you'd have a central location to look up what input the command needs:

function Get-Test {
  [CmdletBinding()]
  param (
    $Test
  )
  $Test
}
$Test = 42
Get-Test -Test $Test

I know, it's a slightly greater time investment upfront, but it pays its dividends in easier maintainability for your busy future self.

My rant on this topic is definitely not inspired by me having been hired several times to fix legacy code bases and having to figure out just where the heck that crappy piece of information is coming from and why it sometimes isn't what it should be :grml: ... ;)

[muvijay]

I'm actually stuck with my habit of keeping the code without any flags (of whatever color) telling me "something is not right here" in VS Code. Right now my problem is PSUseDeclaredVarsMoreThanAssignments in similar fashion. I ended up in this post from desperate googling for solutions. I really couldn't understand what is going on here and what can I do to avoid this bug-flag.

$timeConsumption = Measure-Command -Expression { `
     $hashCsv = Test-Fn -HashData $someData }
write-host $hashCsv

[FriedrichWeinmann]

Yeah, I need to patch that rule as well so it can do the same thing with Command Traversal as I did for ReviewUnusedParameter
It's on my backlog, but not in the week before the conference ;)

[Hrxn]

The scriptblocks provided to ForEach-Object & Where-Object do not become part of the code implementing the command - they are code provided to the commands, which the commands then execute for the caller (script). The scriptblocks are not executed within the context of those commands.

But the scriptblocks are passed on to ForEach-Object and Where-Object as arguments, or not? They do net get evaluated, or invoked or something beforehand, or what am I missing here? Granted, they are not in a child scope, they are still in the same (top) scope.

Pipeline usage doesn't really factor in here.

I see, thanks.

There is a commonly propagated best practice where functions should not directly access variables from their parent scopes - all information a command needs should be provided via parameter, all results returned via output.
There is no technical issue with access variables directly from the parent command, so it's not like you are hurting your code with something like this:

Yeah, I get what you mean. And I agree, although there's one possible difference to make here, and that is, like in my ad-hoc testcase here, if one is doing a read-only access into the parent scope. This is supported just fine by Powershell, without needing to use any scope modifiers. But I see the benefit of always using parameters for the inputs of a function etc.

Not sure, I've actually read https://github.com/PoshCode/PowerShellPracticeAndStyle
Maybe it's not really mentioned there, or at least not explicitly (enough).

[FriedrichWeinmann]

This is supported just fine by Powershell, without needing to use any scope modifiers.

Yepp. Technically it works just fine - it's just usually not a great idea (but faster, especially if you need to access a dozen different pieces of data). I'm mostly arguing against expedience in the short term at the cost of long term pain.

But the scriptblocks are passed on to ForEach-Object and Where-Object as arguments, or not?

They are, but they keep their context - so when they get run from inside the command, they are not in the command context.
Otherwise they would be unable to access local variables in your script - those are invisible to ForEach-Object and its friends.
The scope pyramid goes like this:

Golbal > Script > Function in Script

Global > Module > Command of Module

What does NOT happen is this:

Global > Script > Function of Script > Module > Command of Module

The script scope (and the functions in that script) are invisible to any commands from modules you call.

What is more important however from a PSScriptAnalyzer perspective is the Abstract Syntax Tree - the structured view on the code as processed by the parser. There is a huge difference between a scriptblock and a function definition there. Making sure we do a proper, reliable detection is a lot more iffy when we have to traverse into a FunctionDefinitionAst.
Not saying it can't be done, but it would cost more performance and would require more coding, testing and troubleshooting.

And that to support/enable a practice I really don't want to encourage :)

[iRon7]

@FriedrichWeinmann,

It is good to see that this issue is finally fixed in 1.22.0

• PSReviewUnusedParameter: Add CommandsToTraverse option by @FriedrichWeinmann in PSReviewUnusedParameter: Add CommandsToTraverse option #1921 🎉

There is although a quiet similar (I think) issue with PSUseDeclaredVarsMoreThanAssignment, see: #1163 (also note the linked issues.)

Is it possible to apply your experience with this issue on that issue as well?