Jens Geyer created THRIFT-6116:
----------------------------------
Summary: Python generator still misses keyword escaping in six
more spots (consts, enum-value defaults, required-field checks,
type_hints/twisted/enum modes)
Key: THRIFT-6116
URL: https://issues.apache.org/jira/browse/THRIFT-6116
Project: Thrift
Issue Type: Bug
Components: Python - Compiler
Reporter: Jens Geyer
Fifth and (per a full audit of every get_name()-family call in
t_py_generator.cc, 105 sites classified) hopefully final follow-up in the
THRIFT-5927 chain (THRIFT-6113: regression test wasn't running in CI;
THRIFT-6114: service module filename, "__all__", and -remote script;
THRIFT-6115: service extends and cross-module type references).
Six more spots never got maybe_escape_identifier() wired in. Each confirmed
with a real SyntaxError via py_compile/ast.parse, not just inferred from
reading:
1. t_py_generator.cc:598, generate_const(): a top-level "const i32 break = 42"
const whose own name is a keyword renders "break = 42" in constants.py.
2. t_py_generator.cc:647, render_const_value()'s enum branch: an unescaped enum
*value* reference (e.g. "Lambda.None") in constant rendering. Reached from
top-level consts AND, via render_field_default_value() calling the same
function, from any field's default value -- struct __init__ defaults,
thrift_spec default-value entries, and type-hint parameter defaults all funnel
through this one line. One fix covers all four manifestations (verified by
regenerating a probe fixture under every relevant mode post-fix).
3. t_py_generator.cc:925, generate_py_struct_definition() (-gen py:enum + a
"(python.immutable = "")" struct): the __setattr__ override's
"EnumType.__members__.get(<field>)" call uses the raw field name, inconsistent
with the escaped uses of the same identifier earlier in the same statement.
4. t_py_generator.cc:1276, generate_py_struct_required_validator(): "if
self.<field> is None:" for a required field uses the raw field name.
5. t_py_generator.cc:2179-2183, generate_process_function() (-gen py:twisted):
the "except <Type> as <name>:" clause and its two subsequent uses use the raw
exception field name (xname), unescaped -- inconsistent with the tornado-style
and default-style equivalents at :2246/:2248 and :2322/:2326, which already
call maybe_escape_identifier(xname). The exception *type* half of this line was
already fixed by THRIFT-6115; only the "as" binding was still broken.
6. t_py_generator.cc:855, generate_py_struct_definition() (-gen
py:type_hints,enum + immutable struct): the class-level type-hint annotation
("<field>: typing.Optional[...]") uses the raw field name.
Four of these six only surface under non-default generator options
(gen_enum_/gen_type_hints_/gen_twisted_), which is why test_keyword_escape.py's
existing single "-gen py" run never caught them -- same class of blind spot as
THRIFT-6113 (a check that only ever exercises the default path). Fixed by
extending the test to loop the compiler over three generation modes ("py",
"py:type_hints,enum", "py:twisted") and running every existing static check
(compile, __all__, keyword-literal-except) against each.
New fixture additions to Thrift5927.thrift: a keyword-named plain const, a
keyword-named const of enum type (reusing the existing Lambda enum's None
value), a struct with a required keyword-named field, and a struct with a
keyword-named, enum-typed, default-valued field marked "(python.immutable =
"")".
Const-reference note: consts referencing other consts (e.g. "const i32 B = A;")
are resolved to their literal value by the compiler at parse time ("B = 5", not
"B = A"), so there's no separate const-reference call site needing escaping
beyond the definition itself.
Explicitly out of scope (different bug category, not an identifier-escaping
gap): get_real_py_module()'s fallback to program->get_name() when no "namespace
py ..." is declared -- if a .thrift file's own base name collides with a Python
keyword, the generated top-level package name would too. This is a
package/namespace-naming concern, not an identifier-escaping one, and the
project already has an established workaround pattern for exactly this (see the
"namespace d share" comment in tutorial/shared.thrift, choosing a different
namespace name rather than auto-escaping).
Branched from THRIFT-6115 (not master): the twisted except-fix (item 5) is
layered on top of THRIFT-6115's type_name() fix (the exception type half of
that same line), so this depends on THRIFT-6115 (PR #3660), which in turn
depends on THRIFT-6114 (PR #3659).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
