Skip to content

Commit

Permalink
Introduce weirder names for the oneof generated structs to minimize c…
Browse files Browse the repository at this point in the history
…ollisions.

Given:
oneof hello_world {}

We now generate:
fn hello_world(&self) -> HelloWorldOneof
fn hello_world_case(&self) -> HelloWorldCase

This change is to help mitigate a realistic risk of collisions in cases like:

message M {
  oneof x { ... }
  enum X { ... }
}

PiperOrigin-RevId: 714998247
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Jan 13, 2025
1 parent dca34c7 commit 5106654
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 31 deletions.
10 changes: 5 additions & 5 deletions rust/test/shared/accessors_proto3_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ fn test_foreign_enum_accessors() {

#[gtest]
fn test_oneof_accessors() {
use test_all_types::OneofField::*;
use test_all_types::OneofFieldOneof::*;

let mut msg = TestAllTypes::new();
assert_that!(msg.oneof_field(), matches_pattern!(not_set(_)));
Expand Down Expand Up @@ -202,7 +202,7 @@ fn test_oneof_accessors() {

#[gtest]
fn test_oneof_accessors_view_long_lifetime() {
use test_all_types::OneofField::*;
use test_all_types::OneofFieldOneof::*;

let mut msg = TestAllTypes::new();
msg.set_oneof_uint32(7);
Expand All @@ -219,18 +219,18 @@ fn test_oneof_accessors_view_long_lifetime() {
#[gtest]
fn test_oneof_enum_accessors() {
use unittest_proto3_rust_proto::{
test_oneof2::{Foo, FooCase, NestedEnum},
test_oneof2::{FooCase, FooOneof, NestedEnum},
TestOneof2,
};

let mut msg = TestOneof2::new();
assert_that!(msg.foo_enum_opt(), eq(Optional::Unset(NestedEnum::Unknown)));
assert_that!(msg.foo(), matches_pattern!(Foo::not_set(_)));
assert_that!(msg.foo(), matches_pattern!(FooOneof::not_set(_)));
assert_that!(msg.foo_case(), matches_pattern!(FooCase::not_set));

msg.set_foo_enum(NestedEnum::Bar);
assert_that!(msg.foo_enum_opt(), eq(Optional::Set(NestedEnum::Bar)));
assert_that!(msg.foo(), matches_pattern!(Foo::FooEnum(eq(NestedEnum::Bar))));
assert_that!(msg.foo(), matches_pattern!(FooOneof::FooEnum(eq(NestedEnum::Bar))));
assert_that!(msg.foo_case(), matches_pattern!(FooCase::FooEnum));
}

Expand Down
4 changes: 2 additions & 2 deletions rust/test/shared/accessors_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ fn test_default_import_enum_accessors() {

#[gtest]
fn test_oneof_accessors() {
use unittest_rust_proto::test_oneof2::{Foo::*, FooCase, NestedEnum};
use unittest_rust_proto::test_oneof2::{FooCase, FooOneof::*, NestedEnum};
use unittest_rust_proto::TestOneof2;

let mut msg = TestOneof2::new();
Expand Down Expand Up @@ -818,7 +818,7 @@ fn test_oneof_accessors() {

#[gtest]
fn test_msg_oneof_default_accessors() {
use unittest_rust_proto::test_oneof2::{Bar::*, BarCase, NestedEnum};
use unittest_rust_proto::test_oneof2::{BarCase, BarOneof::*, NestedEnum};

let mut msg = unittest_rust_proto::TestOneof2::new();
assert_that!(msg.bar(), matches_pattern!(not_set(_)));
Expand Down
14 changes: 11 additions & 3 deletions rust/test/shared/fields_with_imported_types_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,21 @@ fn test_enum_field_generated() {

#[gtest]
fn test_oneof_message_field_generated() {
use fields_with_imported_types_rust_proto::msg_with_fields_with_imported_types::ImportedTypesOneof::not_set;
use fields_with_imported_types_rust_proto::MsgWithFieldsWithImportedTypes;
use fields_with_imported_types_rust_proto::{
msg_with_fields_with_imported_types, MsgWithFieldsWithImportedTypes,
};
use imported_types_rust_proto::ImportedEnum;
use imported_types_rust_proto::ImportedMessageView;

let msg = MsgWithFieldsWithImportedTypes::new();
assert_that!(msg.imported_message_oneof(), matches_pattern!(ImportedMessageView { .. }));
assert_that!(msg.imported_enum_oneof(), eq(ImportedEnum::Unknown));
assert_that!(msg.imported_types_oneof(), matches_pattern!(not_set(_)));
assert_that!(
msg.imported_types_oneof(),
matches_pattern!(msg_with_fields_with_imported_types::ImportedTypesOneofOneof::not_set(_))
);
assert_that!(
msg.imported_types_oneof_case(),
matches_pattern!(msg_with_fields_with_imported_types::ImportedTypesOneofCase::not_set)
);
}
12 changes: 9 additions & 3 deletions src/google/protobuf/compiler/rust/naming.cc
Original file line number Diff line number Diff line change
Expand Up @@ -333,13 +333,19 @@ std::string EnumValueRsName(const MultiCasePrefixStripper& stripper,
return RsSafeName(name);
}

std::string OneofViewEnumRsName(const OneofDescriptor& oneof) {
std::string DeprecatedOneofViewEnumRsName(const OneofDescriptor& oneof) {
return RsSafeName(SnakeToUpperCamelCase(oneof.name()));
}

std::string OneofViewEnumRsName(const OneofDescriptor& oneof) {
return SnakeToUpperCamelCase(oneof.name()) + "Oneof";
}

std::string OneofCaseEnumRsName(const OneofDescriptor& oneof) {
// Note: This is the name used for the cpp Case enum, we use it for both
// the Rust Case enum as well as for the cpp case enum in the cpp thunk.
return SnakeToUpperCamelCase(oneof.name()) + "Case";
}

std::string OneofCaseEnumCppName(const OneofDescriptor& oneof) {
return SnakeToUpperCamelCase(oneof.name()) + "Case";
}

Expand Down
6 changes: 6 additions & 0 deletions src/google/protobuf/compiler/rust/naming.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,14 @@ std::string RsViewType(Context& ctx, const FieldDescriptor& field,
std::string EnumRsName(const EnumDescriptor& desc);
std::string EnumValueRsName(const EnumValueDescriptor& value);

// The old names for these types, which we intend to imminently burn down
// to avoid collisions.
std::string DeprecatedOneofViewEnumRsName(const OneofDescriptor& oneof);

std::string OneofViewEnumRsName(const OneofDescriptor& oneof);
std::string OneofCaseEnumRsName(const OneofDescriptor& oneof);

std::string OneofCaseEnumCppName(const OneofDescriptor& oneof);
std::string OneofCaseRsName(const FieldDescriptor& oneof_field);

std::string FieldInfoComment(Context& ctx, const FieldDescriptor& field);
Expand Down
28 changes: 10 additions & 18 deletions src/google/protobuf/compiler/rust/oneof.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ namespace rust {
// Example:
// For this oneof:
// message SomeMsg {
// oneof some_oneof {
// oneof some {
// int32 field_a = 7;
// SomeMsg field_b = 9;
// }
Expand All @@ -52,33 +52,23 @@ namespace rust {
// }
//
// #[repr(C)]
// pub enum SomeOneofCase {
// pub enum SomeCase {
// FieldA = 7,
// FieldB = 9,
// not_set = 0
// }
// }
// impl SomeMsg {
// pub fn some_oneof(&self) -> SomeOneof {...}
// pub fn some_oneof_case(&self) -> SomeOneofCase {...}
// pub fn some_oneof_case(&self) -> SomeCase {...}
// }
// impl SomeMsgMut {
// pub fn some_oneof(&self) -> SomeOneof {...}
// pub fn some_oneof_case(&self) -> SomeOneofCase {...}
// pub fn some_oneof_case(&self) -> SomeCase {...}
// }
// impl SomeMsgView {
// pub fn some_oneof(self) -> SomeOneof {...}
// pub fn some_oneof_case(self) -> SomeOneofCase {...}
// }
//
// An additional "Case" enum which just reflects the corresponding slot numbers
// is emitted for usage with the FFI (exactly matching the Case struct that both
// cpp and upb generate).
//
// #[repr(C)] pub(super) enum SomeOneofCase {
// FieldA = 7,
// FieldB = 9,
// not_set = 0
// pub fn some_oneof_case(self) -> SomeCase {...}
// }

namespace {
Expand Down Expand Up @@ -134,6 +124,7 @@ std::string RsTypeNameView(Context& ctx, const FieldDescriptor& field) {
void GenerateOneofDefinition(Context& ctx, const OneofDescriptor& oneof) {
ctx.Emit(
{
{"deprecated_view_enum_name", DeprecatedOneofViewEnumRsName(oneof)},
{"view_enum_name", OneofViewEnumRsName(oneof)},
{"view_fields",
[&] {
Expand Down Expand Up @@ -164,9 +155,11 @@ void GenerateOneofDefinition(Context& ctx, const OneofDescriptor& oneof) {
pub enum $view_enum_name$<'msg> {
$view_fields$
#[allow(non_camel_case_types)]
not_set(std::marker::PhantomData<&'msg ()>) = 0
}
#[deprecated(note="Use $view_enum_name$ instead (will be imminently moved)")]
pub use $view_enum_name$ as $deprecated_view_enum_name$;
)rs");

// Note: This enum is used as the Thunk return type for getting which case is
Expand Down Expand Up @@ -205,7 +198,6 @@ void GenerateOneofDefinition(Context& ctx, const OneofDescriptor& oneof) {
pub enum $case_enum_name$ {
$cases$
#[allow(non_camel_case_types)]
not_set = 0
}
Expand Down Expand Up @@ -313,7 +305,7 @@ void GenerateOneofThunkCc(Context& ctx, const OneofDescriptor& oneof) {
ctx.Emit(
{
{"oneof_name", oneof.name()},
{"case_enum_name", OneofCaseEnumRsName(oneof)},
{"case_enum_name", OneofCaseEnumCppName(oneof)},
{"case_thunk", ThunkName(ctx, oneof, "case")},
{"QualifiedMsg", cpp::QualifiedClassName(oneof.containing_type())},
},
Expand Down

0 comments on commit 5106654

Please sign in to comment.