sklose / NCalc2

expression evaluator for .NET with built-in compiler
MIT License
170 stars 58 forks source link

Allow custom Functions to return async values #12

Closed dgeller-OUHSC closed 9 months ago

dgeller-OUHSC commented 6 years ago

As part of a need for a work project we added the ability to have custom functions return values asynchronously, I'm happy to contribute this back if you think it's not too niche.

I would have included Lambda async calls as well, but going a little ways down that rabbit hole I didn't see any way to have linq expressions execute async methods.

sklose commented 6 years ago

Hi dgeller-OUHSC,

Thanks for your PR. Can you maybe explain a little more in detail what the use case for async is? Off the top of my head I would expect this to have a huge performance overhead since function evaluation is purely CPU bound.

Best, Sebastian

dgeller-OUHSC commented 6 years ago

Of course. Our use case is that certain pieces of an expression that we were wanting to use were only available via async methods, and we only wanted to fetch them if they were part of the expression. for example, we defined a custom method like "fetchDistanceReading(1,2) > 5" where fetchDistanceReading() would run and grab a value from a database via an asynchronous call.

The performance is definitely worse, but since it's a separate function it should only come into play in cases when the user explicitly calls it: Expression.EvaluateAsync();

sklose commented 6 years ago

thanks for the explanation, that totally makes sense. I think the lambda portion needs some work though for this. essentially when you call ToLambda<,> and specify a Task<> as a return type, e.g. ToLambda<Context, Task<int>> then it would need to invoke the async version of the lambda visitor and create an awaited Task whenever it invokes any function on the context. Also all functions on the expression context would need to be async (or the code is smart enough to detect it and only generate async code if the function on the expression context is async either). So I guess an example that should compile and work would be:

class Context
{
    public async Task<int> Foo()
    {
        return Task.Complete(5);
    }
}

var expr = new Expression("Foo()");
var lambda = expr.ToLambda<Context, Task<int>>();
int val = await lambda(new Context());
dgeller-OUHSC commented 6 years ago

Hi Sebastian,

Thank you for the feedback. I attempted to implement what you describe, but I am stuck at a point that maybe you can help with:

I am able to use some of the reflection properties to determine if a method is async in the LambdaExpressionVisitor.cs => Visit(Function function), however in the following code

var mi = _context.Type.GetTypeInfo().DeclaredMethods.FirstOrDefault( m => m.Name.Equals(function.Identifier.Name, StringComparison.OrdinalIgnoreCase) && m.IsPublic && !m.IsStatic); if (mi == null) throw new MissingMethodException($"method not found: {function.Identifier.Name}"); _result = L.Expression.Call(_context, mi, args); I don't see a way to make an expression call async, and this makes me think that's a feature that's still in the works.

sklose commented 6 years ago

yeah, compiling the async calls is probably a bit tricky. if the Expression API doesn't include anything then I guess we would have to generate the code the C# compiler generates for async/await. there are some good examples here: https://weblogs.asp.net/dixin/understanding-c-sharp-async-await-1-compilation how this works under the covers.

dgeller-OUHSC commented 6 years ago

There may be a more elegant way to accomplish async expression calls, but at least this way works for my test cases. Perhaps by the time it needs revisiting async expressions calls will be baked in to the framework.

sklose commented 6 years ago

I don't quite agree with the async lambda implementation ... the ToLambdaAsync function shouldn't return a Task<lambda>, but a lambda that has a Task as its return value. Otherwise there is nothing async about the lambda execution - just about the construction of the lambda which doesn't need to be asynchronous.

dgeller-OUHSC commented 6 years ago

I agree that it's not ideal. Despite reading that article you sent (and even understanding a word or two) I don't see how that would map on to what we're trying to accomplish. A linq expression doesn't appear to have a type that handles asycs, so I don't know what I could store into the variable System.Linq.Expressions.Expression _result; inside the LamdaExpressionVisitor.Visit() method that would handle async expressions that aren't executed in the constructor.

sklose commented 6 years ago

I think from a pseudo code perspective it would look something like this:

class LambdaVisitor : IVisitor
{
  public override void Visit(ValueExpression expression)
  {
    /* this would need to be done as an Expression Tree */
    return Task.Result(expression.Value));
  }

  public override void Visit(BinaryExpression expression)
  {
    var left = this.Accept(expression.LeftExpression);
    var right = this.Accept(expression.RightExpression);

    /* this would need to be done as an Expression Tree */
    return Task.WhenAll(left, right).ContinueWith((l, r) => expression.Operator(r.Result, l.Result)));
  }

  // ...
}

This way the Task is chained all the way thru the evaluation of the compiled lambda and the result of the lambda would be a Task<TResult>.

dgeller-OUHSC commented 6 years ago

I have already written async versions of all the visit methods. The issue is when there is a reflected async method call within a lambda. In the code below, I can't find a way to store an async call in to the _result eg: _result = L.Expression.Call(_context, mi, args):

edit - I should say, I can store it, but it doesn't work at runtime. I can add a failing unit test if that would be helpful.


        public override void Visit(Function function)
        {
            var args = new L.Expression[function.Expressions.Length];
            for (int i = 0; i < function.Expressions.Length; i++)
            {
                function.Expressions[i].Accept(this);
                args[i] = _result;
            }

            switch (function.Identifier.Name.ToLowerInvariant())
            {
                case "if":
                    _result = L.Expression.Condition(args[0], args[1], args[2]);
                    break;
                case "in":
                    var items = L.Expression.NewArrayInit(args[0].Type,
                        new ArraySegment<L.Expression>(args, 1, args.Length - 1));
                    var smi = typeof (Array).GetRuntimeMethod("IndexOf", new[] { typeof(Array), typeof(object) });
                    var r = L.Expression.Call(smi, L.Expression.Convert(items, typeof(Array)), L.Expression.Convert(args[0], typeof(object)));
                    _result = L.Expression.GreaterThanOrEqual(r, L.Expression.Constant(0));
                    break;
                default:
                    var mi = _context.Type.GetTypeInfo().DeclaredMethods.FirstOrDefault(
                        m => m.Name.Equals(function.Identifier.Name, StringComparison.OrdinalIgnoreCase) &&
                             m.IsPublic && !m.IsStatic);
                    if (mi == null)
                        throw new MissingMethodException($"method not found: {function.Identifier.Name}");
                    _result = L.Expression.Call(_context, mi, args);
                    break;
            }
        }`
kfrancis commented 3 years ago

Any chance of making this work?

sklose commented 9 months ago

closing cause it's stale