PSReviewUnusedParameter has false positive when using $MyInvocation.BoundParameters

[jegannathanmaniganadan]

Steps to reproduce

There may be some rare cases where we would be using ParameterSetName instead of ParameterName, for example

$sb = {
        function Test-PSReviewUnusedParameter {
        <#
        #>
        [CmdletBinding()]
        Param(
            [Parameter(ParameterSetName = 'Foo')]
            [switch]
            $FooBar
        )

        try{
            if ($PSCmdlet.ParameterSetName -eq 'Foo') {
                # I am doing something
            }
        } catch {
            throw
        }
    }
}

# Invoke-ScriptAnalyzer -ScriptDefinition [scriptblock]$sb

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSReviewUnusedParameter             Warning                 9     The parameter 'FooBar' has been declared but not used.

Expected behavior

I expect this to be not flagged

Actual behavior

Flags it as violation

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      5.1.18362.752
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.18362.752
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

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

[bergmeister]

I don't see this as a false positive. You are not using the FooBar variable. All you do is check whether a certain parameter set is being used, which is different to using the parameter itself. You should be using $FooBar.IsPresent in your case.
Do you agree @Jawz84?

[jegannathanmaniganadan]

Hey @bergmeister , Hope you are doing good.

You are not using the FooBar variable. All you do is check whether a certain parameter set is being used, which is different to using the parameter itself.

It is a switch, so I may never use that parameter and go with just parameterset name. You may wonder why would someone do this. Imagine a function that calls another function with bound parameter, like someone splat $PSBoundParameters to another function.

@mattpwhite

[Jawz84]

I think pssa is doing exactly what can be expected from a static analysis tool; warn for suspect situations. Because the scenario you describe can not be statically analyzed in Powershell, I think there is little that could be done. This is my humble opinion and I may be wrong.

[bergmeister]

@manigandan-jegannathan-developer I still don't understand why you cannot check the parameter directly, even when splatting with PSBoundParameters in a nested function. Please provide a full (but simple) example where using the parameter directly is not working as expected.

[jegannathanmaniganadan]

I appreaicte the effort on writing this rule, I am sure it would help many out there. I am trying to add my inputs so that we can improve this rule further.

About that PSBoundParameters, I did not actually test it, but this rule seems to be working fine when PSBoundParameters used, I mean it did not flag. But it flags if $MyInvocation.BoundParameters used ?

For example,

$sb = {
    function Get-ContentUMyFunc {
        <#
        #>
        [CmdletBinding()]
        Param(
            [Parameter()]
            [string]
            $Path
        )

        try{
            $bp = $MyInvocation.BoundParameters
            Get-Content @bp
        } catch {
            throw
        }
    }
}

# Invoke-ScriptAnalyzer -ScriptDefinition [scriptblock]$sb | ft -a

RuleName                Severity ScriptName Line Message
--------                -------- ---------- ---- -------
PSReviewUnusedParameter Warning             9    The parameter 'Path' has been declared but not used.

I still don't understand why you cannot check the parameter directly

I can definitely use that parameter directly. I just wanted to note that it may flag when someone uses ParameterSetName (Also I do not see anything wrong in using ParameterSetName instead of Parameter). I feel its a false positive in my opinion. We can recommend Suppressing such violations, but I am afraid that would be a bad habit . We are suppressing a rule violation because of a bug in the rule.

Also I just happened to notice that it flags if the parameter is being used in different runspace.

$sb = {
        function Test-PSReviewUnusedParameter {
        <#
        #>
        [CmdletBinding()]
        Param(
            [Parameter(ParameterSetName = 'Foo')]
            [string]
            $FooBar
        )

        try{
            Invoke-Command -Session $someSession -ScriptBlock {
                $Using:FooBar
            }
        } catch {
            throw
        }
    }
}

# Invoke-ScriptAnalyzer -ScriptDefinition [scriptblock]$sb | ft -a

RuleName                Severity ScriptName Line Message
--------                -------- ---------- ---- -------
PSReviewUnusedParameter Warning             9    The parameter 'FooBar' has been declared but not used.

[bergmeister]

@manigandan-jegannathan-developer Aha, I see, if you use $PSBoundParameters directly instead of $MyInvocation.BoundParameters, then the rule is already setup to not warn. That's because of this code here: https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/ReviewUnusedParameter.cs#L49
That following line could probably be enhanced to include $MyInvocation.BoundParameters as well, I'd like to double check with another expert, whether using the property on $MyInvocation is equivalent in terms of scoping.

[bergmeister]

I just checked with someone and $MyInvocation.BoundParameters should be equivalent to $PSBoundParameters, therefore, the following line should be enhanced to include this variable as well: https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/ReviewUnusedParameter.cs#L49

[rjmholt]

That following line could probably be enhanced to include $MyInvocation.BoundParameters as well, I'd like to double check with another expert, whether using the property on $MyInvocation is equivalent in terms of scoping.

@JamesWTruher do you know if this is the case?

I think pssa is doing exactly what can be expected from a static analysis tool; warn for suspect situations. Because the scenario you describe can not be statically analyzed in Powershell, I think there is little that could be done. This is my humble opinion and I may be wrong.

I would like to echo this sentiment. This doesn't necessarily mean that we shouldn't do something like turn off the rule when particular variables are used, but PowerShell leans heavily on dynamic features and I would prefer not to create the expectation that static analysis can do it all.

In this case, it seems reasonable that we would detect a common scenario like this and disengage the rule, although we should document cases where the rule gives up.

[bergmeister]

I would like to echo this sentiment. This doesn't necessarily mean that we shouldn't do something like turn off the rule when particular variables are used, but PowerShell leans heavily on dynamic features and I would prefer not to create the expectation that static analysis can do it all.

Yh, I agree, hence why I marked it as an enhancement instead of a bug. The enhancement is relatively easy, therefore it's also marked as up for grabs.

We should look into a generic solution/improvement to the scoping issue though around child scriptblocks from which a few rules suffer and is probably the number one user annoyance in terms of false positives.

[jegannathanmaniganadan]

I can take this up, but I never made any contribution to PSSA. Any advise or sample PR fixing similar small issue would be helpful

[bergmeister]

Just look at the line that I linked before: https://github.com/PowerShell/PSScriptAnalyzer/blob/master/Rules/ReviewUnusedParameter.cs#L49
You could do a check if variableCount contains MyInvocation and if so, do another search on scriptBlockAst to get the VariableExpressionAst of MyInvocation out again and do a recursive .FindAll search on it to get its child element of type StringConstantExpressionAst out and check if it is BoundParameters.
The readme has instructions on local development setup here or alternatively you can just use Visual Studio Codespaces or the remote Containers feature in VS-Code that will set everything up to work out of the box. Once you're setup, just call ./build.ps1 and the built module will be in a local folder called out. Familiarity with C# development and AST is assumed, the ShowPSAst is quite useful to visualize the PowerShell AST. At the start of June, I will give a talk at the online and free PSConfEU 2020 where I will demo how to build/debug PSSA as well and you will be able to watch that video on-demand: https://powershell.one/psconfeu/psconf.eu-2020/about

[jegannathanmaniganadan]

Cool, Thanks. I will submit a PR. Thanks @bergmeister

[jegannathanmaniganadan]

do another search on scriptBlockAst to get the VariableExpressionAst of MyInvocation out again and do a recursive .FindAll search on it to get its child element of type StringConstantExpressionAst out and check if it is BoundParameters.

I am afraid whether that would work. BoundParameters is not a child element of MyInvocation (VariableExpressionAst). Its a member of andMemberExpressionAst, where the expression is VariableExpressionAst. Both of them are child elements of MemberExpressionAst. Can you tell me if I am missing anything here ? Also Show-Ast is really useful.

[bergmeister]

do another search on scriptBlockAst to get the VariableExpressionAst of MyInvocation out again and do a recursive .FindAll search on it to get its child element of type StringConstantExpressionAst out and check if it is BoundParameters.

I am afraid whether that would work. BoundParameters is not a child element of MyInvocation (VariableExpressionAst). Its a member of andMemberExpressionAst, where the expression is VariableExpressionAst. Both of them are child elements of MemberExpressionAst. Can you tell me if I am missing anything here ? Also Show-Ast is really useful.

Yh, you're right, it looked like a child to me first but is actually at the same level as VariableExpressionAst. I guess the best is to then search for VariableExpressionAst, get the parent MemberExpressionAst by using the .Parent property and from that element do something like a .FindAll(oneAst -> oneAst is StringConstantExpressionAst, searchNestedScriptBlocks: false) search to get the StringConstantExpressionAst that you could then check for the BoundParameters property. That's just a rough idea that should get you there. Have a look at the properties and methods of the individual Ast implementations (Intellisense or docs) that can sometimes be helpful in the AST search as well.

[rjmholt]

{ param($x) $MyInvocation.BoundParameters }.Ast.FindAll({ $args[0] -is [System.Management.Automation.Language.MemberExpressionAst] -and $args[0].Expression -is [System.Management.Automation.Language.VariableExpressionAst] -and $args[0].Expression.VariablePath.UserPath -eq 'MyInvocation' -and $args[0].Member -is [System.Management.Automation.Language.StringConstantExpressionAst] -and $args[0].Member.Value -eq 'BoundParameters' }, $true)

[jegannathanmaniganadan]

Thanks @rjmholt, that is exactly what I am doing.

[jegannathanmaniganadan]

I have the changes ready, waiting for my firm to approve my OSS contribution.

[jegannathanmaniganadan]

I have submitted a PR fixing this issue. It occurred to me that it should not flag $PScmdlet.MyInvocation.PSBoundParameters as well. So I made required changes to handle all three cases,

• PSBoundParameters
• MyInvocation.BoundParameters
• PSCmdlet.MyInvocation.BoundParameters