slachiewicz commented on PR #3812: URL: https://github.com/apache/thrift/pull/3812#issuecomment-5592133690
@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](https://issues.apache.org/jira/browse/THRIFT-4901) I also used the two files from the reporter's own test branch, [johnboiles@c2a6220](https://github.com/johnboiles/thrift/commit/c2a62207057e001a5fa4722edf1be3bf3160f54f), unmodified. | Ticket | Baseline | Patched | |---|---|---| | [THRIFT-3037](https://issues.apache.org/jira/browse/THRIFT-3037) | `cannot use &c.Foo{} … as *Foo`, `p.F.Read/Write/Equals undefined` | compiles | | [THRIFT-3491](https://issues.apache.org/jira/browse/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](https://issues.apache.org/jira/browse/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](https://issues.apache.org/jira/browse/THRIFT-5489) | `MyFirstStruct.MyTypedef int32` beside `MySecondStruct.MyTypedef MyTypedef` | both fields use `MyTypedef` | | [THRIFT-5601](https://issues.apache.org/jira/browse/THRIFT-5601) | `Foo *int32` | `Foo *Foo`, the output the ticket calls expected | | [THRIFT-5685](https://issues.apache.org/jira/browse/THRIFT-5685) | `Bar *Foo`, `var Bar_Bar_DEFAULT *Foo`, `GetBar() *Foo` | byte-identical | | [THRIFT-5463](https://issues.apache.org/jira/browse/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: ```go 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](https://issues.apache.org/jira/browse/THRIFT-3037) and [THRIFT-3491](https://issues.apache.org/jira/browse/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.* -- 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]
