THRIFT-6197: Fix Go code generation for typedefs of structs and forward typedefs

[slachiewicz]

A typedef of a struct is now generated as a Go type alias (type Alias = Inner) instead of a defined type over a pointer (type Alias *Inner), and forward-typedef unwrapping stops at the first declared type instead of jumping to the underlying one.

The backward-compatibility question is the right one to ask of this change, and the sections below are the measurements that answer it.

Why the alias

type Alias *Inner is a distinct Go type whose underlying type is a pointer. It carries none of the struct's methods, and the generated read path immediately needs them:

cannot use &Inner{} (value of type *Inner) as *UsedAlias value in assignment
p.A.Read undefined (type *UsedAlias has no field or method Read)
p.A.Write undefined (type *UsedAlias has no field or method Write)
p.A.Equals undefined (type *UsedAlias has no field or method Equals)

So the package does not build. Three reporters filed those same errors independently — THRIFT-3037 (2015), THRIFT-3491 (2015), THRIFT-4901 (2019) — against three different Thrift versions, for the include, service-signature and struct-field shapes of one defect.

A Go alias names the struct itself and shares its method set, which is what a Thrift typedef means. The alias gets exactly what the struct gets: the <Name>Ptr helper that generate_typedef emits for every other typedef is skipped for an alias of a struct, since structs have no such helper and one taking the struct by value would only copy it. Typedefs of base types keep theirs. The alternative, type Alias Inner, has no methods either; making it work would mean generating forwarding Read, Write and Equals for every typedef of a struct.

THRIFT-5685 does not come back

The forward-typedef half of this is the fix that was reverted once. THRIFT-5601 was fixed, the fix made a forward-declared struct field a value instead of a pointer, bar.GetBar().GetFoo() stopped compiling, and THRIFT-5685 reverted it in 0.18.1.

Generating THRIFT-5685's own IDL with this branch produces the output that ticket calls expected:

Bar *Foo `thrift:"bar,1" db:"bar" json:"bar,omitempty"`
var Bar_Bar_DEFAULT *Foo
func (p *Bar) GetBar() *Foo { ... }

The while loop stops at the first declared type, and a forward-declared struct resolves to a struct, which takes the pointer branch. UsesForwardStruct in TypedefStructTest.thrift and forwardStructGetterChain in the Go test hold this shape so a future change has to trip over it.

Blast radius

Regenerating every IDL under test/, lib/go/test/ and tutorial/ with the master generator and with this branch produces 325 files. Three differ:

File	Change
test/TypedefTest.thrift	type MyStruct *TypedefTestStruct → = TypedefTestStruct, and the MyStructPtr helper is no longer generated. The alias is unused in fields.
lib/go/test/DuplicateImportsTest.thrift	type A *common.A → = common.A, same for B, and the APtr and BPtr helpers are no longer generated.
lib/go/test/StructKeyTest.thrift	Under --gen go:struct_key_entries, which is what the Makefile uses: the declaration and the dropped KeyAliasPtr helper, and field types stay []thrift.MapEntry[*Key, string]. Under default generation the map key types also move, from map[KeyAlias]string to map[*KeyAlias]string, along with the DEFAULT vars, the getters, and the make() calls. That output did not compile before, with k.Write undefined (type KeyAlias has no field or method Write), so it is a further fix rather than a regression.

Outside the default-generation StructKeyTest case described in the table, every changed line is a typedef-of-struct declaration or the <Name>Ptr helper that went with it. Nothing else in the corpus moves.

As an out-of-tree check, jaeger-idl generates byte-identical Go with both compilers, and all eight generated packages build. The only typedefs in that project's history are typedef string BaggageKey and typedef i32 MaxValueLength, both base types, which this change leaves alone.

lib/go/test/tests/struct_key_test.go carried a comment explaining that a struct key behind a typedef has to be written as *Key "because the generated type KeyAlias *Key has no methods of its own". That workaround is no longer needed; the comment is updated and the code is unchanged.

What can still break

A typedef of a struct that the IDL declares but never uses is the only shape that compiles today, so it is the only shape hand-written Go can already depend on. Compiling the same consumer code against both compilers' output, these change:

Hand-written code	Before	After
var x InnerAlias = &Inner{}	compiles	rejected, the pointer moves outside the alias
the generated InnerAliasPtr helper	exists, takes *Inner	not generated
a type switch with case *Inner: and case *InnerAlias:	compiles	duplicate case *InnerAlias in type switch
type W struct{ InnerAlias }	embedded field type cannot be a pointer	compiles, and W satisfies thrift.TStruct by promotion
%T and reflect.TypeOf(x).String() for a value held as an InnerAlias	pkg.InnerAlias	*pkg.Inner

The first four are compile errors. The last is silent, and so is the loss of distinction between two aliases of one struct: after this change a value of one is assignable to the other. DuplicateImportsTest and TypedefTest are that shape. All of it is worth a release note.

Tickets

Ticket	Shape
THRIFT-3037	typedef of a struct across an include
THRIFT-3491	typedef'd struct in a service signature
THRIFT-4901	typedef of a struct as a struct field
THRIFT-5489	forward typedef loses the declared name
THRIFT-5601	forward typedef generates uncompilable code
THRIFT-5685	the revert of the first THRIFT-5601 fix; guarded here

Release note

CHANGES.md carries the change under Breaking Changes for 0.25.0, and lib/go/README.md gains a "A note about typedefs of structs" section covering the alias, the dropped <Name>Ptr helper and what moves for hand-written code. The struct-key note in the same README no longer says the typedef carries none of the struct's methods.

Tests

TypedefStructTest.thrift and TypedefIncludeTest.thrift cover a typedef declared after use, a struct declared after use, a typedef of a struct in the same file and across an include, a typedef of a typedef, a typedef of an exception, a typedef of a container, and a typedef'd struct in a service signature, which also exercises the -remote stub. tests/typedef_struct_test.go adds compile-time identity assertions and binary, compact and JSON round trips.

This change was created with AI assistance.

[fishy]

changing typedef to alias in go is a much bigger change that can have consequences and backward-incompatibilities.

[slachiewicz]

Thanks for pushing back on this — it sent me to measure instead of argue, and three things should have been in the PR from the start.

The generated code does not compile today. For a typedef of a struct the IDL actually uses, master emits type Alias *Inner and the read path then does p.A = &Inner{} followed by p.A.Read(...):

cannot use &Inner{} (value of type *Inner) as *UsedAlias value in assignment
p.A.Read undefined (type *UsedAlias has no field or method Read)

THRIFT-3037, THRIFT-3491 and THRIFT-4901 are three people reporting those same errors against three different versions. For the used case there is no working code to break.

THRIFT-5685 does not come back. That is the one I would worry about too, since it is why the last THRIFT-5601 fix was reverted. Generating 5685's own IDL on this branch gives the block that ticket calls expected:

var Bar_Bar_DEFAULT *Foo
func (p *Bar) GetBar() *Foo { ... }

The while loop stops at the first declared type, and a forward-declared struct resolves to a struct, so it takes the pointer branch. I have added UsesForwardStruct to the test IDL and a compile-only getter chain in the Go test, so a future change here has to trip over it.

Three of 325 files change. Regenerating every IDL under test/, lib/go/test/ and tutorial/ with both compilers, only TypedefTest, DuplicateImportsTest and StructKeyTest differ, and every changed line is a typedef-of-struct declaration — StructKeyTest included, under the struct_key_entries option the Makefile uses. Details in the PR body.

The one real compatibility case is a typedef of a struct the IDL declares but never uses. That compiles today, so hand-written Go can hold one, and the pointer moves from inside the alias to outside it. DuplicateImportsTest and TypedefTest are that shape; it is called out in the PR body and I think it wants a release note.

Separately: I was wrong to suggest THRIFT-5463 might be the same defect. It is not, and I have removed that line from THRIFT-6197.

If you would still rather this not land unflagged in a minor, I am happy to put the alias behind a go: option defaulting off, or hold it for the next major. Say which and I will rework it.

This comment was created with AI assistance.

[slachiewicz]

Unrelated find while running the Go suite against this branch: lib/go/test does not build includestest/*-remote on master either — the enum branch of generate_service_remote hardcodes package_name_aliased, so an enum from an included file comes out as includestest.Numberz. That branch is untouched here and generates identically before and after; filed as THRIFT-6200 so it does not get read as fallout from this PR.

This comment was created with AI assistance.

[fishy]

The discussion of whether to use type alias in go had came up before and rejected. The reason we don't want it is it will lose some compile time enforcement.

For example, someone can have this typedef in thrift:

typedef i64 TimestampMilliseconds

With the current not-type-alias implementation, the generated go code is:

type TimestampMilliseconds int64

It's convertible with int64 but not the same type. so when using it, you mostly are required to use an explicit typecasting like this

obj.StartTime = package.TimestampMilliseconds(t.UnixMilli())

while this won't compilre:

obj.StartTime = t.UnixMilli()

as t.UnixMilli() gives you int64 not TimestampMilliseconds.

now imagine if someone has a bug and used UnixNano instead. With forced explicit casting, it's very obvious at a glance that there is a bug:

obj.StartTime = package.TimestampMilliseconds(t.UnixNano())

You lost that when using type alias.

[slachiewicz]

That holds for base types, and this PR leaves them alone. The alias is gated on is_struct() || is_xception() — typedef i64 TimestampMilliseconds still generates type TimestampMilliseconds int64, so the explicit cast at the call site stays. In the 325-file regeneration, every line that moved is a typedef-of-struct declaration.

Structs have no equivalent enforcement to lose. type Alias *Inner is nominally distinct but carries no method set, and the generated read path calls Read on it, so a typedef of a struct the IDL actually uses does not compile — THRIFT-3037, THRIFT-3491, THRIFT-4901. Keeping it nominal would mean type Alias Inner plus generated Read, Write, Equals, String and every getter, and *Alias would still not satisfy a signature naming *Inner, including the service methods the compiler writes for it.

Do you have a pointer to the earlier discussion? I could not find it on the tracker. If it covered typedef-of-struct specifically rather than typedefs in general, I would like to read it before pushing this further.

This comment was created with AI assistance.

[fishy]

that discussion probably came up in one of the ticket comments.

@dcelasun what do you think? this creates a divergence between typedef implementations in go that I don't like, but maybe it's acceptable?

[dcelasun]

this creates a divergence between typedef implementations in go that I don't like

I don't like it either, but it does address a real problem and I can't think of a cleaner solution. As long as this is limited to is_struct() || is_xception() it's good enough for me.

Definitely needs a BREAKING entry in the changelog, though.

[slachiewicz]

@fishy, the drift question is the right one, so I measured it rather than argued it. In short: the alias does remove a distinction. Exactly two of the changes it causes are silent rather than compile errors, and both are bounded to a shape that is rare and already close to unusable. Everything below is reproducible from this branch.

How I measured

Two compilers built from source, compared on identical inputs:

• Baseline: ab3412891^, this PR's parent, which is on master.
• Patched: ab3412891, the single commit in this PR.
• Toolchain: Go 1.27.1 on darwin/arm64, with generated packages built against the lib/go/thrift package from this branch.

The reported defects, on each reporter's own IDL

The IDL below is verbatim from each ticket. For THRIFT-4901 I also used the two files from the reporter's own test branch, johnboiles@c2a6220, unmodified.

Ticket	Baseline	Patched
THRIFT-3037	cannot use &c.Foo{} … as *Foo, p.F.Read/Write/Equals undefined	compiles
THRIFT-3491	cannot use retval … as *Bar, p.Success.Read/Write undefined	compiles, and the -remote stub calls NewFoo() instead of the nonexistent NewBar()
THRIFT-4901	cannot use &testa.ThingA{} … as *TThingA, Read/Write/Equals undefined	compiles, from both the ticket snippet and the reporter's branch
THRIFT-5489	MyFirstStruct.MyTypedef int32 beside MySecondStruct.MyTypedef MyTypedef	both fields use MyTypedef
THRIFT-5601	Foo *int32	Foo *Foo, the output the ticket calls expected
THRIFT-5685	Bar *Foo, var Bar_Bar_DEFAULT *Foo, GetBar() *Foo	byte-identical
THRIFT-5463	a different defect	generated output byte-identical for its container and string shapes

The illegal-IDL case Jens recorded on THRIFT-5685, a forward-declared exception in a struct field, compiles before and after and generates identically. This PR neither legitimizes nor breaks it.

The typedef example from your review, in both declaration orders

This is the part of your review I most wanted to check, because if the alias reached base types you would be right to block it.

IDL	Baseline	Patched
typedef i64 TimestampMilliseconds before use	type TimestampMilliseconds int64, field *TimestampMilliseconds	identical
the same typedef after use	field *int64, GetStartTime() int64	field *TimestampNanoseconds, GetStartTime() TimestampNanoseconds

The alias is gated on is_struct() || is_xception(), so base typedefs keep the defined type and the explicit cast at the call site. In the second row that enforcement is already lost on master, and the forward-typedef half of this PR restores it. On your own example, this change is neutral in one order and stricter in the other.

The drift surface

To find out whether code that already exists can drift, I generated a package whose struct typedefs are declared but never used in a field or a signature. That's the only shape that compiles on master, so it's the only shape hand-written Go can already depend on. The same hand-written consumer file then went through both compilers' output.

Hand-written code	Baseline	Patched
var x drift.A = &drift.Inner{}	compiles	cannot use &drift.Inner{} (value of type *drift.Inner) as drift.A value
a call to the generated APtr helper	takes *Inner	takes Inner
a type switch with case *drift.Inner: and case *drift.A:	compiles	duplicate case *drift.A in type switch
type W struct{ drift.A }	embedded field type cannot be a pointer	compiles, w.A resolves, and W satisfies thrift.TStruct by promotion
var b drift.B; take(b), where take accepts a drift.A	cannot use b … as drift.A value	accepted
a map[string]drift.B passed where a map[string]drift.A is wanted	rejected	accepted
%T and reflect.TypeOf(x).String() for a value held as a drift.A	drift.A	*drift.Inner
a method on the alias, in a file added to the generated package	invalid receiver type A (pointer or interface type)	compiles, and the method lands on Inner for every importing package
a value held as an exception alias, asserted to error	the assertion fails, because the alias has an empty method set	the assertion succeeds

Four of those are compile errors, which is the kind of break a caller can see. Two are silent: the %T and reflect strings, and the loss of distinction between two aliases of one struct, which is also what makes the method-on-alias vector possible. That second one is your objection, and it's real. It reaches code written after this change, and existing code only where a struct typedef is unused in every field and every signature. Across the test/, lib/go/test/, and tutorial/ directories, that's three IDL files.

Change scope, measured independently

Regenerating every IDL under test/, lib/go/test/, and tutorial/ with both compilers, 260 files with -r on each input, produces the same three pre-existing files the PR description reports: TypedefTest, DuplicateImportsTest, and StructKeyTest. The three inputs that fail to generate, BrokenConstants, IncludesTest, and NamespacedTest, fail identically on both.

Compiling both corpora is the stronger check. The baseline fails in five packages: debugprototest, enumtest, nameconflicttest, structkeytest, and test/ExceptionStruct. The patched corpus fails in the same set minus structkeytest. Nothing that built before stops building.

The Go gate runs clean with the patched compiler. All 26 packages in the check target build, including typedefstructtest/alias_service-remote, and go test passes for github.com/apache/thrift/lib/go/thrift, gopath/src/tests, and gopath/src/dontexportrwtest. The new cases run: TestForwardType, TestTypedefStructRoundTrip over binary, compact, and JSON, TestTypedefStructFromIncludedFile, and TestForwardTypedefRoundTrip. clang-format reports 426 replacements on the t_go_generator.cc file before and after, so the change adds no style deviations.

Beyond the tickets, one more IDL covers a const of an aliased struct, field defaults, union and exception aliases, an alias of an alias, list, set, and map aliases, a binary typedef, service extends, a forward reference to a typedef of a struct, and a forward chain of base typedefs. The baseline doesn't compile it. The patched output does, with the field and getter types the IDL asks for.

One correction to the PR description

The scope table says the StructKeyTest change is declaration-only. That holds under struct_key_entries, which is what the Makefile uses, and only the two type lines move there. Under default generation more moves: map key types go from map[KeyAlias]string to map[*KeyAlias]string, along with the DEFAULT vars, the getters, and the make() calls. That output didn't compile before, with k.Write undefined (type KeyAlias has no field or method Write), and compiles now, so it's a further fix rather than a regression. The sentence in the description is still too broad, and I'll correct it.

What the other bindings do

Generating THRIFT-3491's IDL for each language shows which bindings give a struct typedef an identity of its own.

Binding	typedef Foo Bar	typedef i64 TimestampMilliseconds
C++	typedef class Foo Bar;	typedef int64_t TimestampMilliseconds;
Rust	pub type Bar = Foo;	pub type TimestampMilliseconds = i64;
Java	no alias type, getBar() returns Foo	field is long
netstd	no alias type, getBar() returns Foo
Python	no representation	no representation
Go, on master	type Bar *Foo, which doesn't compile	type TimestampMilliseconds int64

None of them give a struct typedef a nominal identity. For base typedefs it inverts: Go alone gives a defined type, and this PR keeps that.

The nominal alternative

If the distinction matters more than the ergonomics, the alternative is a defined type over the struct with the exported surface forwarded. Here it is prototyped by hand for the cross-file shape, which is the hard one:

type NominalTThingA testa.ThingA

func (p *NominalTThingA) Read(ctx context.Context, iprot thrift.TProtocol) error {
	return (*testa.ThingA)(p).Read(ctx, iprot)
}
// Write, Equals, String, and GetValue forward the same way

var _ thrift.TStruct = (*NominalTThingA)(nil)

That compiles, satisfies thrift.TStruct, and stays distinct: passing a *testa.ThingA into a *NominalTThingA parameter is rejected. So this is a scoping decision, not a feasibility one. The cost is that the generator emits Read, Write, Equals, String, LogValue, and Validate per typedef of a struct, plus one GetX per field, one IsSetX per optional field, and CountSetFieldsX for unions, all kept in sync with the target struct. User code that crosses between the alias and the struct then needs an explicit conversion that no other binding asks for.

What I propose

Land the alias, and widen the release note from the pointer move to the two silent classes above, naming the XPtr helper signature and the %T change. If you would rather have the nominal type, say so and I'll rework this PR that way rather than gate it behind an option. An option would leave the five tickets open by default and split the generated API in two.

One request stands from my earlier reply: if the discussion that rejected aliases covered typedef-of-struct specifically, I would like to read it. The tracker carries the opposite on record, in THRIFT-3037 and THRIFT-3491, where Duru Can Celasun proposed Go aliases for these typedefs in 2017 and offered a PR.

This comment was created with AI assistance.

[slachiewicz]

Four IDL corpora from outside this repository, generated with the parent commit and with this branch, to see what the change moves in the wild. One of them reproduces the defect on its own IDL, and the other three are byte-identical.

Corpus	Files that parse	Output difference	Build
uber/thriftrw-go test IDL	18 of 23	3 files	fails on the parent commit, builds on this branch
jaeger-idl	4 of 4	none	builds on both
colbygk/evernote-sdk-golang	5 of 5	none	builds on both
facebook/fbthrift compiler fixtures	8 of 217	none	builds on both

uber/thriftrw-go reproduces it independently

The test IDL under gen/internal/tests/thrift was written for a different Thrift implementation by a different team, and it contains the shapes these tickets describe. Three generated files differ, and the difference is a package that doesn't build:

Generated file	The IDL behind it	Parent commit	This branch
structs.go	typedef Node List	type List *Node, then cannot use &Node{} (value of type *Node) as *List value in assignment, p.Tail.Read/Write/Equals undefined	compiles
typedefs.go	typedef i128 UUID, where struct i128 is declared after the typedef, plus typedef UUID MyUUID	the same failures, on a required field and through the alias of an alias	compiles
nozap.go	typedef PrimitiveRequiredStruct Primitives, never used in a field	compiles	compiles, and only the declaration line changes

That is THRIFT-4901's error text arriving from a third party's IDL, and nozap.go is the unused-alias shape from the compatibility section of the description, in the wild.

Two caveats on method. The typedefs and structs packages sit behind an unrelated pre-existing defect, a duplicate case in the String() method of EnumWithDuplicateValues, which masks them; I removed that case identically in both trees before rebuilding. The exceptions, enum-text-marshal-strict, and hyphenated-file packages fail on both compilers for unrelated reasons: an Error field that collides with the Error() method, and hyphens in package names. Five of the 23 inputs fail to generate on both, on thriftrw dialect the Apache grammar doesn't accept.

facebook/fbthrift fixtures confirm parity, not much more

Of 217 fixture sources, 8 parse under the Apache grammar and 209 don't, because they use the modern dialect: package statements and @-prefixed structured annotations. The generate status is identical on both compilers for all 217, and all 209 failure diagnostics are byte-identical. The 8 that parse produce identical output and build with both.

The failures are clean and correctly positioned, which is worth asserting even though this change doesn't touch that path. For the line-numbers fixture, both compilers report module.thrift:19, which is exactly the @thrift.AllowLegacyMissingUris line, with Unexpected token in input: "@" and a warning naming the include that could not be found.

The java-typedef fixture turns out to cover base types only, i16, string, and a map of the two, so it never reaches this change.

Jaeger and the Evernote SDK don't move

Both generate byte-identical Go, and every generated package builds, including the -remote stubs each project deletes. Neither exercises the change: jaeger-idl has no typedefs at HEAD, and the only two in its history, typedef string BaggageKey and typedef i32 MaxValueLength, are base types. The Evernote IDL has seven typedefs across five files, all base types, and none is used before its declaration.

What this adds

The in-repo measurements show that the corpus doesn't move. These show the same thing on four projects that don't share this repository's test IDL, and one of them turns the defect into something other than a synthetic reproduction: an independent project's checked-in IDL that produces a Go package which doesn't compile.

This comment was created with AI assistance.

[slachiewicz]

Rebased on master (f780929) to pick up the TNonblockingServerTest fix from THRIFT-6244 behind the AppVeyor failure. The diff is unchanged (same patch-id).

[fishy]

looks good to me. the failed cpp tests don't seem related