THRIFT-6108: implemented exception documentation generation

[SLAVONchick]

Client: cpp,haxe,java,javame,kotlin,netstd,py

In this PR I've implemented the documentation generation of the exceptions thrown in the service methods.
I skipped the clients where there is no documentation is generated at all, so it only touched cpp,haxe,java,netstd,py.

• Did you create an Apache Jira ticket? (Request account here, not required for trivial changes) - Yes
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? - Yes
• Did you squash your changes to a single commit? (not required, but preferred) - Yes
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"? - Yes
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources. - Does involve code.

[Jens-G]

No, really, don't. We have OOP these days, like in "t_oop_generator", so we can use virtual methods to do such stuff.

PS: Pun intended - SCNR 🤣

[SLAVONchick]

Totally agree, my bad. Fixed.

[Jens-G]

I like the idea. Would be glad to see this move forward.

[Jens-G]

IIRC we don't use boost in the compiler. Can this be replaced by sth else?

[SLAVONchick]

Found identical implementations in several generators and left only one in base t_oop_generator.

[SLAVONchick]

I like the idea. Would be glad to see this move forward.

Hi! Thank you for the review! I'm really looking forward to finish this work.

[Jens-G]

Not sure how related that change is?

[SLAVONchick]

Otherwise I get an error during compilation:

[ 48%] Linking CXX executable ../../../bin/TTransportFactoryConfigTest
/usr/bin/x86_64-linux-gnu-ld.bfd: CMakeFiles/TTransportFactoryConfigTest.dir/TTransportFactoryConfigTest.cpp.o: in function `test_framed_transport_factory_enforces_custom_max_frame_size::test_method()':
/home/slavonchick/repos/cpp/thrift-fork/lib/cpp/test/TTransportFactoryConfigTest.cpp:107:(.text+0x3959): undefined reference to `apache::thrift::TConfiguration::DEFAULT_MAX_MESSAGE_SIZE'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [lib/cpp/test/CMakeFiles/TTransportFactoryConfigTest.dir/build.make:112: bin/TTransportFactoryConfigTest] Error 1
make[1]: *** [CMakeFiles/Makefile2:2415: lib/cpp/test/CMakeFiles/TTransportFactoryConfigTest.dir/all] Error 2
make: *** [Makefile:166: all] Error 2

Although it works for me, I can delete this fix if necessary.

[Jens-G]

Code review

Found 5 issues:

1. Deleting javame's generate_java_doc(ostream&, t_function*) override silently regresses javame output. t_javame_generator still declares two other overloads of that name and has no using t_oop_generator::generate_java_doc;, so C++ name hiding removes the base t_function* overload from the candidate set. Since t_function : public t_doc, the call sites at lines 1956 and 1981 now bind to the t_doc* overload instead. Javame loses the auto-generated @param tags it emitted before this PR and never gains the new @throws tags. It compiles without a warning.

thrift/compiler/cpp/src/thrift/generate/t_javame_generator.cc

Lines 188 to 192 in b3754d4

	void generate_java_doc(std::ostream& out, t_field* field) override;
	void generate_java_doc(std::ostream& out, t_doc* tdoc) override;

Worth fixing together with this: javame's get_namespace() appends no trailing separator, unlike the base default (".") and the cpp override ("::"), while the call site concatenates straight onto the type name. Once the overload resolution above is corrected, javame will emit @throws thrift.testXception. Kotlin's override has the same shape.

2. The unconditional ss << '\n'; runs whether or not the function declares any exceptions, and the @throws loop below already prefixes each entry with \n. Functions with no throws() clause get a trailing blank * line; functions with one get a doubled blank line. This affects every documented service function emitted by the cpp and java generators, which are the two that reach this shared method. Guarding it with if (!exceptions.empty()) fixes both cases.

thrift/compiler/cpp/src/thrift/generate/t_oop_generator.h

Lines 112 to 121 in b3754d4

	}
	ss << '\n';
	const std::vector<t_field*>& exceptions = tfunction->get_xceptions()->get_members();
	std::vector<t_field*>::const_iterator e_iter;
	for (e_iter = exceptions.begin(); e_iter != exceptions.end(); ++e_iter) {
	t_field* e = *e_iter;
	ss << "\n@throws " << get_namespace(e->get_type()) << e->get_type()->get_name();
	if (e->has_doc()) {

3. t_type::get_name() returns an empty string for inline container types, because t_list/t_set/t_map construct through t_container() into the default t_type() ctor, which never sets name_. A field declared 1: list<string> items renders as - items (). Base types also come out as IDL names (i32, string) rather than Python ones. The file already has type_to_py_type() for exactly this, used by arg_hint/member_hint/func_hint, which renders list[str] and dict[str, int] correctly.

thrift/compiler/cpp/src/thrift/generate/t_py_generator.cc

Lines 2774 to 2780 in b3754d4

	t_field* p = *p_iter;
	ss << " - " << p->get_name();
	if (gen_type_hints_) {
	ss << " (" << p->get_type()->get_name() << ")";
	}
	if (p->has_doc()) {
	ss << ": " << p->get_doc();

4. The @throws name is taken from e->get_type() without resolving typedefs. validate_throws() in main.cc checks the thrown type via get_true_type(), so typedef SomeException Alias followed by throws (1: Alias e) is legal IDL, and this would emit @throws ns.Alias — a name with no generated class behind it, since typedefs produce no class in Java or C++. The sibling generate_java_doc(ostream&, t_field*) two functions above already goes through get_true_type() for the same reason (THRIFT-4086, 4f63573f5), and the netstd path in this PR gets it right via type_name().

thrift/compiler/cpp/src/thrift/generate/t_oop_generator.h

Lines 118 to 122 in b3754d4

	for (e_iter = exceptions.begin(); e_iter != exceptions.end(); ++e_iter) {
	t_field* e = *e_iter;
	ss << "\n@throws " << get_namespace(e->get_type()) << e->get_type()->get_name();
	if (e->has_doc()) {
	std::string doc_string = e->get_doc();

5. This change to lib/cpp/test/TTransportFactoryConfigTest.cpp is unrelated to THRIFT-6108 and is a no-op — it extracts TConfiguration::DEFAULT_MAX_MESSAGE_SIZE into a local and passes that instead of the constant. Worth dropping so the PR stays scoped to the doc generators.

thrift/lib/cpp/test/TTransportFactoryConfigTest.cpp

Lines 106 to 109 in b3754d4

	const int kMaxFrameSize = 64;
	const int kMaxMessageSize = TConfiguration::DEFAULT_MAX_MESSAGE_SIZE;
	auto config = std::make_shared<TConfiguration>(kMaxMessageSize, kMaxFrameSize);

On the design, separately from the defects above: get_gen_name() looks like more machinery than the feature needs.

thrift/compiler/cpp/src/thrift/generate/t_oop_generator.h

Lines 39 to 45 in b3754d4

	virtual const std::string& get_gen_name() const = 0;
	virtual std::string get_namespace(t_type* type) {
	return type->get_program()->get_namespace(get_gen_name()) + ".";
	}

Making it pure virtual obliges all 18 t_oop_generator subclasses to implement it, each with its own gen_name_ member, which is most of why this PR touches 21 generator files when the machinery only ever serves the shared doc path. It has three call sites and all of them are inside get_namespace(), whose only consumer is the new @throws line — reachable from cpp, java and javame alone. Since javame's override hardcodes "java", only the cpp and java implementations are ever actually called.

The bigger issue is that the abstraction doesn't fit its users: four of the six get_namespace() overrides discard get_gen_name() outright — javame hardcodes "java", c_glib returns nspace, and perl and php delegate to the existing perl_namespace()/php_namespace() helpers. It is only usable where a generator's registration name and its IDL namespace key happen to coincide, which is exactly the cpp and java case.

That string also already exists in the compiler, in one place. THRIFT_REGISTER_GENERATOR registers each generator under #language:

thrift/compiler/cpp/src/thrift/generate/t_generator_registry.h

Lines 96 to 104 in b3754d4

	#define THRIFT_REGISTER_GENERATOR(language, long_name, doc) \
	class t_##language##_generator_factory_impl \
	: public t_generator_factory_impl<t_##language##_generator> { \
	public: \
	t_##language##_generator_factory_impl() \
	: t_generator_factory_impl<t_##language##_generator>(#language, long_name, doc) {} \
	}; \
	static t_##language##_generator_factory_impl _registerer;

and t_program::set_namespace() validates every namespace <key> directive against that same map:

thrift/compiler/cpp/src/thrift/parse/t_program.h

Lines 297 to 306 in b3754d4

	t_generator_registry::gen_map_t my_copy = t_generator_registry::get_generator_map();
	t_generator_registry::gen_map_t::iterator it;
	it = my_copy.find(base_language);
	if (it == my_copy.end()) {
	std::string warning = "No generator named '" + base_language + "' could be found!";
	pwarning(1, warning.c_str());
	} else {

Keeping get_namespace() as the virtual hook and dropping get_gen_name() entirely would give the same result: t_cpp_generator::get_namespace() passes "cpp" directly, the base default passes "java", and the other 16 generators need no change at all. That is the pattern the rest of the compiler already uses, e.g. t_netstd_generator.cc:140 and t_kotlin_generator.cc:202.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Jens-G]

Code review

The replace_all() consolidation from this PR is now on master as 2ae9c11db, so a rebase will drop that part. Reviewed the rest. The main path works — testMultiException in test/ThriftTest.thrift comes out as @throws thrift.test.Xception Thrown when a bad thing happens. Everything below was reproduced against a compiler built from b80a1464d.

Found 4 issues:

1. The @throws name is resolved through typedefs but the namespace is not, so an exception reached through a typedef gets a qualified name that no generated class has. get_true_type() is applied to the name half only, while get_namespace() still receives the raw type — and a t_typedef carries the program the typedef was declared in, not the one the underlying exception lives in. With inc.thrift (namespace java com.inc, exception RealExc) and main.thrift (namespace java com.main, include "inc.thrift", typedef inc.RealExc Alias, void go() throws (1: Alias e)), the generated file contradicts itself three lines apart:

    * @throws com.main.RealExc
    */
   public void go() throws com.inc.RealExc, org.apache.thrift.TException;
   

   Same in C++ (@throws main_ns::RealExc for a class that is really inc_ns::RealExc) and in JavaME through the new delegation. The haxe and netstd paths added in this PR avoid it by going through type_name(), which resolves once and derives both halves from the resolved type.

thrift/compiler/cpp/src/thrift/generate/t_oop_generator.h

Lines 117 to 119 in b80a146

	t_field* e = *e_iter;
	ss << "\n@throws " << get_namespace(e->get_type()) << get_true_type(e->get_type())->get_name();
	if (e->has_doc()) {

2. The default get_namespace() appends the . separator unconditionally, so a file with no namespace java yields @throws .SomeError, which is not a resolvable Javadoc reference. exception NoNsErr plus void doThing() throws (1: NoNsErr e) with no namespace declared generates:

    * @throws .NoNsErr
    */
   public void doThing() throws NoNsErr, org.apache.thrift.TException;
   

   type_name() in the java generator guards this case with if (!package.empty()) (t_java_generator.cc:4687), and get_enum_class_name() in this same file sidesteps it by prefixing only for types from another program. C++ is unaffected — ::NoNsErr is a valid global-namespace qualifier — but Java and JavaME are.

thrift/compiler/cpp/src/thrift/generate/t_oop_generator.h

Lines 39 to 43 in b80a146

	virtual std::string get_namespace(t_type* type) {
	return type->get_program()->get_namespace("java") + ".";
	}

3. The if (!exceptions.empty()) guard fixed the no-exceptions case from the last round, but for functions that do declare exceptions the guard's own ss << '\n' still runs immediately before a loop whose every entry already begins with \n. Since IDL doc text carries its own trailing newline, a function with no parameters, or one whose last parameter has its own doc comment, gets two blank * lines:

    * zero params, throws
    * 
    * 
    * @throws com.probe.MyError
   

   A function with undocumented parameters renders with a single blank line, which is why testMultiException looks right.

thrift/compiler/cpp/src/thrift/generate/t_oop_generator.h

Lines 112 to 119 in b80a146

	const std::vector<t_field*>& exceptions = tfunction->get_xceptions()->get_members();
	if (!exceptions.empty()) {
	ss << '\n';
	std::vector<t_field*>::const_iterator e_iter;
	for (e_iter = exceptions.begin(); e_iter != exceptions.end(); ++e_iter) {
	t_field* e = *e_iter;
	ss << "\n@throws " << get_namespace(e->get_type()) << get_true_type(e->get_type())->get_name();
	if (e->has_doc()) {

4. Python's Raises: section lists the throws-clause field label rather than the exception type, so in the default configuration the docstring never names the exception. It shows up only as a parenthetical under py:type_hints,enum ( - err1 (Xception)). test/DebugProtoTest.thrift already demonstrates the effect — void methodThatThrowsAnException() throws (1: ExceptionWithAMap xwamap) gains a docstring whose entire content is the label:

   def methodThatThrowsAnException(self):
       """
       Raises:
        - xwamap
   
       """

   The field label is the salient name for a parameter, since it is the real keyword argument, but in a throws clause it is an arbitrary tag — java, haxe and netstd all emit the type as the primary token. (Checked the rest of the refactor: plain --gen py over DebugProtoTest.thrift is otherwise byte-identical to master, so the has_doc pointer change did not disturb existing docstrings.)

thrift/compiler/cpp/src/thrift/generate/t_py_generator.cc

Lines 2734 to 2737 in b80a146

	}
	generate_python_params_docstring(ss, tfunction->get_arglist(), &has_doc, "Parameters");
	generate_python_params_docstring(ss, tfunction->get_xceptions(), &has_doc, "Raises");
	if (has_doc) {

thrift/compiler/cpp/src/thrift/generate/t_py_generator.cc

Lines 2774 to 2778 in b80a146

	t_field* p = *p_iter;
	ss << " - " << p->get_name();
	if (gen_type_hints_) {
	ss << " (" << type_to_py_type(p->get_type()) << ")";
	}

Three suggestions, below the bar for the list above but verified:

• The Client: trailer lists cpp,haxe,java,netstd,py but omits javame, whose output this change alters — swapping its own implementation for a delegation to the base method gains it @throws lines it did not emit before. AGENTS.md asks for a "comma-separated list of affected languages", and this repo tags javame separately from java (e.g. 8e8e58a80).

thrift/compiler/cpp/src/thrift/generate/t_javame_generator.cc

Lines 3059 to 3062 in b80a146

	*/
	void t_javame_generator::generate_java_doc(ostream& out, t_function* tfunction) {
	t_oop_generator::generate_java_doc(out, tfunction);
	}

• The new kotlin get_namespace() override is unreachable. Its only caller is the @throws block in t_oop_generator::generate_java_doc(ostream&, t_function*), and kotlin emits function docs through its own generate_kdoc_comment() instead, so a kotlin service function with a documented throws clause generates no @throws at all — consistent with Client: omitting kotlin. Either wire kotlin up or drop the override.

thrift/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc

Lines 176 to 185 in b80a146

	std::string t_kotlin_generator::get_namespace(t_type *type) {
	std::string namespace_str = type->get_program()->get_namespace("kotlin");
	if (namespace_str.empty()) {
	namespace_str = type->get_program()->get_namespace("java");
	}
	return namespace_str + ".";
	}

• The gen_type_hints_ block annotates every parameter and every struct attribute with its Python type, which is a separate feature from documenting exceptions. It changes existing output for current py:type_hints users beyond the new Raises sections: - arg becomes - arg (str) in service docstrings and - m becomes - m (str) in ttypes.py. Worth its own commit so THRIFT-6108 stays about exception docs.

thrift/compiler/cpp/src/thrift/generate/t_py_generator.cc

Lines 2775 to 2778 in b80a146

	ss << " - " << p->get_name();
	if (gen_type_hints_) {
	ss << " (" << type_to_py_type(p->get_type()) << ")";
	}

One design observation, no action needed for this PR: a base default that reads get_namespace("java") makes "java" the inherited namespace language for all 18 t_oop_generator subclasses, which is a little surprising in a class named for OOP rather than for Java. It is harmless today because the shared doc path is the only caller and only cpp, java and javame reach it. Both fixes for issues 1 and 2 land inside that same two-line default.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Jens-G]

FYI I extracted the replace_all() refactoring into a separate commit, keeping all the credits etc.

[SLAVONchick]

FYI I extracted the replace_all() refactoring into a separate commit, keeping all the credits etc.

Yeah, thanks! Really appreciate that.