From b39370ec3bc96d201bbc82fbde136f98ae605ed1 Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Thu, 29 Dec 2022 15:31:38 -0800 Subject: THRIFT-5601: Fix forward typedef in go compiler Client: go While https://github.com/apache/thrift/pull/951 fixed the bug with forward typedef used in container values, it also introduced a bug that broke forward typedef used in other cases in go code. Limit the fix of it to only the container key and value types to fix other cases. --- compiler/cpp/src/thrift/generate/t_go_generator.cc | 47 +++++++++++++--------- compiler/cpp/src/thrift/generate/t_go_generator.h | 3 +- lib/go/test/ForwardTypedef.thrift | 33 +++++++++++++++ lib/go/test/Makefile.am | 7 +++- 4 files changed, 68 insertions(+), 22 deletions(-) create mode 100644 lib/go/test/ForwardTypedef.thrift diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.cc b/compiler/cpp/src/thrift/generate/t_go_generator.cc index 90353ce9b..680243d1a 100644 --- a/compiler/cpp/src/thrift/generate/t_go_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_go_generator.cc @@ -175,7 +175,7 @@ bool t_go_generator::is_pointer_field(t_field* tfield, bool in_container_value) return has_default; } - throw "INVALID TYPE IN type_to_go_type: " + type->get_name(); + throw "INVALID TYPE IN is_pointer_field: " + type->get_name(); } std::string t_go_generator::camelcase(const std::string& value) const { @@ -1088,7 +1088,7 @@ string t_go_generator::render_const_value(t_type* type, t_const_value* value, co t_type* ktype = ((t_map*)type)->get_key_type(); t_type* vtype = ((t_map*)type)->get_val_type(); const map& val = value->get_map(); - out << "map[" << type_to_go_key_type(ktype) << "]" << type_to_go_type(vtype) << "{" << endl; + out << "map[" << type_to_go_key_type(ktype) << "]" << type_to_go_container_value_type(vtype) << "{" << endl; indent_up(); map::const_iterator v_iter; @@ -1102,7 +1102,7 @@ string t_go_generator::render_const_value(t_type* type, t_const_value* value, co } else if (type->is_list()) { t_type* etype = ((t_list*)type)->get_elem_type(); const vector& val = value->get_list(); - out << "[]" << type_to_go_type(etype) << "{" << endl; + out << "[]" << type_to_go_container_value_type(etype) << "{" << endl; indent_up(); vector::const_iterator v_iter; @@ -1115,7 +1115,7 @@ string t_go_generator::render_const_value(t_type* type, t_const_value* value, co } else if (type->is_set()) { t_type* etype = ((t_set*)type)->get_elem_type(); const vector& val = value->get_list(); - out << "[]" << type_to_go_type(etype) << "{" << endl; + out << "[]" << type_to_go_container_value_type(etype) << "{" << endl; indent_up(); vector::const_iterator v_iter; @@ -1258,7 +1258,7 @@ void t_go_generator::generate_go_struct_definition(ostream& out, } t_type* fieldType = (*m_iter)->get_type(); - string goType = type_to_go_type_with_opt(fieldType, is_pointer_field(*m_iter)); + string goType = type_to_go_type_with_opt(fieldType, is_pointer_field(*m_iter), false); maptags; tags["db"]=escape_string((*m_iter)->get_name()); @@ -1317,7 +1317,7 @@ void t_go_generator::generate_go_struct_definition(ostream& out, t_const_value* def_value; get_publicized_name_and_def_value(*m_iter, &publicized_name, &def_value); t_type* fieldType = (*m_iter)->get_type(); - string goType = type_to_go_type_with_opt(fieldType, false); + string goType = type_to_go_type_with_opt(fieldType, false, false); string def_var_name = tstruct_name + "_" + publicized_name + "_DEFAULT"; if ((*m_iter)->get_req() == t_field::T_OPTIONAL || is_pointer_field(*m_iter)) { out << indent() << "var " << def_var_name << " " << goType; @@ -1335,7 +1335,7 @@ void t_go_generator::generate_go_struct_definition(ostream& out, } if (is_pointer_field(*m_iter)) { - string goOptType = type_to_go_type_with_opt(fieldType, true); + string goOptType = type_to_go_type_with_opt(fieldType, true, false); string maybepointer = goOptType != goType ? "*" : ""; out << indent() << "func (p *" << tstruct_name << ") Get" << publicized_name << "() " << goType << " {" << endl; @@ -1758,7 +1758,7 @@ void t_go_generator::generate_go_struct_equals(ostream& out, field_name = (*f_iter)->get_name(); t_type* field_type = (*f_iter)->get_type(); publicize_field_name = publicize(field_name); - string goType = type_to_go_type_with_opt(field_type, is_pointer_field(*f_iter)); + string goType = type_to_go_type_with_opt(field_type, is_pointer_field(*f_iter), false); string tgt = "p." + publicize_field_name; string src = "other." + publicize_field_name; @@ -3431,7 +3431,7 @@ void t_go_generator::generate_serialize_container(ostream& out, if (pointer_field) { wrapped_prefix = "(" + prefix + ")"; } - string goType = type_to_go_type(tset->get_elem_type()); + string goType = type_to_go_container_value_type(tset->get_elem_type()); out << indent() << "if func(tgt, src " << goType << ") bool {" << endl; indent_up(); generate_go_equals(out, tset->get_elem_type(), "tgt", "src"); @@ -3892,31 +3892,40 @@ string t_go_generator::type_to_go_key_type(t_type* type) { } if (resolved_type->is_map() || resolved_type->is_list() || resolved_type->is_set()) { - throw "Cannot produce a valid type for a Go map key: " + type_to_go_type(type) + " - aborting."; + throw "Cannot produce a valid type for a Go map key: " + type_to_go_container_value_type(type) + " - aborting."; } if (resolved_type->is_binary()) return "string"; - return type_to_go_type(type); + return type_to_go_container_value_type(type); +} + +/** + * Converts the parse type to a go type to be used in a container value + */ +string t_go_generator::type_to_go_container_value_type(t_type* type) { + return type_to_go_type_with_opt(type, false, true); } /** * Converts the parse type to a go type */ string t_go_generator::type_to_go_type(t_type* type) { - return type_to_go_type_with_opt(type, false); + return type_to_go_type_with_opt(type, false, false); } /** * Converts the parse type to a go type, taking into account whether the field - * associated with the type is T_OPTIONAL. + * associated with the type is T_OPTIONAL and whether it's used in a container + * type. */ string t_go_generator::type_to_go_type_with_opt(t_type* type, - bool optional_field) { + bool optional_field, + bool is_container_value) { string maybe_pointer(optional_field ? "*" : ""); - if (type->is_typedef() && ((t_typedef*)type)->is_forward_typedef()) { + if (is_container_value && type->is_typedef() && ((t_typedef*)type)->is_forward_typedef()) { type = ((t_typedef*)type)->get_true_type(); } @@ -3965,21 +3974,21 @@ string t_go_generator::type_to_go_type_with_opt(t_type* type, } else if (type->is_map()) { t_map* t = (t_map*)type; string keyType = type_to_go_key_type(t->get_key_type()); - string valueType = type_to_go_type(t->get_val_type()); + string valueType = type_to_go_container_value_type(t->get_val_type()); return maybe_pointer + string("map[") + keyType + "]" + valueType; } else if (type->is_set()) { t_set* t = (t_set*)type; - string elemType = type_to_go_type(t->get_elem_type()); + string elemType = type_to_go_container_value_type(t->get_elem_type()); return maybe_pointer + string("[]") + elemType; } else if (type->is_list()) { t_list* t = (t_list*)type; - string elemType = type_to_go_type(t->get_elem_type()); + string elemType = type_to_go_container_value_type(t->get_elem_type()); return maybe_pointer + string("[]") + elemType; } else if (type->is_typedef()) { return maybe_pointer + publicize(type_name(type)); } - throw "INVALID TYPE IN type_to_go_type: " + type->get_name(); + throw "INVALID TYPE IN type_to_go_type_with_opt: " + type->get_name(); } /** See the comment inside generate_go_struct_definition for what this is. */ diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.h b/compiler/cpp/src/thrift/generate/t_go_generator.h index a67485c55..ad6cee636 100644 --- a/compiler/cpp/src/thrift/generate/t_go_generator.h +++ b/compiler/cpp/src/thrift/generate/t_go_generator.h @@ -264,8 +264,9 @@ public: std::string argument_list(t_struct* tstruct); std::string type_to_enum(t_type* ttype); std::string type_to_go_type(t_type* ttype); - std::string type_to_go_type_with_opt(t_type* ttype, bool optional_field); + std::string type_to_go_type_with_opt(t_type* ttype, bool optional_field, bool is_container_value); std::string type_to_go_key_type(t_type* ttype); + std::string type_to_go_container_value_type(t_type* ttype); std::string type_to_spec_args(t_type* ttype); void indent_up() { t_generator::indent_up(); } diff --git a/lib/go/test/ForwardTypedef.thrift b/lib/go/test/ForwardTypedef.thrift new file mode 100644 index 000000000..4266b7a28 --- /dev/null +++ b/lib/go/test/ForwardTypedef.thrift @@ -0,0 +1,33 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// https://issues.apache.org/jira/browse/THRIFT-5601 + +namespace go forwardtypedef + +struct Struct { + 1: optional Def foo + 2: optional Exc bar +} + +typedef i32 Def + +exception Exc { + 1: optional i32 code +} diff --git a/lib/go/test/Makefile.am b/lib/go/test/Makefile.am index 663b42e89..c255a8e48 100644 --- a/lib/go/test/Makefile.am +++ b/lib/go/test/Makefile.am @@ -61,7 +61,8 @@ gopath: $(THRIFT) $(THRIFTTEST) \ ConstOptionalField.thrift \ ProcessorMiddlewareTest.thrift \ ClientMiddlewareExceptionTest.thrift \ - ValidateTest.thrift + ValidateTest.thrift \ + ForwardTypedef.thrift mkdir -p gopath/src grep -v list.*map.*list.*map $(THRIFTTEST) | grep -v 'set' > ThriftTest.thrift $(THRIFT) $(THRIFTARGS) -r IncludesTest.thrift @@ -96,6 +97,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \ $(THRIFT) $(THRIFTARGS_SKIP_REMOTE) ProcessorMiddlewareTest.thrift $(THRIFT) $(THRIFTARGS) ClientMiddlewareExceptionTest.thrift $(THRIFT) $(THRIFTARGS) ValidateTest.thrift + $(THRIFT) $(THRIFTARGS) ForwardTypedef.thrift ln -nfs ../../tests gopath/src/tests cp -r ./dontexportrwtest gopath/src touch gopath @@ -121,7 +123,8 @@ check: gopath ./gopath/src/conflictargnamestest \ ./gopath/src/processormiddlewaretest \ ./gopath/src/clientmiddlewareexceptiontest \ - ./gopath/src/validatetest + ./gopath/src/validatetest \ + ./gopath/src/forwardtypedef $(GO) test github.com/apache/thrift/lib/go/thrift $(GO) test ./gopath/src/tests ./gopath/src/dontexportrwtest -- cgit v1.2.1