Repository navigation
THRIFT-6391: Import packages that collide with -remote stub locals under an alias - #3971
slachiewicz wants to merge 1 commit into
Conversation
mergeable
Bot
added
golang
032e112 to
5189995
Compare
|
The lib-d job here timed out in |
…der an alias Client: go The Go -remote stub's main() declares locals such as client, cmd and host, which shadowed an imported package of the same name, so the stub did not compile. Apache Accumulo (client.thrift) and Apache Pegasus (package cmd) hit it. The generator now reserves those names before rendering imports, so such a package is imported under an alias; nothing changes when -remote stubs are skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5189995 to
4339c64
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Alias generation can select an identifier already assigned to another imported package.
1 open finding
What changed in this PR
Updates Go code generation to prevent -remote stub locals from shadowing imported Thrift packages.
Changes:
- Reserves generated remote-stub local names before import allocation.
- Adds compile-time regression fixtures for
clientandhostpackage collisions. - Includes generated packages and remote executable in Go checks.
| File | Description |
|---|---|
compiler/cpp/src/thrift/generate/t_go_generator.cc |
Reserves remote-stub identifiers. |
lib/go/test/Makefile.am |
Generates, builds, and distributes regression fixtures. |
lib/go/test/RemoteShadowTest.thrift |
Defines the regression service. |
lib/go/test/RemoteShadowClient.thrift |
Adds the colliding client package. |
lib/go/test/RemoteShadowHost.thrift |
Adds the colliding host package. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "host", "httptrans", "iprot", "m", "oprot", "parsedUrl", | ||
| "parts", "port", "portStr", "protocol", "protocolFactory", "trans", | ||
| "urlString", "useHttp"}) { | ||
| package_identifiers_set_.insert(local); |
| "host", "httptrans", "iprot", "m", "oprot", "parsedUrl", | ||
| "parts", "port", "portStr", "protocol", "protocolFactory", "trans", | ||
| "urlString", "useHttp"}) { | ||
| package_identifiers_set_.insert(local); |
There was a problem hiding this comment.
package_identifiers_set_ is actually global so this impacts more than the -remote code? do we have a way to only do this for the -remote part?
There was a problem hiding this comment.
also the hardcoded list here doesn't seem maintainable, like it's easy to get out of sync with the actual local variables used in main inside -remote, but that part haven't changed in a long time though.
The Go
-remotestub'smain()declares locals such asclient,cmdandhost, which shadowed an imported package of the same name, so the stub did not compile. The generator now reserves those names before rendering imports, so such a package is imported under an alias (for exampleclient0) in every file of that program; nothing changes when-remotestubs are skipped. Programs that hit this also see generated temporaries renumbered, since allocating the alias uses the same counter. lib/go/test gains a service whose arguments come from packages namedclientandhost.Verified:
make -C lib/go check,make -C test/go checkandmake distpass; Apache Accumulo's generated Go,-remotestubs included, now builds (its hyphenated compaction-coordinator.thrift also needs #3968).