GeezerVison

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:

   PSScriptAnalyzer/Tests/Rules/AvoidDefaultValueForMandatoryParameter.tests.ps1

   Lines 29 to 34 in ea70855

   	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).

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

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.