From d529720e2f11ff76b9094fd537888b7bfe448a58 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Mon, 17 Dec 2018 17:20:56 -0500 Subject: [PATCH] If enum aliases overlap in ObjC names skip generating the extras. Some protos have enum values of "FOO" and "Foo", which the ObjC generation then makes into the same thing. Just skip generating the enum element for the duplicate as it would be a compile error because of the name collision. The descriptors are still generated to support reflection and TextFormat more completely. --- objectivec/Tests/GPBDescriptorTests.m | 37 +++++++++++++++++++ objectivec/Tests/unittest_objc.proto | 15 ++++++++ .../compiler/objectivec/objectivec_enum.cc | 23 ++++++++++++ .../compiler/objectivec/objectivec_enum.h | 1 + 4 files changed, 76 insertions(+) diff --git a/objectivec/Tests/GPBDescriptorTests.m b/objectivec/Tests/GPBDescriptorTests.m index d47cc30fce..46c1729150 100644 --- a/objectivec/Tests/GPBDescriptorTests.m +++ b/objectivec/Tests/GPBDescriptorTests.m @@ -244,7 +244,44 @@ XCTAssertTrue([descriptor getValue:&value forEnumName:enumName]); XCTAssertEqual(value, 2); XCTAssertEqualObjects([descriptor getEnumTextFormatNameForIndex:4], @"BAR2"); +} + +- (void)testEnumAliasNameCollisions { + GPBEnumDescriptor *descriptor = TestEnumObjCNameCollision_EnumDescriptor(); + NSString *textFormatName; + int32_t value; + + XCTAssertEqual(descriptor.enumNameCount, 5U); + + XCTAssertEqualObjects([descriptor getEnumNameForIndex:0], @"TestEnumObjCNameCollision_Foo"); + textFormatName = [descriptor getEnumTextFormatNameForIndex:0]; + XCTAssertEqualObjects(textFormatName, @"FOO"); + XCTAssertTrue([descriptor getValue:&value forEnumTextFormatName:textFormatName]); + XCTAssertEqual(value, 1); + XCTAssertEqualObjects([descriptor getEnumNameForIndex:1], @"TestEnumObjCNameCollision_Foo"); + textFormatName = [descriptor getEnumTextFormatNameForIndex:1]; + XCTAssertEqualObjects(textFormatName, @"foo"); + XCTAssertTrue([descriptor getValue:&value forEnumTextFormatName:textFormatName]); + XCTAssertEqual(value, 1); + + XCTAssertEqualObjects([descriptor getEnumNameForIndex:2], @"TestEnumObjCNameCollision_Bar"); + textFormatName = [descriptor getEnumTextFormatNameForIndex:2]; + XCTAssertEqualObjects(textFormatName, @"BAR"); + XCTAssertTrue([descriptor getValue:&value forEnumTextFormatName:textFormatName]); + XCTAssertEqual(value, 2); + + XCTAssertEqualObjects([descriptor getEnumNameForIndex:3], @"TestEnumObjCNameCollision_Mumble"); + textFormatName = [descriptor getEnumTextFormatNameForIndex:3]; + XCTAssertEqualObjects(textFormatName, @"mumble"); + XCTAssertTrue([descriptor getValue:&value forEnumTextFormatName:textFormatName]); + XCTAssertEqual(value, 2); + + XCTAssertEqualObjects([descriptor getEnumNameForIndex:4], @"TestEnumObjCNameCollision_Mumble"); + textFormatName = [descriptor getEnumTextFormatNameForIndex:4]; + XCTAssertEqualObjects(textFormatName, @"MUMBLE"); + XCTAssertTrue([descriptor getValue:&value forEnumTextFormatName:textFormatName]); + XCTAssertEqual(value, 2); } - (void)testEnumValueValidator { diff --git a/objectivec/Tests/unittest_objc.proto b/objectivec/Tests/unittest_objc.proto index 1245ebefa0..91c213921e 100644 --- a/objectivec/Tests/unittest_objc.proto +++ b/objectivec/Tests/unittest_objc.proto @@ -867,3 +867,18 @@ message BoolOnlyMessage { message WKTRefereceMessage { optional google.protobuf.Any an_any = 1; } + +// This is in part a compile test, it ensures that when aliases end up with +// the same ObjC name, we drop them to avoid the duplication names. There +// is a test to ensure the descriptors are still generated to support +// reflection and TextFormat. +enum TestEnumObjCNameCollision { + option allow_alias = true; + + FOO = 1; + foo = 1; + + BAR = 2; + mumble = 2; + MUMBLE = 2; +} diff --git a/src/google/protobuf/compiler/objectivec/objectivec_enum.cc b/src/google/protobuf/compiler/objectivec/objectivec_enum.cc index 978e985cf5..3b2ca55368 100644 --- a/src/google/protobuf/compiler/objectivec/objectivec_enum.cc +++ b/src/google/protobuf/compiler/objectivec/objectivec_enum.cc @@ -35,6 +35,7 @@ #include #include #include +#include // std::find() namespace google { namespace protobuf { @@ -44,6 +45,17 @@ namespace objectivec { EnumGenerator::EnumGenerator(const EnumDescriptor* descriptor) : descriptor_(descriptor), name_(EnumName(descriptor_)) { + // Track the names for the enum values, and if an alias overlaps a base + // value, skip making a name for it. Likewise if two alias overlap, the + // first one wins. + // The one gap in this logic is if two base values overlap, but for that + // to happen you have to have "Foo" and "FOO" or "FOO_BAR" and "FooBar", + // and if an enum has that, it is already going to be confusing and a + // compile error is just fine. + // The values are still tracked to support the reflection apis and + // TextFormat handing since they are different there. + std::set value_names; + for (int i = 0; i < descriptor_->value_count(); i++) { const EnumValueDescriptor* value = descriptor_->value(i); const EnumValueDescriptor* canonical_value = @@ -51,6 +63,14 @@ EnumGenerator::EnumGenerator(const EnumDescriptor* descriptor) if (value == canonical_value) { base_values_.push_back(value); + value_names.insert(EnumValueName(value)); + } else { + string value_name(EnumValueName(value)); + if (value_names.find(value_name) != value_names.end()) { + alias_values_to_skip_.insert(value); + } else { + value_names.insert(value_name); + } } all_values_.push_back(value); } @@ -90,6 +110,9 @@ void EnumGenerator::GenerateHeader(io::Printer* printer) { "name", name_); } for (int i = 0; i < all_values_.size(); i++) { + if (alias_values_to_skip_.find(all_values_[i]) != alias_values_to_skip_.end()) { + continue; + } SourceLocation location; if (all_values_[i]->GetSourceLocation(&location)) { string comments = BuildCommentsString(location, true).c_str(); diff --git a/src/google/protobuf/compiler/objectivec/objectivec_enum.h b/src/google/protobuf/compiler/objectivec/objectivec_enum.h index d9dfc8a1d9..50a6564479 100644 --- a/src/google/protobuf/compiler/objectivec/objectivec_enum.h +++ b/src/google/protobuf/compiler/objectivec/objectivec_enum.h @@ -59,6 +59,7 @@ class EnumGenerator { const EnumDescriptor* descriptor_; std::vector base_values_; std::vector all_values_; + std::set alias_values_to_skip_; const string name_; };