Jens Geyer created THRIFT-6114:
----------------------------------
Summary: 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
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)
