Jens-G commented on PR #3666:
URL: https://github.com/apache/thrift/pull/3666#issuecomment-5455567156
### Code review
Thanks for the update, and sorry for the slow turnaround. Re-reviewed at
`c554a5997`; everything below was reproduced against compilers built from the
PR head and from the merge base (`64601a565`).
From the last round: issues 1 (typedef namespace) and 4 (Python listing the
field label) are fixed, and so are suggestions 1 (`Client:` trailer) and 3 (the
`gen_type_hints_` block). Issue 2 is fixed in the base method but reintroduced
in the new Kotlin override, and issue 3 is fixed in the base but present in the
new haxe copy — both are below as issues 2 and 6. Suggestion 2 was addressed by
wiring Kotlin up, which is where issues 2 and 3 come from.
Found 7 issues:
1. The whole `@throws` block sits inside `if (tfunction->has_doc())`, so
documenting only the throws clause and not the method emits nothing at all.
Python is the only one of the seven bindings where the feature fires in that
case — its rewritten path builds `has_doc` up from the sections, so `Raises:`
can stand alone. cpp, java, javame, kotlin, haxe (`t_haxe_generator.cc:3139`)
and netstd (`t_netstd_generator.cc:3945`) all generate zero comment lines for:
```thrift
void f() throws (/** this exception doc is dropped */ 1: E1 e)
```
Documenting the exceptions rather than the method is the natural way to
write this, and it silently produces nothing in six of seven languages.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_oop_generator.h#L104-L106
2. The Kotlin override has two problems. It reads `namespace kotlin`, which
is a key nothing else in the compiler uses — the Kotlin generator derives every
real package from `namespace java`
([:195](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc#L195),
:325, :408), and `namespace kotlin` appears in no `doc/` page and no `.thrift`
in the tree. With both keys set, the generated file contradicts itself:
```kotlin
package com.main // from namespace java
...
* @throws com.mainkt.LocalExc // from namespace kotlin
```
Second, it returns `namespace_str + "."` unconditionally, dropping the
empty-package guard the base method grew in this round. `exception NoNsErr`
with no namespace declared yields `@throws .NoNsErr` — exactly round-2 issue 2,
fixed in the base and reintroduced here.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc#L177-L184
3. Swapping `generate_kdoc_comment` for `generate_java_doc` at this call
site also adopts the base `@param` loop, which Kotlin did not emit before.
`generate_kdoc_comment`
([:2022](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc#L2022-L2026))
writes the doc text and nothing else. Diffing the generated `ThriftTest.kt`
before and after, every interface method gains a blank ` * ` line and a
description-less `@param thing` — sitting directly under the hand-written
`@param string thing - the string to print` the IDL already carries. Only the
`@throws` loop was needed here.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc#L1430-L1434
4. `get_true_type(...)->get_name()` emits the raw IDL name, bypassing the
target language's identifier escaping, so the `@throws` reference does not
match the class the same generator emits three lines below it. This is the
THRIFT-5927 family — java routes every class name through
`make_valid_java_identifier()` and kotlin through `kotlin_safe_name()`
([:410](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc#L410)),
and neither is reached from here:
```
exception native → class file $native.java, signature `throws $native`,
but the doc says @throws kw.pkg.native
```
javadoc with default doclint reports one error (`unexpected text *
@throws kw.pkg.native`) plus `warning: no @throws for kw.pkg.$native`. Kotlin
has the same shape with `exception object`. netstd gets it right via
`type_name()`.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_oop_generator.h#L121-L124
5. Newlines are erased from the exception doc without a space in their
place, so the last word of one line is glued to the first of the next:
```
@throws com.nasty.MultiLineExc First line of the explanationsecond line
continues here
```
Same in the netstd copy at `t_netstd_generator.cc:3971`. The erase is
also unnecessary: the `@param` loop ten lines above does not strip, and
`generate_docstring_comment()`
([t_generator.cc:170-192](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_generator.cc#L170-L192))
already splits on newlines and re-prefixes each line.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_oop_generator.h#L125-L128
6. The haxe copy emits the throws-clause field name between the type and the
description, so under the `@throws <Type> <description>` grammar the field
label becomes the first word of the description. The PR's own fixture change
shows it: `@throws Xception err1 Thrown when a bad thing happens`. For an
undocumented exception the result is a dangling `@throws Boom e` whose entire
description is a field label. The base, netstd, php and py paths all omit the
name.
The same loop does not strip the trailing newline `get_doc()` carries, so
consecutive documented entries are separated by a stray blank line — round-2
issue 3, in the haxe copy:
```
* @throws Xception err1 Thrown when a bad thing happens
*
* @throws Xception2 err2 Thrown when the input is in incorrect format,
for example
```
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_haxe_generator.cc#L3152-L3160
7. Author-controlled doc text goes verbatim into an XML doc element. `/**
thrown if a < b && c > d */` produces `/// <exception
cref="global::Ns.XmlExc">thrown if a < b && c > d</exception>`; building that
with `GenerateDocumentationFile=true` gives four `CS1570` warnings and drops
the entire member from the generated `.xml` (`<!-- Badly formed XML comment
ignored for member "M:..." -->`). The `<param>`/`<summary>` text in the same
function has the same hole today, so this is a new injection site rather than a
new class of bug — `t_delphi_generator.cc:477` already carries the `&`/`<`/`>`
helper.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_netstd_generator.cc#L3963-L3975
On the design: a correct implementation of this exact feature already exists
in a sibling `t_oop_generator` subclass, and the PR leaves it untouched.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_php_generator.cc#L2833-L2874
`t_php_generator::generate_php_doc(ostream&, t_function*)` emits `@throws
<qualified type> <doc>` and avoids issues 1, 4 and 5 structurally rather than
by getting three separate details right. `has_doc()` wraps only the function's
own text, so an exception-only doc still renders. It reuses the generator's own
`type_to_phpdoc()`, so qualification and escaping come for free instead of via
a new namespace lookup. And it does not strip newlines. Its `@param` and
`@return` sections are built the same way.
After this PR there are four parallel `@throws` loops in four formats —
`t_oop_generator.h:121`, `t_haxe_generator.cc:3156`,
`t_netstd_generator.cc:3968`, `t_php_generator.cc:2866` — so every fix above
reaches exactly one of them, and php gets none of them. If the shared loop
delegated the type rendering to a per-generator hook the way php delegates to
`type_to_phpdoc()`, issues 4 and 5 would not be expressible, and haxe and
netstd would not need copies at all.
Three suggestions, below the bar for the list above but verified:
- The new C++ `get_namespace()` re-implements `namespace_prefix()`
([:4724](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_cpp_generator.cc#L4724-L4746))
and already disagrees with it: `namespace_prefix()` returns ` ::a::b::` with a
leading global qualifier and space, and carries a comment explaining why; the
new one returns `a::b::`. So `@throws main_ns::LocalExc` where the rest of the
same generator writes ` ::main_ns::LocalExc`. `type_name(e->get_type())` would
replace both this override and the `get_namespace` + `get_name` concatenation
at the call site.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_cpp_generator.cc#L459-L463
- The shared params helper branches on `is_method_xcepts()` to choose
type-vs-name rendering, using an AST property as a proxy for which of its two
callers invoked it — both of which already pass the mode explicitly as
`subheader`. The flag does not mean "this is a throws clause":
[t_struct.h:162](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/parse/t_struct.h#L162)
documents it as "struct holds the exceptions declared at a service method",
and `t_delphi_generator.cc:2624` sets it on a `<fn>_result` struct that also
carries `success`. Passing a bool alongside `subheader` removes the coupling.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_py_generator.cc#L2771-L2780
- A few style points, since the checklist asks for `make style`: line 117 is
whitespace-only, so `git diff --check` exits non-zero; line 123 is 120
characters against the 100-column limit in `.clang-format` and
[doc/coding_standards.md:34](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/doc/coding_standards.md#L34),
and evaluates `get_true_type(e->get_type())` twice — hoisting a local fixes
both; and `t_cpp_generator.cc:459` / `t_kotlin_generator.cc:177` write `t_type
*type` where their own declarations and `PointerAlignment: Left` say `t_type*
type`.
https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/src/thrift/generate/t_oop_generator.h#L116-L124
Last thing — the only change under `test/` is two doc comments in a
cross-language interop fixture that asserts nothing about generated
documentation, and it exercises none of the paths above: not the exception-only
doc, not the multi-line doc, not the empty namespace, not a reserved
identifier, not XML escaping. `compiler/cpp/test/compiler/markdown_doc_test.py`
with `DocTest.thrift`/`DocTest.md` is a working golden-output test for
generated docs, already wired up at
[compiler/cpp/test/CMakeLists.txt:36](https://github.com/apache/thrift/blob/c554a5997061e7cb1f89cb122c47fdedc8ae99cd/compiler/cpp/test/CMakeLists.txt#L36)
— it passes unchanged against this build, which is to say it covers none of
the new code. That harness looks like the right home for a case or two here.
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react
with 👎.</sub>
--
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]
