Jens-G opened a new pull request, #3660:
URL: https://github.com/apache/thrift/pull/3660
## Summary
**Depends on #3659 (THRIFT-6114)** -- this branch is based on `THRIFT-6114`,
not `master`, so the diff below includes THRIFT-6114's commit until that one
merges. Please review/merge #3659 first; this will show a clean 1-commit diff
once it's rebased onto `master` after that lands. See "Why the 6114 dependency"
below for why.
Fourth in the THRIFT-5927 follow-up chain (THRIFT-6113: regression test
wasn't running in CI; THRIFT-6114: service module filename and `-remote` script
weren't escaped).
`t_py_generator::type_name()` is the shared helper that renders any
struct/enum/exception/service reference as a Python expression (class
instantiation, type hints, extends declarations, deserialization, `except`
clauses, etc.). None of its three return paths escaped the identifier:
```cpp
string t_py_generator::type_name(t_type* ttype) {
...
if (ttype->is_service()) {
return get_real_py_module(...) + "." + ttype->get_name(); //
service extends / references
}
if (program != nullptr && program != program_) {
return get_real_py_module(...) + ".ttypes." + ttype->get_name(); //
cross-module (include) type references
}
return ttype->get_name(); //
same-module type references
}
```
This meant:
- A service `extends`ing another service whose name is a Python keyword
generated a broken `import module.<keyword>` statement
(`t_py_generator.cc:1298-1301`, a direct call that doesn't go through
`type_name()`) and a broken `class Client(module.<keyword>.Client):` (via
`type_name()`).
- A struct/enum/exception referenced across an `include` boundary whose name
is a keyword generated a broken `module.ttypes.<keyword>` reference wherever
it's used -- including the `except <Type> as <name>:` clauses generated for
service exceptions.
Same class of regression as THRIFT-6114 (5927 escaped identifiers in some
code paths, not others), just reached via `extends`/`include` instead of the
service module and `-remote` script.
**One added wrinkle:** for exception types specifically,
`True`/`False`/`None` are Python keyword *literals*, not statement keywords --
`except True as e:` parses without a `SyntaxError` (`True` is a valid
expression atom), so `py_compile`-based testing alone doesn't catch that
sub-case; it silently binds the wrong object and would raise a runtime
`TypeError` if ever hit. Same fix either way (escape to `True_`), but the test
needed a small addition to see it.
## Fix
- Escape the identifier at `type_name()`'s three return points.
- Escape the parent service name in the direct `import module.<service>`
extends statement (`t_py_generator.cc:1298-1301`).
- Extend `test_keyword_escape.py` with an AST-based check: fail if any
generated file has an `except` clause whose type expression is a bare
`True`/`False`/`None` constant (which `py_compile` alone can't see).
## Why the 6114 dependency
The service-extends-a-keyword-named-service scenario needs both fixes to
work end-to-end: 6114 fixes the *parent* service's own module filename
(`continue.py` -> `continue_.py`); this fix corrects the *child's* reference to
it. Neither alone is sufficient for that one scenario. The
struct/enum/exception cross-module and same-file cases in this PR are
independent of 6114 (those types live in `ttypes.py`, already correctly escaped
by the original THRIFT-5927) -- only the service-extends-service path is
coupled.
## Test fixtures
- Same-file: added `service AlsoDerived extends continue` to the existing
`Thrift5927.thrift`, exercising extends of a keyword-named parent in the same
file.
- Cross-module: added `lib/py/test/test_compiler/thrift5927include.thrift`,
modeled on `tutorial/shared.thrift` + `tutorial/tutorial.thrift`'s
`include`/`extends` pattern, providing a keyword-named struct (`except`) and
service (`class`). `Thrift5927.thrift` now `include`s it, adds `struct
UsesIncluded { 1: thrift5927include.except item }`, and `service Derived
extends thrift5927include.class`.
## Out of scope
Found while investigating, same missed-escaping category but each a
different specific variable bolted onto an otherwise-correct `type_name()` call
site (not part of `type_name()`'s own output, so not covered by this fix) --
flagging for a possible future consolidated pass:
- `t_py_generator.cc:647`, `render_const_value()`: the enum *value* name in
`EnumClass.VALUE` constant rendering is unescaped, inconsistent with that same
value being escaped everywhere else (e.g. as a class attribute).
- `t_py_generator.cc:925`: a trailing field-name use in a `__setattr__`
override's `__members__.get(...)` call is unescaped, inconsistent with two
escaped sibling uses of the same identifier two lines earlier in the same
statement.
- `t_py_generator.cc:2180`, `generate_service_client()`: the exception field
name (`xname`) is unescaped in one `except <Type> as xname:` binding and its
two subsequent uses, inconsistent with the same pattern at `:2246` and `:2322`
which do escape it.
## Test plan
- [x] Before the fix: extending the fixture (extends + include) reproduced
three distinct `SyntaxError`s via `py_compile` -- `import thrift5927.continue`,
`import thrift5927include.class`, and `thrift5927include.ttypes.except()`.
- [x] After the fix: `test_keyword_escape.py` passes (`OK: All 15 generated
Python files compile successfully`), including the new AST check.
- [x] Real import/cross-reference test (not just syntax):
`AlsoDerived.Client`/`Derived.Client` correctly inherit from the escaped parent
`Client` classes; `UsesIncluded`'s field type correctly resolves to the escaped
`except_` class from the included module.
- [x] Regression check: an ordinary (non-keyword) two-file extends+include
fixture generates byte-identical output to before.
- [x] Wider regression: `make -C lib/py check` (full suite, not just this
test) and `make -C test/py check`'s code-generation step both still pass -- 189
generated files across
`ThriftTest.thrift`/`DebugProtoTest.thrift`/`DoubleConstantsTest.thrift`/`Recursive.thrift`
in all 8 generation flavors
(default/slots/oldstyle/no_utf8strings/dynamic/dynamicslots/enum/type_hints)
still compile cleanly, since `type_name()` is used file-wide.
---
This PR includes AI-assisted changes (Claude Code); see commit trailer.
--
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]
