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

It doesn't go deep enough #29

Closed
VagyokC4 opened this issue Jul 3, 2020 · 11 comments
Closed

It doesn't go deep enough #29

VagyokC4 opened this issue Jul 3, 2020 · 11 comments
Labels
bug Something isn't working

Comments

@VagyokC4
Copy link

VagyokC4 commented Jul 3, 2020

Describe the bug
This library works good with simple expression trees. But for expression trees that run deep, it simply stops returning values, and reverts to displaying non specific information.

To Reproduce
Any multi-step expression that has multiple layers of functions e.g.
f => f.EmailAddress.ToLower() == Request.UserSession().Email.ToLower())

which produces the following tree

Equal (bool)
    · Left - Call (string) ToLower
        · Object - MemberAccess (string) EmailAddress
            · Expression - Parameter (CustomerSummary) f
    · Right - Call (string) ToLower
        · Object - MemberAccess (string) Email
            · Expression - Call (IAuthSession) RequestExtensions.UserSession
                · Arguments[0] - MemberAccess (IRequest) Request
                    · Expression - Constant (CustomerService) = #CustomerService

Expected behavior
I would expect this library to run recursively and capture all levels of the expression tree, not just the top levels, but most importantly, it will display the actual values being used for the query as it does with this simple expression:

var sortCode = "123456";
var accountNumer = "789012345678";
f => f.Code.Contains($"{sortCode}{accountNumer}")
Call (bool) Contains
    · Object - MemberAccess (string) Code
        · Expression - Parameter (Account) f
    · Arguments[0] - Call (string) string.Format
        · Arguments[0] - Constant (string) = "{0}{1}"
        · Arguments[1] - MemberAccess (string) sortCode = "123456"
            · Expression - Constant (<closure>)
        · Arguments[2] - MemberAccess (string) accountNumer = "789012345678"
            · Expression - Constant (<closure>)

Version info

  • .NET Core / .NET Framework version:
  • Version: [either NuGet package version / GitHub release version, or commit]

Additional context
Add any other context about the problem here.

@VagyokC4 VagyokC4 added the bug Something isn't working label Jul 3, 2020
@zspitz
Copy link
Owner

zspitz commented Jul 5, 2020

Thanks for the bug report.

I'm not sure what further information you would expect to see. Could you clarify, using the first tree you provided with additional details you would like to see?

Is it because the Customer instance is represented as #Customer, and not using its constituent properties / values? The library returns a string representing the entire expression tree, and I haven't been able to come up with a straightforward way of representing an arbitrary object (i.e. not Expression or related type) within that string. You can find more information about which objects / values / types are represented and how, in the wiki.

Alternatively, if the expression is being generated within the CustomerService instance, and Request is a property on that instance, the instance should perhaps be represented as this. The trouble is, by the time the expression tree has been built, I haven't yet found a way to distinguish between the enclosing this and a different CustomerService instance. (This is being tracked at #30.)

As far as the depth of the expression tree, I don't see anything in the expression tree you provided:

f => f.EmailAddress.ToLower() == Request.UserSession().Email.ToLower()

that isn't in the resultant string.

(NB. It seems almost too trivial to mention, but the parts of an expression tree are often not in the same order as the code representation. For example, using the string representation you provided, the various code parts of the expression tree would be distributed like this:

==
    .ToLower()
        .EmailAddress
            f
    .ToLower()
        .Email
            .UserSession( ... )
                .Request
                    #CustomerService

Note also that UserSession is actually an extension method on the RequestExtensions class, and that the Request property is reading off an implied instance of CustomerService.)

@VagyokC4
Copy link
Author

VagyokC4 commented Jul 7, 2020

Hi @zspitz ,

Yes, so it's more that I would expect to see the tree with the actual values that are being tucked away deeper in the object graph.

So basically, when I pass in

f=> f.prop == object.function().property.value()

I would expect to see somewhere in the tree a line that showed what the actual value produced by the right side value().

i.e.

Arguments[x] - MemberAccess (string) value = "123456"

when object.function().property.value() => "123456"

As for the order, I am only concerned in the data returned, insomuch that it gives me the actual values assigned to the various nodes of the tree, like it does with you use a simple expression with no object.function().

@zspitz
Copy link
Owner

zspitz commented Jul 7, 2020

@VagyokC4 OK, I see now; thanks. This looks like a good idea, and I will try to implement it. Is this what you had in mind?

Equal (bool)
    · Left - Call (string) ToLower
        · Object - MemberAccess (string) EmailAddress
            · Expression - Parameter (CustomerSummary) f
    · Right - Call (string) ToLower = "abc@def.com"
        · Object - MemberAccess (string) Email = "ABC@DEF.COM"
            · Expression - Call (IAuthSession) RequestExtensions.UserSession = #IAuthSessionImplementingTypeName
                · Arguments[0] - MemberAccess (IRequest) Request = #IRequestImplementingTypeName
                    · Expression - Constant (CustomerService) = #CustomerService

Note that we can't provide values for any expression tree containing a parameter expression, which doesn't have an associated value embedded in the expression tree. In this case, the entire left side of the == uses the parameter f. In order to provide a value for any given expression tree, we'd have to walk the entire tree making sure there were no parameters. (The source for the extension method that gets the value can be found here).

@VagyokC4
Copy link
Author

VagyokC4 commented Jul 7, 2020

Equal (bool)
    · Left - Call (string) ToLower
        · Object - MemberAccess (string) EmailAddress
            · Expression - Parameter (CustomerSummary) f
    · Right - Call (string) ToLower = "abc@def.com"
        · Object - MemberAccess (string) Email = "ABC@DEF.COM"
            · Expression - Call (IAuthSession) RequestExtensions.UserSession = #IAuthSessionImplementingTypeName
                · Arguments[0] - MemberAccess (IRequest) Request = #IRequestImplementingTypeName
                    · Expression - Constant (CustomerService) = #CustomerService

@zspitz Exactly Right! How soon before we could see this functionality?

@zspitz
Copy link
Owner

zspitz commented Jul 7, 2020

@VagyokC4 This is what needs to be resolved / done. Perhaps by the end of the week, although I can't fully commit to that.

Questions:

  • Within the TextualTreeFormatter, for each processed node, I don't want to walk the expression tree of that node in search of parameter expressions. IOW, once we've determined that the left side of the == has a ParameterExpression, I shouldn't check the == node. Store a hashset of previously found nodes that contain parameter expressions?

Parts of the implementation:

  • Create an ExpressionTreeVisitor derived class which checks for nodes of ParameterExpression
    • Cache the checked expressions in the tree instance
  • Test performance against test objects library (test objects library has no external dependencies)
  • Use in TextualTreeFormatter
  • Use in visualizer

@VagyokC4
Copy link
Author

@zspitz How is this progressing?

@zspitz
Copy link
Owner

zspitz commented Jul 15, 2020

@VagyokC4 Could I have your input on something? The following expression:

// using static System.Linq.Expressions.Expression;
var EmptyLoop = Loop(Constant(true));

represents a never-ending empty loop:

while (true) { }

Ordinarily, loop expressions have a value -- the value of the last executed expression within the loop block -- but a never-ending loop cannot be evaluated. Wrapping the loop expression in a try...catch doesn't seem to work.

AFAICT there's no way to tell from looking at a LoopExpression that it's a never-ending loop, without actually trying to run it. I think the only way out is to not provide a value for any LoopExpression.

What do you think?

@zspitz zspitz closed this as completed in 301312e Jul 15, 2020
@zspitz zspitz reopened this Jul 15, 2020
@zspitz
Copy link
Owner

zspitz commented Jul 15, 2020

@VagyokC4 I've committed the changes, but there are a few failing tests because I never anticipated the test expressions would actually be evaluated. It should be available by tomorrow.

@zspitz
Copy link
Owner

zspitz commented Jul 16, 2020

As of NuGet package version 2.0.25, this should be working.

@zspitz zspitz closed this as completed Jul 16, 2020
@VagyokC4
Copy link
Author

VagyokC4 commented Jul 17, 2020

@VagyokC4 Could I have your input on something? The following expression:

// using static System.Linq.Expressions.Expression;
var EmptyLoop = Loop(Constant(true));

represents a never-ending empty loop:

while (true) { }

Ordinarily, loop expressions have a value -- the value of the last executed expression within the loop block -- but a never-ending loop cannot be evaluated. Wrapping the loop expression in a try...catch doesn't seem to work.

AFAICT there's no way to tell from looking at a LoopExpression that it's a never-ending loop, without actually trying to run it. I think the only way out is to not provide a value for any LoopExpression.

What do you think?

Sorry I just saw this. So what was the fix for this empty loop scenario? How often does one get an empty loop? I assume this is edge case?

BTW: 2.0.25 is looking good. Thank-you for all your hard work!

@zspitz
Copy link
Owner

zspitz commented Jul 17, 2020

@VagyokC4

So what was the fix for this empty loop scenario?

No fix, as such; as I said, there's no attempt to provide a value for the LoopExpression itself, and also for anything up the expression tree which might use it.

How often does one get an empty loop? I assume this is edge case?

I realized later that the issue is not so much the empty loop, but the never-ending loop, which is probably more common. Although I haven't seen much use in general of the statement-style expression trees (loops, goto, if...else etc.) added in .NET 4.

But even if it is an edge case, the workaround is simple, if a little ham-handed.

zspitz added a commit to zspitz/ExpressionTreeVisualizer that referenced this issue Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants