THRIFT-3037 Go Codegen - Support Struct TypeDefs #2888

ethan-gallant · 2023-11-24T22:29:55Z

No description provided.

fishy · 2023-11-25T18:43:37Z

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 · 2023-11-27T15:26:32Z

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 · 2023-11-27T16:57:41Z

Thanks. Please

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

dcelasun · 2023-11-27T17:01:59Z

Thanks for the PR!

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

ethan-gallant · 2023-11-27T22:48:48Z

@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 · 2023-11-27T23:06:59Z

test/TypedefTest.thrift

+typedef TypedefTestStruct MyStruct
+typedef string MyStringTypedef


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

Suggested change

typedef string MyStringTypedef

typedef MyStringTypedef string

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 · 2023-11-27T23:17:36Z

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 · 2023-11-27T23:50:27Z

@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)

Client: Go

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 · 2023-11-28T23:45:24Z

lib/go/test/Makefile.am

@@ -172,4 +175,5 @@ EXTRA_DIST = \
 	TypedefFieldTest.thrift \
 	UnionBinaryTest.thrift \
 	UnionDefaultValueTest.thrift \
-	ValidateTest.thrift
+	ValidateTest.thrift \
+    TypedefServiceTest.thrift


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

fishy · 2023-11-28T23:45:42Z

lib/go/test/Makefile.am

@@ -124,7 +126,8 @@ check: gopath
 				./gopath/src/processormiddlewaretest \
 				./gopath/src/clientmiddlewareexceptiontest \
 				./gopath/src/validatetest \
-				./gopath/src/forwardtypetest
+				./gopath/src/forwardtypetest \
+                ./gopath/src/typedefservicetest


wrong indentation (should be 4 ~~spaces~~ tabs)

fishy · 2023-11-28T23:47:19Z

lib/go/test/Makefile.am

@@ -62,7 +62,8 @@ gopath: $(THRIFT) $(THRIFTTEST) \
 				ProcessorMiddlewareTest.thrift \
 				ClientMiddlewareExceptionTest.thrift \
 				ValidateTest.thrift \
-				ForwardType.thrift
+				ForwardType.thrift \
+                TypedefServiceTest.thrift


fishy · 2023-11-28T23:48:44Z

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

@@ -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 = "");


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 · 2023-11-28T23:49:26Z

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

@@ -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 = "");


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

fishy · 2023-12-05T15:44:39Z

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

dcelasun · 2024-03-14T14:58:42Z

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

binalp7 · 2024-03-28T08:34:59Z

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

ethan-gallant changed the title ~~Go Codegen - Support Struct TypeDefs~~ THRIFT-3037 | Go Codegen - Support Struct TypeDefs Nov 24, 2023

ethan-gallant force-pushed the go-support-struct-typedefs branch 3 times, most recently from 0ed4cd1 to cc2d009 Compare November 24, 2023 22:47

Jens-G changed the title ~~THRIFT-3037 | Go Codegen - Support Struct TypeDefs~~ THRIFT-3037 Go Codegen - Support Struct TypeDefs Nov 25, 2023

Jens-G added the golang patches related to go language label Nov 25, 2023

fishy requested a review from dcelasun November 27, 2023 16:57

ethan-gallant force-pushed the go-support-struct-typedefs branch 2 times, most recently from b6ebd28 to 2905dfa Compare November 27, 2023 22:47

fishy reviewed Nov 27, 2023

View reviewed changes

ethan-gallant force-pushed the go-support-struct-typedefs branch from 2905dfa to 344119c Compare November 27, 2023 23:48

ethan-gallant force-pushed the go-support-struct-typedefs branch 3 times, most recently from 5084784 to 4c7799b Compare November 28, 2023 13:17

THRIFT-3037: Support Struct Typedefs in Go Codegen

4762b4e

Client: Go

ethan-gallant force-pushed the go-support-struct-typedefs branch from 4c7799b to 4762b4e Compare November 28, 2023 13:33

fishy requested changes Nov 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-3037 Go Codegen - Support Struct TypeDefs #2888

THRIFT-3037 Go Codegen - Support Struct TypeDefs #2888

ethan-gallant commented Nov 24, 2023

fishy commented Nov 25, 2023

ethan-gallant commented Nov 27, 2023

fishy commented Nov 27, 2023

dcelasun commented Nov 27, 2023

ethan-gallant commented Nov 27, 2023

fishy Nov 27, 2023

ethan-gallant Nov 27, 2023

fishy commented Nov 27, 2023

ethan-gallant commented Nov 27, 2023

fishy left a comment •

edited

fishy Nov 28, 2023

fishy Nov 28, 2023 •

edited

fishy Nov 28, 2023

fishy Nov 28, 2023

fishy Nov 28, 2023

fishy commented Dec 5, 2023

dcelasun commented Mar 14, 2024

binalp7 commented Mar 28, 2024

		typedef TypedefTestStruct MyStruct
		typedef string MyStringTypedef

	typedef string MyStringTypedef
	typedef MyStringTypedef string

THRIFT-3037 Go Codegen - Support Struct TypeDefs #2888

Are you sure you want to change the base?

THRIFT-3037 Go Codegen - Support Struct TypeDefs #2888

Conversation

ethan-gallant commented Nov 24, 2023

fishy commented Nov 25, 2023

ethan-gallant commented Nov 27, 2023

Change description

Code changes

Use Structs instead of aliases

Code change

Old Code

New Code

For Forward TypeDefs use the original typedef name

Code change

Old Code

New Code

fishy commented Nov 27, 2023

dcelasun commented Nov 27, 2023

ethan-gallant commented Nov 27, 2023

fishy Nov 27, 2023

Choose a reason for hiding this comment

ethan-gallant Nov 27, 2023

Choose a reason for hiding this comment

fishy commented Nov 27, 2023

ethan-gallant commented Nov 27, 2023

fishy left a comment • edited

Choose a reason for hiding this comment

fishy Nov 28, 2023

Choose a reason for hiding this comment

fishy Nov 28, 2023 • edited

Choose a reason for hiding this comment

fishy Nov 28, 2023

Choose a reason for hiding this comment

fishy Nov 28, 2023

Choose a reason for hiding this comment

fishy Nov 28, 2023

Choose a reason for hiding this comment

fishy commented Dec 5, 2023

dcelasun commented Mar 14, 2024

binalp7 commented Mar 28, 2024

fishy left a comment •

edited

fishy Nov 28, 2023 •

edited