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