search

dynamicexpresso / DynamicExpresso

C# expressions interpreter
http://dynamic-expresso.azurewebsites.net/
MIT License
1.91k stars 364 forks source link

Issues 212, 256: Adding support for internal lambdas to reference parameters from the parent lambda #257

Closed holdenmai closed 1 year ago

holdenmai commented 1 year ago

Issue #212 was really the issue here.

When replicating the code given in Issue #256 the last "x.Grade" was being immediately resolved as the value from the parameter was being inserted directly. In the previous implementation of "InterpreterExpression", it was creating it as a new ParameterExpression.
This code update captures the "parent" ParameterExpressions as identifiers.

This is essentially what happens with compiled code, so instead of us having to create a class, Expression actually allows us to reference the existing Expression as an Identifier.

Fix #212, #200, #256

davideicardi commented 1 year ago

LGTM! Thank you!

@metoule do you see problems?

davideicardi commented 1 year ago

Do you think that this PR will solve #212, #200 and #256?

holdenmai commented 1 year ago

Yes it will.

This will also make it so that #213 can be closed without merging as the settings Identifier dictionary has become our "locals" dictionary.

I just thought of a couple more cases to verify that we are only able to access parameters defined in the enclosing lambda, not a parameter defined by an adjacent lambda.

Will get those included as soon as able.

holdenmai commented 1 year ago

Test cases to cover duplicating parameter names in other lambdas have been added.

davideicardi commented 1 year ago

Thank you @holdenmai . It looks like a very nice feature. There is just a failed test.

holdenmai commented 1 year ago

Unit test has been fixed. It actually had a bad lambda declaration that would not have compiled with real code. I guess the only issue with that is if a consumer of this package had also done the same thing in the past.