THRIFT-6115: Escape Python-keyword names in service extends and cross-module type references - #3660
Merged
Merged
Conversation
3 tasks
…-module type references Client: py type_name(), the shared helper rendering any struct/enum/exception/ service reference as a Python expression, never escaped the identifier on any of its three return paths. This broke two more cases sharing Thrift5927.thrift's existing keyword-named struct/service fixture data: - A service "extends" clause naming a keyword-named parent service produced a broken "import module.<keyword>" statement (a direct call, not through type_name) and a broken base-class reference "class Client(module.<keyword>.Client):" (through type_name). - A struct field, deserializer, or "except <Type> as <name>:" clause referencing a keyword-named type across an "include" boundary produced a broken "module.ttypes.<keyword>" reference. Escape the identifier at type_name()'s three return points and at the one direct (non-type_name) extends-import call site. One added wrinkle: True/False/None are Python keyword *literals*, not statement keywords, so an unescaped "except True as e:" parses without a SyntaxError -- it silently binds the wrong object and would raise a runtime TypeError if ever hit. py_compile-based testing alone can't see that, so test_keyword_escape.py also walks the AST of each generated file and fails if any exception handler's type is a bare keyword-literal Constant. Test fixtures: a same-file extends of the existing "continue" service, plus a new thrift5927include.thrift (modeled on tutorial/shared.thrift + tutorial/tutorial.thrift's include pattern) providing a keyword-named struct and service, included and referenced/extended from Thrift5927.thrift the same way tutorial.thrift uses shared.thrift. This depends on THRIFT-6114 for the service-extends-a-keyword-named- service case specifically (6114 fixes the parent's own module filename; this fixes the child's reference to it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Depends on #3659 (THRIFT-6114) -- this branch is based on
THRIFT-6114, notmaster, so the diff below includes THRIFT-6114's commit until that one merges. Please review/merge #3659 first; this will show a clean 1-commit diff once it's rebased ontomasterafter that lands. See "Why the 6114 dependency" below for why.Fourth in the THRIFT-5927 follow-up chain (THRIFT-6113: regression test wasn't running in CI; THRIFT-6114: service module filename and
-remotescript weren't escaped).t_py_generator::type_name()is the shared helper that renders any struct/enum/exception/service reference as a Python expression (class instantiation, type hints, extends declarations, deserialization,exceptclauses, etc.). None of its three return paths escaped the identifier:This meant:
extendsing another service whose name is a Python keyword generated a brokenimport module.<keyword>statement (t_py_generator.cc:1298-1301, a direct call that doesn't go throughtype_name()) and a brokenclass Client(module.<keyword>.Client):(viatype_name()).includeboundary whose name is a keyword generated a brokenmodule.ttypes.<keyword>reference wherever it's used -- including theexcept <Type> as <name>:clauses generated for service exceptions.Same class of regression as THRIFT-6114 (5927 escaped identifiers in some code paths, not others), just reached via
extends/includeinstead of the service module and-remotescript.One added wrinkle: for exception types specifically,
True/False/Noneare Python keyword literals, not statement keywords --except True as e:parses without aSyntaxError(Trueis a valid expression atom), sopy_compile-based testing alone doesn't catch that sub-case; it silently binds the wrong object and would raise a runtimeTypeErrorif ever hit. Same fix either way (escape toTrue_), but the test needed a small addition to see it.Fix
type_name()'s three return points.import module.<service>extends statement (t_py_generator.cc:1298-1301).test_keyword_escape.pywith an AST-based check: fail if any generated file has anexceptclause whose type expression is a bareTrue/False/Noneconstant (whichpy_compilealone can't see).Why the 6114 dependency
The service-extends-a-keyword-named-service scenario needs both fixes to work end-to-end: 6114 fixes the parent service's own module filename (
continue.py->continue_.py); this fix corrects the child's reference to it. Neither alone is sufficient for that one scenario. The struct/enum/exception cross-module and same-file cases in this PR are independent of 6114 (those types live inttypes.py, already correctly escaped by the original THRIFT-5927) -- only the service-extends-service path is coupled.Test fixtures
service AlsoDerived extends continueto the existingThrift5927.thrift, exercising extends of a keyword-named parent in the same file.lib/py/test/test_compiler/thrift5927include.thrift, modeled ontutorial/shared.thrift+tutorial/tutorial.thrift'sinclude/extendspattern, providing a keyword-named struct (except) and service (class).Thrift5927.thriftnowincludes it, addsstruct UsesIncluded { 1: thrift5927include.except item }, andservice Derived extends thrift5927include.class.Out of scope
Found while investigating, same missed-escaping category but each a different specific variable bolted onto an otherwise-correct
type_name()call site (not part oftype_name()'s own output, so not covered by this fix) -- flagging for a possible future consolidated pass:t_py_generator.cc:647,render_const_value(): the enum value name inEnumClass.VALUEconstant rendering is unescaped, inconsistent with that same value being escaped everywhere else (e.g. as a class attribute).t_py_generator.cc:925: a trailing field-name use in a__setattr__override's__members__.get(...)call is unescaped, inconsistent with two escaped sibling uses of the same identifier two lines earlier in the same statement.t_py_generator.cc:2180,generate_service_client(): the exception field name (xname) is unescaped in oneexcept <Type> as xname:binding and its two subsequent uses, inconsistent with the same pattern at:2246and:2322which do escape it.Test plan
SyntaxErrors viapy_compile--import thrift5927.continue,import thrift5927include.class, andthrift5927include.ttypes.except().test_keyword_escape.pypasses (OK: All 15 generated Python files compile successfully), including the new AST check.AlsoDerived.Client/Derived.Clientcorrectly inherit from the escaped parentClientclasses;UsesIncluded's field type correctly resolves to the escapedexcept_class from the included module.make -C lib/py check(full suite, not just this test) andmake -C test/py check's code-generation step both still pass -- 189 generated files acrossThriftTest.thrift/DebugProtoTest.thrift/DoubleConstantsTest.thrift/Recursive.thriftin all 8 generation flavors (default/slots/oldstyle/no_utf8strings/dynamic/dynamicslots/enum/type_hints) still compile cleanly, sincetype_name()is used file-wide.This PR includes AI-assisted changes (Claude Code); see commit trailer.