DynamicExpresso icon indicating copy to clipboard operation
DynamicExpresso copied to clipboard

Parsing Lambda expression pre-evalutes it with given arguments

Open waclaw66 opened this issue 3 years ago • 7 comments

When Parsing lambda expression, it evaluates the predicate and cannot be reused (e.g. from cache) with another parameter value. See an example that demonstrates it.

var target = new Interpreter(InterpreterOptions.Default | InterpreterOptions.LambdaExpressions);
var list = new Parameter("list", new[] { 1, 2, 3 });
var value1 = new Parameter("value", 1);
var value2 = new Parameter("value", 2);
var expression = "list.Where(x => x > value)";
var lambda = target.Parse(expression, list, value1);
var result = lambda.Invoke(list, value2);
Assert.AreEqual(new[] { 3 }, result);

waclaw66 avatar Dec 13 '21 10:12 waclaw66

It's not, #200 uses Eval, this one divides it into two steps, Parse and Invoke with different parameter value. Parse of expression should be parameter value independent.

waclaw66 avatar Dec 13 '21 10:12 waclaw66

Indeed, it's not a duplicate. The parsing of the lambda expression is correct, but the parameters of the parent expression are converted to variables: they can't be passed to the lambda as parameters, because then the lambda signature would be different than the expected one.

With a proper C# parser, the lambda expression is converted to a class, and each captured variable (such as value) is set to a field of that class. Prior to invocation, the class' field is set, and then the lambda is called, rewritten to use the class' fields. Here, we don't create a class, and there's no capture, so it's not possible to change any field value.

I'm not sure it's possible to fix this issue (in fact, it's the same issue I highlighted as a limitation to the fix of #200), but I'll do some tests.

metoule avatar Dec 13 '21 10:12 metoule

Nevertheless following should work, Parse should accept just name and type...

var target = new Interpreter(InterpreterOptions.Default | InterpreterOptions.LambdaExpressions);
var list = new Parameter("list", new[] { 1, 2, 3 });
var value1 = new Parameter("value", 1);
var value2 = new Parameter("value", 2);
var expression = "list.Where(x => x > value)";
var lambda = target.Parse(expression, (new[] { list, value1 }).Select(p => new Parameter(p.Name, p.Type)).ToArray());
var result = lambda.Invoke(list, value2);
Assert.AreEqual(new[] { 3 }, result);

or throw a corrensponding exception if this feature is not supported, currently it gives only System.ArgumentException: 'Argument types do not match'

waclaw66 avatar Dec 13 '21 12:12 waclaw66

Let me explain how lambda expressions are parsed, and how I fixed #200.

When parsing the inner lambda, it has to be converted to a delegate (i.e. an instance of Func), because otherwise it can't be passed as an argument to the calling method (here, Where). When parsing the expression, we encounter a parameter from the parent expression (value). There are two options to pass a value: as a parameter, or as a variable.

Here, value can't be passed as a parameter to the inner lambda, because then its signature would be (int x, int value), which doesn't match the one expected by Where. The workaround (the fix for #200) is to convert all parameters of the parent expressions to variables of the inner lambda, but that means that value is now set to the initial parameter's value. That proves to be an issue if you want to reuse the parent delegate, e.g. your 1st example.

The issue you encounter in your 2nd example is that the parameter new Parameter("value", typeof(int)) is converted to a variable, which isn't possible because an int can't have a null value. It's equivalent to this:

var interpreter= new Interpreter();
interpreter.SetVariable("x", null, typeof(int));

which calls Expression.Constant(null, typeof(int)), which triggers the exception you encounter.

To be honest, I'm inclined to revert the change I made for #200, and instead throw a NotSupportedException when trying to access a parameter from a lambda expression.

metoule avatar Dec 13 '21 12:12 metoule

Thanks for explanation, I can understand it. Fix of #200 makes lambdas usable with parameters, it's much better than without them. Current state disallows reuse of parsed lambdas only. I can live with that. Thanks for your effort.

waclaw66 avatar Dec 13 '21 13:12 waclaw66

I have an idea that might work, but I'm not yet sure it will. The idea is to create a dictionary of all the captured variables, and access it when needed:

  • create a body (instead of a single expression) for inner lambda expressions
  • create an expression that sets the dictionary's value for all parent parameters on enter
  • parse the expression, and when an identifier is encountered, check whether it's a captured variable

Proof of concept of such a body's creation:

var dict = new Dictionary<string, object>();
var captured = Expression.Constant(dict);

// create an expression for x + 5
var param = Expression.Parameter(typeof(int), "x");
var five = Expression.Constant(5);
var add = Expression.Add(param, five);

// create the expression that sets the captured variable's value
// => dict["x"] = 5
var indexer = Expression.Property(captured, dict.GetType().GetProperty("Item"), new Expression[] { Expression.Constant(param.Name) });
var set = Expression.Assign(indexer, Expression.Convert(param, typeof(object)));

// create the delegate with a body
var block = Expression.Block(set, add);
var lambda = Expression.Lambda(block, param);
var del = lambda.Compile();

del.DynamicInvoke(2).Dump();
dict.Dump(); // contains x == 2

del.DynamicInvoke(3).Dump();
dict.Dump(); // contains x == 3

metoule avatar Dec 13 '21 19:12 metoule

I have a working prototype, that also fixes the limitation I highlighted in #200. It still needs some work, but I might wait until your PR is merged because it impacts the Lambda class.

metoule avatar Dec 14 '21 08:12 metoule