Jens-G opened a new pull request, #3662: URL: https://github.com/apache/thrift/pull/3662
## Summary **Depends on #3660 (THRIFT-6115), which depends on #3659 (THRIFT-6114).** Branched from `THRIFT-6115`, not `master` -- the twisted `except` fix here (item 5 below) is layered on top of THRIFT-6115's `type_name()` fix (the exception-type half of that same generated line). Diff will narrow to this PR's own commit once #3660 and #3659 land and this rebases. Fifth in the THRIFT-5927 follow-up chain (THRIFT-6113: regression test wasn't running in CI; THRIFT-6114: service module filename, `__all__`, and `-remote` script; THRIFT-6115: service extends and cross-module type references) -- and, per a full audit of every `get_name()`-family call in `t_py_generator.cc` (105 call sites classified), the last one needed for this class of bug. Six more spots never got `maybe_escape_identifier()` wired in. Each confirmed with a real `SyntaxError` via `py_compile`/`ast.parse`, not just inferred from reading the code: 1. `t_py_generator.cc:598`, `generate_const()`: a top-level `const i32 break = 42` whose own name is a keyword renders `break = 42` in `constants.py`. 2. `t_py_generator.cc:647`, `render_const_value()`'s enum branch: an unescaped enum *value* reference (e.g. `Lambda.None`). Reached from top-level consts and, via `render_field_default_value()` calling the same function, from every field's default value -- struct `__init__` defaults, `thrift_spec` entries, and type-hint parameter defaults all share this one line. One fix covers all four manifestations (verified by regenerating a probe fixture under every relevant mode post-fix). 3. `t_py_generator.cc:925` (`-gen py:enum` + a `(python.immutable = "")` struct): the `__setattr__` override's `EnumType.__members__.get(<field>)` call uses the raw field name, inconsistent with two escaped uses of the same identifier earlier in the same statement. 4. `t_py_generator.cc:1276`, `generate_py_struct_required_validator()`: `if self.<field> is None:` for a required field uses the raw field name. 5. `t_py_generator.cc:2179-2183` (`-gen py:twisted`): the `except <Type> as <name>:` clause and its two subsequent uses use the raw exception field name, unescaped -- inconsistent with the tornado-style and default-style equivalents a few lines away, which already call `maybe_escape_identifier`. The exception *type* half of this line was already fixed by THRIFT-6115; only the `as` binding itself was not. 6. `t_py_generator.cc:855` (`-gen py:type_hints,enum` + immutable struct): the class-level type-hint annotation (`<field>: typing.Optional[...]`) uses the raw field name. ## Why the test didn't catch these already Four of the six only surface under non-default generator options (`gen_enum_`/`gen_type_hints_`/`gen_twisted_`), and `test_keyword_escape.py` only ever ran a single default `-gen py`. Same blind-spot shape as THRIFT-6113 (a check that only exercises the default path). Fixed by extending the test to loop the compiler over three modes (`py`, `py:type_hints,enum`, `py:twisted`) and running every existing static check (compile, `__all__`, keyword-literal-except) against each mode's output. ## Fixture additions `Thrift5927.thrift` gets: a keyword-named plain const, a keyword-named const of enum type (reusing the existing `Lambda` enum's `None` value), a struct with a required keyword-named field, and a struct with a keyword-named, enum-typed, default-valued field marked `(python.immutable = "")`. ## Const-reference note Consts referencing other consts (`const i32 B = A;`) are resolved to their literal value by the compiler at parse time (`B = 5`, not `B = A`), so there's no separate const-reference call site needing escaping beyond the definition itself -- checked directly, not assumed. ## Out of scope (different bug category) `get_real_py_module()`'s fallback to `program->get_name()` when no `namespace py ...` is declared -- if a `.thrift` file's own base name collides with a Python keyword, the generated top-level *package* name would too. That's a package/namespace-naming concern, not an identifier-escaping one, and the project already has an established workaround for exactly this (see the `namespace d share` comment in `tutorial/shared.thrift` -- pick a different namespace name rather than auto-escape). ## Test plan - [x] Before the fix: extending the fixture + multi-mode test reproduced all six as real `SyntaxError`s (`break = 42`, `Lambda.None` in four different contexts, `.get(with)`, `self.pass is None`, `except True_ as yield:`, `with: typing.Optional[Lambda]`) -- verified via a stash/rebuild/unstash cycle (fixtures+test in place, generator fix stashed out, confirmed failure; fix restored, confirmed pass). - [x] After the fix: `test_keyword_escape.py` passes (`OK: All 45 generated Python files (across 3 generation modes) compile successfully`). - [x] Regression: `make -C lib/py check` (full suite) passes; regenerated the full `ThriftTest.thrift`/`DebugProtoTest.thrift`/etc. suite under `py` and `py:type_hints,enum` and all output still compiles cleanly. --- 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]
