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