Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

THRIFT-3037 Go Codegen - Support Struct TypeDefs #2888

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 17 additions & 16 deletions compiler/cpp/src/thrift/generate/t_go_generator.cc
Expand Up @@ -760,13 +760,14 @@ void t_go_generator::close_generator() {
void t_go_generator::generate_typedef(t_typedef* ttypedef) {
generate_go_docstring(f_types_, ttypedef);
string new_type_name(publicize(ttypedef->get_symbolic()));
string base_type(type_to_go_type(ttypedef->get_type()));
string base_type(type_to_go_type_with_opt(ttypedef->get_type(), false, true));

if (base_type == new_type_name) {
return;
}

f_types_ << "type " << new_type_name << " " << base_type << endl << endl;
f_types_ << "type " << new_type_name << " = " << base_type << endl << endl;

// Generate a convenience function that converts an instance of a type
// (which may be a constant) into a pointer to an instance of a type.
f_types_ << "func " << new_type_name << "Ptr(v " << new_type_name << ") *" << new_type_name
Expand Down Expand Up @@ -1175,8 +1176,9 @@ void t_go_generator::get_publicized_name_and_def_value(t_field* tfield,

void t_go_generator::generate_go_struct_initializer(ostream& out,
t_struct* tstruct,
bool is_args_or_result) {
out << publicize(type_name(tstruct), is_args_or_result) << "{";
bool is_args_or_result,
string alias_name) {
out << publicize((alias_name != "") ? alias_name : type_name(tstruct), is_args_or_result) << "{";
const vector<t_field*>& members = tstruct->get_members();
for (auto member : members) {
bool pointer_field = is_pointer_field(member);
Expand Down Expand Up @@ -3049,7 +3051,8 @@ void t_go_generator::generate_deserialize_field(ostream& out,
(t_struct*)type,
is_pointer_field(tfield, in_container_value),
declare,
name);
name,
(orig_type->is_typedef()) ? orig_type->get_name() : "");
} else if (type->is_container()) {
generate_deserialize_container(out, orig_type, is_pointer_field(tfield), declare, name);
} else if (type->is_base_type() || type->is_enum()) {
Expand Down Expand Up @@ -3150,11 +3153,12 @@ void t_go_generator::generate_deserialize_struct(ostream& out,
t_struct* tstruct,
bool pointer_field,
bool declare,
string prefix) {
string prefix,
string alias_name) {
string eq(declare ? " := " : " = ");

out << indent() << prefix << eq << (pointer_field ? "&" : "");
generate_go_struct_initializer(out, tstruct);
generate_go_struct_initializer(out, tstruct, false, alias_name);
out << indent() << "if err := " << prefix << "." << read_method_name_ << "(ctx, iprot); err != nil {" << endl;
out << indent() << " return thrift.PrependError(fmt.Sprintf(\"%T error reading struct: \", "
<< prefix << "), err)" << endl;
Expand Down Expand Up @@ -3914,20 +3918,19 @@ string t_go_generator::type_to_go_key_type(t_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);
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.
*/
string t_go_generator::type_to_go_type_with_opt(t_type* type,
bool optional_field) {
string t_go_generator::type_to_go_type_with_opt(t_type* ttype,
bool optional_field,
bool no_struct_ptr) {
string maybe_pointer(optional_field ? "*" : "");

if (type->is_typedef() && ((t_typedef*)type)->is_forward_typedef()) {
type = ((t_typedef*)type)->get_true_type();
}
t_type* type = get_true_type(ttype);

if (type->is_base_type()) {
t_base_type::t_base tbase = ((t_base_type*)type)->get_base();
Expand Down Expand Up @@ -3970,7 +3973,7 @@ string t_go_generator::type_to_go_type_with_opt(t_type* type,
} else if (type->is_enum()) {
return maybe_pointer + publicize(type_name(type));
} else if (type->is_struct() || type->is_xception()) {
return "*" + publicize(type_name(type));
return (no_struct_ptr ? "" : "*") + publicize(type_name(ttype));
} else if (type->is_map()) {
t_map* t = (t_map*)type;
string keyType = type_to_go_key_type(t->get_key_type());
Expand All @@ -3984,8 +3987,6 @@ string t_go_generator::type_to_go_type_with_opt(t_type* type,
t_list* t = (t_list*)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: " + type->get_name();
Expand Down
8 changes: 5 additions & 3 deletions compiler/cpp/src/thrift/generate/t_go_generator.h
Expand Up @@ -124,7 +124,8 @@ class t_go_generator : public t_generator {
bool is_args = false);
void generate_go_struct_initializer(std::ostream& out,
t_struct* tstruct,
bool is_args_or_result = false);
bool is_args_or_result = false,
string alias_name = "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar here, let's use std::string instead

void generate_isset_helpers(std::ostream& out,
t_struct* tstruct,
const string& tstruct_name,
Expand Down Expand Up @@ -176,7 +177,8 @@ class t_go_generator : public t_generator {
t_struct* tstruct,
bool is_pointer_field,
bool declare,
std::string prefix = "");
std::string prefix = "",
string alias_name = "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use std::string over string here?

it doesn't look like we are very consistent here, but being consistent partially is still better, while the line right above is using std::string it's better to keep that.


void generate_deserialize_container(std::ostream& out,
t_type* ttype,
Expand Down Expand Up @@ -264,7 +266,7 @@ 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);
std::string type_to_go_type_with_opt(t_type* ttype, bool optional_field = false, bool no_struct_ptr = false);
std::string type_to_go_key_type(t_type* ttype);
std::string type_to_spec_args(t_type* ttype);

Expand Down
10 changes: 7 additions & 3 deletions lib/go/test/Makefile.am
Expand Up @@ -62,7 +62,8 @@ gopath: $(THRIFT) $(THRIFTTEST) \
ProcessorMiddlewareTest.thrift \
ClientMiddlewareExceptionTest.thrift \
ValidateTest.thrift \
ForwardType.thrift
ForwardType.thrift \
TypedefServiceTest.thrift
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

mkdir -p gopath/src
grep -v list.*map.*list.*map $(THRIFTTEST) | grep -v 'set<Insanity>' > ThriftTest.thrift
$(THRIFT) $(THRIFTARGS) -r IncludesTest.thrift
Expand Down Expand Up @@ -98,6 +99,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \
$(THRIFT) $(THRIFTARGS) ClientMiddlewareExceptionTest.thrift
$(THRIFT) $(THRIFTARGS) ValidateTest.thrift
$(THRIFT) $(THRIFTARGS) ForwardType.thrift
$(THRIFT) $(THRIFTARGS) TypedefServiceTest.thrift
ln -nfs ../../tests gopath/src/tests
cp -r ./dontexportrwtest gopath/src
touch gopath
Expand All @@ -124,7 +126,8 @@ check: gopath
./gopath/src/processormiddlewaretest \
./gopath/src/clientmiddlewareexceptiontest \
./gopath/src/validatetest \
./gopath/src/forwardtypetest
./gopath/src/forwardtypetest \
./gopath/src/typedefservicetest
Copy link
Member

@fishy fishy Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong indentation (should be 4 spaces tabs)

$(GO) test github.com/apache/thrift/lib/go/thrift
$(GO) test ./gopath/src/tests ./gopath/src/dontexportrwtest

Expand Down Expand Up @@ -172,4 +175,5 @@ EXTRA_DIST = \
TypedefFieldTest.thrift \
UnionBinaryTest.thrift \
UnionDefaultValueTest.thrift \
ValidateTest.thrift
ValidateTest.thrift \
TypedefServiceTest.thrift
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong indentation (should be a tab instead of 4 spaces)

63 changes: 63 additions & 0 deletions lib/go/test/TypedefServiceTest.thrift
@@ -0,0 +1,63 @@
#
# 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.
#

# We are only testing that generated code compiles, no correctness checking is done

struct ExampleRequest {
1: required string example_field
}

struct ExampleResponse {
1: required string example_field
}

struct ExampleOptionalRequest {
1: optional string example_field
}

struct ExampleOptionalResponse {
1: optional string example_field
}

struct ExampleNested {
1: required string example_field
}

struct ExampleOptionalNestedRequest {
1: optional ExampleNested example_field
}

struct ExampleOptionalNestedResponse {
1: optional ExampleNested example_field
}

typedef ExampleRequest TypedefExampleRequest
typedef ExampleResponse TypedefExampleResponse

typedef string PrimativeTypedefExampleRequest
typedef string PrimativeTypedefExampleResponse

service TypeDefService {
ExampleResponse exampleMethod(1: ExampleRequest request)
TypedefExampleResponse typedefExampleMethod(1: TypedefExampleRequest request)
string primativeExampleMethod(1: string request)
PrimativeTypedefExampleResponse primativeTypedefExampleMethod(1: PrimativeTypedefExampleRequest request)
ExampleOptionalResponse exampleOptionalMethod(1: ExampleOptionalRequest request)
ExampleOptionalNestedResponse exampleOptionalNestedMethod(1: ExampleOptionalNestedRequest request)
}
31 changes: 31 additions & 0 deletions lib/go/test/tests/typedef_service_test.go
@@ -0,0 +1,31 @@
/*
* 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/typedefservicetest"
)

func TestTypedefService(t *testing.T) {
// We need to import the generated code to ensure it compiles
_ = &typedefservicetest.TypedefExampleRequest{}
}