You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@thrift.apache.org by yu...@apache.org on 2021/07/30 15:47:57 UTC
[thrift] branch master updated: THRIFT-5389: Fix const generation
for optional fields
This is an automated email from the ASF dual-hosted git repository.
yuxuan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/thrift.git
The following commit(s) were added to refs/heads/master by this push:
new f695535 THRIFT-5389: Fix const generation for optional fields
f695535 is described below
commit f6955351222f51e5662ce41de43c75b7c3e640e1
Author: Yuxuan 'fishy' Wang <yu...@reddit.com>
AuthorDate: Thu Jul 29 15:59:10 2021 -0700
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.
---
compiler/cpp/src/thrift/generate/t_go_generator.cc | 68 ++++++++--
lib/go/test/ConstOptionalField.thrift | 111 +++++++++++++++
lib/go/test/ConstOptionalFieldImport.thrift | 36 +++++
lib/go/test/Makefile.am | 7 +-
lib/go/test/tests/const_optional_field_test.go | 150 +++++++++++++++++++++
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 8d25d48..4ee0e8f 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 0000000..dc1df8f
--- /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 0000000..a271be1
--- /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 a5d0797..9f174bb 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 0000000..d323b9a
--- /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)
+ }
+ })
+}