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