◐ Shell
clean mode source ↗

PSAvoidDefaultValueForMandatoryParameter: Fix param block and parameter set handling by liamjpeters · Pull Request #2121 · PowerShell/PSScriptAnalyzer

liamjpeters

PR Summary

While looking into issue #1623, I noticed that the PSAvoidDefaultValueForMandatoryParameter rule has several issues. This PR attempts to resolve them and increase test coverage.

Observed issues are:

  1. Rule doesn't flag for parameters which are part of a param block not nested within a function. So the below .ps1 file contents are not flagged:

    [CmdletBinding()]
Param
(
    [Parameter(Mandatory)]
    $Parameter1 = 'default Value'
)

  2. If a parameter is used in multiple parametersets, only being mandatory in one of them, the rule still flags the parameter if it has a default value. (As raised in issue AvoidDefaultValueForMandatoryParameter mistakenly reported. #1623)

    function Test
{
    [CmdletBinding()]
    Param
    (
        [Parameter(Mandatory, ParameterSetName = "Mandatory")]
        [Parameter(ParameterSetName = "Optional")]
        $Parameter1 = 'default Value'
    )
}

  3. When checking whether a parameter is mandatory, the rule does not validate that mandatory is a named argument of a Parameter Attribute. So the below is flagged:

    function Test {
    [CmdletBinding()]
    Param
    (
        [MyAttribute(Mandatory)]
        $Parameter1 = 'default Value'
    )
}

  4. The below test case is faulty. The tested script definition should cause a violation and isn't due to an issue:

    Context "When there are no violations" {
    It "has 1 provide default value for mandatory parameter violation" {
    $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$false)]$Param1=''val1'', [Parameter(Mandatory)]$Param2=''val2'', $Param3=''val3'') }' |
    Where-Object { $_.RuleName -eq $ruleName }
    $violations.Count | Should -Be 0
    }

    I believe any parameter in the block having Mandatory=$false causes the rule to break for subsequent parameters in the block.

Fixes #1623

The performance of the old rule and new rule are comparable, whilst flagging more cases. This is running the old test file (correcting for the erroneous test) 1,000 times. Times for each run reported by Pester (Duration.TotalMilliseconds field).

Average (ms) Min (ms) Max (ms)
Old Rule 108.68 92.74 174.55
New Rule 105.28 85.46 195.63

Note: Caching of the string comparer suggested by copilot as an improvement. Not sure on the effectiveness of this change. Function comment-based help also written mostly by copilot - but reading it, it seems sensible.

PR Checklist