@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)

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.)

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

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

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

@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?

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

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!

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

@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?

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.

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.

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: ... ;)

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

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 ;)

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).

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 :)

@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?

I'm curious what opinion people have of scriptblock's GetNewClosure() method. In other languages closures are fairly standard.

I'm approaching a problem where a user may need to specify a subset of actions and reorder them. In python, I've handled this by assigning functions to a hashtable, validating user input and indexing the hashtable to invoke the function. A very simplified Powershell version of this would look like this

param($Foo = 'Foo', $Bar = 'Bar', $Actions = @('DoFoo', 'DoBar'))

$ActionsTable = @{}
$ActionsTable['DoFoo'] = { Write-Host $Foo }.GetNewClosure()
$ActionsTable['DoBar'] = { Write-Host $Bar }.GetNewClosure()

foreach($Action in $Actions) {
    $ActionsTable[$Action].Invoke()
}

From my perspective, setting aside best practices, the variable is being used here. A param block to could be added to those scriptblocks and the values passed via Invoke but then I have to check each action to pass a subset of arguments to each one which isn't really necessary. There are probably better examples than mine though. All that said, I have no idea how difficult it would be to capture that programmatically so if not supporting this use case is a technical issue I can certainly understand.