Allow explicitly specified named parameter to supersede the same one from hashtable splatting #13162

daxian-dbw · 2020-07-12T23:45:46Z

PR Summary

Allow explicitly specified named parameter to supersede the same one from hashtable splatting.
The work is done in parameter binder, so that parameters can be resolved to cover a parameter's official name, alias name, and unambiguous partial prefix name.

The changes covers covers Hashtable splatting in 3 scenarios:

Cmdlet or advanced script invocation;
Simple function invocation;
ScriptBlock.GetPowerShell(...), where the script block contains command invocation only and uses Hashtable splatting.

Some code refactoring is done to ParameterBinderController to avoid redundant code being duplicated in CmdletParameterBinderController and ScriptParameterBinderController.

Breaking Change

This change introduces a minor breaking change to how Hashtable splatting works with simple functions when it contains key/value pairs that not a named parameters of the function. (no breaking change to cmdlet/advanced function)

With this change, the named parameters from Hashtable splatting will be moved to the end of the parameter/argument list, so as to late bind them after all explicitly specified named parameters are bound. Parameter binding for simple functions won't throw error when a specified named parameter cannot be found, but stuff the unknown named parameters to the $args of the simple function. Since the named parameters from Hashtable splatting are moved to the end of the argument list, the order it appears in args will be different, also, other explicitly specified parameters for a simple function gets bound earlier.

For example:

function SimpleTest {
    param(
        $Name,
        $Path
    )
    "Name: $Name; Path: $Path; Args: $args"
}

$hash = @{ Name = "Hello"; Blah = "World" }
SimpleTest @hash "MyPath"

Current Behavior

## 'MyPath' is not bound to `-Path` because it's the third argument in the argument list.
## So it ends up being stuffed into '$args' along with `Blah = "World"`

PS> SimpleTest @hash "MyPath"
Name: Hello; Path: ; Args: -Blah: World MyPath

New Behavior

## The arguments from @hash are moved to the end of the argument list,
## so `MyPath` becomes the first argument in the list and thus bound to '-Path'

PS> SimpleTest @hash "MyPath"
Name: Hello; Path: MyPath; Args: -Blah: World

I think the breaking change is in Bucket 3 Unlikely Grey Area because:

the current behavior is confusing because how the user explicitly specified arguments after the splatted Hashtable depends on the order of them in the argument list after expanding the Hashtable.
it should be rare for user to call a simple function with Hashtable splatting which contains keys that are not parameter names of the function.
even if (2) happens, say the Hashtable is a config with all possible parameters and an user sends the config to functions, it's very unlikely for $args to be used in a real scenario like this.

PR Checklist

…rs from splatting

iSazonov · 2020-07-13T05:28:03Z

it should be rare for user to call a simple function with Hashtable splatting which contains keys that are not parameter names of the function.

I have a concern about this statement. What if such Hashtable is a config with all possible parameters and an user sends the config to functions?

daxian-dbw · 2020-07-13T13:39:00Z

I have a concern about this statement. What if such Hashtable is a config with all possible parameters and an user sends the config to functions?

For cmdlet/advanced functions, this will fail.
For simple functions, then the user must be ignoring $args in those functions. It's unlikely they are depending on $args in this case at all.

JamesWTruher · 2020-07-15T17:11:55Z

@BrucePay would love your opinion on this behavior

bpayette · 2020-07-15T21:53:24Z

@daxian-dbw you said: "so MyPath becomes the second argument in the list and thus bound". But that's wrong - the splatted value doesn't occupy a parameter position so MyPath should be the first argument.

daxian-dbw · 2020-07-15T22:46:40Z

@bpayette Right, I just corrected that comment. Thanks!

SteveL-MSFT · 2020-07-16T00:48:41Z

@PowerShell/powershell-committee discussed this and would like more community feedback. The disagreement was on whether the splatted hashtable order matters. In one case, the order dictates who overrides:

$hash = @{ Name = 'hello' }
test-function @hash -Name World
test-function -Name World @hash

If the ordering matters, then in the first case you get Name = World and the second case results in Name = hello. However, if order does not affect parameter binding then in both cases Name = World.

The argument for ordering is that it's been a useful construct in bash where latter parameters win so you can use it to override parameters declared in aliases.

The argument against is that ordering (ignoring positional parameters) has never mattered in PowerShell so this is introducing a new concept and may confuse users.

vexx32 · 2020-07-16T02:27:00Z

Ordering shouldn't matter. As @daxian-dbw it simply increases fragility and massively decreases readability if order matters for this.

With the knowledge that order doesn't matter, I can confidently state that Some-Command -ThisParam $value @otherParams will definitely have the expected value for -ThisParam without ever looking at the $otherParams hashtable. That's powerful.

If order mattered, you would have to always keep track of if the parameter is before or after, and if it's before the hashtable you then have to look up whether the param you're worried about is in the hashtable. Way less obvious what's going on.

Also, the hashtable is much more likely to be the reused element there, not the individual command call statement. I think it stands to reason that the more deliberate statement (assuming reuse of hashtable means that the params it contains are probably more general than just the one command call) with the lesser scope of effect should take precedence.

iSazonov · 2020-07-16T03:45:28Z

Ordering shouldn't matter.

I agree. It seems it is not PowerShell native behavior to depend on order of named parameters.

iSazonov · 2020-07-16T03:49:23Z

Also if we consider -Force switch I think users would expect the priority of this parameter regardless of its location.

AspenForester · 2020-07-16T16:53:55Z

It seems to me that this behavior of overriding a splat is a natural follow-on from $PSDefaultParameterValues that can be overridden by providing parameters... Like @SteveL-MSFT just said as I was about to click on the Comment button.

markdomansky · 2020-07-16T18:01:40Z

The argument for ordering is that it's been a useful construct in bash where latter parameters win so you can use it to override parameters declared in aliases.

This argument would extend that parameter behavior should be this way in all cases; so Test -Name A -Name B would not error and would output B. It doesn't seem that's intended, desired, or included in this change. Also, aliases in PS don't support parameters so people aren't leveraging that ability today. I would not support this. I want the error.

The explicit parameter should override anything splatted. It's clearer, and more natural.

As a personal preference, I generally splat at the end. Parameters and variables get long, so hitting the @ means I'm done with the line and know what's happening. With order-matters, I (and presumably others) would have to alter their coding practice to leverage this. That's the breaking change with order-matters. If you splat first today, there's no change in behavior/code; if you splat late, it does.

rkeithhill · 2020-07-19T02:33:44Z

It seems it is not PowerShell native behavior to depend on order of named parameters.

Agreed.

SteveL-MSFT · 2020-07-22T22:05:25Z

@PowerShell/powershell-committee reviewed this, we agree that within PowerShell ordering has not mattered, so we should continue that. On the breaking change for SimpleFunctions, parameters have always been implicitly positional, so the new change is accepted.

ghost · 2020-07-31T02:00:26Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

src/System.Management.Automation/engine/hostifaces/Parameter.cs

src/System.Management.Automation/engine/CmdletParameterBinderController.cs

src/System.Management.Automation/engine/ParameterBinderController.cs

src/System.Management.Automation/engine/hostifaces/PSCommand.cs

PaulHigin

LGTM

test/powershell/engine/ParameterBinding/Splatting.Tests.ps1

iSazonov · 2020-08-05T05:06:21Z

src/System.Management.Automation/engine/CmdletParameterBinderController.cs

+        /// and then bind the argument to the parameter.
        /// </summary>
-        /// <param name="parameterSets">
-        ///     The parameter set used to bind the arguments.
-        /// </param>
-        /// <param name="arguments">
-        ///     The arguments that should be attempted to bind to the parameters of the specified
-        ///     parameter binder.
-        /// </param>
-        /// <exception cref="ParameterBindingException">
-        /// if multiple parameters are found matching the name.
-        /// or
-        /// if no match could be found.
-        /// or
-        /// If argument transformation fails.
-        /// or
-        /// The argument could not be coerced to the appropriate type for the parameter.
-        /// or
-        /// The parameter argument transformation, prerequisite, or validation failed.
-        /// or
-        /// If the binding to the parameter fails.
-        /// </exception>
-        private Collection<CommandParameterInternal> BindParameters(uint parameterSets, Collection<CommandParameterInternal> arguments)
+        protected override void BindNamedParameter(


(As side note, there is a full mess with 'bind the argument' vs 'bind the parameter' in comments and method names.)

When talking about parameter binding, it should be bind an argument to a parameter, an argument is bound (to a parameter), a parameter is bound (by an argument).

iSazonov · 2020-08-05T06:00:08Z

Great work!

sdwheeler · 2020-08-10T03:45:24Z

@daxian-dbw Should this be assigned to a milestone?

ghost · 2020-08-17T22:21:40Z

🎉v7.1.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

daxian-dbw added 5 commits July 3, 2020 22:31

Splatting initial commit

72a9a3a

Make explicitly specified named parameter supersede the name paramete…

252bd05

…rs from splatting

Generalize to simple functions

1281217

Make it work for hashtable splatting that happens in 'GetPowerShell'

b06caf8

Add tests

7a3e789

ghost assigned adityapatwardhan Jul 12, 2020

daxian-dbw requested a review from adityapatwardhan July 13, 2020 00:23

daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jul 13, 2020

joeyaiello mentioned this pull request Jul 15, 2020

PS Community Call for July 16, 2020 PowerShell/PowerShell-RFC#257

Closed

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jul 22, 2020

daxian-dbw requested review from JamesWTruher, PaulHigin, SteveL-MSFT and rjmholt July 23, 2020 16:37

ghost added the Review - Needed The PR is being reviewed label Jul 31, 2020

Add more tests for steppable pipeline

8adc77e

rjmholt approved these changes Aug 3, 2020

View reviewed changes

src/System.Management.Automation/engine/hostifaces/Parameter.cs Outdated Show resolved Hide resolved

Address Rob's comment

4af4617

PaulHigin suggested changes Aug 3, 2020

View reviewed changes

Address Paul's comments

40dc192

PaulHigin approved these changes Aug 3, 2020

View reviewed changes

iSazonov reviewed Aug 4, 2020

View reviewed changes

Address Ilya's comments

1ed1df4

daxian-dbw mentioned this pull request Aug 4, 2020

Explicitly specified named arguments will supersede the same ones from a Hashtable splatting MicrosoftDocs/PowerShell-Docs#6431

Closed

Address Ilya's comment on '-BeOffType'

31e431a

iSazonov reviewed Aug 5, 2020

View reviewed changes

iSazonov approved these changes Aug 5, 2020

View reviewed changes

iSazonov merged commit 63cf0c3 into PowerShell:master Aug 5, 2020

daxian-dbw deleted the splatting branch August 5, 2020 06:24

daxian-dbw added this to the 7.1.0-preview.6 milestone Aug 10, 2020

ghost mentioned this pull request Aug 17, 2020

Allow explicitly specified named parameter to supersede the one from hashtable splatting #13114

Closed

iRon7 mentioned this pull request May 21, 2025

Common -Splat parameter #25574

Closed

Allow explicitly specified named parameter to supersede the same one from hashtable splatting #13162

Allow explicitly specified named parameter to supersede the same one from hashtable splatting #13162

Uh oh!

Conversation

daxian-dbw commented Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Breaking Change

PR Checklist

Uh oh!

iSazonov commented Jul 13, 2020

Uh oh!

daxian-dbw commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesWTruher commented Jul 15, 2020

Uh oh!

bpayette commented Jul 15, 2020

Uh oh!

daxian-dbw commented Jul 15, 2020

Uh oh!

SteveL-MSFT commented Jul 16, 2020

Uh oh!

vexx32 commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jul 16, 2020

Uh oh!

iSazonov commented Jul 16, 2020

Uh oh!

AspenForester commented Jul 16, 2020

Uh oh!

markdomansky commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkeithhill commented Jul 19, 2020

Uh oh!

SteveL-MSFT commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 5, 2020

Uh oh!

sdwheeler commented Aug 10, 2020

Uh oh!

ghost commented Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

daxian-dbw commented Jul 12, 2020 •

edited

Loading

daxian-dbw commented Jul 13, 2020 •

edited

Loading

vexx32 commented Jul 16, 2020 •

edited

Loading

markdomansky commented Jul 16, 2020 •

edited

Loading

SteveL-MSFT commented Jul 22, 2020 •

edited

Loading