Superfluous characters and error handling

[strohhaecker]

Hello, trying to make something about the handling of (possibly) invalid characters inside numbers. I've compiled some test cases where the one I'm interested in is # 2 (2,7 being evaluated as 7 which means the whole part before and including the comma is silently ignored).

[TestMethod]
[DataRow("# 1", "2.7", 2.7)]
[DataRow("# 2", "2,7", 2.7)]
[DataRow("# 3", "2;7", 2.7)]
[DataRow("# 4", "2#7", 2.7)]
[DataRow("# 5", "2&7", 2.7)]
public void TestSimpleCommaExpressionTest(string id, string expression, double expected)
{
    var context = new EvalContext();
    context.SafeMode = true;

    var compiled = context.Compile(expression, new List<Type>());
    var result = compiled.Invoke(new Dictionary<object, double>());

    result.Should().Be(expected);
}

For at least cases # 2 and # 3 I'd expect an error to be thrown (SafeMode does not affect this in any way). Any idea on this?

Thank you

[JonathanMagnan]

Hello @strohhaecker ,

Unfortunately, I don't think our library fits your requirements well.

Our library uses a lot of expression trees, which is way more permissive than the normal compiler.

By example:

2;
7;

Will not work in Visual Studio but will work perfectly if you create this in an expression tree.

As for your 5 tests:

# 1: Return the right result # 2: I agree this is weird. Mostly due to support expression like this new List<int>() {1, 2, 3}; (Will return the same as #3 since 7 is the last expression evaluated) # 3: Return the right expected result as show in my explanation # 4: Throw correctly an error # 5: Return the right result ("&" is a valid binary logical operator: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/boolean-logical-operators)

Our library is currently way too permissive for your requirements.

However, is there a reason why you don't use TryParse?

string value = "2,7";
var cultureInfo = new CultureInfo("en-US");
var numberStyles = NumberStyles.Any & ~NumberStyles.AllowThousands; // Adjust this as needed (we removed the AllowTHousands for not supporting the `,`)
bool isValid = double.TryParse(value, numberStyles, cultureInfo, out double result);

It could probably work better than our library for your scenario.

Best Regards,

Jon

[strohhaecker]

Hi Jon, thanks for your response, the TryParse is indeed no solution because the problem occurs in complex expressions that contain for example "2,7" nested inside braces and whatnot. I'm not sure if I understand your example of the List<> entries, but it sounds like evaluating "2,7" results in a list where the value is the last entry? Is there any way to change this behaviour since for List-operations this makes sense to me but not for maths calculations where I'd expect an error to be returned (especially since one part of the List-entries 2 and 7 is discarded without further notice). Thanks!

[JonathanMagnan]

Hello @strohhaecker ,

It's a little bit more complicated than a list, but nothing is discarded.

The following expression var x = 2,7; is evaluated like:

var x = 2;
7;

The comma separates the expression into multiple expressions, which are different expressions.

Since nothing is returned, the value of the last expression is returned. If you use the expression var x = 2,7; return x, it will return 'x'.

We will check if we can do something here. The , is indeed problematic in some scenarios but I'm afraid of the side impact it could cause at this moment.

[strohhaecker]

Ok thanks for the insight, please keep us updated if it's possible to change this behaviour but I understand that the implications may be tricky. Thank you!

[strohhaecker]

A colleague fixed our problem by using "return 2,7" instead of "2,7" for the expression, meaning we prefix the string that should be evaluated with "return" and this triggers an error message correctly for the comma cases. The only problem we see at the moment is that it's an Eval-Expression-internal null reference exception, which does not look intentional (so we fear this may break in the future). Do you have an idea if this can be changed into a more expressive error message or share what's behind the NRE? Thanks for your help and insights! Sebastian

[JonathanMagnan]

Hello @strohhaecker ,

My developer is currently looking to fix this coma issue. We currently try to only fix the case like [number],[number] and nothing else which is your case. I believe he will push a fix for a code review this weekend.

The error happens in the CompileNode method as the first argument is null and the second is 7 probably because the 2 has been taken by the return keyword. The error happen here:

case SyntaxKind.CommaContainer:
    {
        var expressions = new List<Expression>();

        for (var i = 0; i < node.Arguments.Count; i++)
        {
                        // The first argument = null for the `return 2,7` expression
            var argument = node.Arguments[i];
            expressions.Add(CompileNode(scope, argument));
// ...code...

We sure could change it for a more expressive error but I believe waiting with what my employee will come as a fix might be a better idea.

Best Regards,

Jon

[JonathanMagnan]

Hello @strohhaecker ,

Just to let you know that we miss the release this week. However, the fix has been pushed in the master and should be pushed next Tuesday.

Best Regards,

Jon

[JonathanMagnan]

Hello @strohhaecker ,

The v6.1.1 is now available.

Could you let us know if we fixed correctly issue such as 2,7

Best Regards,

Jon

[strohhaecker]

Hello Jon, yes I'll try it as soon as possible, at the moment we use an older version of Eval-Expressions (6.0.1) since we hit this problem: https://github.com/zzzprojects/Eval-Expression.NET/issues/155

Thank you very much for looking into this issue, I'll report back soon.

Sebastian

[JonathanMagnan]

Thank for letting me know.

I will close this issue meanwhile to make it easier to manage on our side.

(We will obviously re-open it if the issue is not correctly fixed).

Best Regards,

Jon

[strohhaecker]

We've updated to the latest version and it works correctly now. Thank you very much for your support! Sebastian