slachiewicz commented on PR #3812:
URL: https://github.com/apache/thrift/pull/3812#issuecomment-5581903856
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:
```go
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.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
