THRIFT-6115: Escape Python-keyword names in service extends and cross-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), so this branch is
based on THRIFT-6114 rather than master.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: Jens-G

compiler/cpp/src/thrift/generate/t_py_generator.cc

@@ -1298,7 +1298,7 @@ void t_py_generator::generate_service(t_service* tservice) {
   if (tservice->get_extends() != nullptr) {
     f_service_ << "import "
                << get_real_py_module(tservice->get_extends()->get_program(), gen_twisted_, package_prefix_) << "."
-               << tservice->get_extends()->get_name() << '\n';
+               << maybe_escape_identifier(tservice->get_extends()->get_name()) << '\n';
   }
 
   f_service_ << "import logging" << '\n'
@@ -2877,12 +2877,12 @@ string t_py_generator::type_name(t_type* ttype) {
 
   t_program* program = ttype->get_program();
   if (ttype->is_service()) {
-    return get_real_py_module(program, gen_twisted_, package_prefix_) + "." + ttype->get_name();
+    return get_real_py_module(program, gen_twisted_, package_prefix_) + "." + maybe_escape_identifier(ttype->get_name());
   }
   if (program != nullptr && program != program_) {
-    return get_real_py_module(program, gen_twisted_, package_prefix_) + ".ttypes." + ttype->get_name();
+    return get_real_py_module(program, gen_twisted_, package_prefix_) + ".ttypes." + maybe_escape_identifier(ttype->get_name());
   }
-  return ttype->get_name();
+  return maybe_escape_identifier(ttype->get_name());
 }
 
 string t_py_generator::arg_hint(t_type* type) {

lib/py/test/test_compiler/Thrift5927.thrift

@@ -17,9 +17,11 @@
 
 namespace py thrift5927
 
-enum Lambda { 
-	None = 0, 
-	FluxCapacitor = 1 
+include "thrift5927include.thrift"
+
+enum Lambda {
+	None = 0,
+	FluxCapacitor = 1
 }
 
 struct False {
@@ -39,3 +41,20 @@ service continue {
 	import return(1: False while) throws (1: True yield)
 }
 
+# Exercises type_name()'s cross-module branch: a field typed with a struct
+# defined in an included file, where that struct's name is a keyword.
+struct UsesIncluded {
+	1: thrift5927include.except item
+}
+
+# Exercises the extends import/base-class code path with a keyword-named
+# parent service defined in the same file.
+service AlsoDerived extends continue {
+}
+
+# Exercises type_name()'s service branch for both "extends" (import of the
+# parent's module + base class reference) and the cross-module include path
+# at once, since the parent service's name is also a keyword.
+service Derived extends thrift5927include.class {
+}
+

lib/py/test/test_compiler/test_keyword_escape.py

@@ -16,6 +16,7 @@
 # specific language governing permissions and limitations
 # under the License.
 
+import ast
 import os
 import sys
 import subprocess
@@ -61,6 +62,27 @@ def find_thrift():
     return None
 
 
+def find_keyword_literal_except_types(py_files):
+    """
+    "True"/"False"/"None" are valid Python expression atoms (unlike most
+    other reserved words), so an unescaped "except True as e:" parses fine
+    -- it just silently tries to catch the literal bool/None instead of the
+    intended exception class, and would raise a runtime TypeError once hit.
+    py_compile can't see this; walk the AST instead.
+    """
+    problems = []
+    for py_file in py_files:
+        with open(py_file) as f:
+            try:
+                tree = ast.parse(f.read(), filename=py_file)
+            except SyntaxError:
+                continue  # already reported by the compile check
+        for node in ast.walk(tree):
+            if isinstance(node, ast.ExceptHandler) and isinstance(node.type, ast.Constant):
+                problems.append((py_file, node.lineno, repr(node.type.value)))
+    return problems
+
+
 def test_keyword_escape_compilation():
     """
     Test that the Python generator produces valid Python code
@@ -118,6 +140,13 @@ def test_keyword_escape_compilation():
                 print("  " + file_path + ": " + error)
             return 1
 
+        keyword_literal_excepts = find_keyword_literal_except_types(py_files)
+        if keyword_literal_excepts:
+            print("ERROR: Generated code catches a keyword literal instead of an exception class:")
+            for file_path, lineno, value in keyword_literal_excepts:
+                print("  " + file_path + ":" + str(lineno) + ": except " + value + " as ...")
+            return 1
+
         print("OK: All " + str(len(all_files)) + " generated Python files compile successfully")
         return 0
 

lib/py/test/test_compiler/thrift5927include.thrift

@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Included by Thrift5927.thrift to exercise cross-module (include) references
+# to Python-keyword-colliding struct and service names, the same way
+# tutorial/tutorial.thrift includes tutorial/shared.thrift.
+
+namespace py thrift5927include
+
+struct except {
+	1: i32 value
+}
+
+service class {
+	except getValue(1: i32 key)
+}