diff --git a/src/google/protobuf/compiler/rust/extension.cc b/src/google/protobuf/compiler/rust/extension.cc index 8003a07de794c..9941f572e02f7 100644 --- a/src/google/protobuf/compiler/rust/extension.cc +++ b/src/google/protobuf/compiler/rust/extension.cc @@ -9,7 +9,6 @@ #include -#include "absl/strings/ascii.h" #include "absl/strings/str_cat.h" #include "google/protobuf/compiler/cpp/names.h" #include "google/protobuf/compiler/rust/accessors/default_value.h" @@ -55,7 +54,7 @@ void GenerateRs(Context& ctx, const FieldDescriptor& extension, // The extension symbol defined by both backends is the same. ctx.Emit({{"extendee", RsTypePath(ctx, *extension.containing_type())}, - {"extension", absl::AsciiStrToUpper(extension.name())}, + {"extension", ExtensionRsName(extension)}, {"type", extension.is_repeated() ? absl::StrCat("::protobuf::Repeated<", RsTypePath(ctx, extension), ">") diff --git a/src/google/protobuf/compiler/rust/generator.cc b/src/google/protobuf/compiler/rust/generator.cc index 7e7cdb53ff13c..6a6cad55577eb 100644 --- a/src/google/protobuf/compiler/rust/generator.cc +++ b/src/google/protobuf/compiler/rust/generator.cc @@ -112,6 +112,59 @@ void EmitPublicImports(const RustGeneratorContext& rust_generator_context, } } +// Checks whether any two files in the crate export symbols that would collide +// at the crate root when re-exported via `pub use ::*;`. +// +// Return true if there is at least one collision in this crate. +bool CrateHasSymbolCollision(const std::vector& files) { + absl::flat_hash_set type_names; + absl::flat_hash_set value_names; + + // Returns true if `symbol` was already contributed by an earlier symbol. + auto collides = [](absl::flat_hash_set& names, + const std::string& symbol) { + return !names.insert(symbol).second; + }; + + for (const FileDescriptor* file : files) { + for (int i = 0; i < file->message_type_count(); ++i) { + const Descriptor* msg = file->message_type(i); + std::string name = MessageRsName(*msg); + // A top-level message is emitted as 'Msg, MsgView, MsgMut' + if (collides(type_names, name) || + collides(type_names, absl::StrCat(name, "View")) || + collides(type_names, absl::StrCat(name, "Mut"))) { + return true; + } + + // A submodule is emitted if the message has nested messages, enums, + // extensions, or oneofs. + if (msg->nested_type_count() > 0 || msg->enum_type_count() > 0 || + msg->extension_count() > 0 || msg->real_oneof_decl_count() > 0) { + if (collides(type_names, RsSafeName(CamelToSnakeCase(msg->name())))) { + return true; + } + } + } + + // Enums + for (int i = 0; i < file->enum_type_count(); ++i) { + if (collides(type_names, EnumRsName(*file->enum_type(i)))) { + return true; + } + } + + // Extensions are emitted as `pub const `. + for (int i = 0; i < file->extension_count(); ++i) { + if (collides(value_names, ExtensionRsName(*file->extension(i)))) { + return true; + } + } + } + + return false; +} + void EmitEntryPointRsFile(GeneratorContext* generator_context, Context& ctx_without_printer, const std::vector& files) { @@ -125,31 +178,50 @@ void EmitEntryPointRsFile(GeneratorContext* generator_context, io::Printer printer(outfile.get()); Context ctx = ctx_without_printer.WithPrinter(&printer); - // Declare the submodules for all of the the generated code and pub re-export - // all of them into a flat namespace. + // Declare the submodules for all of the generated code and, where safe, + // pub re-export all of them into a flat namespace. RelativePath primary_relpath(entry_point_rs_file_path); + const bool has_collision = CrateHasSymbolCollision(files); + for (const FileDescriptor* file : files) { std::string non_primary_file_path = GetRsFile(ctx, *file); std::string relative_mod_path = primary_relpath.Relative(RelativePath(non_primary_file_path)); + std::string mod_name = RustModuleName(*file); + // Expose each generated .proto file as a public module named after its // (flattened) file path, providing a fully-qualified path to every type // (e.g. `my_crate::google_network_api_proto::Config`). See // RustModuleName for how the module name is derived from the path. // - // The flat `pub use` re-export into the crate root is also emitted so that - // existing consumers relying on the crate-root namespace continue to work. - ctx.Emit( - {{"file_path", relative_mod_path}, {"mod_name", RustModuleName(*file)}}, - R"rs( + // The flat `pub use` re-export into the crate root is conditionally emitted + ctx.Emit({{"file_path", relative_mod_path}, {"mod_name", mod_name}}, + R"rs( #[path="$file_path$"] #[allow(nonstandard_style, unused)] pub mod $mod_name$; - - #[allow(nonstandard_style, unused)] - #[doc(inline)] - pub use $mod_name$::*; )rs"); + + if (!has_collision) { + ctx.Emit({{"mod_name", mod_name}}, + R"rs( + #[allow(nonstandard_style, unused)] + #[doc(inline)] + pub use $mod_name$::*; + )rs"); + } + } + + // When the crate has cross-file symbol collisions, the flat `pub use` + // re-exports above are omitted for every file. Emit a single breadcrumb (not + // one per file) explaining why, so consumers know to use fully-qualified + // paths. + if (has_collision) { + ctx.Emit(R"rs( + // Crate-root re-exports (`pub use ::*`) are disabled because + // this crate contains symbol name collisions across file modules. + // Use fully-qualified paths, e.g. `::::YourType`. + )rs"); } auto v = ctx.printer().WithVars({ diff --git a/src/google/protobuf/compiler/rust/generator_test.cc b/src/google/protobuf/compiler/rust/generator_test.cc index 2ab34ef1fa3f1..132dc2365514b 100644 --- a/src/google/protobuf/compiler/rust/generator_test.cc +++ b/src/google/protobuf/compiler/rust/generator_test.cc @@ -434,6 +434,211 @@ TEST_F(RustGeneratorTest, EmitsQualifiedExtendeePathForExtensions) { HasSubstr("ExtensionId")); } +TEST_F(RustGeneratorTest, DropsReexportForEntireCrate) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + message Foo { optional int32 x = 1; })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + message Foo { optional int32 y = 1; })schema"); + CreateTempFile("c.proto", R"schema( + syntax = "proto2"; + package pkg_c; + message Unique { optional int32 z = 1; })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto c.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, HasSubstr("pub mod a_proto;")); + EXPECT_THAT(entry_point, HasSubstr("pub mod b_proto;")); + EXPECT_THAT(entry_point, HasSubstr("pub mod c_proto;")); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use a_proto::*;"))); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use b_proto::*;"))); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use c_proto::*;"))); +} + +TEST_F(RustGeneratorTest, DropsReexportOnEnumCollision) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + enum Status { UNKNOWN = 0; OK = 1; })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + enum Status { PENDING = 0; DONE = 1; })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, HasSubstr("pub mod a_proto;")); + EXPECT_THAT(entry_point, HasSubstr("pub mod b_proto;")); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use a_proto::*;"))); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use b_proto::*;"))); +} + +TEST_F(RustGeneratorTest, KeepsReexportForNonCollidingFiles) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + message Alpha { + optional int32 x = 1; + })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + message Beta { + optional int32 y = 1; + })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, HasSubstr("pub use a_proto::*;")); + EXPECT_THAT(entry_point, HasSubstr("pub use b_proto::*;")); +} + +TEST_F(RustGeneratorTest, DropsReexportOnViewCollision) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + message BarView { optional int32 x = 1; })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + message Bar { optional int32 y = 1; })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use a_proto::*;"))); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use b_proto::*;"))); +} + +TEST_F(RustGeneratorTest, DropsReexportOnMutCollision) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + message QuxMut { optional int32 x = 1; })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + message Qux { optional int32 y = 1; })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use a_proto::*;"))); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use b_proto::*;"))); +} + +TEST_F(RustGeneratorTest, DropsReexportOnSubmoduleCollision) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + message data { optional int32 x = 1; })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + message Data { + message Row { optional int32 v = 1; } + optional Row row = 1; + })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use a_proto::*;"))); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use b_proto::*;"))); +} + +TEST_F(RustGeneratorTest, DropsReexportOnExtensionCollision) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + message TargetA { extensions 100 to 200; } + extend TargetA { optional int32 shared = 100; })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + message TargetB { extensions 100 to 200; } + extend TargetB { optional int32 shared = 100; })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use a_proto::*;"))); + EXPECT_THAT(entry_point, Not(HasSubstr("pub use b_proto::*;"))); +} + +TEST_F(RustGeneratorTest, KeepsReexportForCrateWithFeaturesButNoCollision) { + CreateTempFile("a.proto", R"schema( + syntax = "proto2"; + package pkg_a; + message AlphaMsg { + message AlphaInner { optional int32 v = 1; } + extensions 100 to 200; + oneof alpha_choice { + int32 a = 1; + int32 b = 2; + } + } + enum AlphaEnum { AE0 = 0; AE1 = 1; } + extend AlphaMsg { optional int32 alpha_ext = 100; })schema"); + CreateTempFile("b.proto", R"schema( + syntax = "proto2"; + package pkg_b; + message BetaMsg { + message BetaInner { optional int32 v = 1; } + extensions 100 to 200; + oneof beta_choice { + int32 a = 1; + int32 b = 2; + } + } + enum BetaEnum { BE0 = 0; BE1 = 1; } + extend BetaMsg { optional int32 beta_ext = 100; })schema"); + RunProtoc( + "protocol_compiler --proto_path=$tmpdir " + "--rust_out=$tmpdir " + "--rust_opt=experimental-codegen=enabled,kernel=cpp " + "a.proto b.proto"); + ExpectNoErrors(); + + std::string entry_point = FileContents("generated.rs"); + EXPECT_THAT(entry_point, HasSubstr("pub use a_proto::*;")); + EXPECT_THAT(entry_point, HasSubstr("pub use b_proto::*;")); +} + } // namespace } // namespace rust } // namespace compiler diff --git a/src/google/protobuf/compiler/rust/naming.cc b/src/google/protobuf/compiler/rust/naming.cc index 45cb4f7e35257..1e6663b58c0a4 100644 --- a/src/google/protobuf/compiler/rust/naming.cc +++ b/src/google/protobuf/compiler/rust/naming.cc @@ -391,6 +391,10 @@ std::string EnumRsName(const EnumDescriptor& desc) { return name; } +std::string ExtensionRsName(const FieldDescriptor& extension) { + return absl::AsciiStrToUpper(extension.name()); +} + std::string EnumValueRsName(const EnumValueDescriptor& value) { MultiCasePrefixStripper stripper(value.type()->name()); return EnumValueRsName(stripper, value.name()); diff --git a/src/google/protobuf/compiler/rust/naming.h b/src/google/protobuf/compiler/rust/naming.h index 2af094902805a..d789b5f40db0e 100644 --- a/src/google/protobuf/compiler/rust/naming.h +++ b/src/google/protobuf/compiler/rust/naming.h @@ -64,6 +64,12 @@ std::string MessageRsName(const Descriptor& desc); std::string EnumRsName(const EnumDescriptor& desc); std::string EnumValueRsName(const EnumValueDescriptor& value); +// Returns the Rust identifier for a message extension, emitted as a +// `pub const : ExtensionId<...>`. Centralizing this here keeps the name +// used by the code generator in sync with any future mangling (e.g. for +// extension names that are not valid Rust identifiers). +std::string ExtensionRsName(const FieldDescriptor& extension); + std::string OneofViewEnumRsName(const OneofDescriptor& oneof); std::string OneofCaseEnumRsName(const OneofDescriptor& oneof);