Skip to content

Conversation

@dlwyatt
Copy link
Contributor

@dlwyatt dlwyatt commented Oct 13, 2017

Expands on a previous PR based on conversation here: #2038 (comment)

Unwrapping of ValueFromRemainingArguments was being performed only for object[] arrays (which covers the most common PowerShell binding scenarios), but could be skipped if a collection of any other type were passed to the parameter.

// we unwrap our List, but only if there is a single argument of type object[].
if (valueFromRemainingArguments.Count == 1 && valueFromRemainingArguments[0] is object[])
// we unwrap our List, but only if there is a single argument which is a collection.
if (valueFromRemainingArguments.Count == 1 && null != LanguagePrimitives.GetEnumerable(valueFromRemainingArguments[0]))
Copy link
Member

@daxian-dbw daxian-dbw Oct 13, 2017

Choose a reason for hiding this comment

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

I suggest using LanguagePrimitives.IsTypeEnumerable(Type) for 2 reasons:

  1. LanguagePrimitives.GetEnumerable unwrap PSObject, which might strip off ETS properties associated with the PSObject. This, in fact, shouldn't be a concern as we will do parameter binding using the original object.
  2. LanguagePrimitives.GetEnumerable actually calls the GetEnumerable delegate which may throw an exception or cause a long delay (e.g. fetching object from Azure).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@daxian-dbw daxian-dbw self-assigned this Oct 13, 2017
@dlwyatt
Copy link
Contributor Author

dlwyatt commented Oct 13, 2017

Actually, I don't like how that was changed. Sec for another commit.

@daxian-dbw
Copy link
Member

How about changing IsTypeEnumerable to return false when the passed-in type is null? Then the change could become

if (valueFromRemainingArguments.Count == 1 && LanguagePrimitives.IsTypeEnumerable(valueFromRemainingArguments[0]?.GetType()))

@dlwyatt
Copy link
Contributor Author

dlwyatt commented Oct 13, 2017

That's part of what I'm doing, but I also want the code from GetEnumerable that checks for PSObject / BaseObject to run.

@dlwyatt
Copy link
Contributor Author

dlwyatt commented Oct 13, 2017

I added a new LanguagePrimitives.IsObjectEnumerable method to do this, and refactored some duplicated code to a new internal method. Let me know if this is acceptable, since it's making a bit of new public API in LanguagePrimitives.

Type objectType = GetBaseObjectType(obj);
if (objectType == null) { return null; }
GetEnumerableDelegate getEnumerable = GetOrCalculateEnumerable(objectType);
return getEnumerable(obj);
Copy link
Member

@daxian-dbw daxian-dbw Oct 13, 2017

Choose a reason for hiding this comment

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

This is not right. Before the change, the obj passed in getEnumerable is the base object if the original one is PSObject, but now it will be the PSObject in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, you're right. Fixing now.

/// </param>
[SuppressMessage("Microsoft.Naming", "CA1720:IdentifiersShouldNotContainTypeNames", MessageId = "obj", Justification = "Since V1 code is already shipped, excluding this message.")]
public static IEnumerable GetEnumerable(object obj)
internal static Type GetBaseObjectType(object obj)
Copy link
Member

Choose a reason for hiding this comment

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

It's recommended to always start from private, since currently nothing outside the type depends on it.

@daxian-dbw
Copy link
Member

As for the new public API LanguagePrimitives.IsObjectEnumerable(object), it could be useful. I will let @PowerShell/powershell-committee review it.

@daxian-dbw daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 13, 2017
[SuppressMessage("Microsoft.Naming", "CA1720:IdentifiersShouldNotContainTypeNames", MessageId = "obj", Justification = "Since V1 code is already shipped, excluding this message.")]
public static IEnumerable GetEnumerable(object obj)
{
obj = GetBaseObject(obj);
Copy link
Member

Choose a reason for hiding this comment

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

How about replace GetBaseObject(object) with PSObject.Base(object)? It seems a good fit here.

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 just knew there was already going to be a method for this somewhere in the code. :) I searched for .BaseObject and didn't find it; Base() uses the field name instead.

@daxian-dbw
Copy link
Member

Need @PowerShell/powershell-committee to review the new public API public static bool IsObjectEnumerable(object obj) in LanguagePrimitives.

Just noticed the PR #5123 was submitted to always unwrap single argument for ValueFromRemainingArguments. @dlwyatt thoughts?

@dlwyatt
Copy link
Contributor Author

dlwyatt commented Oct 16, 2017

I don't think I like that other PR. It's changed Write-Output's parameter from an array type to a single PSObject (which may be undesirable by itself); ValueFromRemainingArguments should really only be used on array parameters, and the change to the parameter binder is to try to shoehorn it into use on a non-array type.

@dlwyatt
Copy link
Contributor Author

dlwyatt commented Oct 16, 2017

The issue #5122 is interesting, though. I'm not sure whether to consider it a bug or not; we knew that the ValueFromRemainingArguments fix was a breaking change.

@dlwyatt
Copy link
Contributor Author

dlwyatt commented Oct 16, 2017

I suppose it doesn't matter if the change in #5123 is made to the binder, though. If you assume that ValueFromRemainingArguments should only be used for array type parameters, then it's no harm done; the non-collection argument will be converted back into an array anyway.

If ValueFromRemainingArguments should be usable on non-array parameters, then the change in #5123 has value. However, I can't think of any case where that would work other than [object] and [psobject]. Otherwise it's just going to fail when you do pass in multiple arguments to that parameter. I've only used it like "params" in C#, and that only works for arrays.

@PetSerAl
Copy link
Contributor

@dlwyatt ValueFromRemainingArguments was never prohibited for non-array types. All you need is to specify a conversion:

PS> class MyClass {
>>>     $a; $b; $c
>>>     MyClass([Object]$Object) {
>>>         $this.a, $this.b, $this.c = $Object
>>>     }
>>> }
PS> function f { param( [Parameter(ValueFromRemainingArguments)][MyClass] $Class ) $Class }
PS> f 1 2 3

a b c
- - -
1 2 3


PS> f (f 1 2 3)

a       b c
-       - -
MyClass

So, if array pass thru, when passed as single argument, why MyClass should not?

@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this and on the topic of a single element array being a scalar, our decision is that it is simpler for the developer to always expect an array rather than having code handle both an array and a scalar, so it should be an array

@SteveL-MSFT 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 Oct 19, 2017
@daxian-dbw
Copy link
Member

daxian-dbw commented Oct 19, 2017

Nothing stops a user from applying ValueFromRemainingArguments on non-array type parameters, but I don't think that's the right way to use the attribute. I agree with @dlwyatt that ValueFromRemainingArguments should be similar to params in C#, which means when it gets to the point to call BindParameter, the provided argument should always be a collection.

Actually, the changes in this PR cannot guarantee the RemainingArguments binding step to always try binding a collection object to the target parameter because of the IsObjectEnumerable check. The remaining argument could be a single custom object that happens to implement IEnumerable interface, in which case this single object will be used to bind to the target parameter. I'd love to have a more restricted check that makes sure it's a collection/list, but as long as we can agree that "use ValueFromRemainingArguments only for array type parameter" is a guideline/best practice, then we should be good.

@daxian-dbw
Copy link
Member

@lzybkr Could you please weigh in? Your thoughts would be helpful.

@lzybkr
Copy link
Contributor

lzybkr commented Oct 19, 2017

There are a couple ways to think about this.

In C#, it just doesn't make sense for params to not be an array - otherwise you need annoying code testing the type of the argument.

In PowerShell, that's somewhat less of an issue, the pipeline doesn't care, and since V3, many other places handle a single instance an array of objects in a somewhat similar way.

On the other hand, we do far too little checking on the use of attributes and if they make sense. I'm all for adding more errors on inappropriate use of an an attribute.

One more random thought - not necessarily a good idea, but the only case that truly needs to be a breaking change if we want an error - if the parameter type is explicitly specified and is not object or PSObject or an array of anything. For singular object or PSObject, the cmdlet must handle arrays anyway or it's broken, so there is little risk in always creating an array. I suspect there is very little code out there that explicitly uses a parameter type constraint that isn't object, PSObject, or an array with ValueFromRemainArguments.

@daxian-dbw
Copy link
Member

Given the review from powershell committee, I will merge this PR. But please feel free to open an issue and continue the discussion.

@daxian-dbw daxian-dbw merged commit 1c469aa into PowerShell:master Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants