THRIFT-5685: Revert "THRIFT-5601: Fix forward typedef in go compiler"

This reverts commit b39370e, and also adds a test case for THRIFT-5685.
apache · Feb 16, 2023 · 8fcdf7d · 8fcdf7d
1 parent 423ab39
commit 8fcdf7d
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 39 deletions.
diff --git 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 is_pointer_field: " + type->get_name();
+  throw "INVALID TYPE IN type_to_go_type: " + 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<t_const_value*, t_const_value*, t_const_value::value_compare>& val = value->get_map();
-    out << "map[" << type_to_go_key_type(ktype) << "]" << type_to_go_container_value_type(vtype) << "{" << endl;
+    out << "map[" << type_to_go_key_type(ktype) << "]" << type_to_go_type(vtype) << "{" << endl;
     indent_up();
     map<t_const_value*, t_const_value*, t_const_value::value_compare>::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<t_const_value*>& val = value->get_list();
-    out << "[]" << type_to_go_container_value_type(etype) << "{" << endl;
+    out << "[]" << type_to_go_type(etype) << "{" << endl;
     indent_up();
     vector<t_const_value*>::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<t_const_value*>& val = value->get_list();
-    out << "[]" << type_to_go_container_value_type(etype) << "{" << endl;
+    out << "[]" << type_to_go_type(etype) << "{" << endl;
     indent_up();
     vector<t_const_value*>::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), false);
+      string goType = type_to_go_type_with_opt(fieldType, is_pointer_field(*m_iter));
 
       map<string,string>tags;
       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, false);
+    string goType = type_to_go_type_with_opt(fieldType, 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, false);
+      string goOptType = type_to_go_type_with_opt(fieldType, true);
       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), false);
+    string goType = type_to_go_type_with_opt(field_type, is_pointer_field(*f_iter));
 
     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_container_value_type(tset->get_elem_type());
+    string goType = type_to_go_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,40 +3892,31 @@ 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_container_value_type(type) + " - aborting.";
+    throw "Cannot produce a valid type for a Go map key: " + type_to_go_type(type) + " - aborting.";
   }
 
   if (resolved_type->is_binary())
     return "string";
 
-  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);
+  return type_to_go_type(type);
 }
 
 /**
  * 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, false);
+  return type_to_go_type_with_opt(type, false);
 }
 
 /**
  * Converts the parse type to a go type, taking into account whether the field
- * associated with the type is T_OPTIONAL and whether it's used in a container
- * type.
+ * associated with the type is T_OPTIONAL.
  */
 string t_go_generator::type_to_go_type_with_opt(t_type* type,
-                                                bool optional_field,
-                                                bool is_container_value) {
+                                                bool optional_field) {
   string maybe_pointer(optional_field ? "*" : "");
 
-  if (is_container_value && type->is_typedef() && ((t_typedef*)type)->is_forward_typedef()) {
+  if (type->is_typedef() && ((t_typedef*)type)->is_forward_typedef()) {
     type = ((t_typedef*)type)->get_true_type();
   }
 
@@ -3974,21 +3965,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_container_value_type(t->get_val_type());
+    string valueType = type_to_go_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_container_value_type(t->get_elem_type());
+    string elemType = type_to_go_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_container_value_type(t->get_elem_type());
+    string elemType = type_to_go_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_with_opt: " + type->get_name();
+  throw "INVALID TYPE IN type_to_go_type: " + 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
@@ -264,9 +264,8 @@ class t_go_generator : public t_generator {
   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, bool is_container_value);
+  std::string type_to_go_type_with_opt(t_type* ttype, bool optional_field);
   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 → lib/go/test/ForwardType.thrift b/lib/go/test/ForwardTypedef.thrift → lib/go/test/ForwardType.thrift
@@ -17,17 +17,14 @@
  * under the License.
  */
 
-// https://issues.apache.org/jira/browse/THRIFT-5601
+// https://issues.apache.org/jira/browse/THRIFT-5685
 
-namespace go forwardtypedef
+namespace go forwardtypetest
 
 struct Struct {
-  1: optional Def foo
-  2: optional Exc bar
+  1: optional Exc foo
 }
 
-typedef i32 Def
-
 exception Exc {
   1: optional i32 code
 }
diff --git a/lib/go/test/Makefile.am b/lib/go/test/Makefile.am
@@ -62,7 +62,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \
 				ProcessorMiddlewareTest.thrift \
 				ClientMiddlewareExceptionTest.thrift \
 				ValidateTest.thrift \
-				ForwardTypedef.thrift
+				ForwardType.thrift
 	mkdir -p gopath/src
 	grep -v list.*map.*list.*map $(THRIFTTEST) | grep -v 'set<Insanity>' > ThriftTest.thrift
 	$(THRIFT) $(THRIFTARGS) -r IncludesTest.thrift
@@ -97,7 +97,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \
 	$(THRIFT) $(THRIFTARGS_SKIP_REMOTE) ProcessorMiddlewareTest.thrift
 	$(THRIFT) $(THRIFTARGS) ClientMiddlewareExceptionTest.thrift
 	$(THRIFT) $(THRIFTARGS) ValidateTest.thrift
-	$(THRIFT) $(THRIFTARGS) ForwardTypedef.thrift
+	$(THRIFT) $(THRIFTARGS) ForwardType.thrift
 	ln -nfs ../../tests gopath/src/tests
 	cp -r ./dontexportrwtest gopath/src
 	touch gopath
@@ -124,7 +124,7 @@ check: gopath
 				./gopath/src/processormiddlewaretest \
 				./gopath/src/clientmiddlewareexceptiontest \
 				./gopath/src/validatetest \
-				./gopath/src/forwardtypedef
+				./gopath/src/forwardtypetest
 	$(GO) test github.com/apache/thrift/lib/go/thrift
 	$(GO) test ./gopath/src/tests ./gopath/src/dontexportrwtest
 
@@ -155,6 +155,7 @@ EXTRA_DIST = \
 	DuplicateImportsTest.thrift \
 	ErrorTest.thrift \
 	EqualsTest.thrift \
+	ForwardType.thrift \
 	GoTagTest.thrift \
 	IgnoreInitialismsTest.thrift \
 	IncludesTest.thrift \

diff --git a/lib/go/test/tests/forwardtype_test.go b/lib/go/test/tests/forwardtype_test.go
@@ -0,0 +1,41 @@
+/*
+ * 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.
+ */
+
+package tests
+
+import (
+	"testing"
+
+	"github.com/apache/thrift/lib/go/test/gopath/src/forwardtypetest"
+	"github.com/apache/thrift/lib/go/thrift"
+)
+
+func TestForwardType(t *testing.T) {
+	// See https://issues.apache.org/jira/browse/THRIFT-5685
+
+	const code = int32(1)
+	foo := &forwardtypetest.Struct{
+		Foo: &forwardtypetest.Exc{
+			Code: thrift.Pointer(code),
+		},
+	}
+	if got, want := foo.GetFoo().GetCode(), code; got != want {
+		t.Errorf("code got %v want %v", got, want)
+	}
+}