Mock loses parameter defaults, ArgumentCompleter and custom attributes

[nohwnd]

Mocks do not inherit parameter default values, so the mock body sees 0 where the real function sees 3. Same for ArgumentCompleter and custom validation attributes, those are dropped.

BeforeAll {
    function Get-Thing {
        param(
            [int] $Count = 3,
            [string] $Mode = 'fast'
        )
        "real Count=$Count Mode=$Mode"
    }
    function Invoke-Caller { Get-Thing }   # no arguments, relies on defaults
}

Describe 'defaults' {
    It 'mock body sees the default value' {
        Mock Get-Thing { "mock Count=$Count Mode=$Mode" }
        Invoke-Caller | Should -Be 'mock Count=3 Mode=fast'
    }

    It 'ParameterFilter can match on a defaulted parameter' {
        Mock Get-Thing { 'x' }
        Invoke-Caller
        Should -Invoke Get-Thing -Times 1 -ParameterFilter { $Count -eq 3 }
    }
}

Both fail on 6.1.0:

[-] mock body sees the default value
 Expected: 'mock Count=3 Mode=fast'
 But was:  'mock Count=0 Mode='

[-] ParameterFilter can match on a defaulted parameter
 Expected Get-Thing to be called at least 1 time, but was called 0 times
 Performed invocations:
   [ ] Get-Thing

The second one is the worse of the two. The call is recorded but the filter never matches, so you get "called 0 times" and go looking for a bug in your own code.

Why

We build the mock bootstrap function from [CommandMetadata]::new($cmd) and [ProxyCommand]::GetParamBlock($meta). That regenerates the param block from resolved metadata, and the regeneration drops things.

	real function	GetParamBlock
default values (= 3)	kept	dropped
ArgumentCompleter	kept	dropped
custom ValidateArgumentsAttribute	kept, enforced	dropped, not enforced
Parameter / Alias / ValidateRange / ValidateScript	kept	kept

What I would try

Copy the param block from the AST instead of regenerating it from metadata. ParamBlock.Copy() keeps everything verbatim and never resolves attribute type names.

$ast  = (Get-Command Get-Thing).ScriptBlock.Ast
$body = [System.Management.Automation.Language.Parser]::ParseInput(
    'end { "mock Count=$Count Mode=$Mode" }', [ref]$null, [ref]$null).EndBlock.Copy()

$sb = [System.Management.Automation.Language.ScriptBlockAst]::new(
    $ast.Extent, $ast.Body.ParamBlock.Copy(), $null, $null, $body, $null).GetScriptBlock()

${function:global:AstMock} = $sb

Get-Thing   # real Count=3 Mode=fast
AstMock     # mock Count=3 Mode=fast

That fixes all four, defaults, ArgumentCompleter, custom attributes and the enforcement of those attributes. It also composes with what we already do for module scoping, I set SessionStateInternal on the new scriptblock the same way Set-ScriptBlockScope does and the swapped body runs inside the module.

Three things I ran into while testing it:

• [CmdletBinding()] and [OutputType()] are not in ParamBlock.Extent.Text, they sit on ParamBlock.Attributes. Copy() carries them, string composition does not, you silently lose SupportsShouldProcess that way.
• You have to drop the original Begin / Process / End blocks. If you copy ProcessBlock along with ParamBlock then the original body runs too.
• Compiled cmdlets have no AST, Get-ChildItem is a CmdletInfo with a null ScriptBlock. So this covers functions only and we keep the metadata path for cmdlets and binary commands.

What I did not verify

I could not reproduce #1772 with this. That one needs the attribute type to be unresolvable at mock creation time, my repro resolved fine and both routes worked. The AST route does skip the CommandMetadata conversion that the error in #1772 names, but I have not proven it fixes that case.

I also did not check how many of our own tests depend on the current behaviour. That number decides whether this is a fix or a breaking change.

@fflaten what do you think, is there a reason we go through CommandMetadata that I am forgetting? #339 and #2154 look like they could have the same root cause.

🤖

[nohwnd]

Note on keeping this backward compatible, because a verbatim AST copy would change behaviour for existing tests.

We deliberately modify the metadata today in four places, and three of them are public behaviour we cannot lose:

• Repair-ConflictingParameters renames parameters that collide with automatic variables, $PSEdition becomes $_PSEdition with PSEdition added as an alias. The list is $script:ConflictingParameterNames. A verbatim copy would define $PSEdition as a parameter and break.
• -RemoveParameterType sets the type to [object] and drops PSTypeNameAttribute.
• -RemoveParameterValidation drops ValidateArgumentsAttribute, and forwards the names to Get-MockDynamicParameter so it reaches dynamic parameters too (Mock Command with dynamic ValidateSet #1557).
• Repair-EncodingParameters relaxes [System.Text.Encoding] to [object] (Mocking Out-File in PowerShell 7.* Produces Error #1877). That one only exists because GetParamBlock cannot reproduce the internal transformation attribute, and it only applies to cmdlets, so it is not affected either way.

The first three are done by mutating CommandMetadata, which is a mutable property bag. AST nodes are immutable, so on the AST route we have to rebuild the ParameterAst nodes instead of mutating them. That is more code, but it is the same three operations, rename, retype, drop attributes.

Cmdlets have no AST anyway, so we keep the metadata path for them. Only functions change route, which means Repair-EncodingParameters and the cmdlet dynamicparam path stay exactly as they are today.

The part that actually breaks people is the extra fidelity, not the repairs:

• Validation we silently drop today would start being enforced. A test that calls a mock with a value the real command's custom validator rejects passes today, and would throw after the change.
• Defaults appearing changes what the mock body and -ParameterFilter see. Anyone who worked around this bug by writing -ParameterFilter { $Count -eq 0 } would have that filter stop matching.

So I would not just switch the route. Either put the fidelity behind a config option for one release with the default off, or take it in a major and write it up in the release notes. Running our own suite on the AST route first tells us which one it is, if it is a handful of tests it is a fix, if it is a lot it is a breaking change.

🤖