Skip to content
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

The PSObject implementation does not handle classes derived from DynamicObject properly #6567

Closed
jazzdelightsme opened this issue Apr 5, 2018 · 10 comments · Fixed by #6898
Closed
Labels
Issue-Bug Issue has been identified as a bug in the product Resolution-Fixed The issue is fixed. WG-Engine core PowerShell engine, interpreter, and runtime

Comments

@jazzdelightsme
Copy link
Contributor

jazzdelightsme commented Apr 5, 2018

This repro’s on 5.1 and 6. The problem is that if I have a DynamicObject that has a dynamic property “Foo”, things like $obj.Foo work, but Select-Object -ExpandProperty Foo do not.

Steps to reproduce

Add-Type @"
using System;
using System.Collections.Generic;
using System.Dynamic;

public class MyDynamicObject : DynamicObject
{
    public override IEnumerable<string> GetDynamicMemberNames()
    {
        return new string[] { "Foo" };
    }

    public override bool TryGetMember( GetMemberBinder binder, out object result )
    {
        result = null;
        bool succeeded = false;

        if( binder.Name == "Foo" )
        {
            result = 123;
            succeeded = true;
        }

        return succeeded;
    }
}
"@

$myObj = [MyDynamicObject]::new()

$myObj.Foo                                  # <-- works
$myObj | %{ $_.Foo }                        # <-- works
$myObj | Select-Object -ExpandProperty Foo  # <-- DOES NOT WORK

Expected behavior

The last three lines should all work (yield 123).

Actual behavior

The Select-Object statement throws. ("Select-Object : Property "Foo" cannot be found.")

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      6.1.0-preview.1
PSEdition                      Core
GitCommitId                    v6.1.0-preview.1
OS                             Microsoft Windows 10.0.16299
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
@BrucePay
Copy link
Collaborator

BrucePay commented Apr 5, 2018

This is not specific to select. For example:

PS[1] (459) > $myObj | foreach { $_.Foo }
123
PS[1] (460) > $myObj | foreach Foo
PS[1] (461) >

It looks like we aren't handling DynamicObject in the PSObject wrapper. Foo doesn't show up as a property:

PS[1] (467) > $myObj.PSObject.Properties
PS[1] (468) >

@BrucePay BrucePay added Issue-Bug Issue has been identified as a bug in the product WG-Engine core PowerShell engine, interpreter, and runtime labels Apr 5, 2018
@BrucePay BrucePay changed the title Select-Object -ExpandProperty does not like dynamic (DLR) properties The PSObject implementation does not handle classes derived from DynamicObject properly Apr 5, 2018
@BrucePay
Copy link
Collaborator

BrucePay commented Apr 5, 2018

The original title was a bit confusing given that PowerShell, from V3 on, uses the DLR extensively. I've updated it to more accurately reflect what I think the problem is. @jazzdelightsme - are you ok with the updated title?

@jazzdelightsme
Copy link
Contributor Author

Sure. Thank you!

@jazzdelightsme
Copy link
Contributor Author

PSDynamicMember appears to be a mostly-useless stub: the Value getter/setters just throw, and it doesn't even hold a reference to the owning object. It seems like the solution will involve teaching this thing how to get a value... first I suppose we need to be able to determine if the "Foo" member even is a property or not (maybe it's a method). Then, if it's a property, we have to figure out if it is settable and/or gettable. @lzybkr / @BrucePay, is that possible with some DLR-fu?

Looking at the DLR stuff is ... intimidating. It's hard to tell where to grab on first. Any hints?

@lzybkr
Copy link
Member

lzybkr commented Apr 19, 2018

PSDynamicMember exists just for Get-Member - and the results of Get-Member don't reference an owning object.

I believe this is the code that is supposed to cover your scenario. It was likely tested (I don't typically write code like that without testing) so I'm not sure why it doesn't work - but I'd start there.

@jazzdelightsme
Copy link
Contributor Author

Thanks @lzybkr!

I believe that code does in fact work. That's why things like $myObj.Foo work: PSObject dynamically binds the member access, and all is well.

But for Select-Object -ExpandProperty, on the other hand... that code does not come into play.

When using wildcards (like $myObj | select -exp *Foo*), DotNetAdapter.AddAllDynamicMembers correctly finds the "Foo" member, and adds a PSDynamicMember to the collection, so the Match method, when it calls GetIntegratedMembers, has the "Foo" member in the list.

However, the Match method is instructed to only return members of type Properties or PropertySet (as specified by MshExpression.ResolveNames(PSObject, bool)), but the "Foo" member is a Dynamic, so it gets filtered out.

For the non-wildcard case (like $myObj | select -exp Foo), ResolveNames just looks for target.Members[_stringValue] (where _stringValue is "Foo"), and dynamic members don't seem to show up in that PSMemberInfoIntegratingCollection.

If dynamic members were to be exposed to ResolveNames (via Members.Match or Members[string indexer], though... perhaps things could work. But wouldn't PSDynamicMember would have to built up a bit, for that to work? And I'm not sure if PSDynamicMember can be taught to do what it needs to do.

If that's not the right approach... then what will
MshExpression.GetValue do? (if the dynamic member does not show up in Properties?)

@lzybkr
Copy link
Member

lzybkr commented Apr 19, 2018

Ah, of course. That's the problem then. The PSObject pseudo-reflection like api can't work for dynamic objects, at least not in general.

The GetDynamicMemberNames api that a dynamic object implements is not a contract. For example, a dynamic object could synthesize a property just by trying to access it - essentially giving the object infinite members. The primary use case for the api is a debugger, or in PowerShell's case, Get-Member - in other words - for a somewhat improved UI, but nothing more.

I believe the right fix is to use a dynamic site in the same way that PowerShell does for the $obj.member syntax.

This problem affects Select-Object, ForEach-Object, Sort-Object, Measure-Object, Where-Object, Format-List, Format-Table and maybe more.

It would seem this scenario is a corner case - it has never worked and this is the first report I've seen. That said, fixing it has a nice benefit - performance should improve even when not using dynamic objects.

A fix does need to be careful though - some of those commands accept wildcards for property names. It might be best to keep the existing code for any use of wildcards.

@jazzdelightsme
Copy link
Contributor Author

Okay, it may be beyond my ken, but I'll buy you lunch, @lzybkr, if you are willing to talk me through what you think the "right" change would look like...

@lzybkr
Copy link
Member

lzybkr commented Apr 26, 2018

No need to buy me lunch, but I'm happy to chat.

To get you started - you can find similar code here.

The difference is - you'll use PSGetMemberBinder.Get("PropertyName"). It really does just boil down to a couple of lines of code - creating the call site with the right binder, then invoking the delegate for that site.

jazzdelightsme added a commit to jazzdelightsme/PowerShell that referenced this issue Apr 27, 2018
This change allows dynamic (DLR) objects to work better with
Select-Object.

Dynamic (DLR) objects work in some places, but not others.
MshExpression is a place where they previously didn't work, and this
change fixes that (see the linked issue for the simple repro and
explanation: fixes PowerShell#6567)

This change addresses both wildcard and non-wildcard cases.

In the non-wildcard case, if the target base object is an IDMOP, we just
attempt a dynamic bind--if it fails, you'll get the same error that you
would have before ("The property 'Blarg' cannot be found").

In the wildcard case, we already call the IDMOP's
`GetDynamicMemberNames` function and match the returned names--this
change just allows us to use those results.

However, this change *only* affects things that use `MshExpression`,
like `Select-Object`. Other cmdlets, such as `Where-Object`, do not use
`MshExpression`, and so have similar problems.

Changing every cmdlet that accesses `.Members[_propName]` in
a way similar to what has been done for `MshExpression` in this commit
would be painful, would be easy to miss in new commands, and wouldn't
help external code that accesses via `.Members`, so I think a better
solution might be to expose the dynamic members as part of the
`.Members` collection directly.
@jazzdelightsme
Copy link
Contributor Author

jazzdelightsme commented Apr 27, 2018

I came up with an PoC (see here) that seems to work well, but it only addresses Select-Object--not other things like Where-Object.

Next I'll try pushing the solution down a bit further, into the .Members collection itself.

jazzdelightsme added a commit to jazzdelightsme/PowerShell that referenced this issue May 11, 2018
This change allows dynamic (DLR) objects to work better with
Select-Object.

Dynamic (DLR) objects work in some places, but not others.
MshExpression is a place where they previously didn't work, and this
change fixes that (see the linked issue for the simple repro and
explanation: fixes PowerShell#6567)

This change addresses both wildcard and non-wildcard cases.

In the non-wildcard case, if the target base object is an IDMOP, we just
attempt a dynamic bind--if it fails, you'll get the same error that you
would have before ("The property 'Blarg' cannot be found").

In the wildcard case, we already call the IDMOP's
`GetDynamicMemberNames` function and match the returned names--this
change just allows us to use those results.

However, this change *only* affects things that use `MshExpression`,
like `Select-Object`. Other cmdlets, such as `Where-Object`, do not use
`MshExpression`, and so have similar problems.

Changing every cmdlet that accesses `.Members[_propName]` in
a way similar to what has been done for `MshExpression` in this commit
would be painful, would be easy to miss in new commands, and wouldn't
help external code that accesses via `.Members`, so I think a better
solution might be to expose the dynamic members as part of the
`.Members` collection directly.
jazzdelightsme added a commit to jazzdelightsme/PowerShell that referenced this issue Jun 4, 2018
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.
@iSazonov iSazonov added the Resolution-Fixed The issue is fixed. label Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Issue has been identified as a bug in the product Resolution-Fixed The issue is fixed. WG-Engine core PowerShell engine, interpreter, and runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants