Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSReviewUnusedParameter generates a warning parameter if we use $MyInvocation.MyCommand #1575

Closed
aikiox opened this issue Aug 25, 2020 · 3 comments

Comments

@aikiox
Copy link

aikiox commented Aug 25, 2020

Steps to reproduce

In a function, I am using $MyInvocation.MyCommand to retrieve the parameters in order to generate an SQL query.

Run Invoke-ScriptAnalyzer against the following with the new 1.19.1 release.

function Format-ConditionQuery ($parameters) {
    $Condition = $null
    $ExcludeList = 'Verbose', 'Debug', 'ErrorAction', 'WarningAction', 'InformationAction', 'ErrorVariable', 'WarningVariable', 'InformationVariable', 'OutVariable', 'OutBuffer', 'PipelineVariable'
    foreach ($Params in ($parameters | Where-Object { $_ -notin $ExcludeList })) {
        $GetContentOnParam = $null
        $GetContentOnParam = Get-Variable $Params
        if ($GetContentOnParam.Value) {
            $Value = $GetContentOnParam.Value.tostring().replace("'", "''")
            $operator = ' = '
            $conditionOperator = " where "
            if ($condition) {
                $conditionOperator = " and "
            }
            $Condition += (" {0} [{1}] {2} '{3}' " -f $conditionOperator, $Params, $operator, $Value )
        }
    }
    return $condition
}
Function foo  {
    Param
    (
        [Parameter(Position = 0)]
        $id,
        [Parameter(Position = 1)]
        $name
    )

    Begin {

    }
    Process {
        $Condition = ''
        $Condition = Format-ConditionQuery -parameters ($MyInvocation.MyCommand).parameters.Keys
        if ($Condition) {
            $Condition
        }
        else {
            Write-Error -Message "No condition generate"
        }
    }
    End {

    }
}
foo -name "ok"

Expected behavior

No rule violations.

Actual behavior

The new PSReviewUnusedParameter rule doesn't notice the usage.

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSReviewUnusedParameter             Warning      TryAnalyse 23    The parameter 'id' has been declared but not used.
                                                 .ps1
PSReviewUnusedParameter             Warning      TryAnalyse 25    The parameter 'name' has been declared but not used.        
                                                 .ps1

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      5.1.18362.145
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.18362.145
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.1
1.19.0
@ghost ghost added the Needs: Triage 🔍 label Aug 25, 2020
@SydneyhSmith
Copy link
Collaborator

Thanks @aikiox looks like you are using a paren expression with .parameters rather than .boundparameters, it you are actually not using the values of the parameters, just the name of the parameters...the rule is written for boundparameters (see #1520), additionally the use of parens complicates things in an unexpected way for PSSA.
In other words, the rule is behaving as expected because the value of the parameters is not being used.

With the .parameters you will be passing along all of the parameters, with .boundparameters you will pass just those that you are using (so you will not need the exclude list).

Let us know if we maybe missed something in your script that explains this use case. Thanks!

@rjmholt
Copy link
Contributor

rjmholt commented Aug 25, 2020

additionally the use of parens complicates things in an unexpected way for PSSA

To be clear, I think there's a case to be made that not identifying BoundParameters through parens is a bug.

With that said, PSScriptAnalyzer is a static analyser, and just cannot pick up all possible runtime occurrences. In fact there are plenty of cases where it's impossible to detect ahead of time whether behaviour is correct or not. In those situations, it's up to the user to say "I know what I'm doing" and suppress the relevant diagnostic.

The important parts here I think that make this something the rule can't be expected to check are:

  • The use of $MyInvocation.MyCommand.Parameters, which is a static list of all parameters, rather than just the bound ones
  • The use of Get-Variable, which is a runtime/dynamic variable discovery mechanism
  • The dependency on dynamic scope with Get-Variable to look up the call stack for the right parameter value. There's no way to know in general what variables will be defined from a function's perspective due to dynamic scope, and PSScriptAnalyzer's rule just try to encourage less dependency on that (because it's hard to reason about).

In the case of this script, I think it's fair that diagnostics are issued; generally that's a warning that your script is hard to reason about. I think an appropriate refactor would use $PSBoundParameters, which this rule is specifically implemented to recognise.

@aikiox
Copy link
Author

aikiox commented Aug 25, 2020

Hello @SydneyhSmith and @rjmholt,,
Thank you for your very enlightening answers. Indeed, I suspect that PSScriptAnalyzer does not handle all cases and my outcome was more like "Hey, I meet case, have you thought about it or do you have another way of doing it?"
And you answered my question :)
I have reviewed the code and it no longer returns a warning. I did not know the existence of $PSBoundParameters which perfectly meets my need and much more as mentioned @SydneyhSmith (the exclusion list).

Thanks for your help and sorry for the inconvenience.
You are doing a great job!
Here is the modified code:

function Format-ConditionQuery ($parameters) {
    $Condition = $null
    foreach ($Params in $parameters.GetEnumerator()) {
        if ($Params.Value) {
            $Value = $Params.Value.replace("'", "''")
            $operator = ' = '
            $conditionOperator = " where "
            if ($condition) {
                $conditionOperator = " and "
            }
            $Condition += (" {0} [{1}] {2} '{3}' " -f $conditionOperator, $Params.key, $operator, $Value )
        }
    }
    return $condition
}
Function foo {
    Param
    (
        [Parameter(Position = 0)]
        $id,
        [Parameter(Position = 1)]
        $name
    )

    Begin {
    }
    Process {
        $Condition = ''
        $Condition = Format-ConditionQuery -parameters $PSBoundParameters
        if ($Condition) {
            $Condition
        }
        else {
            Write-Error -Message "No condition generate"
        }
    }
    End {

    }
}

foo -name "ok" -id 'aa'

Thanks !

@aikiox aikiox closed this as completed Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants