Make Select/ForEach/Where see dynamic properties #6898

jazzdelightsme · 2018-05-18T16:13:59Z

PR Summary

Dynamic (DLR) objects work in some places today, but not others. This
change expands that support to ForEach-Object, Where-Object and the
family of cmdlets that use MshExpression (Select-Object, etc.). (see the
linked issue for the simple repro and explanation: fixes #6567)

This change addresses both wildcard and non-wildcard cases. In wildcard
cases, it uses the existing support of generating PSDynamicMember
objects for names returned by GetDynamicMemberNames.

In non-wildcard cases, a dynamic property access is attempted whether or
not the name shows up in GetDynamicMemberNames, but a truly "blind"
access is only attempted if we see that the base object is an IDMOP. If
the dynamic access fails, you'll get the same or a similar error
experience as before ("The property 'Blarg' cannot be found", or no
error at all, depending on the cmdlet and the strict mode setting).

The included test coverage includes a stub for the .ForEach
operator--once people are happy with this change, I can continue by
adding support there.

This change should allegedly also have positive perf impact, though I
have not tested that assertion.

PR Checklist

jazzdelightsme · 2018-05-28T17:11:56Z

About the CodeFactor checks: it looks like most of these are not caused by my commit (although I exacerbated an existing issue--added another small helper class to a file that already contains multiple classes). What is the strategy for dealing with these? Ignore them? Take an IOU to fix them in a separate commit?

iSazonov · 2018-05-28T17:23:27Z

CodeFactor issues is optional. Please fix only code you created/changed.

lzybkr

Changes look great, just some minor feedback.

It'd be great if you can measure the improvement using dynamic sites.

lzybkr · 2018-05-29T17:11:40Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

Maybe also test *Prop (should be error with -ExpandProperty, maybe also test without -ExpandProperty.)

Good ideas; will do.

lzybkr · 2018-05-29T17:20:28Z

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

Why was the null test added? The C# operator is can be safely used with null values.

D'oh! Thanks.

lzybkr · 2018-05-29T17:25:16Z

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

I know you're not a fan of silent errors, but I think consistency here is important - you wouldn't want an error just because the object implemented IDMOP, e.g. if some object didn't in the past, but a new version does, scripts would break when they silently "worked" before.

The problem here is that I can't tell the difference between "it failed because the property does not exist" (let's call this case 1) and "it failed because accessing it actually threw some exception" (let's call that case 2).

Existing PowerShell behavior is different for these two cases: case 1 gets an error (which is possible because the ETS tells us up front if the property exists or not), and case 2 does not.

For non-IDMOP objects, there's no problem; but for IDMOPs, we have the chance to attempt a "blind" access, but the cost is that we must have the same response to both cases (because we cannot distinguish between the two). So we have to make a choice: we can either swallow ALL errors (including "The property 'Blarg' does not exist"), or expose them all.

I felt like it was more important to preserve the behavior of showing "The property 'Blarg' does not exist" (case 1) errors than to suppress "FooException thrown when accessing Bloop property" (case 2) errors. This decision certainly must be influenced by my predisposition against silent errors, but actually I was going for usability: a typo or otherwise trying to access a property that does not exist is a common mistake, and generally easily fixed when presented with the error, and if I had to pick one of "error suppression" or "tell the user s/he goofed", I'd choose the latter.

What do you think?

(I can at least add a comment about this)

You can make some assumptions though:

non-dynamic members must exist

if GetDynamicMemberNames says the member exists, trust it

Yes--in fact, those are exactly the assumptions I am making: if it had shown up in GetDynamicMemberNames, it would show up as a dynamic member, and isBlindDynamicAccess would be false. (And it would be false for normal properties as well, of course.) I'll add more comments about this.

lzybkr · 2018-05-29T17:28:46Z

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

I don't have any intuition for how often wildcards are used to specify multiple properties, but I know I use them that way for formatting commands.

I'm not sure perf matters much here, but it would be good to understand the impact on something like ps | select *mem*.

For select, the wildcard gets resolved into individual MshExpressionResult objects, so no problems there. So this case here really is for the heterogenous-objects-with-different-matching-names case, which I think is probably a rare enough case not to care too much about (though it will work, of course).

jazzdelightsme · 2018-06-03T03:22:40Z

Regarding the perf:

Ultimately the property access itself is such a small part of what goes on that I found it difficult to find any difference at all.

Here's what I did:

I added a switch to the code, [danthom]::IsDynamicStuffEnabled, to allow me to switch back and forth between the old and new behavior at runtime. Then I had a script to create an array of objects (normal ones, not dynamic) to run through a pipeline, and then had a for loop to repeatedly run the pipeline.

There was enough variability that I tried to eliminate the effect of GCs (by using [gc]::TryStartNoGCRegion), but that only worked for fairly small amounts of work (a handful of seconds), and besides, how much garbage each method produced would actually be an interesting and material effect on perf, so I just printed out the number of GCs along with the times.

I tested where, select, and foreach-object, with and without wildcards (and for select, with wildcards to match multiple properties), and couldn't see much difference between them. The dynamic stuff seemed like it might be slightly faster (and generated a little less garbage--perhaps that's what made the difference).

I tried adding artificial magnification--doing either the dynamic access or normal access in a for loop--but even that didn't make much of a difference. But in general, the dynamic access seemed ever-so-slightly faster, probably because it usually generated a little less garbage. (And as a result of my GC-consciousness, I also switched the DynamicPropertyGetter class to be a struct.)

iSazonov · 2018-06-03T04:21:58Z

test/powershell/Add-TestDynamicType.ps1

Please put the comment on separate line.

iSazonov · 2018-06-03T04:23:18Z

test/powershell/Add-TestDynamicType.ps1

I'd move the line to line 50 - it is more clear.

iSazonov · 2018-06-03T04:25:24Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

Please use full names instead of aliases like -EA.

iSazonov · 2018-06-03T04:27:25Z

test/powershell/Add-TestDynamicType.ps1

We don't use "m" prefix - right pattern is _serialNumber.

iSazonov · 2018-06-03T04:29:12Z

test/powershell/Add-TestDynamicType.ps1

Can we make it static?

iSazonov · 2018-06-03T04:32:00Z

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs

Please move to line 135.

Please address the comment.

if ((x == null) && (target.BaseObject is System.Dynamic.IDynamicMetaObjectProvider))

iSazonov · 2018-06-03T04:36:23Z

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

Maybe throw here with user friendly message then in line 413 with standard null exception?

I'm not going to change this; it's totally unrelated to my change.

I leave this open - we should fix this in the PR or later.

iSazonov · 2018-06-03T04:40:02Z

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

Please use C# 7 syntax

if ( ex is MethodException mex ...

This code is also unrelated to my change (harder to tell for this one, but it's just moved a bit).

iSazonov · 2018-06-03T04:43:08Z

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

Please remove the 3 comments - modern editors work well with large files.

Please remove the unneeded comments.

iSazonov · 2018-06-03T04:45:36Z

test/powershell/Modules/Microsoft.PowerShell.Core/Where-Object.Tests.ps1

Please remove. We can put the code in the file. Or in HelpCommon module.

iSazonov · 2018-06-03T04:46:01Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

iSazonov · 2018-06-03T04:46:27Z

test/powershell/engine/ETS/Adapter.Tests.ps1

iSazonov · 2018-06-03T09:05:56Z

test/powershell/Modules/Microsoft.PowerShell.Core/Where-Object.Tests.ps1

The module is auto-loaded. Please remove the line.

iSazonov · 2018-06-03T09:06:57Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1

iSazonov · 2018-06-03T09:07:31Z

test/powershell/engine/ETS/Adapter.Tests.ps1

Dynamic (DLR) objects work in some places today, but not others. This change expands that support to ForEach-Object, Where-Object and the family of cmdlets that use MshExpression (Select-Object, etc.). (see the linked issue for the simple repro and explanation: fixes PowerShell#6567) This change addresses both wildcard and non-wildcard cases. In wildcard cases, it uses the existing support of generating PSDynamicMember objects for names returned by GetDynamicMemberNames. In non-wildcard cases, a dynamic property access is attempted whether or not the name shows up in GetDynamicMemberNames, but a truly "blind" access is only attempted if we see that the base object is an IDMOP. If the dynamic access fails, you'll get the same or a similar error experience as before ("The property 'Blarg' cannot be found", or no error at all, depending on the cmdlet and the strict mode setting). The included test coverage includes a stub for the .ForEach operator--once people are happy with this change, I can continue by adding support there. This change should allegedly also have positive perf impact, though in actual perf testing, although it does seem ever-so-slightly faster, I found it difficult to measure much difference at all.

TravisEz13 · 2018-06-05T00:09:35Z

@lzybkr Can you update your review?

jazzdelightsme · 2018-06-07T00:41:35Z

Thanks for the reviews, people! @TravisEz13 what is the process for getting this merged? Is there any transparency to how you track proposed merges, or is it a "it happens when it happens" sort of thing?

I don't mean to be impatient; I'm just wondering how it works, and I want to make sure there aren't any extra steps I need to do or anything like that.

iSazonov · 2018-06-07T03:36:13Z

@jazzdelightsme MSFT team has announced that they is actively working on porting Windows PowerShell modules with minimal activity on GitHub. So now some PRs is waiting reviews from MSFT experts.

daxian-dbw · 2018-06-08T19:16:00Z

Hey @jazzdelightsme, thanks for your contribution and patience!
Can you please push another commit with the [feature] tag at the beginning of the commit message? That way it will trigger the daily build run which runs more tests. I want the chagnes to be validated by the daily test run. Thanks!

jazzdelightsme · 2018-06-11T15:39:56Z

Can you please push another commit with the [feature] tag at the beginning of the commit message?

Did it work (to trigger the extra testing)?

TravisEz13 · 2018-06-11T22:06:22Z

@jazzdelightsme The feature tag did work.

daxian-dbw

LGTM.
@jazzdelightsme I only have one comment about a comment in the code.
I can update the comment for you if you are OK with what I suggest.

daxian-dbw · 2018-06-12T18:46:42Z

src/System.Management.Automation/FormatAndOutput/common/Utilities/Mshexpression.cs


+                if ((x == null) && (target.BaseObject is System.Dynamic.IDynamicMetaObjectProvider))
+                {
+                    // We could check if GetDynamicMemberNames includes the name...  but


About this comment, we have already checked GetDynamicMemberNames when reaching here (target.Members includes the dynamic members returned from GetDynamicMemberNames). So maybe we can change comment to: GetDynamicMemberNames does not return this name, but GetDynamicMemberNames is only a hint ...

Your comment is not quite correct. (Maybe it should be correct and there is another bug; I'm not sure what the intended behavior is.)

If you iterate through .Members you will indeed see the dynamic members. However, if you do .Members[propName], where propName is the name of one of those dynamic members... you will get null back.

Sample output:

PS C:\src\PowerShell> $myObj.PSObject.Members | where Name -eq FooProp MemberType : Dynamic Value : TypeNameOfValue : dynamic Name : FooProp IsInstance : True PS C:\src\PowerShell> $myObj.PSObject.Members['FooProp'] PS C:\src\PowerShell> $myObj.PSObject.Members['FooProp'] -eq $null True PS C:\src\PowerShell>

I'm not sure if that's a bug or By Design. Because you can't do much with PSDynamicMembers (for instance, the getter/setter just throws), maybe it is intentionally excluded when indexing directly in. But it does seem odd.

I see. That does seem odd. I will look into it tomorrow. Thanks for bringing it up!

This is because DotNetAdapter.GetMember<T>(object obj, string memberName) doesn't return dynamic members. I think it makes sense to not return dynamic member from this method, and submit #7087 to clarify it in the code comments.

iSazonov · 2018-06-13T10:51:21Z

@jazzdelightsme Thanks for the great contribution!

jazzdelightsme requested review from BrucePay and daxian-dbw as code owners May 18, 2018 16:13

iSazonov requested a review from lzybkr May 19, 2018 19:54

lzybkr suggested changes May 29, 2018

View reviewed changes

iSazonov reviewed Jun 3, 2018

View reviewed changes

jazzdelightsme requested review from TravisEz13 and adityapatwardhan as code owners June 3, 2018 07:20

iSazonov reviewed Jun 3, 2018

View reviewed changes

jazzdelightsme force-pushed the user/danthom/PwshHeartsIDMOP branch from 14349eb to 58ebb74 Compare June 4, 2018 14:40

lzybkr approved these changes Jun 5, 2018

View reviewed changes

TravisEz13 assigned daxian-dbw Jun 8, 2018

[feature] CR feedback: remove unnecessary Import-Module

460beb8

jazzdelightsme force-pushed the user/danthom/PwshHeartsIDMOP branch from 58ebb74 to 460beb8 Compare June 9, 2018 16:38

jazzdelightsme requested a review from anmenaga as a code owner June 9, 2018 16:38

daxian-dbw approved these changes Jun 12, 2018

View reviewed changes

daxian-dbw merged commit ce3cb68 into PowerShell:master Jun 13, 2018

jazzdelightsme deleted the user/danthom/PwshHeartsIDMOP branch June 13, 2018 14:31

daxian-dbw mentioned this pull request Jun 15, 2018

Comment about dynamic members for the DotNetAdapter 'GetMember' and 'GetMembers' #7087

Merged

11 tasks

daxian-dbw mentioned this pull request Jul 18, 2018

The latest daily throws Cannot create boxed ByRef-like values when accessing $OutputEncoding #7301

Closed

Make Select/ForEach/Where see dynamic properties #6898

Make Select/ForEach/Where see dynamic properties #6898

Uh oh!

Conversation

jazzdelightsme commented May 18, 2018 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

jazzdelightsme commented May 28, 2018

Uh oh!

iSazonov commented May 28, 2018

Uh oh!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzdelightsme commented Jun 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzdelightsme commented May 18, 2018 •

edited by daxian-dbw

Loading

daxian-dbw Jun 13, 2018 •

edited

Loading