Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,8 @@ protected override void ProcessRecord()
{
enumerate = false;
}
foreach (PSObject inputObject in _inputObjects) // compensate for ValueFromRemainingArguments
{
WriteObject(inputObject, enumerate);
}

WriteObject(_inputObjects, enumerate);
Copy link
Member

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:

Write-Output aa,bb cc,dd

Copy link
Contributor Author

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-Output all along.

Write-Output aa,bb cc,dd

# is equivalent to

Write-Output -InputObject @( @('aa', 'bb'), @('cc', 'dd') )

Either way, InputObject is 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 -NoEnumerate switch, 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,dd instead sends 4 strings down the pipeline. It enumerates two levels deep instead of one. If you use the -NoEnumerate switch 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-Output tests failed when I ran Start-PSPester. With this change, everything was green, but apparently that just means that the test coverage isn't good enough right now.

Copy link
Contributor Author

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.ps1 with the binder fix but without the change to Write-Output:

Describing Write-Output DRT Unit Tests
 [-] Simple Write Object Test 1.28s
   Expected: {4}
   But was:  {6}
   at line: 5 in C:\Users\dlwya_000\Documents\GitHub\powershell\test\powershell\Modules\Microsoft.PowerShell.Utility\Write-Output.Tests.ps1
   5:         $results.Length | Should Be $objectWritten.Length
 [-] Works with NoEnumerate switch 144ms
   Expected: {1 2.2 System.Object[] abc}
   But was:  {1}
   at line: 19 in C:\Users\dlwya_000\Documents\GitHub\powershell\test\powershell\Modules\Microsoft.PowerShell.Utility\Write-Output.Tests.ps1
   19:         Write-Output $objectWritten -NoEnumerate 6>&1 | Should be  '1 2.2 System.Object[] abc'
Describing Write-Output
   Context Input Tests
    [+] Should allow piped input 335ms
    [+] Should write output to the output stream when using piped input 23ms
    [+] Should use inputobject switch 25ms
    [+] Should write output to the output stream when using inputobject switch 38ms
    [+] Should be able to write to a variable 21ms
   Context Pipeline Command Tests
    [+] Should send object to the next command in the pipeline 69ms
    [+] Should have the same result between inputobject switch and piped input 23ms
   Context Enumerate Objects
    [+] Should see individual objects when not using the NoEnumerate switch 62ms
    [-] Should be able to treat a collection as a single object using the NoEnumerate switch 32ms
      Expected: {1}
      But was:  {3}
      at line: 71 in C:\Users\dlwya_000\Documents\GitHub\powershell\test\powershell\Modules\Microsoft.PowerShell.Utility\Write-Output.Tests.ps1
      71:           $singleCollection | Should Be 1
Tests completed in 2.06s
Passed: 8 Failed: 3 Skipped: 0 Pending: 0

Copy link
Contributor Author

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:

 write-output -NoEnumerate a b c | % { $_.GetType().FullName; $_.Count; $_ -join '/' }
System.String
1
a
System.String
1
b
System.String
1
c

 write-output -NoEnumerate a,b,c | % { $_.GetType().FullName; $_.Count; $_ -join '/' }
System.Object[]
3
a/b/c

This is a symptom of the flawed binding logic I changed (assuming the first call to BindParameter would fail when the a,b,c syntax 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:

Do-Something a b c
Do-Something a,b,c

The values bound to the parameter should be identical in both cases.

Copy link
Contributor Author

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.

Copy link
Member

@daxian-dbw daxian-dbw Aug 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write-Output a,b,c
This is a symptom of the flawed binding logic I changed (assuming the first call to BindParameter would fail when the a,b,c syntax was used)

in this case, the first call to BindParameter is successful, because the parameter type of InputObject is PSObject[], and type conversion from list<object[]> to it would be successful.

write-output -NoEnumerate a b c | % { $_.GetType().FullName; $_.Count; $_ -join '/' }
System.String
1
a
System.String
1
b
System.String
1
c

write-output -NoEnumerate a,b,c | % { $_.GetType().FullName; $_.Count; $_ -join '/' }
System.Object[]
3
a/b/c

IMHO, this behavior of Write-Output is pretty intuitive -- when -NoEnumerate is specified, give me back what ever items I gave you. In case of a b c, I gave you 3 items, so 3 items should be returned by Write-Output; in case of a,b,c, I gave you 1 item, which is an array, so the same item should be returned back.

However, it gets confusing when -InputObject is explicitly specified:

PS D:\> Write-Output -InputObject a,b,c -NoEnumerate | % { $_.GetType().FullName; $_.Count; $_ -join '/' }
System.String
1
a
System.String
1
b
System.String
1
c
PS D:\> Write-Output a,b,c -NoEnumerate | % { $_.GetType().FullName; $_.Count; $_ -join '/' }
System.Object[]
3
a/b/c

This is the part I don't like about ValueFromRemainingArgument, but changing it would definitely break something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of a b c, I gave you 3 items, so 3 items should be returned by Write-Output
in case of a,b,c, I gave you 1 item,

I disagree with that. ValueFromRemainingArguments is just syntax sugar, similar to a params parameter in C#. You gave 3 objects in both versions of the command.

}//processrecord
}//WriteOutputCommand
#endregion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be valueFromRemainingArguments[0]?.GetType() == typeof(object[]) or valueFromRemainingArguments[0] is Array?
Given array covariance in .NET, any array of reference type (like string[]) will pass is object[] check, but not array of value type (like int[]). Should it be only object[] or all arrays instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PetSerAl - good question, I think the intent was all arrays, though object[] in this context is nearly the same thing.

@dlwyatt , @daxian-dbw - thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're right. Most of the time, PowerShell's building object[] arrays from arguments at that point, but if you explicitly make a variable of another type and pass it in, it doesn't get unwrapped.

Rather than is Array, though, I'd recommend using LanguagePrimitives.GetEnumerable(valueFromRemainingArguments[0]) . That will cover other types of collections using the same rules that you'd expect from PowerShell (not treating strings / dictionaries as collections, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@
[CmdletBinding()]
param (
[array]$Parameter1,
[int[]]$Parameter2
[int[]]$Parameter2
)

Process {
Expand Down Expand Up @@ -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'
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use '$TestDrive' instead of '$env:temp' and '/tmp'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a comment explaining why that doesn't work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Please clarify - Pester will fail to clean up or Pester will fail all test batch?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use explicit assembly?
Can we use:

Add-Type -TypeDefinition $a -PassThru | ForEach-Object {$_.assembly} | Import-module -Force 


$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
}
}
}