[PR] THRIFT-6114: Escape Python-keyword service/function names in service module and -remote script [thrift]

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

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

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

Re: [PR] THRIFT-6114: Escape Python-keyword service/function names in service module and -remote script [thrift]

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

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


-- 
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