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