From 0964475c5e17cf1a0fb3de3c80e4670cfb2e4e2a Mon Sep 17 00:00:00 2001 From: Sylwester Lachiewicz Date: Tue, 8 Sep 2026 10:42:01 +0200 Subject: [PATCH] THRIFT-6200: Use the declaring package for arguments in the Go -remote stub Client: go The stub qualified two things with the package it was generated into even when they live in an included file, and the stub then did not compile: - an enum argument, written as package_name_aliased unconditionally while the struct branch a few lines below already resolves the package with module_name(); - the args struct of a container or exception argument on a function inherited from a service declared in an included file. func_to_service recorded only the declaring service's name, so its package was lost. func_to_service now records the service itself, the enum and container branches resolve the package the same way the struct branch does, and the stub imports the package of every ancestor service, so a function inherited across more than one include level resolves too. lib/go/test built includestest but none of its -remote packages, which is why this stayed green. NamespacedService gains list, map and exception arguments and now extends a service in a new NamespacedBaseTest.thrift, so Extended2Service inherits functions from one and two include levels away; its three -remote packages join the check list. Co-Authored-By: Claude Fable 5.1 --- .../cpp/src/thrift/generate/t_go_generator.cc | 40 ++++++++++++++----- lib/go/test/Makefile.am | 5 +++ lib/go/test/NamespacedBaseTest.thrift | 24 +++++++++++ lib/go/test/NamespacedTest.thrift | 10 ++++- 4 files changed, 69 insertions(+), 10 deletions(-) create mode 100644 lib/go/test/NamespacedBaseTest.thrift diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.cc b/compiler/cpp/src/thrift/generate/t_go_generator.cc index 14fda81f31d..f4eb982b869 100644 --- a/compiler/cpp/src/thrift/generate/t_go_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_go_generator.cc @@ -2551,7 +2551,7 @@ void t_go_generator::generate_service_client(t_service* tservice) { */ void t_go_generator::generate_service_remote(t_service* tservice) { vector functions; - std::unordered_map func_to_service; + std::unordered_map func_to_service; // collect all functions including inherited functions t_service* parent = tservice; @@ -2559,14 +2559,15 @@ void t_go_generator::generate_service_remote(t_service* tservice) { vector p_functions = parent->get_functions(); functions.insert(functions.end(), p_functions.begin(), p_functions.end()); - // We need to maintain a map of functions names to the name of their parent. - // This is because functions may come from a parent service, and if we need - // to create the arguments struct (e.g. `NewParentServiceNameFuncNameArgs()`) - // we need to make sure to specify the correct service name. + // We need to maintain a map of function names to the service that declares + // them. This is because functions may come from a parent service, and if we + // need to create the arguments struct (e.g. `NewParentServiceNameFuncNameArgs()`) + // we need to make sure to specify the correct service name and, when the + // parent service lives in an included file, the correct package. for (vector::iterator f_iter = p_functions.begin(); f_iter != p_functions.end(); ++f_iter) { auto it = func_to_service.find((*f_iter)->get_name()); if (it == func_to_service.end()) { - func_to_service.emplace((*f_iter)->get_name(), parent->get_name()); + func_to_service.emplace((*f_iter)->get_name(), parent); } } @@ -2616,6 +2617,13 @@ void t_go_generator::generate_service_remote(t_service* tservice) { for (auto include : program_->get_includes()) { local_programs.emplace(get_real_go_module(include), include); } + // A function inherited from a service declared in a file that this program + // does not include directly still needs that file's package for its args + // struct. + for (t_service* ancestor = tservice->get_extends(); ancestor != nullptr; + ancestor = ancestor->get_extends()) { + local_programs.emplace(get_real_go_module(ancestor->get_program()), ancestor->get_program()); + } local_programs.emplace(get_real_go_module(program_), program_); if (!local_programs.empty()) { f_remote << '\n'; @@ -2840,7 +2848,15 @@ void t_go_generator::generate_service_remote(t_service* tservice) { std::vector::size_type num_args = args.size(); string funcName((*f_iter)->get_name()); string pubName(publicize(funcName)); - string argumentsName(publicize(funcName + "_args", true, func_to_service[funcName])); + t_service* declaring_service = func_to_service[funcName]; + string argumentsName(publicize(funcName + "_args", true, declaring_service->get_name())); + // The args struct is generated next to the service that declares the + // function, which is another package when that service comes from an + // included file. + string argumentsModule(module_name(declaring_service)); + if (argumentsModule.empty()) { + argumentsModule = package_name_aliased; + } f_remote << indent() << "case \"" << escape_string(funcName) << "\":" << '\n'; indent_up(); f_remote << indent() << "if flag.NArg()-1 != " << num_args << " {" << '\n'; @@ -2865,7 +2881,13 @@ void t_go_generator::generate_service_remote(t_service* tservice) { f_remote << indent() << "return" << '\n'; indent_down(); f_remote << indent() << "}" << '\n'; - f_remote << indent() << "argvalue" << i << " := " << package_name_aliased << "." + // An enum declared in an included file lives in that file's package, not + // in the one this service was generated into. + std::string enum_module(module_name(the_type)); + if (enum_module.empty()) { + enum_module = package_name_aliased; + } + f_remote << indent() << "argvalue" << i << " := " << enum_module << "." << publicize(the_type->get_name()) << "(tmp" << i << ")" << '\n'; } else if (the_type2->is_base_type()) { t_base_type::t_base e = ((t_base_type*)the_type2)->get_base(); @@ -3024,7 +3046,7 @@ void t_go_generator::generate_service_remote(t_service* tservice) { f_remote << indent() << factory << " := thrift.NewTJSONProtocolFactory()" << '\n'; f_remote << indent() << jsProt << " := " << factory << ".GetProtocol(" << mbTrans << ")" << '\n'; - f_remote << indent() << "containerStruct" << i << " := " << package_name_aliased << ".New" + f_remote << indent() << "containerStruct" << i << " := " << argumentsModule << ".New" << argumentsName << "()" << '\n'; f_remote << indent() << err2 << " := containerStruct" << i << ".ReadField" << (i + 1) << "(context.Background(), " << jsProt << ")" << '\n'; diff --git a/lib/go/test/Makefile.am b/lib/go/test/Makefile.am index 9ba38596f01..bba20989ccf 100644 --- a/lib/go/test/Makefile.am +++ b/lib/go/test/Makefile.am @@ -33,6 +33,7 @@ gopath: $(THRIFT) $(THRIFTTEST) \ $(RECURSIVE) \ IncludesTest.thrift \ NamespacedTest.thrift \ + NamespacedBaseTest.thrift \ MultiplexedProtocolTest.thrift \ OnewayTest.thrift \ OptionalFieldsTest.thrift \ @@ -115,6 +116,9 @@ gopath: $(THRIFT) $(THRIFTTEST) \ check: gopath $(GO) build $(GOBUILDEXTRA) \ ./gopath/src/includestest \ + ./gopath/src/includestest/test_service-remote \ + ./gopath/src/includestest/extended_service-remote \ + ./gopath/src/includestest/extended2_service-remote \ ./gopath/src/binarykeytest \ ./gopath/src/containerkeytest \ ./gopath/src/structkeytest \ @@ -180,6 +184,7 @@ EXTRA_DIST = \ InitialismsTest.thrift \ MultiplexedProtocolTest.thrift \ NamespacedTest.thrift \ + NamespacedBaseTest.thrift \ NamesTest.thrift \ OnewayTest.thrift \ OptionalFieldsTest.thrift \ diff --git a/lib/go/test/NamespacedBaseTest.thrift b/lib/go/test/NamespacedBaseTest.thrift new file mode 100644 index 00000000000..121bdb06b26 --- /dev/null +++ b/lib/go/test/NamespacedBaseTest.thrift @@ -0,0 +1,24 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +namespace go lib.go.test.namespacedbasetest + +service NamespacedBaseService { + void baseList(1: list xs), +} diff --git a/lib/go/test/NamespacedTest.thrift b/lib/go/test/NamespacedTest.thrift index a9103506295..058a8ace177 100644 --- a/lib/go/test/NamespacedTest.thrift +++ b/lib/go/test/NamespacedTest.thrift @@ -18,6 +18,7 @@ # include "ThriftTest.thrift" +include "NamespacedBaseTest.thrift" namespace go lib.go.test.namespacedtest @@ -34,7 +35,14 @@ struct StuffStruct { 2: Stuff stuff, } -service NamespacedService { +exception StuffException { + 1: string message, +} + +service NamespacedService extends NamespacedBaseTest.NamespacedBaseService { ThriftTest.UserId getUserID(), + void takeList(1: list xs), + void takeMap(1: map m), + void takeException(1: StuffException e), }