THRIFT-3037 Go Codegen - Support Struct TypeDefs

[ethan-gallant]

No description provided.

[fishy]

Please create a jira ticket to describe what problem you are trying to resolve, what's the before and after generated go code from what thrift code.

[ethan-gallant]

Please create a jira ticket to describe what problem you are trying to resolve, what's the before and after generated go code from what thrift code.

Hello @fishy ! :)

The ticket THRIFT-3037 best describes this issue so I figured it best not to create a duplicate. Here is an example of how the generated code changes. I've listed them as bullet points for the major changes.

Change description

This PR makes it possible for typedefs that don't refer to a primative type to compile. This is accomplished by using a struct instead of declaring a type pointer (fixes the issue with READ methods etc. not existing).

Code changes

Here is the example schema which we will be using to describe the issue. Without the change, the code does not compile.

struct ExampleRequest {
    1: required string example_field
}

struct ExampleResponse {
    1: required string example_field
}

typedef ExampleRequest TypedefExampleRequest
typedef ExampleResponse TypedefExampleResponse

service EnvironmentService {
    ExampleResponse exampleMethod(1: ExampleRequest request)
    TypedefExampleResponse typedefExampleMethod(1: TypedefExampleRequest request)
}

Each change will have a title and will be formatted in three sections. Code Change (this PR), previous generated code and new generated code.

Use Structs instead of aliases

This was needed as you can't refer to methods on aliased types. You instead need to implement the existing struct to inherit it's methods.

Code change

thrift/compiler/cpp/src/thrift/generate/t_go_generator.cc

Lines 769 to 773 in cc2d009

	if (ttypedef->get_type()->is_base_type()) {
	f_types_ << "type " << new_type_name << " " << base_type << endl << endl;
	} else {
	f_types_ << "type " << new_type_name << " struct { " << base_type << " }" << endl << endl;
	}

Old Code

type TypedefExampleRequest *ExampleRequest

func TypedefExampleRequestPtr(v TypedefExampleRequest) *TypedefExampleRequest { return &v }

type TypedefExampleResponse *ExampleResponse

func TypedefExampleResponsePtr(v TypedefExampleResponse) *TypedefExampleResponse { return &v }

New Code

type TypedefExampleRequest struct { *ExampleRequest }

func TypedefExampleRequestPtr(v TypedefExampleRequest) *TypedefExampleRequest { return &v }

type TypedefExampleResponse struct { *ExampleResponse }

func TypedefExampleResponsePtr(v TypedefExampleResponse) *TypedefExampleResponse { return &v }

For Forward TypeDefs use the original typedef name

This was needed as it wouldn't use the correct type. For example it would use ExampleRequestwhere instead the TypeDefExampleRequest should have been used.

Code change

thrift/compiler/cpp/src/thrift/generate/t_go_generator.cc

Lines 2093 to 2094 in cc2d009

	bool is_alias = (*f_iter)->get_returntype()->is_typedef();
	if ((*f_iter)->get_returntype()->is_struct() || is_alias) {

Old Code

func (p *EnvironmentServiceTypedefExampleMethodArgs)  ReadField1(ctx context.Context, iprot thrift.TProtocol) error {
  p.Request = &ExampleRequest{}
  if err := p.Request.Read(ctx, iprot); err != nil {
    return thrift.PrependError(fmt.Sprintf("%T error reading struct: ", p.Request), err)
  }
  return nil
}

New Code

func (p *EnvironmentServiceTypedefExampleMethodArgs)  ReadField1(ctx context.Context, iprot thrift.TProtocol) error {
  p.Request = &TypedefExampleRequest{}
  if err := p.Request.Read(ctx, iprot); err != nil {
    return thrift.PrependError(fmt.Sprintf("%T error reading struct: ", p.Request), err)
  }
  return nil
}

Pleasse let me know if there's anything else I can clarify :)

[fishy]

Thanks. Please

1. Add a test to verify the fix
2. Claim the jira ticket (cc @dcelasun you claimed the ticket previously)
3. instead of type TypedefExampleRequest struct { *ExampleRequest }, why not just type TypedefExampleRequest = ExampleRequest? that will make things much simpler isn't it?

[dcelasun]

Thanks for the PR!

I've unassigned myself from the ticket and I agree with @fishy that an alias would be better.

[ethan-gallant]

@dcelasun and @fishy thank you for the quick response :)

I have a Jira account Ethan-Gallant but seem to lack the permissions to self-assign the issue. I left a comment on it if someone is able to assign it to me when able.

As for the struct implementation, I've added it to simplify the code ex: type MyTypedefOfString = string it also supports more complicated types like type MyTypeDefOfStruct = MyRequest.

For testing I added a service to the Go test which will cause a compile failure. I'm struggling to get the suite working locally. Do you have any other guidance aside from the existing Readme file I can reference?

[fishy]

you have the order wrong here, you want this instead:

Suggested change

	typedef string MyStringTypedef
	typedef MyStringTypedef string

[ethan-gallant]

Oddly my IDE says that string needs to be put first in this case.

Is it not typedef actual_type alias_name as the format?

[fishy]

also, thrift files under test/ are for all languages and are not necessarily run by all language tests (currently there's no go test specifically run against test/TypedefTest.thrift, it's just the compiler change you made complete broke the go code so it failed the test). for go specific tests I'd suggest you to put it under lib/go/test instead. Note that currently tests under lib/go/test is not run by CI, see #2886 (after that is merged the CI will run those tests)

[ethan-gallant]

@fishy I've added a test in the lib/go/test directory with some more extensive use-cases.

Currently it will test:

• Optional typedef properties
• Typedefs for primitive types
• Typedefs for struct types
• Services using the above types (where the compile issues originated from)

[fishy]

it looks like this change also changes primitive type typedefs into aliases, from the compiler generated go code:

type PrimativeTypedefExampleRequest = string

while previously that's

type PrimativeTypedefExampleRequest string

while this does not seem to break any tests, I'd prefer the old behavior. The reason is that with the old behavior, it forces you to do a typecasting when writing the go code, so the name of the type can be used as both documentation and reminder of the unit of the value. Take this example:

typedef i64 UnixMilliseconds

struct Foo {
  1: optional UnixMilliseconds timestamp;
}

with alias you could write this go code:

foo.Timestamp = thrift.Pointer(int64(now.Seconds()))

without compiler complaining anything, and that's wrong. with the old behavior you have to write this to make compiler happy:

foo.Timestamp = thrift.Pointer(UnixMilliseconds(now.Seconds()))

which makes it obvious that the code has a bug.

[fishy]

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

[fishy]

wrong indentation (should be 4 spaces tabs)

[fishy]

same here

[fishy]

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.

[fishy]

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

[fishy]

also, now #2886 is merged, please rebase your change on top of latest master so the CI will actually run your tests.

[dcelasun]

@ethan-gallant any progress on this? Happy to help with any issues.

[binalp7]

@ethan-gallant Is there any update on this? I am also facing the similar issue.