[
https://issues.apache.org/jira/browse/THRIFT-6115?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer updated THRIFT-6115:
-------------------------------
Fix Version/s: 0.25.0
> Python service extends and cross-module (include) type references also skip
> keyword escaping
> --------------------------------------------------------------------------------------------
>
> Key: THRIFT-6115
> URL: https://issues.apache.org/jira/browse/THRIFT-6115
> 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
>
> Fourth follow-up in the THRIFT-5927 chain (see also THRIFT-6113: regression
> test wasn't running in CI; THRIFT-6114: service module filename and -remote
> script weren't escaped).
> t_py_generator::type_name() is the shared helper used to render any
> struct/enum/exception/service reference as a Python expression (class
> instantiation, type hints, extends declarations, deserialization, etc.). None
> of its three return paths call maybe_escape_identifier():
> string t_py_generator::type_name(t_type* ttype) {
> ...
> if (ttype->is_service()) {
> return get_real_py_module(...) + "." + ttype->get_name(); //
> service extends / references
> }
> if (program != nullptr && program != program_) {
> return get_real_py_module(...) + ".ttypes." + ttype->get_name(); //
> cross-module (include) type references
> }
> return ttype->get_name(); //
> same-module type references
> }
> This means:
> - A service that "extends" another service whose name is a Python keyword
> generates a broken "import module.<keyword>" statement
> (t_py_generator.cc:1298-1301, a direct call not going through type_name())
> and a broken "class Client(module.<keyword>.Client):" (via type_name(), used
> at :1387-1388, 1432-1433, 1978-1979).
> - A struct/enum/exception referenced across an "include" boundary whose name
> is a keyword generates a broken "module.ttypes.<keyword>" reference wherever
> that type is used (constant rendering, deserialization, type hints, and the
> "except <Type> as <name>:" clauses used for service exceptions).
> This is the same class of regression as THRIFT-6114 (5927 escaped identifiers
> in some code paths but not others), just reached through extends/include
> instead of the service module and -remote script.
> One added wrinkle: for exception types specifically
> (generate_service_client's "except <Type> as <name>:" clauses),
> True/False/None are Python keyword *literals*, not statement keywords --
> "except True as e:" parses without a SyntaxError (True is a valid expression
> atom), so py_compile-based testing alone won't catch that particular
> sub-case; it silently binds the wrong object and would raise a runtime
> TypeError instead. The fix (escaping to True_) is the same either way; the
> test needs a light AST-based check (no ExceptHandler.type is a bare
> keyword-literal Constant) in addition to compileability.
> Fix (scoped to what type_name() and the one direct extends-import call
> control):
> - Escape the identifier at type_name()'s three return points (service name,
> cross-module type name, same-module type name).
> - Escape the service name in the direct "import module.<service>" extends
> statement (t_py_generator.cc:1298-1301).
> Test coverage:
> - Same-file extends of a keyword-named service (Thrift5927.thrift's existing
> "continue" service).
> - A new included file (thrift5927include.thrift, modeled on
> tutorial/shared.thrift + tutorial/tutorial.thrift's include pattern)
> providing a keyword-named struct and a keyword-named service, referenced and
> extended from Thrift5927.thrift the same way tutorial.thrift does "include
> shared.thrift" / "extends shared.SharedService".
> - An AST-based check that no exception handler's type expression is a bare
> True/False/None constant.
> Depends on THRIFT-6114 (PR #3659) for the
> service-extends-a-keyword-named-service scenario specifically: that scenario
> needs both fixes to work end-to-end (6114 fixes the parent service's own
> module filename; this fixes the child's reference to it). The
> struct/enum/exception cross-module and same-file cases are independent of
> 6114 (those types live in ttypes.py, already correctly escaped by the
> original THRIFT-5927). This PR is branched from THRIFT-6114 rather than
> master to allow real end-to-end verification of the combined scenario; see PR
> for details.
> Out of scope, found while investigating but not part of type_name()'s own
> output (separate unescaped identifiers bolted onto otherwise-correct
> type_name() call sites, same missed-escaping category but a different
> specific variable each time) -- flagging for a possible future consolidated
> pass rather than fixing piecemeal:
> - t_py_generator.cc:647, render_const_value(): the enum *value* name
> (enum_val->get_name()) in "EnumClass.VALUE" constant rendering is unescaped,
> inconsistent with the enum value being escaped everywhere else (e.g. as a
> class attribute).
> - t_py_generator.cc:925: a trailing (*m_iter)->get_name() in a __setattr__
> override's __members__.get(...) call is unescaped, inconsistent with two
> sibling escaped uses of the same identifier two lines earlier in the same
> statement.
> - t_py_generator.cc:2180, generate_service_client(): xname (an exception
> field name) is unescaped in the "except <Type> as xname:" binding and its two
> subsequent uses, inconsistent with the same pattern at :2246 and :2322 which
> do call maybe_escape_identifier(xname).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
