Fix name shadowing in python pyi generator for enum and message types When a protobuf message contains fields or nested classes whose names shadow an enum or message type used in field annotations (e.g. an `int32 kind` field shadowing enum `kind`, or an enclosing class/nested type shadowing an outer type), Python class attributes shadow the type names in class and method scope. Consequently, `__init__` parameter annotations and class attributes emitted by `pyi_generator.cc` fail type checking (e.g. Mypy `Variable "..." is not valid as a type [valid-type]`). Note that this shadowing can only happen when schemas do not follow the Protobuf naming guidelines (https://protobuf.dev/programming-guides/style/), which recommend TitleCase for messages/enums and snake_case for field names. This change: 1. Introduces `IsTypeShadowed` to detect whether a type name's root token collides with any field, nested enum, nested message, or enclosing class in the Python scope hierarchy. 2. Emits private module-level type aliases (`_Type_<name> = <name>`) on-demand when top-level types are shadowed. 3. Updates `GetFieldType` and `PrintInit` to use the alias when shadowed, ensuring robust resolution across all class and method scopes. 4. Adds tests covering top-level, nested, and map field shadowing scenarios. PiperOrigin-RevId: 970661048
diff --git a/src/google/protobuf/compiler/python/plugin_unittest.cc b/src/google/protobuf/compiler/python/plugin_unittest.cc index e792566..e51925c 100644 --- a/src/google/protobuf/compiler/python/plugin_unittest.cc +++ b/src/google/protobuf/compiler/python/plugin_unittest.cc
@@ -22,6 +22,7 @@ #include "google/protobuf/compiler/command_line_interface_tester.h" #include "google/protobuf/compiler/cpp/generator.h" #include "google/protobuf/compiler/python/generator.h" +#include "google/protobuf/compiler/python/pyi_generator.h" #include "google/protobuf/cpp_features.pb.h" #include "google/protobuf/io/printer.h" #include "google/protobuf/io/zero_copy_stream.h" @@ -105,6 +106,223 @@ EXPECT_TRUE(found_expected_import); } +TEST(PythonPluginTest, PyiFieldTypeDisambiguation) { + ABSL_CHECK_OK( + File::SetContents(absl::StrCat(::testing::TempDir(), "/collision.proto"), + R"pb( + syntax = "proto3" + ; +package foo; + +enum kind { + KIND_UNSPECIFIED = 0; +} +message item {} +message container { + int32 kind = 1; + int32 item = 2; + kind other_kind = 3; + repeated kind repeated_kind = 4; + item other_item = 5; + repeated item repeated_item = 6; + map<string, kind> map_kind = 7; + map<string, item> map_item = 8; +} + +enum shadowed_enum { + TOP_ENUM_UNSPECIFIED = 0; +} +message shadowed_msg {} +message shadow_scope { + enum shadowed_enum { + NESTED_ENUM_UNSPECIFIED = 0; + } + message shadowed_msg {} + message inner { + foo.shadowed_enum top_enum_ref = 1; + foo.shadowed_msg top_msg_ref = 2; + shadowed_enum nested_enum_ref = 3; + shadowed_msg nested_msg_ref = 4; + } +} + +message outer { + enum nested_enum { + NESTED_UNSPECIFIED = 0; + } + message nested_leaf {} + message middle { + message inner { + int32 outer = 1; + nested_enum other_nested_enum = 2; + nested_leaf other_nested_leaf = 3; + map<string, nested_enum> map_nested_enum = 4; + map<string, nested_leaf> map_nested_leaf = 5; + } + } +} + +enum map_val_enum { + MAP_VAL_UNSPECIFIED = 0; +} +message map_val_msg {} +message map_only_container { + int32 map_val_enum = 1; + int32 map_val_msg = 2; + map<string, map_val_enum> enum_map = 3; + map<string, map_val_msg> msg_map = 4; +} +)pb", + true)); + + compiler::CommandLineInterface cli; + cli.SetInputsAreProtoPathRelative(true); + python::PyiGenerator pyi_generator; + cli.RegisterGenerator("--pyi_out", &pyi_generator, ""); + std::string proto_path = absl::StrCat("-I", ::testing::TempDir()); + std::string pyi_out = absl::StrCat("--pyi_out=", ::testing::TempDir()); + const char* argv[] = {"protoc", proto_path.c_str(), "-I.", pyi_out.c_str(), + "collision.proto"}; + ASSERT_EQ(0, cli.Run(5, argv)); + + std::string output; + ABSL_CHECK_OK(File::GetContents( + absl::StrCat(::testing::TempDir(), "/collision_pb2.pyi"), &output, + true)); + + // 1. Verify top-level message with fields shadowing enum and message types. + EXPECT_TRUE(absl::StrContains(output, R"pyi( +class container(_message.Message): + __slots__ = ("kind", "item", "other_kind", "repeated_kind", "other_item", "repeated_item", "map_kind", "map_item") + class MapKindEntry(_message.Message): + __slots__ = ("key", "value") + KEY_FIELD_NUMBER: _ClassVar[int] + VALUE_FIELD_NUMBER: _ClassVar[int] + key: str + value: _Type_kind + def __init__(self, key: _Optional[str] = ..., value: _Optional[_Union[_Type_kind, str]] = ...) -> None: ... + class MapItemEntry(_message.Message): + __slots__ = ("key", "value") + KEY_FIELD_NUMBER: _ClassVar[int] + VALUE_FIELD_NUMBER: _ClassVar[int] + key: str + value: _Type_item + def __init__(self, key: _Optional[str] = ..., value: _Optional[_Union[_Type_item, _Mapping]] = ...) -> None: ... + KIND_FIELD_NUMBER: _ClassVar[int] + ITEM_FIELD_NUMBER: _ClassVar[int] + OTHER_KIND_FIELD_NUMBER: _ClassVar[int] + REPEATED_KIND_FIELD_NUMBER: _ClassVar[int] + OTHER_ITEM_FIELD_NUMBER: _ClassVar[int] + REPEATED_ITEM_FIELD_NUMBER: _ClassVar[int] + MAP_KIND_FIELD_NUMBER: _ClassVar[int] + MAP_ITEM_FIELD_NUMBER: _ClassVar[int] + kind: int + item: int + other_kind: _Type_kind + repeated_kind: _containers.RepeatedScalarFieldContainer[_Type_kind] + other_item: _Type_item + repeated_item: _containers.RepeatedCompositeFieldContainer[_Type_item] + map_kind: _containers.ScalarMap[str, _Type_kind] + map_item: _containers.MessageMap[str, _Type_item] + def __init__(self, kind: _Optional[int] = ..., item: _Optional[int] = ..., other_kind: _Optional[_Union[_Type_kind, str]] = ..., repeated_kind: _Optional[_Iterable[_Union[_Type_kind, str]]] = ..., other_item: _Optional[_Union[_Type_item, _Mapping]] = ..., repeated_item: _Optional[_Iterable[_Union[_Type_item, _Mapping]]] = ..., map_kind: _Optional[_Mapping[str, _Type_kind]] = ..., map_item: _Optional[_Mapping[str, _Type_item]] = ...) -> None: ... +)pyi")); + + // 2. Verify nested scope referencing outer types vs nested types. + EXPECT_TRUE(absl::StrContains(output, R"pyi( + class inner(_message.Message): + __slots__ = ("top_enum_ref", "top_msg_ref", "nested_enum_ref", "nested_msg_ref") + TOP_ENUM_REF_FIELD_NUMBER: _ClassVar[int] + TOP_MSG_REF_FIELD_NUMBER: _ClassVar[int] + NESTED_ENUM_REF_FIELD_NUMBER: _ClassVar[int] + NESTED_MSG_REF_FIELD_NUMBER: _ClassVar[int] + top_enum_ref: _Type_shadowed_enum + top_msg_ref: _Type_shadowed_msg + nested_enum_ref: shadow_scope.shadowed_enum + nested_msg_ref: shadow_scope.shadowed_msg + def __init__(self, top_enum_ref: _Optional[_Union[_Type_shadowed_enum, str]] = ..., top_msg_ref: _Optional[_Union[_Type_shadowed_msg, _Mapping]] = ..., nested_enum_ref: _Optional[_Union[shadow_scope.shadowed_enum, str]] = ..., nested_msg_ref: _Optional[_Union[shadow_scope.shadowed_msg, _Mapping]] = ...) -> None: ... +)pyi")); + + // 3. Verify deeply nested message with int32 field shadowing outer class + // name. + EXPECT_TRUE(absl::StrContains(output, R"pyi( + class inner(_message.Message): + __slots__ = ("outer", "other_nested_enum", "other_nested_leaf", "map_nested_enum", "map_nested_leaf") + class MapNestedEnumEntry(_message.Message): + __slots__ = ("key", "value") + KEY_FIELD_NUMBER: _ClassVar[int] + VALUE_FIELD_NUMBER: _ClassVar[int] + key: str + value: _Type_outer.nested_enum + def __init__(self, key: _Optional[str] = ..., value: _Optional[_Union[_Type_outer.nested_enum, str]] = ...) -> None: ... + class MapNestedLeafEntry(_message.Message): + __slots__ = ("key", "value") + KEY_FIELD_NUMBER: _ClassVar[int] + VALUE_FIELD_NUMBER: _ClassVar[int] + key: str + value: _Type_outer.nested_leaf + def __init__(self, key: _Optional[str] = ..., value: _Optional[_Union[_Type_outer.nested_leaf, _Mapping]] = ...) -> None: ... + OUTER_FIELD_NUMBER: _ClassVar[int] + OTHER_NESTED_ENUM_FIELD_NUMBER: _ClassVar[int] + OTHER_NESTED_LEAF_FIELD_NUMBER: _ClassVar[int] + MAP_NESTED_ENUM_FIELD_NUMBER: _ClassVar[int] + MAP_NESTED_LEAF_FIELD_NUMBER: _ClassVar[int] + outer: int + other_nested_enum: _Type_outer.nested_enum + other_nested_leaf: _Type_outer.nested_leaf + map_nested_enum: _containers.ScalarMap[str, _Type_outer.nested_enum] + map_nested_leaf: _containers.MessageMap[str, _Type_outer.nested_leaf] + def __init__(self, outer: _Optional[int] = ..., other_nested_enum: _Optional[_Union[_Type_outer.nested_enum, str]] = ..., other_nested_leaf: _Optional[_Union[_Type_outer.nested_leaf, _Mapping]] = ..., map_nested_enum: _Optional[_Mapping[str, _Type_outer.nested_enum]] = ..., map_nested_leaf: _Optional[_Mapping[str, _Type_outer.nested_leaf]] = ...) -> None: ... +)pyi")); + + // 4. Verify message where shadowing only occurs in map values. + EXPECT_TRUE(absl::StrContains(output, R"pyi( +class map_only_container(_message.Message): + __slots__ = ("map_val_enum", "map_val_msg", "enum_map", "msg_map") + class EnumMapEntry(_message.Message): + __slots__ = ("key", "value") + KEY_FIELD_NUMBER: _ClassVar[int] + VALUE_FIELD_NUMBER: _ClassVar[int] + key: str + value: _Type_map_val_enum + def __init__(self, key: _Optional[str] = ..., value: _Optional[_Union[_Type_map_val_enum, str]] = ...) -> None: ... + class MsgMapEntry(_message.Message): + __slots__ = ("key", "value") + KEY_FIELD_NUMBER: _ClassVar[int] + VALUE_FIELD_NUMBER: _ClassVar[int] + key: str + value: _Type_map_val_msg + def __init__(self, key: _Optional[str] = ..., value: _Optional[_Union[_Type_map_val_msg, _Mapping]] = ...) -> None: ... + MAP_VAL_ENUM_FIELD_NUMBER: _ClassVar[int] + MAP_VAL_MSG_FIELD_NUMBER: _ClassVar[int] + ENUM_MAP_FIELD_NUMBER: _ClassVar[int] + MSG_MAP_FIELD_NUMBER: _ClassVar[int] + map_val_enum: int + map_val_msg: int + enum_map: _containers.ScalarMap[str, _Type_map_val_enum] + msg_map: _containers.MessageMap[str, _Type_map_val_msg] + def __init__(self, map_val_enum: _Optional[int] = ..., map_val_msg: _Optional[int] = ..., enum_map: _Optional[_Mapping[str, _Type_map_val_enum]] = ..., msg_map: _Optional[_Mapping[str, _Type_map_val_msg]] = ...) -> None: ... +)pyi")); + + // 5. Verify on-demand aliases are emitted only for shadowed types with a + // preceding blank line. + EXPECT_TRUE(absl::StrContains(output, "\n_Type_kind = kind\n")); + EXPECT_TRUE(absl::StrContains(output, "\n_Type_item = item\n")); + EXPECT_TRUE(absl::StrContains(output, "\n_Type_outer = outer\n")); + EXPECT_TRUE( + absl::StrContains(output, "\n_Type_shadowed_enum = shadowed_enum\n")); + EXPECT_TRUE( + absl::StrContains(output, "\n_Type_shadowed_msg = shadowed_msg\n")); + EXPECT_TRUE( + absl::StrContains(output, "\n_Type_map_val_enum = map_val_enum\n")); + EXPECT_TRUE(absl::StrContains(output, "\n_Type_map_val_msg = map_val_msg\n")); + EXPECT_FALSE(absl::StrContains(output, "_Type_container = container\n")); + EXPECT_FALSE( + absl::StrContains(output, "_Type_shadow_scope = shadow_scope\n")); + EXPECT_FALSE(absl::StrContains( + output, "_Type_map_only_container = map_only_container\n")); + EXPECT_FALSE(absl::StrContains(output, "TypeAlias")); +} + class PythonGeneratorTest : public CommandLineInterfaceTester, public testing::WithParamInterface<bool> { protected:
diff --git a/src/google/protobuf/compiler/python/pyi_generator.cc b/src/google/protobuf/compiler/python/pyi_generator.cc index 541bec0..5272132 100644 --- a/src/google/protobuf/compiler/python/pyi_generator.cc +++ b/src/google/protobuf/compiler/python/pyi_generator.cc
@@ -89,10 +89,62 @@ // LINT.ThenChange(//depot/google3/net/proto2/python/internal/well_known_types.py:wktbases) } +// Checks if the first component of a type name could be shadowed by a class +// attribute (e.g. a field) or enclosing class in the Python scope hierarchy. +bool IsTypeShadowed(absl::string_view name, const Descriptor& containing_des) { + auto pos = name.find('.'); + absl::string_view first_token = + (pos == absl::string_view::npos) ? name : name.substr(0, pos); + for (const Descriptor* cur = &containing_des; cur != nullptr; + cur = cur->containing_type()) { + if (cur->FindFieldByName(first_token) != nullptr || + cur->FindEnumTypeByName(first_token) != nullptr || + cur->FindNestedTypeByName(first_token) != nullptr || + (cur->containing_type() != nullptr && cur->name() == first_token)) { + return true; + } + } + return false; +} + +void CheckFieldShadowing(const FieldDescriptor* field, + const Descriptor* containing_des, + absl::flat_hash_set<std::string>* shadowed_types) { + if (field->is_map()) { + const Descriptor* map_entry = field->message_type(); + CheckFieldShadowing(map_entry->field(0), containing_des, shadowed_types); + CheckFieldShadowing(map_entry->field(1), containing_des, shadowed_types); + return; + } + if (field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE && + field->cpp_type() != FieldDescriptor::CPPTYPE_ENUM) { + return; + } + const FileDescriptor* type_file = + field->cpp_type() == FieldDescriptor::CPPTYPE_ENUM + ? field->enum_type()->file() + : field->message_type()->file(); + if (type_file != containing_des->file()) { + return; + } + std::string name = + field->cpp_type() == FieldDescriptor::CPPTYPE_ENUM + ? NamePrefixedWithNestedTypes(*field->enum_type(), ".") + : NamePrefixedWithNestedTypes(*field->message_type(), "."); + if (!IsTypeShadowed(name, *containing_des)) { + return; + } + auto pos = name.find('.'); + std::string first_token = + (pos == std::string::npos) ? name : std::string(name.substr(0, pos)); + shadowed_types->insert(first_token); +} + // Checks what modules should be imported for this message // descriptor. void CheckImportModules(const Descriptor* descriptor, - ImportModules* import_modules) { + ImportModules* import_modules, + absl::flat_hash_set<std::string>* shadowed_types) { if (descriptor->extension_range_count() > 0) { import_modules->has_extendable = true; } @@ -107,6 +159,7 @@ if (IsPythonKeyword(field->name())) { continue; } + CheckFieldShadowing(field, descriptor, shadowed_types); import_modules->has_optional = true; if (field->is_repeated()) { import_modules->has_repeated = true; @@ -144,7 +197,8 @@ } } for (int i = 0; i < descriptor->nested_type_count(); ++i) { - CheckImportModules(descriptor->nested_type(i), import_modules); + CheckImportModules(descriptor->nested_type(i), import_modules, + shadowed_types); } } @@ -198,7 +252,8 @@ import_modules.has_union = true; } for (int i = 0; i < file_->message_type_count(); i++) { - CheckImportModules(file_->message_type(i), &import_modules); + CheckImportModules(file_->message_type(i), &import_modules, + &shadowed_top_level_types_); } if (import_modules.has_datetime) { printer_->Print("import datetime\n\n"); @@ -357,6 +412,10 @@ for (int i = 0; i < file_->enum_type_count(); ++i) { printer_->Print("\n"); PrintEnum(*file_->enum_type(i)); + if (shadowed_top_level_types_.contains(file_->enum_type(i)->name())) { + printer_->Print("\n_Type_$name$ = $name$\n", "name", + file_->enum_type(i)->name()); + } } } @@ -390,23 +449,26 @@ return "float"; case FieldDescriptor::CPPTYPE_BOOL: return "bool"; - case FieldDescriptor::CPPTYPE_ENUM: - return ModuleLevelName(*field_des.enum_type()); case FieldDescriptor::CPPTYPE_STRING: if (field_des.type() == FieldDescriptor::TYPE_STRING) { return "str"; } else { return "bytes"; } + case FieldDescriptor::CPPTYPE_ENUM: case FieldDescriptor::CPPTYPE_MESSAGE: { - // If the field is inside a nested message and the nested message has the - // same name as a top-level message, then we need to prefix the field type - // with the module name for disambiguation. - std::string name = ModuleLevelName(*field_des.message_type()); - if ((containing_des.containing_type() != nullptr && - name == containing_des.name())) { - std::string module = ModuleName(field_des.file()); - name = absl::StrCat(module, ".", name); + const FileDescriptor* type_file = + field_des.cpp_type() == FieldDescriptor::CPPTYPE_ENUM + ? field_des.enum_type()->file() + : field_des.message_type()->file(); + std::string name = field_des.cpp_type() == FieldDescriptor::CPPTYPE_ENUM + ? ModuleLevelName(*field_des.enum_type()) + : ModuleLevelName(*field_des.message_type()); + // If the type name is shadowed by a field, nested type, or enclosing + // class in the Python scope hierarchy, use the private module-level alias + // for disambiguation. + if (type_file == file_ && IsTypeShadowed(name, containing_des)) { + name = absl::StrCat("_Type_", name); } return name; } @@ -564,7 +626,7 @@ } else { if (field_des->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) { printer_->Print("_Union[$type_name$, str]", "type_name", - ModuleLevelName(*field_des->enum_type())); + GetFieldType(*field_des, message_descriptor)); } else { printer_->Print( "$type_name$", "type_name", @@ -588,6 +650,10 @@ // Deterministically order the descriptors. for (int i = 0; i < file_->message_type_count(); ++i) { PrintMessage(*file_->message_type(i), false); + if (shadowed_top_level_types_.contains(file_->message_type(i)->name())) { + printer_->Print("\n_Type_$name$ = $name$\n", "name", + file_->message_type(i)->name()); + } } } @@ -609,6 +675,7 @@ std::string* error) const { absl::MutexLock lock(&mutex_); import_map_.clear(); + shadowed_top_level_types_.clear(); // Calculate file name. file_ = file; // In google3, devtools/python/bazel/pytype/pytype_impl.bzl uses --pyi_out to
diff --git a/src/google/protobuf/compiler/python/pyi_generator.h b/src/google/protobuf/compiler/python/pyi_generator.h index 1beb4d1..377f245 100644 --- a/src/google/protobuf/compiler/python/pyi_generator.h +++ b/src/google/protobuf/compiler/python/pyi_generator.h
@@ -96,6 +96,11 @@ // import_map will be a mapping from filename to module alias, e.g. // "google3/foo/bar.py" -> "_bar" mutable absl::flat_hash_map<std::string, std::string> import_map_; + // Set of top-level message and enum names defined in this file that are + // shadowed by fields, nested types, or enclosing scopes, and thus need + // private type aliases (e.g. `_Type_<name> = <name>`) emitted at module + // scope. + mutable absl::flat_hash_set<std::string> shadowed_top_level_types_; }; } // namespace python