Skip to content

Commit

Permalink
cue: add experiment to default to int64 in Value.Decode for integers
Browse files Browse the repository at this point in the history
We were defaulting to int rather than int32 or int64, so the behavior
changed depending on whether the host platform was 32-bit or 64-bit.

This got caught by the encoding/toml tests, which use a hexadecimal
value which happens to be large enough to not fit into int32.
Thus, the tests roundtripping from TOML to CUE and back to TOML would
make the value overflow and become negative, causing a test failure.

This issue was not covered in TestDecode as none of the test cases
used large enough numbers. Add them, verifying that they showed
the wrong behavior on GOARCH=386.

    We can soon start testing this by adding a CI step on amd64 like:

        GOARCH=386 go test -short ./...

The TOML tests also needed tweaking. First, don't rely on JSONEquals,
as that doesn't spot cases where we lose type information,
and we were doing just that by re-encoding dates and times as strings.
Second, the use of a large integer in one of the tests again caused
the value to be re-encoded as a string on 32-bit architectures.
Both of these subtest cases are skipped with a TODO for now.

We don't do this change abruptly, as it could subtly break a number of
downstream CUE users who may be using type switches or other reflection
logic on the Go values obtained when decoding into `any`.
Instead, we add a new experiment via `CUE_EXPERIMENT=decodeint64`,
which will be off by default in v0.11, on by default in v0.12,
and then sunset entirely in v0.13 assuming all goes well.

This should give end users two full release cycles, or about six months,
to adapt to the slight change in semantics underfoot.

Updates #3540.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I2b810ec972b60b4f0146ce512acaa1b585ad837f
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1203193
Reviewed-by: Marcel van Lohuizen <mpvl@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Roger Peppe <rogpeppe@gmail.com>
  • Loading branch information
mvdan committed Oct 31, 2024
1 parent dd85ce4 commit 00bd25b
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 4 deletions.
3 changes: 3 additions & 0 deletions cmd/cue/cmd/help.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,9 @@ If an environment variable is unset or empty, sensible default setting is used.
embed
Enable support for embedded data files as described in
https://cuelang.org/discussion/3264.
decodeint64
Tweak cue.Value.Decode to choose "int64" rather than "int"
as the default Go type for CUE integer values.
CUE_DEBUG
Comma-separated list of debug flags to enable or disable, such as:
Expand Down
14 changes: 13 additions & 1 deletion cue/decode.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"cmp"
"encoding"
"encoding/json"
"math"
"reflect"
"slices"
"strconv"
Expand All @@ -29,6 +30,7 @@ import (

"cuelang.org/go/cue/errors"
"cuelang.org/go/internal/core/adt"
"cuelang.org/go/internal/cueexperiment"
)

// Decode initializes the value pointed to by x with Value v.
Expand Down Expand Up @@ -268,7 +270,17 @@ func (d *decoder) interfaceValue(v Value) (x interface{}) {

case IntKind:
if i, err := v.Int64(); err == nil {
return int(i)
cueexperiment.Init()
if cueexperiment.Flags.DecodeInt64 {
return i
}
// When the decodeint64 experiment is not enabled, we want to return the value
// as `int`, but that's not possible for large values on 32-bit architectures.
// To avoid overflows causing entirely wrong values to be returned to the user,
// let the logic continue below so that we return a *big.Int instead.
if i <= math.MaxInt && i >= math.MinInt {
return int(i)
}
}
x, err = v.Int(nil)

Expand Down
58 changes: 57 additions & 1 deletion cue/decode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ package cue_test

import (
"fmt"
"math/big"
"math/bits"
"reflect"
"strconv"
"testing"
"time"

Expand Down Expand Up @@ -239,21 +242,74 @@ func TestDecode(t *testing.T) {
value: `[]`,
dst: new(interface{}),
want: []interface{}{},
}, {
// large integer which doesn't fit into an int32 or int on 32-bit platforms
value: `8000000000`,
dst: new(interface{}),
want: intOver32("8000000000"),
}, {
// same as the above, but negative
value: `-8000000000`,
dst: new(interface{}),
want: intOver32("-8000000000"),
}, {
// even larger integer which doesn't fit into an int64
value: `9500000000000000000`,
dst: new(interface{}),
want: bigInt("9500000000000000000"),
}, {
// same as the above, but negative
value: `-9500000000000000000`,
dst: new(interface{}),
want: bigInt("-9500000000000000000"),
}, {
// large float which doesn't fit into a float32
value: `1.797693134e+308`,
dst: new(interface{}),
want: float64(1.797693134e+308),
}, {
// even larger float which doesn't fit into a float64
// TODO(mvdan): this should work via *big.Float, just like we do with *big.Int above.
value: `1.99769313499e+508`,
dst: new(interface{}),
err: "value was rounded up",
}, {
// same as the above, but negative
// TODO(mvdan): this should work via *big.Float, just like we do with *big.Int above.
value: `-1.99769313499e+508`,
dst: new(interface{}),
err: "value was rounded down",
}}
for _, tc := range testCases {
cuetdtest.FullMatrix.Run(t, tc.value, func(t *testing.T, m *cuetdtest.M) {
err := getValue(m, tc.value).Decode(tc.dst)
checkFatal(t, err, tc.err, "init")

got := reflect.ValueOf(tc.dst).Elem().Interface()
if diff := cmp.Diff(got, tc.want); diff != "" {
if diff := cmp.Diff(got, tc.want, cmp.Comparer(func(a, b *big.Int) bool {
return a.Cmp(b) == 0
})); diff != "" {
t.Error(diff)
t.Errorf("\n%#v\n%#v", got, tc.want)
}
})
}
}

func intOver32(s string) any {
if bits.UintSize == 32 {
return bigInt(s)
}
// Work around "constant overflows int" typechecking errors on 32 bits.
n, _ := strconv.Atoi(s)
return n
}

func bigInt(s string) *big.Int {
n, _ := big.NewInt(0).SetString(s, 10)
return n
}

func TestDecodeIntoCUEValue(t *testing.T) {
cuetdtest.FullMatrix.Do(t, func(t *testing.T, m *cuetdtest.M) {
// We should be able to decode into a CUE value so we can
Expand Down
12 changes: 11 additions & 1 deletion encoding/toml/decode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"bytes"
"encoding/json"
"io"
"math/bits"
"path"
"reflect"
"strings"
Expand Down Expand Up @@ -867,6 +868,14 @@ line two.\
// Ensure that the decoded CUE can be re-encoded as TOML,
// and the resulting TOML is still JSON-equivalent.
t.Run("reencode", func(t *testing.T) {
switch test.name {
case "Integers":
if bits.UintSize == 32 {
t.Skip("TODO(mvdan): big integers always encode as TOML strings today; can be resolved once CUE_EXPERIMENT=decodeint64 is enabled")
}
case "DateTimes":
t.Skip("TODO(mvdan): dates and times always encode as TOML strings today")
}
sb := new(strings.Builder)
enc := toml.NewEncoder(sb)

Expand All @@ -878,7 +887,8 @@ line two.\
var unmarshalCueTOML any
err = gotoml.Unmarshal([]byte(cueTOML), &unmarshalCueTOML)
qt.Assert(t, qt.IsNil(err))
qt.Assert(t, qt.JSONEquals(jsonCUE, unmarshalCueTOML))

qt.Assert(t, qt.CmpEquals(unmarshalCueTOML, unmarshalTOML))
})
})
}
Expand Down
7 changes: 6 additions & 1 deletion internal/cueexperiment/exp.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,13 @@ var Flags struct {
// performance concerns.
EvalV3 bool

// Embed enabled file embedding.
// Embed enables file embedding.
Embed bool

// DecodeInt64 changes [cuelang.org/go/cue.Value.Decode] to choose
// `int64` rather than `int` as the default type for CUE integer values
// to ensure consistency with 32-bit platforms.
DecodeInt64 bool
}

// Init initializes Flags. Note: this isn't named "init" because we
Expand Down

0 comments on commit 00bd25b

Please sign in to comment.