Parse and Compile separation #207

[metoule]

This is the continuation of PR #211 by @halamah; I didn't find a way to update their PR so it's duplicated.

PR #211 introduced a lot of changes, in particular duplicated code around Interpreter. Instead of reimplementing everything, here are the changes I kept, and the changes I made:

• ParserResult and ParserResult<TDelegate> are used whenever the caller doesn't need / use the Lambda wrapper
• the Lambda wrapper is kept for backward compatibility, but now inherits from ParserResult, and is used to wrap the Invoke method
• the Lambda wrapper no longer compiles the expression in the constructor; instead, the expression is compiled during the first call to Invoke. That way, there's no overhead if the lambda is not invoked
• I've kept the current logic that uses UsedParameters when the lambda's delegate type is inferred, and DeclaredParameters when the lambda's delegate is provided
• however, I've added a new parser settings to disable the detection of used parameters. If set, the used parameters are the same as the declared parameters (I suspect this alone should solve Unclear SetExpression with parameter behavior  #207, but the other changes seemed to make sense as well)
• for now, I removed all the Eval methods from the InterpreterExtensions class because they were not strictly needed for the refactoring, which I wanted to keep to a minimum

This PR should be backward compatible unless I missed something, and there's no breaking change, so the new version can be 2.10.0.

[ealeykin]

The only thing on my eye is various Parse, ParseAs, ParseAsDelegate, ParseAsExpression - basically all those could be a single Parse overloads, but I suppose this is to keep backward compatible Lambda.

[metoule]

Yes, all the Parse methods could all be merged into a single one (and in the end, they all call the same ParseRawExpression method), but the existing methods are part of the public interface... Same goes for the Lambda class.

[ealeykin]

@metoule Is it possible to:

1. Create ExpressionInterpreter with a single Parse method (+ overloads) which returns ParseResult, like in my PR.
2. Inherit Interpreter (containing all legacy methods perhaps marked with 'new') from ExpressionInterpreter.
3. Revert Lambda code and change it to dependency inject ParseResult in constructor. Change all internal property references to injected ParseResult.
4. Change legacy Interpreter, which now extends ExpressionInterpreter, to call base.Parse and create Lambda passing ParseResult to its constructor. So this will aggregate parse result rather than inherit.
5. Mark Interpreter as obsolete.
6. Mark Lambda as obsolete.

In this case it would be possible to drop legacy code in the future, but keep it for now for backward compatibility.

[davideicardi]

In my opinion is fine to break binary compatibility as long as we create a new major version.

So we can create just the new Interpreter class with the new functions and extensions.
Some functions can return the new ParseResult, other can return the old Lambda that encapsulate a ParseResult.

In this way we should maintain compatibility as long as you recompile (no binary compatibility but we will release as a major version) but we have a clean structure for the future.

What do you think?

[metoule]

I don't think we need a new Interpreter class, because most of the Parse overloads are just syntaxic sugar over the ParserResult class (they could be moved to dedicated extension methods to clean up the Interpreter interface).

We could mark the method that returns a ParserResult as public, but it can't be simply be called Parse, because that name is already used for the overload that returns a Lambda. ParseRaw?

I'm also not sure it makes sense to mark all the old methods and the Lambda class as deprecated, because the alternative from a user standpoint would be to just reimplement those extension methods, or make the library harder to use.

Let's consider the current code:

var x = new Parameter("x", typeof(double));
var y = new Parameter("y", typeof(double));

var target = new Interpreter();
var functionX = target.Parse("Math.Pow(x, y) + 5", x, y);
Assert.Equal(30d, functionX.Invoke(5, 2));

With ParserResult, the user would first need to compile the expression themselves:

var x = new Parameter("x", typeof(double));
var y = new Parameter("y", typeof(double));

var target = new Interpreter();
var functionX = target.Parse("Math.Pow(x, y) + 5", x, y).Compile();
Assert.Equal(30d, functionX.DynamicInvoke(5, 2));

That works, but it requires the user to know how delegates work, and thus is less user friendly.

[davideicardi]

I agree with @metoule about user expectation. But @halamah is true on the fact that the current implementation is not ideal.

So my point is: if we create a new major version we are free to break binary compatibility without worries, BUT we must try to maintain most of the user experience. So that if someone change the version they need to change only a small portion (or potentially nothing) of their code.

[ealeykin]

Regarding user expectation, I'm definitely not aware of all use cases, but let's consider the following scenarios

• Performance is not concerned

Basically an expression is executed once - in this case we could just use Eval extension method. This will compile under the hood every time when invoking, since the performance is not concerned at all - this would be fine, but I'm not 100% sure here.

• Performance is concerned

This will involve several stages - parsing, compilation, caching. Subsequent invokes will use cached compiled result. I think this case is the most important. ParseRawExpression is internal, and I can use only method that return Lambda, but I don't need Lambda, I need an expression to compile and cache. Ok, we can make it public, then I need to go though the docs and check which one I need - Parse, ParseAs, ParseAsDelegate, ParseAsExpression, ParseRawExpression and suggested new ParseRaw.

So all those helpers looks like reimplementing Lambdas on top of dotnet Lambdas. From my point of view it's harder to use custom code, because having the following flow => text -> parse -> compile -> invoke - has a strict definition and you can find tons of official documentation on each stage. On the other hand having a custom Lambda introduces custom layer to read and understand how it works - for example UsedParameters, this logic retained - was definitely unclear for me and I had to go through codebase to understand how it works.