Adding Min,Max,Pow functionality to Lambda expressions

[sklose]

Hi @victoriatolls,

thanks for submitting this PR. Can you add a few unit tests for the new Min/Max/Pow functions?

Best, Sebastian

[fadulalla]

Hi all,

This broke some functionality for us because we have our own implementations of Min and Max in our context, which have now been overshadowed by these built-in min and max.

As a solution, I propose we move them after the keywords and after the context lookup, something like the snippet at the end of this comment.

My reasoning:

• Min, Max and Pow are not reserved C# keywords. So we shouldn't short-circuit them like we do with if and in (we can't expect context users not to override them).
• Context method lookup should have precedence to give users freedom. If context had precedence, those that want the built-in Min/Max have the options of either removing any implementation in their context, and it'd default to the built-in ones. Or they can re-implement the functions themselves. Whereas the way it is now restricts everyone to using the built in function with no way to override it with their own implementation.
• The built-in ones only support double and they cannot be overridden. (Consider something like Min(pointA, pointB) where points A and B are custom coordinate objects with custom Min implementation -- this is our case).
• Context method look-up is more strict and meticulous (it also considers arguments and their types in addition to the method's name) so it would make more sense to have that first before loosening/broadening our search.
• This change would be nonbreaking for people already using the newly introduced built-in Min/Max functions because their contexts wouldn't have had it in the first place (and if it did, they wouldn't have been able to use it anyway).

My proposal is something like:

string functionName = function.Identifier.Name.ToLowerInvariant();
if (functionName == "if") {
    _result = L.Expression.Condition(args[0], args[1], args[2]);
    return;
} else if (functionName == "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));
    return;
}

//"FindMethod" would be changed so it would just return null instead of throwing "method not found".
var mi = FindMethod(function.Identifier.Name, args);
if (mi != null) {
    _result = L.Expression.Call(_context, mi.BaseMethodInfo, mi.PreparedArguments);
    return;
}

// default function implementation like min, max, pow, and any other in the future.
switch (functionName) {
    case "min":
        var min_arg0 = L.Expression.Convert(args[0], typeof(double));
        var min_arg1 = L.Expression.Convert(args[1], typeof(double));
        _result = L.Expression.Condition(L.Expression.LessThan(min_arg0, min_arg1), min_arg0, min_arg1);
        break;
    case "max":
        var max_arg0 = L.Expression.Convert(args[0], typeof(double));
        var max_arg1 = L.Expression.Convert(args[1], typeof(double));
        _result = L.Expression.Condition(L.Expression.GreaterThan(max_arg0, max_arg1), max_arg0, max_arg1);
        break;
    case "pow":
        var pow_arg0 = L.Expression.Convert(args[0], typeof(double));
        var pow_arg1 = L.Expression.Convert(args[1], typeof(double));
        _result = L.Expression.Power(pow_arg0, pow_arg1);
        break;
    default:
        throw new Exception($"method not found: {functionName}");
}

@sklose I understand you're only passively maintaining the repo at the moment so I'm happy to prepare this, add tests and open a PR if you concur with the above.

We've only just found out about this because we've only recently upgraded to the latest version. We had to rollback to a version just before this one though because of the breaking change.

Thanks all,

fadulalla

[sklose]

Hi @fadulalla

this sounds very reasonable to me. Happy to merge a PR with those changes.

Best, Sebastian