Expression evaluation errors

[blind-oracle]

Hello and thanks for the useful library, but i've got an issue with it.

I have a quite simple expression:
([X] >= 2887057408 && [X] <= 2887122943) || ([X] >= 168100864 && [X] <= 168118271)

If i pass X = 2887057409, which satisfies first expression in parenthesis, the expression is for some reason evaluated to FALSE.

If i pass X = 168100865, which satisfies second part of an expression in parenthesis, i get TRUE.

If i swap the sub-expressions, then i get the inverted behaviour.

If i use smaller numbers, like:
([X] >= 0 && [X] <= 10) || ([X] >= 20 && [X] <= 30)

then the expression is evaluated right in all cases.

I think the problem is with numbers being converted to float64, however looking at tokens after expression compilation the numbers seem fine:
{Kind:13 Value:40} {Kind:7 Value:X} {Kind:10 Value:>=} {Kind:2 Value:2.887057408e+09} {Kind:11 Value:&&} {Kind:7 Value:X} {Kind:10 Value:<=} {Kind:2 Value:2.887122943e+09} {Kind:14 Value:41} {Kind:11 Value:||} {Kind:13 Value:40} {Kind:7 Value:X} {Kind:10 Value:>=} {Kind:2 Value:1.68100864e+08} {Kind:11 Value:&&} {Kind:7 Value:X} {Kind:10 Value:<=} {Kind:2 Value:1.68118271e+08} {Kind:14 Value:41}

And this does not explain why the behaviour changes if i just reverse the parts before and after || operator...

[blind-oracle]

My fault, one correction: when i swap the sub-expressions to get (in the very simple case):
([X] >= 10 && [X] <= 20) || ([X] >= 30 && [X] <= 40)

Then the whole expression evaluates correctly when using X from both ranges (15 and 35 for example).

So it seems that if expressions go from smaller to bigger it works fine.

[blind-oracle]

Further research... As i have no time now to dig into this bug myself, i tried to walk around it like this:

    functions := map[string]govaluate.ExpressionFunction{
    "InRange": func(args ...interface{}) (interface{}, error) {
        return (args[0].(float64) >= args[1].(float64)) && (args[0].(float64) <= args[2].(float64)), nil
    },
    }

    x, _ := govaluate.NewEvaluableExpressionWithFunctions("InRange([X], 30, 40) || InRange([X], 10, 20)", functions)
    t, _ := x.Evaluate(p);
    fmt.Printf("%+v\n", t)

This way it works fine.

But when i add another condition to the expression, like this:
[X] == 1 || InRange([X], 30, 40) || InRange([X], 10, 20)

Then I get panic:

panic: interface conversion: interface is []interface {}, not float64

goroutine 1 [running]:
panic(0x4dbd20, 0xc42004c3c0)
    /opt/go/src/runtime/panic.go:500 +0x1a1
main.main.func1(0xc42000e5c0, 0x2, 0x2, 0xc42009bb10, 0xc42000a528, 0x469609, 0x4d1620)
    /usr/src/go-test/main.go:22 +0x176
mt/govaluate.makeFunctionStage.func1(0x0, 0x0, 0x4d0160, 0xc42000e5e0, 0x5622e0, 0xc42000a510, 0x0, 0x0, 0x0, 0x0)
    /root/go/src/mt/govaluate/evaluationStage.go:215 +0x179
mt/govaluate.evaluateStage(0xc420014320, 0x5622e0, 0xc42000a510, 0x4d1620, 0xc42000a520, 0x0, 0x0)
    /root/go/src/mt/govaluate/EvaluableExpression.go:181 +0xf5
mt/govaluate.evaluateStage(0xc420014550, 0x5622e0, 0xc42000a510, 0x40e648, 0x8, 0x10, 0x10)
    /root/go/src/mt/govaluate/EvaluableExpression.go:154 +0x404
mt/govaluate.evaluateStage(0xc4200145a0, 0x5622e0, 0xc42000a510, 0xc42009bde8, 0x46fffe, 0xc4200145a0, 0xc4200145a0)
    /root/go/src/mt/govaluate/EvaluableExpression.go:147 +0x48f
mt/govaluate.EvaluableExpression.Eval(0x4fb03e, 0x22, 0xc420092000, 0x15, 0x20, 0xc4200145a0, 0x4fce69, 0x38, 0x562460, 0xc420016480, ...)
    /root/go/src/mt/govaluate/EvaluableExpression.go:138 +0x5c
mt/govaluate.EvaluableExpression.Evaluate(0x4fb03e, 0x22, 0xc420092000, 0x15, 0x20, 0xc4200145a0, 0x4fce69, 0x38, 0xc420016480, 0xc42003bf38, ...)
    /root/go/src/mt/govaluate/EvaluableExpression.go:113 +0x62
main.main()
    /usr/src/go-test/main.go:40 +0x1d6
exit status 2

The value that is passed to the function is for some reason [9.0 [30.0 40.0]], which is not of course float64, but [][]float64... Looks like another bug?

[Knetic]

Looks like this is due to some post-processing that happens when the expression builds the evaluation stage tree. There is logic built as part of #21 which rearranges the tree to make sure that identical operators of the same precedence work as written - but it looks like it's messing up the order of comparators. The first case evaluates the very last compare ([X] <= 168118271) as the root of the tree, so it ends up ignoring all the rest of the operations and considering that one comparison to be the result of the whole expression.

I was able to put together a quick fix by disabling the rearrangement logic for logical operators and comparators, but there's still a larger problem with the stage rearrangement logic. Still looking at it.

[blind-oracle]

Can i peek at the fix? :) By the way, i already tried some time ago to disable reorderStages(), it just reverses the behavior - the first comparison works, while the second does not.

[Knetic]

Just now pushed 8435889 to master, since the tests are good to have and the fix works for these specific cases. I'm positive that it doesn't completely correct the root problem though.

It seems to be a problem with logical operators, specifically (not comparators, like i thought). A case like (true && true) || (true && false) also exhibits the same issue.

[blind-oracle]

Thank you very much, it works like a charm now!
By the way it also fixed the above issue with a function being passed a [][]float64 slice.

[Knetic]

After scrutinizing it more, it looks like it really was just a problem with stage reordering and the logical operators. I can't find any other cases that exhibit the behavior, and those are the only operators which should never be reordered, so it looks like there's no followup.

Thanks for using the library, and for the detailed report!