nreco / lambdaparser

Runtime parser for string expressions (formulas, method calls). Builds dynamic LINQ expression tree and compiles it to lambda delegate.
http://www.nrecosite.com/
MIT License
307 stars 55 forks source link

Changing order of parameters in simple expression produces different evaluation result. #34

Closed mihalios closed 3 years ago

mihalios commented 3 years ago

This evaluates to true: lambdaParser.Eval("true == 2", new Dictionary<string, object>()) This evaluates false: lambdaParser.Eval("2 == true", new Dictionary<string, object>())

In the first instance, LambdaParser will try to convert 2 to a bool, and the expression is reduced to true == true. In the second instance true is converted to an int32, and the expression becomes 2 == 0.

The issue lies in the way LambdaParser attempts to compare the two parameters and the implementation of the Convert.ToBoolean and Convert.ToInt32 methods.

In other words, the methods are not inverse of each other.

I'm not saying this is a bug with LambdaParser but it is an implementation detail that is worth flagging so that people don't get caught out by it.

A different approach would be to allow the caller to specify whether they wish LambdaParser to attempt to convert parameter types. If the caller sets this to false, in the example above, bool and int32 are not comparable (without conversion) and the expression would always evaluate to false.

mihalios commented 3 years ago

@VitaliyMF If my suggestion about having a property on the ValueComparer to denote if type conversations are desired by the caller sounds half-reasonable, I'd be happy to work on it and submit a PR for you, if you wish.

VitaliyMF commented 3 years ago

In other words, the methods are not inverse of each other.

I'm afraid this is correct behavior for LambdaParser, let me explain why.

As you know .NET Expression trees are strictly-typed in meaning that when your parse an expression you should know the type of the parameter(s), otherwise you'll not able to compile them to a delegate. However, if expression is defined by the end-user (in run-time) often it is not possible to determine parameter's type. Actually, it is possible to parse an expression each time for the concrete parameter values, however this will work really slow.

To handle that LambdaParser internally wraps all values with LambdaParameterWrapper class that actually overloads comparisons / math operations, and actually types are aligned in the run-time; this also causes some overhead, however it is rather acceptable in most cases. How types are aligned in case of '==' comparison? That's simple, if both left and right values are non-nulls and types are not compatible for comparison, right value is 'converted' to the left value type.

That's why you got 2 different messages, however if types are compatible (no errors) both expressions lead to the same result, so I don't see any problems here. I don't see any (backward compatible) change that can be made to fix what you described in this issue.

mihalios commented 3 years ago

Thanks for the details @VitaliyMF. It's still a little unclear why we consider acceptable the fact that the below expressions evaluate to different results:

lambdaParser.Eval("2 == true", new Dictionary<string, object>()) lambdaParser.Eval("true == 2", new Dictionary<string, object>())

if both left and right values are non-nulls and types are not compatible for comparison, right value is 'converted' to the left value type.

That's what I was not convinced about, why try to 'convert' one value to the other's type? If the left and right values are not compatible why not fail the comparison? Maybe I'm thinking this very specifically to the example I present, missing other use cases where this is important.

That's why I thought that something like that could work, in the CompareInternal implementation, where AllowTypeConversations is a property in the ValueComparer the caller can set, and defaults to true.

if (AllowTypeConversations)
{
    // try to convert b to a and then compare
    if (a is IComparable) {
        var aComp = (IComparable)a;
        var bConverted = Convert.ChangeType(b, a.GetType(), FormatProvider);
        return aComp.CompareTo(bConverted);
    }
    // try to convert a to b and then compare
    if (b is IComparable) {
        var bComp = (IComparable)b;
        var aConverted =  Convert.ChangeType(a, b.GetType(), FormatProvider);
        return -bComp.CompareTo(aConverted);
    }
}
VitaliyMF commented 3 years ago

why try to 'convert' one value to the other's type? If the left and right values are not compatible why not fail the comparison?

This is by design: LambdaParser tries to align object types implicitely, if possible. Otherwise, user-defined expressions will fail simply because of non-compatible types, for example:

2 == "2" or "2" == 2

With current implementation, both comparisons work fine without any errors because of implicit type conversion. All this become even more important with variables (2 == a, and a could be either decimal or integer or string - anyway evaluation will work and give correct result).

If you don't want to have this logic, you always can provide your own implementation of IValueComparer (that behaves as you want) and set it to LambdaParser.Comparer property. If you want to add AllowTypeConversations property that's ok - but please check how parser works with AllowTypeConversations=false before submitting PR to ensure that this is really what you need.

Also, it would be better to check this flag in this way:

    // try to convert b to a and then compare
    if (a is IComparable) {
        var aComp = (IComparable)a;
        var bConverted =  AllowTypeConversations ? Convert.ChangeType(b, a.GetType(), FormatProvider) : b;
        return aComp.CompareTo(bConverted);
    }
    // try to convert a to b and then compare
    if (b is IComparable) {
        var bComp = (IComparable)b;
        var aConverted = AllowTypeConversations ? Convert.ChangeType(a, b.GetType(), FormatProvider) : a;
        return -bComp.CompareTo(aConverted);
    }
mihalios commented 3 years ago

Thanks for the feedback! @VitaliyMF