[jira] [Assigned] (THRIFT-6114) Python service/function names that are Python keywords generate unimportable modules and a broken -remote script

["Jens Geyer (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/THRIFT-6114?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Jens Geyer reassigned THRIFT-6114:
----------------------------------

    Assignee: Jens Geyer

> Python service/function names that are Python keywords generate unimportable modules and a broken -remote script
> ----------------------------------------------------------------------------------------------------------------
>
>                 Key: THRIFT-6114
>                 URL: https://issues.apache.org/jira/browse/THRIFT-6114
>             Project: Thrift
>          Issue Type: Bug
>          Components: Python - Compiler
>            Reporter: Jens Geyer
>            Assignee: Jens Geyer
>            Priority: Major
>             Fix For: 0.25.0
>
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> Follow-up to THRIFT-5927 (Python keyword-escaping) and THRIFT-6113 (regression test wasn't actually running in CI).
> THRIFT-5927 switched Python codegen from hard-rejecting reserved-keyword identifiers at compile time (validate_id() against the full keyword set) to escaping them with a trailing underscore. That escaping was wired into struct/enum/field/exception generation and the service Client/Processor class bodies, but was missed in two places that are exercised by the existing Thrift5927.thrift test fixture (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 (t_py_generator.cc:1293: "package_dir_ + "/" + service_name_ + ".py""). For a service named "continue" this produces continue.py. That file compiles fine in isolation, but it can only be reached with importlib -- neither statement form a normal consumer (or Thrift's own generated -remote script) would use actually parses:
>      >>> 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() (t_py_generator.cc:1765-1949), which emits the "<service>-remote" CLI helper script, never calls maybe_escape_identifier() at all. For Thrift5927.thrift this produces a script with three genuine syntax errors, reproduced with py_compile:
>      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
>    py_compile.compile() on this file raises "SyntaxError: invalid syntax" at line 19.
> Net effect: for a keyword-named service or function, THRIFT-5927 replaced a safe, loud compile-time rejection with a silent generation of broken Python. This is a regression introduced by THRIFT-5927, not a pre-existing issue -- prior to that fix, compiling Thrift5927.thrift's service/function names would have failed validate_id() outright.
> The existing test (lib/py/test/test_compiler/test_keyword_escape.py) does not catch this because it only globs **/*.py for compileability; the -remote script is intentionally generated without a .py extension (matching every other language's convention for the executable helper script) and is invisible to that glob.
> Proposed fix (scoped to what's covered by the existing fixture):
> - Escape service_name_ at its three real identifier-position usages: the module filename (t_py_generator.cc:1293), the "from <module> import <service>" line in generate_service_remote (:1793), and the "client = <service>.Client(...)" line (:1896). The CLI script's own filename (<service>-remote, :1776) is left as-is since it is a shell-invoked filename, not a Python identifier -- keeping the literal IDL name there is more useful for users typing it at a shell.
> - Escape the function name used for the client dispatch call in generate_service_remote (:1920, "client.<function>(...)"), matching the already-escaped method name on the generated Client class. The other get_name() uses in that function are inside string literals (help text, the sys.argv command-word comparison) and correctly stay unescaped since they reflect the original IDL name a CLI user would type.
> - Extend test_keyword_escape.py to also compile-check generated "*-remote" files, not just "*.py".
> Out of scope for this ticket (found while investigating, but not covered by the current fixture and needs its own fixture + a broader look at the shared type_name()/type_to_spec_args() helpers): t_service extends (inheritance) references and cross-module (included-file) type references also build Python import/attribute-access strings from unescaped names (t_py_generator.cc:1298-1301, 1387-1388, 1432-1433, 1978-1979, and the type_name()/type_to_spec_args() helpers generally). Flagging for a separate follow-up rather than bundling an unverified, untested code path into this fix.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)