THRIFT-5697: Use 32 bit ints for generated go enums

[garymm]

No description provided.

[dcelasun]

I know this isn't spec compliant, but changing it would be a big BC break. I'm leaning towards no.

cc @fishy

[fishy]

I know this isn't spec compliant, but changing it would be a big BC break. I'm leaning towards no.

cc @fishy

yea I plan to assess how breaking this is. not changing it only has very minimal impact (the only thing that could go wrong is someone try to use a value outside of i32 range, which would be very rare).

[jimexist]

I wonder who this can pass without changing any unit tests. There are no tests that cover this?

[fishy]

There's one test that's broken (@garymm you need to rebase your branch on top of latest master to be able to run tests):

$ git diff
diff --git a/lib/go/test/tests/validate_test.go b/lib/go/test/tests/validate_test.go
index 957a8df03..a16d427a1 100644
--- a/lib/go/test/tests/validate_test.go
+++ b/lib/go/test/tests/validate_test.go
@@ -178,7 +178,7 @@ func TestBasicValidator(t *testing.T) {
                t.Errorf("Error cannot be unwrapped into *ValidationError: %v", err)
        }
        bt = validatetest.NewBasicTest()
-       bt.Enum1 = (*validatetest.EnumFoo)(thrift.Int64Ptr(int64(validatetest.EnumFoo_e2)))
+       bt.Enum1 = (*validatetest.EnumFoo)(thrift.Int32Ptr(int32(validatetest.EnumFoo_e2)))
        if err := bt.Validate(); err == nil {
                t.Errorf("Expected vt.in error for Enum1")
        } else if errors.As(err, &ve) {

and we do have cases that even though the enum values themselves do not overflow int32, we use some of the enum values in bitmasks and combine with other enum values that would overflow int32 (e.g. MY_ENUM_VALUE_1 << MY_ENUM_OFFSET_1) so this is breaking.

in compact protocol all ints are zigzag so the bits doesn't really matter (the same number in int32 and int64 will be encoded the same way). in binary protocol int64 will encode differently from int32 (using different number of bytes), but the go library likely just cast enums to int32 then do int32 encoding so it doesn't really break anything (as long as the value itself does not overflow int32).

[garymm]

@fishy fixed the test.

[fishy]

So I tried this change on our code, the bit shift one is actually easy to fix (just cast to int64 before doing bit shifting), but after that is fixed we saw a bunch of tests now breaking, so I agree with @dcelasun that this could be a big backward compatibility break that's not worth it (especially considering that the harm of not fixing it is very limited).

[garymm]

:'-(