Jens-G opened a new pull request, #3659:
URL: https://github.com/apache/thrift/pull/3659
## Summary
Follow-up to THRIFT-5927 (Python keyword escaping) and THRIFT-6113 (its
regression test wasn't actually running in CI). This is a regression introduced
by THRIFT-5927 itself: before that fix, a service or function named after a
Python keyword failed loudly at compile time (`validate_id()` against the full
keyword set). THRIFT-5927 switched to escaping instead of rejecting, but only
wired that escaping into struct/enum/field/exception generation and the service
Client/Processor class bodies — not into two spots that the *same*
`Thrift5927.thrift` test fixture already exercises (`service continue { import
return(1: False while) throws (1: True yield) }`):
1. `t_py_generator::generate_service()` builds the service's own module
filename from the raw, unescaped service name. For a service named `continue`
this emits `continue.py`. The file compiles fine in isolation, but neither
normal import statement a consumer would write can reach it:
```
>>> import ast
>>> ast.parse('import thrift5927.continue')
SyntaxError: invalid syntax
>>> ast.parse('from thrift5927 import continue')
SyntaxError: invalid syntax
```
2. `t_py_generator::generate_service_remote()`, which emits the
`<service>-remote` CLI helper script, never calls `maybe_escape_identifier()`
at all. For this fixture it produced a script with three real syntax errors
(confirmed with `py_compile`):
```python
from thrift5927 import continue # line 19 -- import target is a
keyword
client = continue.Client(protocol) # line 104 -- keyword used as an
identifier
client.return(eval(args[0]),) # line 111 -- keyword used as a
method name
```
The existing regression test didn't catch this because it only globs
`**/*.py` for compileability; the `-remote` script intentionally has no `.py`
suffix (same convention as every other language's generated CLI helper) and was
invisible to that glob.
## Fix
- Escape `service_name_` at its three real Python-identifier usages: the
module filename, the remote script's `from <module> import <service>` line, and
its `client = <service>.Client(...)` line.
- Escape the function name used in the remote script's client dispatch call
(`client.<function>(...)`), matching the already-escaped method name on the
generated `Client` class.
- Left untouched, on purpose: the `<service>-remote` filename itself (a
shell-invoked filename, not a Python identifier -- keeping the literal IDL name
is more useful for a human typing it), and the human-readable help/usage text
and `sys.argv` command-word comparisons (string literals/values, not syntax
positions -- a CLI user should still type and see the real IDL name).
- Extended `test_keyword_escape.py` to also compile-check generated
`*-remote` files.
## Out of scope
Found while investigating, but not covered by the current fixture (no
`extends` relationship in it) and not included here: `t_service` inheritance
(`extends`) references and cross-module (included-file) type references also
build Python import/attribute-access strings from unescaped names, via the same
`generate_service()` extends-handling code and the shared
`type_name()`/`type_to_spec_args()` helpers. That needs its own fixture and a
closer look at those shared helpers, tracked separately rather than bundled
into this fix.
Also note: this PR does not run `clang-format` on the full
`t_py_generator.cc` -- the file already has substantial pre-existing formatting
drift unrelated to this change (not currently enforced by any CI check), and
reformatting it wholesale seemed out of scope for a 4-line bug fix.
## Test plan
- [x] Before the fix: extending the test's glob to include `*-remote` files
reproduces the bug -- `py_compile` raises `SyntaxError: invalid syntax` on
`continue-remote` line 19.
- [x] After the fix: `test_keyword_escape.py` passes (`OK: All 6 generated
Python files compile successfully`).
- [x] Real import test (not just syntax): with the fixed compiler, `from
thrift5927 import continue_`, `import thrift5927.continue_`, and
`hasattr(continue_.Client, 'return_')` all succeed against the generated
package plus the real `thrift` runtime library.
- [x] Regression check: an ordinary (non-keyword) service name generates
byte-identical output to before (verified with a throwaway `MyNormalService`
fixture).
---
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]
