Jens-G commented on PR #3925:
URL: https://github.com/apache/thrift/pull/3925#issuecomment-5768449053
### Code review
No blocking issues found. Checked for bugs and CLAUDE.md compliance.
Two suggestions, below the bar for an issue but verified:
- The new pass in `parse()` does not finish when a typedef refers back to
itself, and the compiler then hangs at 100% CPU without a message:
- `typedef list<L> L` loops in the container branch of
`resolve_forward_types()`.
- `typedef T T`, or `typedef B A` together with `typedef A B`, loops in
`get_true_type()`, which the pass now calls on every declared typedef, even one
that nothing uses.
The base compiler exits 0 on each of these three files with `--gen cpp`
(also java, py, html and netstd). With this PR it hangs for every generator and
for `--audit`. When a struct field uses such a typedef, the base compiler
already crashed or hung, except with `--gen html`.
A fix that I tried: follow typedefs and container elements one step at a
time, and keep a set of the types on the current path. Each of the three files
then fails at once with `Type "<name>" refers to itself`, and on all 240
`.thrift` files in the repository, `--gen cpp` and `--gen html` give the same
exit status and the same output as this PR. A set in the container branch alone
is not enough, because `typedef T T` never leaves `get_true_type()`.
https://github.com/apache/thrift/blob/b5a7d62cc101a36b47be9cbb2b1d4393a8851240/compiler/cpp/src/thrift/main.cc#L1050-L1054
https://github.com/apache/thrift/blob/b5a7d62cc101a36b47be9cbb2b1d4393a8851240/compiler/cpp/src/thrift/main.cc#L937-L959
https://github.com/apache/thrift/blob/b5a7d62cc101a36b47be9cbb2b1d4393a8851240/compiler/cpp/src/thrift/parse/parse.cc#L29-L35
- The new rejections are right, and the description already names the
`--audit` case. Generators that never resolved every type change the same way:
`thrift --gen html -r lib/go/test/NamespacedTest.thrift`, where the include
`ThriftTest.thrift` is not found (only a warning), exits 0 on base and writes 4
files; with this PR it exits 1 with `Type "ThriftTest.UserId" not defined`.
`LightRail.thrift` and `Streetcars.thrift` in
`lib/rs/test_recursive/src/transit/light/` behave the same. The sweep in the
description (`--gen go --gen java --gen json`) could not show this, because
those generators already failed on base, and no build target compiles these
files this way. It could go next to the `--audit` note.
🤖 Generated with [Claude Code](https://claude.ai/code)
--
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]
