Make sure Objective C Proto compiler doesn't "duplicate" prefixes unnecessarily.

In some cases proto files that want/need to use the objc_class_prefix option have
types that already have the prefix on a subset of their names. In this case we don't
want to duplicate the prefix.

Added tests for this (and prefixes in general).

Makefile.am

@@ -572,6 +572,7 @@ objectivec_EXTRA_DIST=                                                       \
   objectivec/Tests/unittest_extension_chain_f.proto                           \
   objectivec/Tests/unittest_extension_chain_g.proto                           \
   objectivec/Tests/unittest_objc.proto                                        \
+  objectivec/Tests/unittest_objc_options.proto                                \
   objectivec/Tests/unittest_objc_startup.proto                                \
   objectivec/Tests/unittest_runtime_proto2.proto                              \
   objectivec/Tests/unittest_runtime_proto3.proto                              \

objectivec/DevTools/compile_testing_protos.sh

@@ -146,4 +146,5 @@ compile_protos \
   objectivec/Tests/unittest_runtime_proto2.proto \
   objectivec/Tests/unittest_runtime_proto3.proto \
   objectivec/Tests/unittest_objc.proto \
-  objectivec/Tests/unittest_objc_startup.proto
+  objectivec/Tests/unittest_objc_startup.proto \
+  objectivec/Tests/unittest_objc_options.proto

objectivec/ProtocolBuffers_OSX.xcodeproj/project.pbxproj

@@ -150,6 +150,7 @@
 		8B09AAF614B663A7007B4184 /* unittest_objc.proto */ = {isa = PBXFileReference; lastKnownFileType = text; path = unittest_objc.proto; sourceTree = "<group>"; };
 		8B210CCD159383D60032D72D /* golden_message */ = {isa = PBXFileReference; lastKnownFileType = file; path = golden_message; sourceTree = "<group>"; };
 		8B210CCF159386920032D72D /* golden_packed_fields_message */ = {isa = PBXFileReference; lastKnownFileType = file; path = golden_packed_fields_message; sourceTree = "<group>"; };
+		8B35468421A616F6000BD30D /* unittest_objc_options.proto */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.protobuf; path = unittest_objc_options.proto; sourceTree = "<group>"; };
 		8B4248B91A8C256900BC1EC6 /* UnitTests-Bridging-Header.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "UnitTests-Bridging-Header.h"; sourceTree = "<group>"; };
 		8B4248BA1A8C256A00BC1EC6 /* GPBSwiftTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = GPBSwiftTests.swift; sourceTree = "<group>"; };
 		8B4248CF1A927E1500BC1EC6 /* GPBWellKnownTypes.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = GPBWellKnownTypes.h; sourceTree = "<group>"; };
@@ -508,6 +509,7 @@
 				8BBD9DB016DD1DC8008E1EC1 /* unittest_lite.proto */,
 				8B7E6A7B14893DBC00F8884A /* unittest_mset.proto */,
 				8B7E6A7C14893DBC00F8884A /* unittest_no_generic_services.proto */,
+				8B35468421A616F6000BD30D /* unittest_objc_options.proto */,
 				F4CF31701B162ED800BD9B06 /* unittest_objc_startup.proto */,
 				8B09AAF614B663A7007B4184 /* unittest_objc.proto */,
 				8B7E6A7D14893DBC00F8884A /* unittest_optimize_for.proto */,

objectivec/ProtocolBuffers_iOS.xcodeproj/project.pbxproj

@@ -151,6 +151,7 @@
 		8B09AAF614B663A7007B4184 /* unittest_objc.proto */ = {isa = PBXFileReference; lastKnownFileType = text; path = unittest_objc.proto; sourceTree = "<group>"; };
 		8B210CCD159383D60032D72D /* golden_message */ = {isa = PBXFileReference; lastKnownFileType = file; path = golden_message; sourceTree = "<group>"; };
 		8B210CCF159386920032D72D /* golden_packed_fields_message */ = {isa = PBXFileReference; lastKnownFileType = file; path = golden_packed_fields_message; sourceTree = "<group>"; };
+		8B35468621A61EB2000BD30D /* unittest_objc_options.proto */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.protobuf; path = unittest_objc_options.proto; sourceTree = "<group>"; };
 		8B4248B21A8BD96D00BC1EC6 /* UnitTests-Bridging-Header.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "UnitTests-Bridging-Header.h"; sourceTree = "<group>"; };
 		8B4248B31A8BD96E00BC1EC6 /* GPBSwiftTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = GPBSwiftTests.swift; sourceTree = "<group>"; };
 		8B4248DD1A929C7D00BC1EC6 /* Duration.pbobjc.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = Duration.pbobjc.h; path = google/protobuf/Duration.pbobjc.h; sourceTree = "<group>"; };
@@ -514,6 +515,7 @@
 				8BBD9DB016DD1DC8008E1EC1 /* unittest_lite.proto */,
 				8B7E6A7B14893DBC00F8884A /* unittest_mset.proto */,
 				8B7E6A7C14893DBC00F8884A /* unittest_no_generic_services.proto */,
+				8B35468621A61EB2000BD30D /* unittest_objc_options.proto */,
 				F4CF31711B162EF500BD9B06 /* unittest_objc_startup.proto */,
 				8B09AAF614B663A7007B4184 /* unittest_objc.proto */,
 				8B7E6A7D14893DBC00F8884A /* unittest_optimize_for.proto */,

objectivec/Tests/GPBMessageTests.m

@@ -40,6 +40,7 @@
 #import "GPBUnknownFieldSet_PackagePrivate.h"
 #import "google/protobuf/Unittest.pbobjc.h"
 #import "google/protobuf/UnittestObjc.pbobjc.h"
+#import "google/protobuf/UnittestObjcOptions.pbobjc.h"
 
 @interface MessageTests : GPBTestCase
 @end
@@ -2097,7 +2098,7 @@
   XCTAssertEqual([msg1 hash], [msg1Prime hash]);
 }
 
-- (void)testCopyingMapFileds {
+- (void)testCopyingMapFields {
   TestMessageOfMaps *msg = [TestMessageOfMaps message];
 
   msg.strToStr[@"foo"] = @"bar";
@@ -2148,4 +2149,38 @@
   XCTAssertEqualObjects([msg.boolToMsg objectForKey:YES], [msg2.boolToMsg objectForKey:YES]);
 }
 
+- (void)testPrefixedNames {
+  // The fact that this compiles is sufficient as a test.
+  // The assertions are just there to avoid "not-used" warnings.
+
+  // Verify that enum types and values get the prefix.
+  GPBTESTTestObjcProtoPrefixEnum value = GPBTESTTestObjcProtoPrefixEnum_Value;
+  XCTAssertNotEqual(value, 0);
+
+  // Verify that roots get the prefix.
+  GPBTESTUnittestObjcOptionsRoot *root = nil;
+  XCTAssertNil(root);
+
+  // Verify that messages that don't already have the prefix get a prefix.
+  GPBTESTTestObjcProtoPrefixMessage *prefixedMessage = nil;
+  XCTAssertNil(prefixedMessage);
+
+  // Verify that messages that already have a prefix aren't prefixed twice.
+  GPBTESTTestHasAPrefixMessage *alreadyPrefixedMessage = nil;
+  XCTAssertNil(alreadyPrefixedMessage);
+
+  // Verify that enums that already have a prefix aren't prefixed twice.
+  GPBTESTTestHasAPrefixEnum prefixedValue = GPBTESTTestHasAPrefixEnum_ValueB;
+  XCTAssertNotEqual(prefixedValue, 0);
+
+  // Verify that classes named the same as prefixes are prefixed.
+  GPBTESTGPBTEST *prefixMessage = nil;
+  XCTAssertNil(prefixMessage);
+
+  // Verify that classes that have the prefix followed by a lowercase
+  // letter DO get the prefix.
+  GPBTESTGPBTESTshouldGetAPrefixMessage *shouldGetAPrefixMessage = nil;
+  XCTAssertNil(shouldGetAPrefixMessage);
+}
+
 @end

objectivec/Tests/unittest_objc_options.proto

+// Protocol Buffers - Google's data interchange format
+// Copyright 2008 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+syntax = "proto2";
+
+package protobuf_objc_unittest;
+option objc_class_prefix = "GPBTEST";
+
+// Verify that enum types and values get the prefix.
+message TestObjcProtoPrefixMessage {
+}
+
+// Verify that messages that don't already have the prefix get a prefix.
+enum TestObjcProtoPrefixEnum {
+  value = 1;
+}
+
+// Verify that messages that already have a prefix aren't prefixed twice.
+message GPBTESTTestHasAPrefixMessage {
+}
+
+// Verify that enums that already have a prefix aren't prefixed twice.
+enum GPBTESTTestHasAPrefixEnum {
+  valueB = 1;
+}
+
+// Verify that classes that have the prefix followed by a lowercase
+// letter DO get the prefix.
+message GPBTESTshouldGetAPrefixMessage {
+}
+
+// Verify that classes named the same as prefixes are prefixed.
+message GPBTEST {
+}

src/google/protobuf/compiler/objectivec/objectivec_helpers.cc

@@ -243,21 +243,37 @@ bool IsReservedCIdentifier(const string& input) {
   return false;
 }
 
-string SanitizeNameForObjC(const string& input,
+string SanitizeNameForObjC(const string& prefix,
+                           const string& input,
                            const string& extension,
                            string* out_suffix_added) {
   static const std::unordered_set<string> kReservedWords =
       MakeWordsMap(kReservedWordList, GOOGLE_ARRAYSIZE(kReservedWordList));
   static const std::unordered_set<string> kNSObjectMethods =
       MakeWordsMap(kNSObjectMethodsList, GOOGLE_ARRAYSIZE(kNSObjectMethodsList));
-  if (IsReservedCIdentifier(input) ||
-      (kReservedWords.count(input) > 0) ||
-      (kNSObjectMethods.count(input) > 0)) {
+  string sanitized;
+  // We add the prefix in the cases where the string is missing a prefix.
+  // We define "missing a prefix" as where 'input':
+  // a) Doesn't start with the prefix or
+  // b) Isn't equivalent to the prefix or
+  // c) Has the prefix, but the letter after the prefix is lowercase
+  if (HasPrefixString(input, prefix)) {
+    if (input.length() == prefix.length() || !ascii_isupper(input[prefix.length()])) {
+      sanitized = prefix + input;
+    } else {
+      sanitized = input;
+    }
+  } else {
+    sanitized = prefix + input;
+  }
+  if (IsReservedCIdentifier(sanitized) ||
+      (kReservedWords.count(sanitized) > 0) ||
+      (kNSObjectMethods.count(sanitized) > 0)) {
     if (out_suffix_added) *out_suffix_added = extension;
-    return input + extension;
+    return sanitized + extension;
   }
   if (out_suffix_added) out_suffix_added->clear();
-  return input;
+  return sanitized;
 }
 
 string NameFromFieldDescriptor(const FieldDescriptor* field) {
@@ -416,12 +432,11 @@ string FilePathBasename(const FileDescriptor* file) {
 }
 
 string FileClassName(const FileDescriptor* file) {
-  string name = FileClassPrefix(file);
-  name += UnderscoresToCamelCase(StripProto(BaseFileName(file)), true);
-  name += "Root";
+  const string prefix = FileClassPrefix(file);
+  const string name = UnderscoresToCamelCase(StripProto(BaseFileName(file)), true) + "Root";
   // There aren't really any reserved words that end in "Root", but playing
   // it safe and checking.
-  return SanitizeNameForObjC(name, "_RootClass", NULL);
+  return SanitizeNameForObjC(prefix, name, "_RootClass", NULL);
 }
 
 string ClassNameWorker(const Descriptor* descriptor) {
@@ -449,9 +464,9 @@ string ClassName(const Descriptor* descriptor) {
 string ClassName(const Descriptor* descriptor, string* out_suffix_added) {
   // 1. Message names are used as is (style calls for CamelCase, trust it).
   // 2. Check for reserved word at the very end and then suffix things.
-  string prefix = FileClassPrefix(descriptor->file());
-  string name = ClassNameWorker(descriptor);
-  return SanitizeNameForObjC(prefix + name, "_Class", out_suffix_added);
+  const string prefix = FileClassPrefix(descriptor->file());
+  const string name = ClassNameWorker(descriptor);
+  return SanitizeNameForObjC(prefix, name, "_Class", out_suffix_added);
 }
 
 string EnumName(const EnumDescriptor* descriptor) {
@@ -463,9 +478,9 @@ string EnumName(const EnumDescriptor* descriptor) {
   //      ...
   //      }
   //    yields Fixed_Class, Fixed_Size.
-  string name = FileClassPrefix(descriptor->file());
-  name += ClassNameWorker(descriptor);
-  return SanitizeNameForObjC(name, "_Enum", NULL);
+  const string prefix = FileClassPrefix(descriptor->file());
+  const string name = ClassNameWorker(descriptor);
+  return SanitizeNameForObjC(prefix, name, "_Enum", NULL);
 }
 
 string EnumValueName(const EnumValueDescriptor* descriptor) {
@@ -475,12 +490,12 @@ string EnumValueName(const EnumValueDescriptor* descriptor) {
   //     FOO = 1
   //   }
   // yields Fixed_Enum and Fixed_Enum_Foo (not Fixed_Foo).
-  const string& class_name = EnumName(descriptor->type());
-  const string& value_str = UnderscoresToCamelCase(descriptor->name(), true);
-  const string& name = class_name + "_" + value_str;
+  const string class_name = EnumName(descriptor->type());
+  const string value_str = UnderscoresToCamelCase(descriptor->name(), true);
+  const string name = class_name + "_" + value_str;
   // There aren't really any reserved words with an underscore and a leading
   // capital letter, but playing it safe and checking.
-  return SanitizeNameForObjC(name, "_Value", NULL);
+  return SanitizeNameForObjC("", name, "_Value", NULL);
 }
 
 string EnumValueShortName(const EnumValueDescriptor* descriptor) {
@@ -496,9 +511,9 @@ string EnumValueShortName(const EnumValueDescriptor* descriptor) {
   // So the right way to get the short name is to take the full enum name
   // and then strip off the enum name (leaving the value name and anything
   // done by sanitize).
-  const string& class_name = EnumName(descriptor->type());
-  const string& long_name_prefix = class_name + "_";
-  const string& long_name = EnumValueName(descriptor);
+  const string class_name = EnumName(descriptor->type());
+  const string long_name_prefix = class_name + "_";
+  const string long_name = EnumValueName(descriptor);
   return StripPrefixString(long_name, long_name_prefix);
 }
 
@@ -515,13 +530,13 @@ string UnCamelCaseEnumShortName(const string& name) {
 }
 
 string ExtensionMethodName(const FieldDescriptor* descriptor) {
-  const string& name = NameFromFieldDescriptor(descriptor);
-  const string& result = UnderscoresToCamelCase(name, false);
-  return SanitizeNameForObjC(result, "_Extension", NULL);
+  const string name = NameFromFieldDescriptor(descriptor);
+  const string result = UnderscoresToCamelCase(name, false);
+  return SanitizeNameForObjC("", result, "_Extension", NULL);
 }
 
 string FieldName(const FieldDescriptor* field) {
-  const string& name = NameFromFieldDescriptor(field);
+  const string name = NameFromFieldDescriptor(field);
   string result = UnderscoresToCamelCase(name, false);
   if (field->is_repeated() && !field->is_map()) {
     // Add "Array" before do check for reserved worlds.
@@ -532,7 +547,7 @@ string FieldName(const FieldDescriptor* field) {
       result += "_p";
     }
   }
-  return SanitizeNameForObjC(result, "_p", NULL);
+  return SanitizeNameForObjC("", result, "_p", NULL);
 }
 
 string FieldNameCapitalized(const FieldDescriptor* field) {