search

robertkrimen / otto

A JavaScript interpreter in Go (golang)
http://godoc.org/github.com/robertkrimen/otto
MIT License
8.04k stars 584 forks source link

TestBinaryShiftOperation test fails #457

Closed andig closed 1 year ago

andig commented 1 year ago 
got ./...
panic triggered: test
interrupt
interrupt
~~~ FAIL: (Terst)
    runtime_test.go:592:
        FAIL (==)
             got: 1073741823 (int32)
        expected: -1073741824 (int)
--- FAIL: TestBinaryShiftOperation (0.00s)
stevenh commented 1 year ago

Latest master is passing for me:

go test -run=TestBinaryShiftOperation -v
=== RUN   TestBinaryShiftOperation
--- PASS: TestBinaryShiftOperation (0.00s)
PASS
ok      github.com/robertkrimen/otto    0.004s

Do you happen to testing on a 32bit architecture?

andig commented 1 year ago

Fails on 64bit OSX:

uname -a
Darwin SkyNetM1.local 22.1.0 Darwin Kernel Version 22.1.0: Sun Oct  9 20:14:30 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T8103 arm64

go test -run=TestBinaryShiftOperation -v
Alias tip: got -run=TestBinaryShiftOperation -v
=== RUN   TestBinaryShiftOperation
~~~ FAIL: (Terst)
    runtime_test.go:592:
        FAIL (==)
             got: 1073741823 (int32)
        expected: -1073741824 (int)
--- FAIL: TestBinaryShiftOperation (0.00s)
FAIL
exit status 1
FAIL    github.com/robertkrimen/otto    0.139s
stevenh commented 1 year ago

Hmm, what does the following report:

package main

import (
    "testing"

    "github.com/robertkrimen/otto"
    "github.com/stretchr/testify/require"
)

func Test_issue457(t *testing.T) {
    vm := otto.New()
    val, err := vm.Run(`
        high = (1 << 30) - 1 + (1 << 30);
        low = -high - 1;
        vwx = low >> 1;
        [high, low, vwx];
    `)
    require.NoError(t, err)
    exp, err := val.Export()
    require.NoError(t, err)
    require.Equal(t, []interface{}{float64(2147483647), float64(-2147483648), int32(-1073741824)}, exp)

    t.Log(exp)
}
andig commented 1 year ago 
=== RUN   Test_issue457
    issue2_test.go:20:
            Error Trace:    /Users/andig/htdocs/otto/issue2_test.go:20
            Error:          Not equal:
                            expected: "2147483647,-2147483648,-1073741824"
                            actual  : "2147483647,-2147483648,1073741823"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -2147483647,-2147483648,-1073741824
                            +2147483647,-2147483648,1073741823
            Test:           Test_issue457
--- FAIL: Test_issue457 (0.00s)
stevenh commented 1 year ago

Well that's strange, what about:

package main

import (
    "fmt"
    "testing"

    "github.com/stretchr/testify/require"
)

func Test_issue457(t *testing.T) {
    v := int32(-2147483648) >> 1
    fmt.Printf("%T = %v\n", v, v)
    require.Equal(t, int32(-1073741824), v)
}
andig commented 1 year ago 
got -v issue2_test.go
=== RUN   Test_issue457
int32 = -1073741824
--- PASS: Test_issue457 (0.00s)
PASS
ok      command-line-arguments  0.254s
stevenh commented 1 year ago

Ok could you apply this patch and rerun the the otto Test_issue457. evaluate.patch.txt

andig commented 1 year ago 
=== RUN   Test_issue457
shift1: otto.Value{kind:2, value:-2.147483648e+09}, otto.Value{kind:2, value:1}
shift2: 2147483647, 1
shift3: 1073741823
    issue2_test.go:20:
            Error Trace:    issue2_test.go:20
            Error:          Not equal:
                            expected: "2147483647,-2147483648,-1073741824"
                            actual  : "2147483647,-2147483648,1073741823"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -2147483647,-2147483648,-1073741824
                            +2147483647,-2147483648,1073741823
            Test:           Test_issue457
--- FAIL: Test_issue457 (0.00s)
stevenh commented 1 year ago

Oh wow, I see:

shift1: otto.Value{kind:2, value:-2.147483648e+09}, otto.Value{kind:2, value:1}
shift2: -2147483648, 1
shift3: -1073741824

This means that converting -2.147483648e+09 to int32 using toInt32(leftValue) is loosing the sign.

Here's an instrumented version, if you could run this.

func Test_issue457_toInt32(t *testing.T) {
    val := int32(-2147483648)
    fval := float64(val)
    fmt.Printf("val: %v, fval: %v\n", val, fval)

    value := Value{kind: 2, value: fval}
    floatValue := value.float64()
    remainder := math.Mod(floatValue, float_2_32)
    fmt.Printf("rem1: %v, float_2_32: %v\n", remainder, float_2_32)
    if remainder > 0 {
        remainder = math.Floor(remainder)
        fmt.Printf("rem2: %v\n", remainder)
    } else {
        remainder = math.Ceil(remainder) + float_2_32
        fmt.Printf("rem3: %v\n", remainder)
    }

    fmt.Printf("rem4: %v, float_2_31: %v\n", remainder, float_2_31)
    if remainder > float_2_31 {
        require.Equal(t, val, int32(remainder-float_2_32))
    }
    require.Equal(t, val, int32(remainder))
}
andig commented 1 year ago 
=== RUN   Test_issue457
shift1: otto.Value{kind:2, value:-2.147483648e+09}, otto.Value{kind:2, value:1}
shift2: 2147483647, 1
shift3: 1073741823
    issue2_test.go:22:
            Error Trace:    issue2_test.go:22
            Error:          Not equal:
                            expected: "2147483647,-2147483648,-1073741824"
                            actual  : "2147483647,-2147483648,1073741823"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -2147483647,-2147483648,-1073741824
                            +2147483647,-2147483648,1073741823
            Test:           Test_issue457
--- FAIL: Test_issue457 (0.00s)
=== RUN   Test_issue457_toInt32
val: -2147483648, fval: -2.147483648e+09
rem1: -2.147483648e+09, float_2_32: 4.294967296e+09
rem3: 2.147483648e+09
rem4: 2.147483648e+09, float_2_31: 2.147483648e+09
    issue2_test.go:47:
            Error Trace:    issue2_test.go:47
            Error:          Not equal:
                            expected: -2147483648
                            actual  : 2147483647
            Test:           Test_issue457_toInt32
--- FAIL: Test_issue457_toInt32 (0.00s)

What I don't get: why aren't you seeing this error on CI? The runners are 64bits afaik. What is different between those and my local machine?

stevenh commented 1 year ago

My machine here is 64bit but x86_64, I thought this was a 32bit arch issue but it could be something more subtle to do with internal architecture.

Looks like you might be on an m1 mac as the uname stated arm64?

Hopefully this final version will uncover the issue:

func Test_issue457_toInt32_2(t *testing.T) {
    val := int32(-2147483648)
    fval := float64(val)
    fmt.Printf("maxInt32: %v minInt32: %v\n", math.MaxInt32, math.MinInt32)
    fmt.Printf("float_2_32: %v, float_2_31: %v\n", int64(float_2_32), int64(float_2_31))
    fmt.Printf("val: %v, fval: %v\n", val, fval)

    value := Value{kind: 2, value: fval}
    floatValue := value.float64()
    remainder := math.Mod(floatValue, float_2_32)
    fmt.Printf("rem1: %v, float_2_32: %v\n", remainder, float_2_32)
    if remainder > 0 {
        remainder = math.Floor(remainder)
        fmt.Printf("rem2: %v\n", remainder)
    } else {
        remainder = math.Ceil(remainder) + float_2_32
        fmt.Printf("rem3: %v\n", remainder)
    }

    fmt.Printf("rem4: %v, float_2_31: %v\n", remainder, float_2_31)
    if remainder > float_2_31 {
        fmt.Printf("out1: %v, %T, %v\n", remainder-float_2_32, remainder-float_2_32, int32(remainder-float_2_32))
        require.Equal(t, val, int32(remainder-float_2_32), "out1")
    }
    fmt.Printf("out2: %v, %T, %v\n", remainder, remainder, int32(remainder))
    require.Equal(t, val, int32(remainder), "out2")
    t.FailNow()
}
stevenh commented 1 year ago

We could be seeing something similar to this issue

stevenh commented 1 year ago

Out of interest if you paste this into the console in chrome or other browser what does the following show:

high = (1 << 30) - 1 + (1 << 30);
low = -high - 1;
vwx = low >> 1;
[high, low, vwx];
andig commented 1 year ago

My machine here is 64bit but x86_64, I thought this was a 32bit arch issue but it could be something more subtle to do with internal architecture.

Interesting. Had a quick look at the code and didn't see anything obvious. Still need to look at the issue.

Now:

=== RUN   Test_issue457_toInt32_2
maxInt32: 2147483647 minInt32: -2147483648
float_2_32: 4294967296, float_2_31: 2147483648
val: -2147483648, fval: -2.147483648e+09
rem1: -2.147483648e+09, float_2_32: 4.294967296e+09
rem3: 2.147483648e+09
rem4: 2.147483648e+09, float_2_31: 2.147483648e+09
out2: 2.147483648e+09, float64, 2147483647
    issue2_test.go:75:
            Error Trace:    issue2_test.go:75
            Error:          Not equal:
                            expected: -2147483648
                            actual  : 2147483647
            Test:           Test_issue457_toInt32_2
            Messages:       out2
--- FAIL: Test_issue457_toInt32_2 (0.00s)

And:

Screenshot 2022-11-30 at 13 07 14
stevenh commented 1 year ago

Ok what about this:

func Test_issue457_toInt32_3(t *testing.T) {
    val := int32(-2147483648)
    floatValue := float64(val)

    if math.IsNaN(floatValue) || math.IsInf(floatValue, 0) || floatValue == 0 {
        fmt.Println("zero")
        t.FailNow()
        return
    }

    abs := math.Abs(floatValue)
    fmt.Printf("abs: %v\n", abs)
    iv := math.Floor(abs)
    fmt.Printf("iv: %v\n", iv)
    iv32 := math.Mod(iv, float_2_32)
    fmt.Printf("iv32: %v\n", iv32)
    if iv32 >= float_2_31 {
        res := int32(iv32 - float_2_32)
        fmt.Printf("res1: %v\n", res)
        t.FailNow()
        return
    }
    fmt.Printf("res2: %v\n", int32(iv32))
    t.FailNow()
}
andig commented 1 year ago 
=== RUN   Test_issue457_toInt32_3
abs: 2.147483648e+09
iv: 2.147483648e+09
iv32: 2.147483648e+09
res1: -2147483648
--- FAIL: Test_issue457_toInt32_3 (0.00s)
andig commented 1 year ago

Maybe I could help: what is the conversion you're trying to achieve? Float to int32? What is the desired over/underflow behaviour? Then I could see if I can code something that works here.

Also see https://stackoverflow.com/questions/8022389/convert-a-float64-to-an-int-in-go:

Such casts have a problem in Go that can be unexpected (at least if you come from Java): "In all non-constant conversions involving floating-point or complex values, if the result type cannot represent the value the conversion succeeds but the result value is implementation-dependent." (golang.org/ref/spec#Conversions). So if you use a very large value effectively as an infinity, and you convert it into an int, the result is undefined (unlike Java, where it is the maximal value of the integer type). – Jaan May 27, 2016 at 14:37

stevenh commented 1 year ago

Can you try this https://github.com/robertkrimen/otto/pull/474

andig commented 1 year ago

Probably offtopic, but anyway:

func TestSimple(t *testing.T) {
    val := int32(-2147483648)
    floatVal := float64(val)

    var res int32

    switch {
    case floatVal > float64(math.MaxInt32):
        res = math.MaxInt32
    case floatVal < float64(math.MinInt32):
        res = math.MinInt32
    default:
        res = int32(floatVal)
    }

    t.Log(res)
}
stevenh commented 1 year ago

Needs more work

stevenh commented 1 year ago

Ok try that https://github.com/robertkrimen/otto/pull/474 now please @andig.

If this works I have one more change to try, which makes the functions really simple, but wanted to try this first.

andig commented 1 year ago

I guess you've fixed the original one? Here's my entire output:

=== RUN   Test_issue457
    issue2_test.go:23: 2147483647,-2147483648,-1073741824
--- PASS: Test_issue457 (0.00s)
=== RUN   Test_issue457_toInt32
val: -2147483648, fval: -2.147483648e+09
rem1: -2.147483648e+09, float_2_32: 4.294967296e+09
rem3: 2.147483648e+09
rem4: 2.147483648e+09, float_2_31: 2.147483648e+09
    issue2_test.go:47:
            Error Trace:    /Users/andig/htdocs/otto/issue2_test.go:47
            Error:          Not equal:
                            expected: -2147483648
                            actual  : 2147483647
            Test:           Test_issue457_toInt32
--- FAIL: Test_issue457_toInt32 (0.00s)
=== RUN   Test_issue457_toInt32_2
maxInt32: 2147483647 minInt32: -2147483648
float_2_32: 4294967296, float_2_31: 2147483648
val: -2147483648, fval: -2.147483648e+09
rem1: -2.147483648e+09, float_2_32: 4.294967296e+09
rem3: 2.147483648e+09
rem4: 2.147483648e+09, float_2_31: 2.147483648e+09
out2: 2.147483648e+09, float64, 2147483647
    issue2_test.go:75:
            Error Trace:    /Users/andig/htdocs/otto/issue2_test.go:75
            Error:          Not equal:
                            expected: -2147483648
                            actual  : 2147483647
            Test:           Test_issue457_toInt32_2
            Messages:       out2
--- FAIL: Test_issue457_toInt32_2 (0.00s)
=== RUN   Test_issue457_toInt32_3
abs: 2.147483648e+09
iv: 2.147483648e+09
iv32: 2.147483648e+09
res1: -2147483648
--- FAIL: Test_issue457_toInt32_3 (0.00s)
stevenh commented 1 year ago

Ok so hopefully one last go, just pushed another update. If you delete all the addon tests you should see a clean pass on go test ./... from the root of the repo.

andig commented 1 year ago

You're not gonna like it:

got ./...
~~~ FAIL: (Terst)
    issue_test.go:639:
        FAIL (==)
             got: 000000000626540e598e480000000000000000004048391f4a1929274d7c695c (*otto._object=ptr)
        expected: aa747c502a898200f9e4fa21bac68136f886a0e27aec70ba06daf2e2a5cb5597
--- FAIL: Test_issue87 (0.01s)
panic triggered: test
interrupt
interrupt
~~~ FAIL: (Terst)
    runtime_test.go:593:
        FAIL (==)
             got: 0 (uint32)
        expected: 1073741824 (int)
--- FAIL: TestBinaryShiftOperation (0.00s)
~~~ FAIL: (Terst)
    string_test.go:67:
        FAIL (==)
             got: 0 (uint16)
        expected: 65535 (int)
--- FAIL: TestString_fromCharCode (0.00s)
~~~ FAIL: (Terst)
    value_test.go:174:
        FAIL (==)
             got: 0 (uint32)
        expected: 2147483647 (uint32)
    value_test.go:174:
        FAIL (==)
             got: 0 (uint32)
        expected: 4294967295 (uint32)
    value_test.go:174:
        FAIL (==)
             got: 0 (uint32)
        expected: 1 (uint32)
--- FAIL: Test_toUint32 (0.00s)
~~~ FAIL: (Terst)
    value_test.go:196:
        FAIL (==)
             got: 0 (uint16)
        expected: 65535 (uint16)
    value_test.go:196:
        FAIL (==)
             got: 0 (uint16)
        expected: 65535 (uint16)
    value_test.go:196:
        FAIL (==)
             got: 0 (uint16)
        expected: 1 (uint16)
--- FAIL: Test_toUint16 (0.00s)
FAIL
FAIL    github.com/robertkrimen/otto    1.214s
ok      github.com/robertkrimen/otto/ast    0.256s
?       github.com/robertkrimen/otto/dbg    [no test files]
?       github.com/robertkrimen/otto/file   [no test files]
?       github.com/robertkrimen/otto/otto   [no test files]
ok      github.com/robertkrimen/otto/parser 0.175s
?       github.com/robertkrimen/otto/registry   [no test files]
?       github.com/robertkrimen/otto/repl   [no test files]
?       github.com/robertkrimen/otto/terst  [no test files]
?       github.com/robertkrimen/otto/test   [no test files]
?       github.com/robertkrimen/otto/token  [no test files]
?       github.com/robertkrimen/otto/underscore [no test files]
FAIL

at

commit 3376c332e8f9c4e21cc2b41b29c85635ff7b7d7b (HEAD -> fix/number-to-ints, origin/fix/number-to-ints)
Author: Steven Hartland <steven.hartland@multiplay.co.uk>
Date:   Wed Nov 30 15:17:41 2022 +0000

    fix: leverage go casts for integer conversion

    Leverage the power of go casts to simplify the conversion from float64
    to integer types.

 value_number.go | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)
stevenh commented 1 year ago

To confirm the previous commit was fine?

stevenh commented 1 year ago

I wonder if it always needs int64 cast to ensure signed, just pushed a change to that effect, see if that fixes those uintXX failures?

andig commented 1 year ago

No. Same result, but we also didn't test these before:

got ./...
~~~ FAIL: (Terst)
    issue_test.go:639:
        FAIL (==)
             got: 000000000626540e598e480000000000000000004048391f4a1929274d7c695c (*otto._object=ptr)
        expected: aa747c502a898200f9e4fa21bac68136f886a0e27aec70ba06daf2e2a5cb5597
--- FAIL: Test_issue87 (0.01s)
panic triggered: test
interrupt
interrupt
~~~ FAIL: (Terst)
    runtime_test.go:593:
        FAIL (==)
             got: 0 (uint32)
        expected: 1073741824 (int)
--- FAIL: TestBinaryShiftOperation (0.00s)
~~~ FAIL: (Terst)
    string_test.go:67:
        FAIL (==)
             got: 0 (uint16)
        expected: 65535 (int)
--- FAIL: TestString_fromCharCode (0.00s)
~~~ FAIL: (Terst)
    value_test.go:174:
        FAIL (==)
             got: 0 (uint32)
        expected: 2147483647 (uint32)
    value_test.go:174:
        FAIL (==)
             got: 0 (uint32)
        expected: 4294967295 (uint32)
    value_test.go:174:
        FAIL (==)
             got: 0 (uint32)
        expected: 1 (uint32)
--- FAIL: Test_toUint32 (0.00s)
~~~ FAIL: (Terst)
    value_test.go:196:
        FAIL (==)
             got: 0 (uint16)
        expected: 65535 (uint16)
    value_test.go:196:
        FAIL (==)
             got: 0 (uint16)
        expected: 65535 (uint16)
    value_test.go:196:
        FAIL (==)
             got: 0 (uint16)
        expected: 1 (uint16)
--- FAIL: Test_toUint16 (0.00s)
FAIL
FAIL    github.com/robertkrimen/otto    1.101s
ok      github.com/robertkrimen/otto/ast    (cached)
?       github.com/robertkrimen/otto/dbg    [no test files]
?       github.com/robertkrimen/otto/file   [no test files]
?       github.com/robertkrimen/otto/otto   [no test files]
ok      github.com/robertkrimen/otto/parser (cached)
?       github.com/robertkrimen/otto/registry   [no test files]
?       github.com/robertkrimen/otto/repl   [no test files]
?       github.com/robertkrimen/otto/terst  [no test files]
?       github.com/robertkrimen/otto/test   [no test files]
?       github.com/robertkrimen/otto/token  [no test files]
?       github.com/robertkrimen/otto/underscore [no test files]
FAIL

at

commit f062ffba5edb28c8e1526ef15a6f1207ea51c83c (HEAD -> fix/number-to-ints)
Author: Steven Hartland <steven.hartland@multiplay.co.uk>
Date:   Wed Nov 30 15:11:54 2022 +0000

    fix: sign and overflow

    Fix sign and overflow / underflow cast issues.
andig commented 1 year ago

I've only run the -run Test_issue457 line before.

andig commented 1 year ago

Passing with latest commit:

commit a4c7202cfd14ce01451d87b48c0bce0620431b86 (HEAD -> fix/number-to-ints, origin/fix/number-to-ints)
Author: Steven Hartland <steven.hartland@multiplay.co.uk>
Date:   Wed Nov 30 16:58:51 2022 +0000

    fix: use int64 not uint64 for uint cases

    Try using int64 instead of uint64 for uint cases.
stevenh commented 1 year ago

Big thanks for all your help on this one @andig!

andig commented 1 year ago

Thanks for figuring it out- I know how tedious that is when you can't test yourself...