Jens-G commented on PR #3666:
URL: https://github.com/apache/thrift/pull/3666#issuecomment-5129692892
### 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.
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_oop_generator.h#L117-L119
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](https://github.com/apache/thrift/blob/2ae9c11db596a90fc8daa21c3bd319821a4da42b/compiler/cpp/src/thrift/generate/t_java_generator.cc#L4686-L4688)),
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.
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_oop_generator.h#L39-L43
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.
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_oop_generator.h#L112-L119
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:
```python
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.)
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_py_generator.cc#L2734-L2737
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_py_generator.cc#L2774-L2778
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](https://github.com/apache/thrift/blob/2ae9c11db596a90fc8daa21c3bd319821a4da42b/AGENTS.md#L34-L38)
asks for a "comma-separated list of affected languages", and this repo tags
javame separately from java (e.g. `8e8e58a80`).
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_javame_generator.cc#L3059-L3062
- 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.
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_kotlin_generator.cc#L176-L185
- 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.
https://github.com/apache/thrift/blob/b80a1464d901fdf0c849d8f11d49b9c0d794c99d/compiler/cpp/src/thrift/generate/t_py_generator.cc#L2775-L2778
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](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]
