THRIFT-5389: Fix const generation for optional fields

Client: go

The current compiler will generate uncompilable code when we use
optional enum and/or typedef'd types in a thrift constant.

This fixes the issue, also adds a test for that.

5 files changed, 361 insertions, 11 deletions

diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.cc b/compiler/cpp/src/thrift/generate/t_go_generator.cc
index 8d25d48b5..4ee0e8fbe 100644
--- a/compiler/cpp/src/thrift/generate/t_go_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_go_generator.cc
@@ -1102,6 +1102,10 @@ void t_go_generator::generate_const(t_const* tconst) {
  * validate_types method in main.cc
  */
 string t_go_generator::render_const_value(t_type* type, t_const_value* value, const string& name, bool opt) {
+  string typedef_opt_ptr;
+  if (type->is_typedef()) {
+    typedef_opt_ptr = type_name(type) + "Ptr";
+  }
   type = get_true_type(type);
   std::ostringstream out;
 
@@ -1109,32 +1113,61 @@ string t_go_generator::render_const_value(t_type* type, t_const_value* value, co
     t_base_type::t_base tbase = ((t_base_type*)type)->get_base();
 
     if (opt) {
-      out << "&(&struct{x ";
       switch (tbase) {
         case t_base_type::TYPE_BOOL:
-          out << "bool}{";
+          if (typedef_opt_ptr != "") {
+            out << typedef_opt_ptr;
+          } else {
+            out << "thrift.BoolPtr";
+          }
+          out << "(";
           out << (value->get_integer() > 0 ? "true" : "false");
           break;
 
         case t_base_type::TYPE_I8:
-          out << "int8}{";
+          if (typedef_opt_ptr != "") {
+            out << typedef_opt_ptr;
+          } else {
+            out << "thrift.Int8Ptr";
+          }
+          out << "(";
           out << value->get_integer();
           break;
         case t_base_type::TYPE_I16:
-          out << "int16}{";
+          if (typedef_opt_ptr != "") {
+            out << typedef_opt_ptr;
+          } else {
+            out << "thrift.Int16Ptr";
+          }
+          out << "(";
           out << value->get_integer();
           break;
         case t_base_type::TYPE_I32:
-          out << "int32}{";
+          if (typedef_opt_ptr != "") {
+            out << typedef_opt_ptr;
+          } else {
+            out << "thrift.Int32Ptr";
+          }
+          out << "(";
           out << value->get_integer();
           break;
         case t_base_type::TYPE_I64:
-          out << "int64}{";
+          if (typedef_opt_ptr != "") {
+            out << typedef_opt_ptr;
+          } else {
+            out << "thrift.Int64Ptr";
+          }
+          out << "(";
           out << value->get_integer();
           break;
 
         case t_base_type::TYPE_DOUBLE:
-          out << "float64}{";
+          if (typedef_opt_ptr != "") {
+            out << typedef_opt_ptr;
+          } else {
+            out << "thrift.Float64Ptr";
+          }
+          out << "(";
           if (value->get_type() == t_const_value::CV_INTEGER) {
             out << value->get_integer();
           } else {
@@ -1143,14 +1176,19 @@ string t_go_generator::render_const_value(t_type* type, t_const_value* value, co
           break;
 
         case t_base_type::TYPE_STRING:
-          out << "string}{";
+          if (typedef_opt_ptr != "") {
+            out << typedef_opt_ptr;
+          } else {
+            out << "thrift.StringPtr";
+          }
+          out << "(";
           out << '"' + get_escaped_string(value) + '"';
           break;
 
         default:
           throw "compiler error: no const of base type " + t_base_type::t_base_name(tbase);
       }
-      out << "}).x";
+      out << ")";
     } else {
       switch (tbase) {
         case t_base_type::TYPE_STRING:
@@ -1193,7 +1231,17 @@ string t_go_generator::render_const_value(t_type* type, t_const_value* value, co
       }
     }
   } else if (type->is_enum()) {
-    indent(out) << value->get_integer();
+    if (opt) {
+      if (typedef_opt_ptr != "") {
+        out << typedef_opt_ptr << "(";
+      } else {
+        out << type_name(type) << "Ptr(";
+      }
+    }
+    out << value->get_integer();
+    if (opt) {
+      out << ")";
+    }
   } else if (type->is_struct() || type->is_xception()) {
     out << "&" << publicize(type_name(type)) << "{";
     indent_up();
diff --git a/lib/go/test/ConstOptionalField.thrift b/lib/go/test/ConstOptionalField.thrift
new file mode 100644
index 000000000..dc1df8f5f
--- /dev/null
+++ b/lib/go/test/ConstOptionalField.thrift
@@ -0,0 +1,111 @@
+/*
+ * 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.
+ */
+
+namespace go constoptionalfieldb
+
+include "ConstOptionalFieldImport.thrift"
+
+typedef ConstOptionalFieldImport.Foo TypedefBFoo
+
+typedef bool TypedefBBool
+typedef i8 TypedefBI8
+typedef i16 TypedefBI16
+typedef i32 TypedefBI32
+typedef i64 TypedefBI64
+typedef double TypedefBDouble
+typedef string TypedefBString
+typedef binary TypedefBBinary
+
+struct Bar {
+  1: optional ConstOptionalFieldImport.Foo optFoo,
+  2: optional ConstOptionalFieldImport.TypedefAFoo aFoo,
+  3: optional TypedefBFoo bFoo,
+
+  4: optional bool optBool,
+  5: optional ConstOptionalFieldImport.TypedefABool aBool,
+  6: optional TypedefBBool bBool,
+
+  7: optional i8 optI8,
+  8: optional ConstOptionalFieldImport.TypedefAI8 aI8,
+  9: optional TypedefBI8 bI8,
+
+  10: optional i16 optI16,
+  11: optional ConstOptionalFieldImport.TypedefAI16 aI16,
+  12: optional TypedefBI16 bI16,
+
+  13: optional i32 optI32,
+  14: optional ConstOptionalFieldImport.TypedefAI32 aI32,
+  15: optional TypedefBI32 bI32,
+
+  16: optional i64 optI64,
+  17: optional ConstOptionalFieldImport.TypedefAI64 aI64,
+  18: optional TypedefBI64 bI64,
+
+  19: optional double optDouble,
+  20: optional ConstOptionalFieldImport.TypedefADouble aDouble,
+  21: optional TypedefBDouble bDouble,
+
+  22: optional string optString,
+  23: optional ConstOptionalFieldImport.TypedefAString aString,
+  24: optional TypedefBString bString,
+
+  25: optional binary optBinary,
+  26: optional ConstOptionalFieldImport.TypedefABinary aBinary,
+  27: optional TypedefBBinary bBinary,
+}
+
+const list<Bar> CONSTANTS = [
+  {
+    "optFoo": ConstOptionalFieldImport.Foo.One,
+    "aFoo": ConstOptionalFieldImport.Foo.One,
+    "bFoo": ConstOptionalFieldImport.Foo.One,
+
+    "optBool": true,
+    "aBool": true,
+    "bBool": true,
+
+    "optI8": 8,
+    "aI8": 8,
+    "bI8": 8,
+
+    "optI16": 16,
+    "aI16": 16,
+    "bI16": 16,
+
+    "optI32": 32,
+    "aI32": 32,
+    "bI32": 32,
+
+    "optI64": 64,
+    "aI64": 64,
+    "bI64": 64,
+
+    "optDouble": 1234,
+    "aDouble": 1234,
+    "bDouble": 1234,
+
+    "optString": "string",
+    "aString": "string",
+    "bString": "string",
+
+    "optBinary": "binary",
+    "aBinary": "binary",
+    "bBinary": "binary",
+  },
+]
diff --git a/lib/go/test/ConstOptionalFieldImport.thrift b/lib/go/test/ConstOptionalFieldImport.thrift
new file mode 100644
index 000000000..a271be10e
--- /dev/null
+++ b/lib/go/test/ConstOptionalFieldImport.thrift
@@ -0,0 +1,36 @@
+/*
+ * 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.
+ */
+
+namespace go constoptionalfielda
+
+enum Foo {
+  One = 1,
+  Two = 2,
+}
+
+typedef Foo TypedefAFoo
+
+typedef bool TypedefABool
+typedef i8 TypedefAI8
+typedef i16 TypedefAI16
+typedef i32 TypedefAI32
+typedef i64 TypedefAI64
+typedef double TypedefADouble
+typedef string TypedefAString
+typedef binary TypedefABinary
diff --git a/lib/go/test/Makefile.am b/lib/go/test/Makefile.am
index a5d0797fc..9f174bbb3 100644
--- a/lib/go/test/Makefile.am
+++ b/lib/go/test/Makefile.am
@@ -48,7 +48,9 @@ gopath: $(THRIFT) $(THRIFTTEST) \
 				ConflictNamespaceServiceTest.thrift \
 				DuplicateImportsTest.thrift \
 				EqualsTest.thrift \
-				ConflictArgNamesTest.thrift
+				ConflictArgNamesTest.thrift \
+				ConstOptionalFieldImport.thrift \
+				ConstOptionalField.thrift
 	mkdir -p gopath/src
 	grep -v list.*map.*list.*map $(THRIFTTEST) | grep -v 'set<Insanity>' > ThriftTest.thrift
 	$(THRIFT) $(THRIFTARGS) -r IncludesTest.thrift
@@ -77,6 +79,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \
 	$(THRIFT) $(THRIFTARGS) -r DuplicateImportsTest.thrift
 	$(THRIFT) $(THRIFTARGS) EqualsTest.thrift
 	$(THRIFT) $(THRIFTARGS) ConflictArgNamesTest.thrift
+	$(THRIFT) $(THRIFTARGS) -r ConstOptionalField.thrift
 	ln -nfs ../../tests gopath/src/tests
 	cp -r ./dontexportrwtest gopath/src
 	touch gopath
@@ -121,6 +124,8 @@ EXTRA_DIST = \
 	ConflictNamespaceTestC.thrift \
 	ConflictNamespaceTestD.thrift \
 	ConflictNamespaceTestSuperThing.thrift \
+	ConstOptionalField.thrift \
+	ConstOptionalFieldImport.thrift \
 	DontExportRWTest.thrift \
 	DuplicateImportsTest.thrift \
 	ErrorTest.thrift \
diff --git a/lib/go/test/tests/const_optional_field_test.go b/lib/go/test/tests/const_optional_field_test.go
new file mode 100644
index 000000000..d323b9afb
--- /dev/null
+++ b/lib/go/test/tests/const_optional_field_test.go
@@ -0,0 +1,150 @@
+/*
+ * 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/constoptionalfielda"
+	"github.com/apache/thrift/lib/go/test/gopath/src/constoptionalfieldb"
+)
+
+func TestConstOptionalField(t *testing.T) {
+	c := constoptionalfieldb.CONSTANTS[0]
+
+	t.Run("foo", func(t *testing.T) {
+		const expected = constoptionalfielda.Foo_One
+		if *c.OptFoo != expected {
+			t.Errorf("Expected %v, got %v", expected, *c.OptFoo)
+		}
+		if *c.AFoo != constoptionalfielda.TypedefAFoo(expected) {
+			t.Errorf("Typedef a expected %v, got %v", expected, *c.AFoo)
+		}
+		if *c.BFoo != constoptionalfieldb.TypedefBFoo(expected) {
+			t.Errorf("Typedef b expected %v, got %v", expected, *c.BFoo)
+		}
+	})
+
+	t.Run("bool", func(t *testing.T) {
+		const expected = true
+		if *c.OptBool != expected {
+			t.Errorf("Expected %v, got %v", expected, *c.OptBool)
+		}
+		if *c.ABool != constoptionalfielda.TypedefABool(expected) {
+			t.Errorf("Typedef a expected %v, got %v", expected, *c.ABool)
+		}
+		if *c.BBool != constoptionalfieldb.TypedefBBool(expected) {
+			t.Errorf("Typedef b expected %v, got %v", expected, *c.BBool)
+		}
+	})
+
+	t.Run("i8", func(t *testing.T) {
+		const expected = 8
+		if *c.OptI8 != expected {
+			t.Errorf("Expected %v, got %v", expected, *c.OptI8)
+		}
+		if *c.AI8 != constoptionalfielda.TypedefAI8(expected) {
+			t.Errorf("Typedef a expected %v, got %v", expected, *c.AI8)
+		}
+		if *c.BI8 != constoptionalfieldb.TypedefBI8(expected) {
+			t.Errorf("Typedef b expected %v, got %v", expected, *c.BI8)
+		}
+	})
+
+	t.Run("i16", func(t *testing.T) {
+		const expected = 16
+		if *c.OptI16 != expected {
+			t.Errorf("Expected %v, got %v", expected, *c.OptI16)
+		}
+		if *c.AI16 != constoptionalfielda.TypedefAI16(expected) {
+			t.Errorf("Typedef a expected %v, got %v", expected, *c.AI16)
+		}
+		if *c.BI16 != constoptionalfieldb.TypedefBI16(expected) {
+			t.Errorf("Typedef b expected %v, got %v", expected, *c.BI16)
+		}
+	})
+
+	t.Run("i32", func(t *testing.T) {
+		const expected = 32
+		if *c.OptI32 != expected {
+			t.Errorf("Expected %v, got %v", expected, *c.OptI32)
+		}
+		if *c.AI32 != constoptionalfielda.TypedefAI32(expected) {
+			t.Errorf("Typedef a expected %v, got %v", expected, *c.AI32)
+		}
+		if *c.BI32 != constoptionalfieldb.TypedefBI32(expected) {
+			t.Errorf("Typedef b expected %v, got %v", expected, *c.BI32)
+		}
+	})
+
+	t.Run("i64", func(t *testing.T) {
+		const expected = 64
+		if *c.OptI64 != expected {
+			t.Errorf("Expected %v, got %v", expected, *c.OptI64)
+		}
+		if *c.AI64 != constoptionalfielda.TypedefAI64(expected) {
+			t.Errorf("Typedef a expected %v, got %v", expected, *c.AI64)
+		}
+		if *c.BI64 != constoptionalfieldb.TypedefBI64(expected) {
+			t.Errorf("Typedef b expected %v, got %v", expected, *c.BI64)
+		}
+	})
+
+	t.Run("double", func(t *testing.T) {
+		// To avoid the annoyance of comparing float numbers,
+		// we convert all floats to int in this test.
+		const expected = 1234
+		if int(*c.OptDouble) != expected {
+			t.Errorf("Expected %v, got %v", expected, *c.OptDouble)
+		}
+		if int(*c.ADouble) != expected {
+			t.Errorf("Typedef a expected %v, got %v", expected, *c.ADouble)
+		}
+		if int(*c.BDouble) != expected {
+			t.Errorf("Typedef b expected %v, got %v", expected, *c.BDouble)
+		}
+	})
+
+	t.Run("string", func(t *testing.T) {
+		const expected = "string"
+		if *c.OptString != expected {
+			t.Errorf("Expected %q, got %q", expected, *c.OptString)
+		}
+		if *c.AString != constoptionalfielda.TypedefAString(expected) {
+			t.Errorf("Typedef a expected %q, got %q", expected, *c.AString)
+		}
+		if *c.BString != constoptionalfieldb.TypedefBString(expected) {
+			t.Errorf("Typedef b expected %q, got %q", expected, *c.BString)
+		}
+	})
+
+	t.Run("binary", func(t *testing.T) {
+		const expected = "binary"
+		if string(c.OptBinary) != expected {
+			t.Errorf("Expected %q, got %q", expected, c.OptBinary)
+		}
+		if string(c.ABinary) != expected {
+			t.Errorf("Typedef a expected %q, got %q", expected, c.ABinary)
+		}
+		if string(c.BBinary) != expected {
+			t.Errorf("Typedef b expected %q, got %q", expected, c.BBinary)
+		}
+	})
+}