Unclear SetExpression with parameter behavior

[ealeykin]

Hi,

var interpreter = new Interpreter();
var parameter = new Parameter("x", typeof(int));
var expression = interpreter.Parse("x + 1", parameter).Expression;

var lambda = interpreter
    .SetExpression("value", expression)
    .Parse("value + 1", parameter);

var result = lambda.Invoke(1);

This will throw

System.InvalidOperationException: variable 'x' of type 'System.Int32' referenced from scope '', but it is not defined

Trying to register identifier like below leads to same exception

var lambda = interpreter
    .SetExpression("value", expression)
    .SetIdentifier(new Identifier("x", parameter.Expression)) 
    .Parse("value + 1", parameter);

And only if I mention the identifier in primary expression interpreter.Parse("x + value + 1", parameter) will parse with success.

Could you please advise how to propagate x parameter to internal expression without mentioning it in primary one?

Thanks

[ealeykin]

I assume this is because of UsedParameters are actually passed when creating Lambda.

So what the point of UsedParameters?

[ealeykin]

It looks like the DynamicExpresso.Lambda wrapper is intended to simplify the usage, but on the other hand introduces some limitations.

For example in mentioned scenario DynamicExpresso.Lambda constructor successfully creates an Expression itself but then fails to create Lambda and even though an Expression itself successfully constructed I can not use it because of thrown exception.

Maybe I am missing something, but it would more flexible to return raw expression result without trying to compile it leaving up to a client to decide whether there is a need to compile an expression or use it as is.

Interpreter.ParseAsExpression looks more or less like what I described above (although in dynamic context it could be complicated to get a Delegate type), but even in this case it first compiles constructed Expression with default behaviour using UsedParameters and then recompile once again as per passed Delegate type.

Could you perhaps consider having a Interpreter.Parse which return DynamicExpresso.Lambda consisting only Expression, DeclaredParameters, UsedParameter and maybe some other properties, but no more methods. The same applies to Interpreter - only a single Parse method.

Then all the lambda compile variations move to Extension methods over DynamicExpresso.Lambda. This approach will simplify further library extension, will separate parse and compile logic and moreover, if something is not implemented in the library, a client could take that raw lambda expression and write an extension on its own (currently all this stuff is internal).

Does it make sense?

[davideicardi]

@halamah It looks like a good idea to me. I'm not sure about all the implications but we can do some experiments.

Also we should consider this as a breaking change (we will change the interface definition), this is not a problem per se but something to be considered.

@metoule What do you think?

[ealeykin]

I haven't fixed all tests, but here is an idea:

Only Parse goes here:
https://github.com/halamah/DynamicExpresso/blob/master/src/DynamicExpresso.Core/ExpressionInterpreter.cs

All compile variations here:
https://github.com/halamah/DynamicExpresso/blob/master/src/DynamicExpresso.Core/InterpreterExtensions.cs

Complete comparing:
master...halamah:master

The only issue so far as per my implementation at least - there is a feature that you can pass Parameters in random order to Lambda.Invoke and since the proposed implementation is not using Lambda wrapper - an additional Delegate extension method must be added I guess this is the only way. On the other hand, such a feature could be out of library's scope and handed over to client's code.

[metoule]

I like the proposal, but I think we should be careful so that the public interface remains as close as possible to the existing one so that users just have to recompile, but not have to update their code because it's always painful.

[ealeykin]

@metoule I've changed PR so it's fully backward compatible, take a look pls.

[metoule]

@halamah

I had another look at your PR, and it doesn't seem to fix your initial issue:

var interpreter = new ExpressionInterpreter();
var expression = interpreter.Parse("x + 1", Expression.Parameter(typeof(int), "x")).Expression;

var lambda = interpreter
	.SetExpression("value", expression)
	.Parse("value + 1", Expression.Parameter(typeof(int), "x"));

var result = lambda.Compile().DynamicInvoke(1); // fails here with the same exception as before
Assert.AreEqual(3, result);

Did I miss something?

[metoule]

My bad, the parameter simply needs to be the same reference:

var interpreter = new ExpressionInterpreter();
var parameter = Expression.Parameter(typeof(int), "x");
var expression = interpreter.Parse("x + 1", parameter).Expression;

var lambda = interpreter
	.SetExpression("value", expression)
	.Parse("value + 1", parameter);

var result = lambda.Compile().DynamicInvoke(1);
Assert.AreEqual(3, result);

[ealeykin]

As per version 2.10 the exception is suppressed by lazy delegate creation inside Lambda constructor:

var interpreter = new Interpreter();
var parameter = new Parameter("x", typeof(int));
var internalExpression = interpreter.Parse("x + 1", parameter).Expression;

interpreter.SetExpression("x", internalExpression);
var lambda = interpreter.Parse("x + 1", parameter);
var func = Expression.Lambda(lambda.Expression, parameter.Expression).Compile();

var result = func.DynamicInvoke(1);
Assert.Equals(result, 3);