[
https://issues.apache.org/jira/browse/THRIFT-6116?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer reassigned THRIFT-6116:
----------------------------------
Assignee: Jens Geyer
> 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
> Assignee: Jens Geyer
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> 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)
