[
https://issues.apache.org/jira/browse/THRIFT-4868?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
John Boiles updated THRIFT-4868:
--------------------------------
Description:
There's a bug in the Thrift Go generator that happens when optional set types
are used with a default field. For example, a simple struct like this:
{code:java}
struct SetPointerTest {
1: optional set<string> with_default = [ "test" ]
}
{code}
Generates a struct like this
{code}
type SetPointerTest struct {
WithDefault *[]string `thrift:"with_default,1" db:"with_default"
json:"with_default"`
}
{code}
And associated Go code with a line like this:
{code:go}
if reflect.DeepEqual(*p.WithDefault[i],*p.WithDefault[j]) {
{code}
Which fails with an error
{code}
gen-go/setpointertest/SetPointerTest.go:125:44: invalid operation:
p.WithDefault[i] (type *[]string does not support indexing)
{code}
Thrift optional sets only get translated to Go pointer values (causing this
error) when a default value is set. This is because the
{{t_go_generator::is_pointer_field}} method has [some
logic|https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_go_generator.cc#L410-L411]
that returns {{true}} if the set has a default value. I'm unsure why this is
the case; it seems simpler to not change the type when there's a default value.
A Golang slice is a pointer value by default, so {{nil}} can still be used for
{{optional}} fields. I'd be super interested to understand why the code was
written this way.
However, since changing the pointer behavior would be a version-incompatible
change, I propose we do the simple thing for now to fix Go compilation. I'll
submit a patch with a test shortly.
was:
There's a bug in the Thrift Go generator that happens when optional set types
are used with a default field. For example, a simple struct like this:
{code:java}
struct SetPointerTest {
1: optional set<string> with_default = [ "test" ]
}
{code}
Generates a struct like this
{code}
type SetPointerTest struct {
WithDefault *[]string `thrift:"with_default,1" db:"with_default"
json:"with_default"`
}
{code}
And associated Go code with a line like this:
{code:go}
if reflect.DeepEqual(*p.WithDefault[i],*p.WithDefault[j]) {
{code}
Which fails with an error
{code}
gen-go/setpointertest/SetPointerTest.go:125:44: invalid operation:
p.WithDefault[i] (type *[]string does not support indexing)
{code}
Thrift sets only get translated to Go pointer values (causing this error) when
a default value is set. This is because the
{{t_go_generator::is_pointer_field}} method has [some
logic|https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_go_generator.cc#L410-L411]
that returns {{true}} if the set has a default value. I'm unsure why this is
the case; it seems simpler to not change the type when there's a default value.
A Golang slice is a pointer value by default, so {{nil}} can still be used for
{{optional}} fields. I'd be super interested to understand why the code was
written this way.
However, since changing the pointer behavior would be a version-incompatible
change, I propose we do the simple thing for now to fix Go compilation. I'll
submit a patch with a test shortly.
> Go bug for optional sets with a default value
> ---------------------------------------------
>
> Key: THRIFT-4868
> URL: https://issues.apache.org/jira/browse/THRIFT-4868
> Project: Thrift
> Issue Type: Task
> Components: Go - Compiler
> Affects Versions: 0.12.0
> Reporter: John Boiles
> Priority: Major
>
> There's a bug in the Thrift Go generator that happens when optional set types
> are used with a default field. For example, a simple struct like this:
> {code:java}
> struct SetPointerTest {
> 1: optional set<string> with_default = [ "test" ]
> }
> {code}
> Generates a struct like this
> {code}
> type SetPointerTest struct {
> WithDefault *[]string `thrift:"with_default,1" db:"with_default"
> json:"with_default"`
> }
> {code}
> And associated Go code with a line like this:
> {code:go}
> if reflect.DeepEqual(*p.WithDefault[i],*p.WithDefault[j]) {
> {code}
> Which fails with an error
> {code}
> gen-go/setpointertest/SetPointerTest.go:125:44: invalid operation:
> p.WithDefault[i] (type *[]string does not support indexing)
> {code}
> Thrift optional sets only get translated to Go pointer values (causing this
> error) when a default value is set. This is because the
> {{t_go_generator::is_pointer_field}} method has [some
> logic|https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/generate/t_go_generator.cc#L410-L411]
> that returns {{true}} if the set has a default value. I'm unsure why this is
> the case; it seems simpler to not change the type when there's a default
> value. A Golang slice is a pointer value by default, so {{nil}} can still be
> used for {{optional}} fields. I'd be super interested to understand why the
> code was written this way.
> However, since changing the pointer behavior would be a version-incompatible
> change, I propose we do the simple thing for now to fix Go compilation. I'll
> submit a patch with a test shortly.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
