[PR] THRIFT-6115: Escape Python-keyword names in service extends and cross-module type references [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] THRIFT-6115: Escape Python-keyword names in service extends and cross-module type references [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G merged PR #3660:
URL: https://github.com/apache/thrift/pull/3660


-- 
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: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org