-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix Parameter Binder bug with Advanced Functions and ValueFromRemainingArguments #2038
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1684,33 +1684,31 @@ private void HandleRemainingArguments() | |
| var cpi = CommandParameterInternal.CreateParameterWithArgument( | ||
| PositionUtilities.EmptyExtent, varargsParameter.Parameter.Name, "-" + varargsParameter.Parameter.Name + ":", | ||
| argumentExtent, valueFromRemainingArguments, false); | ||
|
|
||
| // To make all of the following work similarly (the first is handled elsewhere, but second and third are | ||
| // handled here): | ||
| // Set-ClusterOwnerNode -Owners foo,bar | ||
| // Set-ClusterOwnerNode foo bar | ||
| // Set-ClusterOwnerNode foo,bar | ||
| // we unwrap our List, but only if there is a single argument of type object[]. | ||
| if (valueFromRemainingArguments.Count == 1 && valueFromRemainingArguments[0] is object[]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PetSerAl - good question, I think the intent was all arrays, though @dlwyatt , @daxian-dbw - thoughts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like you're right. Most of the time, PowerShell's building Rather than
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm putting together new tests for this and will follow up with a second PR |
||
| { | ||
| cpi.SetArgumentValue(UnboundArguments[0].ArgumentExtent, valueFromRemainingArguments[0]); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| BindParameter(cpi, varargsParameter, ParameterBindingFlags.ShouldCoerceType); | ||
| } | ||
| catch (ParameterBindingException pbex) | ||
| { | ||
| // To make all of the following work similarly (the first is handled elsewhere, but second and third are | ||
| // handled here): | ||
| // Set-ClusterOwnerNode -Owners foo,bar | ||
| // Set-ClusterOwnerNode foo bar | ||
| // Set-ClusterOwnerNode foo,bar | ||
| // we make one additional attempt at converting, but only if there is a single argument of type object[]. | ||
| if (valueFromRemainingArguments.Count == 1 && valueFromRemainingArguments[0] is object[]) | ||
| if (!DefaultParameterBindingInUse) | ||
| { | ||
| cpi.SetArgumentValue(UnboundArguments[0].ArgumentExtent, valueFromRemainingArguments[0]); | ||
| BindParameter(cpi, varargsParameter, ParameterBindingFlags.ShouldCoerceType); | ||
| throw; | ||
| } | ||
| else | ||
| { | ||
| if (!DefaultParameterBindingInUse) | ||
| { | ||
| throw; | ||
| } | ||
| else | ||
| { | ||
| ThrowElaboratedBindingException(pbex); | ||
| } | ||
| ThrowElaboratedBindingException(pbex); | ||
| } | ||
| } | ||
| UnboundArguments.Clear(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -228,7 +228,7 @@ | |
| [CmdletBinding()] | ||
| param ( | ||
| [array]$Parameter1, | ||
| [int[]]$Parameter2 | ||
| [int[]]$Parameter2 | ||
| ) | ||
|
|
||
| Process { | ||
|
|
@@ -329,4 +329,98 @@ | |
| $result | Should Be $expected | ||
| } | ||
| } | ||
|
|
||
| Context "ValueFromRemainingArguments" { | ||
| BeforeAll { | ||
| function Test-BindingFunction { | ||
| param ( | ||
| [Parameter(ValueFromRemainingArguments)] | ||
| [object[]] $Parameter | ||
| ) | ||
|
|
||
| return [pscustomobject] @{ | ||
| ArgumentCount = $Parameter.Count | ||
| Value = $Parameter | ||
| } | ||
| } | ||
|
|
||
| # Deliberately not using TestDrive:\ here because Pester will fail to clean it up due to the | ||
| # assembly being loaded in our process. | ||
|
|
||
| if ($IsWindows) | ||
| { | ||
| $tempDir = $env:temp | ||
| } | ||
| else | ||
| { | ||
| $tempDir = '/tmp' | ||
| } | ||
|
||
|
|
||
| $dllPath = Join-Path $tempDir TestBindingCmdlet.dll | ||
|
|
||
| Add-Type -OutputAssembly $dllPath -TypeDefinition ' | ||
| using System; | ||
| using System.Management.Automation; | ||
|
|
||
| [Cmdlet("Test", "BindingCmdlet")] | ||
| public class TestBindingCommand : PSCmdlet | ||
| { | ||
| [Parameter(Position = 0, ValueFromRemainingArguments = true)] | ||
| public string[] Parameter { get; set; } | ||
|
|
||
| protected override void ProcessRecord() | ||
| { | ||
| PSObject obj = new PSObject(); | ||
|
|
||
| obj.Properties.Add(new PSNoteProperty("ArgumentCount", Parameter.Length)); | ||
| obj.Properties.Add(new PSNoteProperty("Value", Parameter)); | ||
|
|
||
| WriteObject(obj); | ||
| } | ||
| } | ||
| ' | ||
|
|
||
| Import-Module $dllPath | ||
| } | ||
|
|
||
| AfterAll { | ||
| Get-Module TestBindingCmdlet | Remove-Module -Force | ||
| } | ||
|
|
||
| It "Binds properly when passing an explicit array to an advanced function" { | ||
| $result = Test-BindingFunction 1,2,3 | ||
|
|
||
| $result.ArgumentCount | Should Be 3 | ||
| $result.Value[0] | Should Be 1 | ||
| $result.Value[1] | Should Be 2 | ||
| $result.Value[2] | Should Be 3 | ||
| } | ||
|
|
||
| It "Binds properly when passing multiple arguments to an advanced function" { | ||
| $result = Test-BindingFunction 1 2 3 | ||
|
|
||
| $result.ArgumentCount | Should Be 3 | ||
| $result.Value[0] | Should Be 1 | ||
| $result.Value[1] | Should Be 2 | ||
| $result.Value[2] | Should Be 3 | ||
| } | ||
|
|
||
| It "Binds properly when passing an explicit array to a cmdlet" { | ||
| $result = Test-BindingCmdlet 1,2,3 | ||
|
|
||
| $result.ArgumentCount | Should Be 3 | ||
| $result.Value[0] | Should Be 1 | ||
| $result.Value[1] | Should Be 2 | ||
| $result.Value[2] | Should Be 3 | ||
| } | ||
|
|
||
| It "Binds properly when passing multiple arguments to a cmdlet" { | ||
| $result = Test-BindingCmdlet 1 2 3 | ||
|
|
||
| $result.ArgumentCount | Should Be 3 | ||
| $result.Value[0] | Should Be 1 | ||
| $result.Value[1] | Should Be 2 | ||
| $result.Value[2] | Should Be 3 | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't been changed even if the fix to parameter binder is accepted. it will break the following scenario:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's tricky. In fact I'd say it's been a bug in
Write-Outputall along.Either way,
InputObjectis a 2-element array, and each element also happens to be a 2-element array. What I would expect to happen here is what happens after this PR: You send two objects down the pipeline, both of which are 2-element arrays. If you added the-NoEnumerateswitch, then you would send one object down the pipeline, which is a 2-element array containing two other 2-element arrays. (In fact, with the -NoEnumerate switch, you should only ever send one object, an array, down the pipeline, which is sort of the point.)Today,
Write-Output aa,bb cc,ddinstead sends 4 strings down the pipeline. It enumerates two levels deep instead of one. If you use the-NoEnumerateswitch on that command today, you get two 2-element arrays going down the pipeline (what I'd expect to see without-NoEnumerate).Without this change, several other
Write-Outputtests failed when I ranStart-PSPester. With this change, everything was green, but apparently that just means that the test coverage isn't good enough right now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the results of
Write-Output.Tests.ps1with the binder fix but without the change to Write-Output:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man, what a mess. Here's some of Write-Output's behavior today:
This is a symptom of the flawed binding logic I changed (assuming the first call to BindParameter would fail when the
a,b,csyntax was used), and the workaround that was created for it in Write-Output. Whatever the correct behavior is, there should never be a difference between these two calls when ValueFromRemainingArguments is in effect:The values bound to the parameter should be identical in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible to fix the binding logic without somehow changing (breaking or fixing, depending on your point of view) the behavior of Write-Output. The whole point of the binding fix is to ensure consistent behavior between those two syntax options I listed, and it happens before the cmdlet ever gets to look at the values.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case, the first call to
BindParameteris successful, because the parameter type ofInputObjectisPSObject[], and type conversion fromlist<object[]>to it would be successful.IMHO, this behavior of
Write-Outputis pretty intuitive -- when-NoEnumerateis specified, give me back what ever items I gave you. In case ofa b c, I gave you 3 items, so 3 items should be returned byWrite-Output; in case ofa,b,c, I gave you 1 item, which is an array, so the same item should be returned back.However, it gets confusing when
-InputObjectis explicitly specified:This is the part I don't like about
ValueFromRemainingArgument, but changing it would definitely break something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with that.
ValueFromRemainingArgumentsis just syntax sugar, similar to aparamsparameter in C#. You gave 3 objects in both versions of the command.