expr-lang / expr

Expression language and expression evaluation for Go
https://expr-lang.org
MIT License
6.11k stars 393 forks source link

Cast IntegerNode to float32 in operations like `f32 == 1.5` #609

Open daisy1754 opened 5 months ago

daisy1754 commented 5 months ago

When you try to compare floating number with floating literal like Number == 12.34, it returns false even when Number is 12.34

Below is code to reproduce https://go.dev/play/p/FFgPkXjIURm

package main

import (
    "fmt"

    "github.com/expr-lang/expr"
)

type Env struct {
    Number float32
}

func main() {
    code := `Number == 12.34`

    program, err := expr.Compile(code, expr.Env(Env{}))
    if err != nil {
        panic(err)
    }

    env := Env{
        Number: 12.34,
    }
    output, err := expr.Run(program, env)
    if err != nil {
        panic(err)
    }

    fmt.Println(output)
}
antonmedv commented 5 months ago

This is how float comparison works:

package main

func main() {
    var f32 float32 = 12.34
    var f64 float64 = 12.34
    println(float64(f32) == f64) // false
}

https://go.dev/play/p/XsvrQ66pfrX

daisy1754 commented 3 months ago

@antonmedv I think example I gave is more like below. It is true in golang but false in expr

package main

func main() {
    var f32 float32 = 12.34
    println(f32 == 12.34) // true
}

https://go.dev/play/p/Z9SwZHGSi_J

antonmedv commented 3 months ago

In golang f32 == 12.34, right hand side 12.34 will be interpreted as float32, i.e. float32(12.34). IN Expr 12.34 is a float64.

daisy1754 commented 3 months ago

well in golang literals are automatically casted to proper type right? in below example both float32 and float64 comparison returns true.

package main

func main() {
    var f32 float32 = 12.34
    var f64 float64 = 12.34
    println(f32 == 12.34) // true
    println(f64 == 12.34) // true
}
antonmedv commented 3 months ago

Yes, type in inherited from left.

daisy1754 commented 3 months ago

and that is not the case for expr (please refer to my initial example) and that's why I opened issue. I believe Expr internally always cast number to flat64

antonmedv commented 3 months ago

In your example number is float32

daisy1754 commented 3 months ago

Is this example more clear? I expect all to be true

package main

import (
    "fmt"

    "github.com/expr-lang/expr"
)

type Env struct {
    F32 float32
    F64 float64
}

var env = Env{
    F32: 12.34,
    F64: 12.34,
}

func main() {
    fmt.Printf("f32 expr: %t, go: %t\n", eval(`F32 == 12.34`), env.F32 == 12.34)
    fmt.Printf("f64 expr: %t, go: %t\n", eval(`F64 == 12.34`), env.F64 == 12.34)
}

func eval(code string) bool {
    program, err := expr.Compile(code, expr.Env(Env{}))
    if err != nil {
        panic(err)
    }
    output, err := expr.Run(program, env)
    if err != nil {
        panic(err)
    }
    return output.(bool)
}

result is

f32 expr: false, go: true
f64 expr: true, go: true

https://go.dev/play/p/GH9DSp3YFHF

antonmedv commented 3 months ago

In first example expr does this

F32 == float64(12.34)

which is false.

daisy1754 commented 3 months ago

yeah I understand internal - that's why I sent out https://github.com/expr-lang/expr/pull/610

Don't you think it's confusing though? As shown above

fmt.Printf("f32 expr: %t, go: %t\n", eval(F32 == 12.34), env.F32 == 12.34)

expr expression is false and golang expression is true in this case

antonmedv commented 3 months ago

Yes! I think we can work on a proper solution for this. The solution in #610 is not correct.

What we need is to cast type to float32 only in case of lhs is float32. This should be done in patcher.