Fix bugs in tools/proto_text:

- Oneof values should be printed even when equal to the default.
- Fields should be printed in tag number order, not declaration order.
Change: 120223509

3 files changed, 49 insertions, 5 deletions

diff --git a/tensorflow/tools/proto_text/gen_proto_text_functions_lib.cc b/tensorflow/tools/proto_text/gen_proto_text_functions_lib.cc
index cce483c717..b196fd23f7 100644
--- a/tensorflow/tools/proto_text/gen_proto_text_functions_lib.cc
+++ b/tensorflow/tools/proto_text/gen_proto_text_functions_lib.cc
@@ -28,6 +28,7 @@ using ::tensorflow::protobuf::EnumDescriptor;
 using ::tensorflow::protobuf::FieldDescriptor;
 using ::tensorflow::protobuf::FieldOptions;
 using ::tensorflow::protobuf::FileDescriptor;
+using ::tensorflow::protobuf::OneofDescriptor;
 using ::tensorflow::strings::StrAppend;
 using ::tensorflow::strings::StrCat;
 
@@ -300,8 +301,22 @@ void Generator::AppendFieldAppend(const FieldDescriptor& field) {
                            "msg." + name + "(i)");
     Unnest().Print("}");
   } else {
-    AppendFieldValueAppend(field, true /* omit_default */,
-                           "msg." + name + "()");
+    const auto* oneof = field.containing_oneof();
+    if (oneof != nullptr) {
+      string camel_name = field.camelcase_name();
+      camel_name[0] = toupper(camel_name[0]);
+      Print("if (msg.", oneof->name(), "_case() == ",
+            GetQualifiedName(*oneof->containing_type()), "::k", camel_name,
+            ") {");
+      Nest();
+      AppendFieldValueAppend(field, false /* omit_default */,
+                             "msg." + name + "()");
+      Unnest();
+      Print("}");
+    } else {
+      AppendFieldValueAppend(field, true /* omit_default */,
+                             "msg." + name + "()");
+    }
   }
 }
 
@@ -576,9 +591,18 @@ void Generator::AppendDebugStringFunctions(const Descriptor& md) {
   SetOutput(&cc_);
   Print().Print("namespace internal {").Print();
   Print(sig, " {").Nest();
+  std::vector<const FieldDescriptor*> fields;
   for (int i = 0; i < md.field_count(); ++i) {
+    fields.push_back(md.field(i));
+  }
+  std::sort(fields.begin(), fields.end(),
+            [](const FieldDescriptor* left, const FieldDescriptor* right) {
+              return left->number() < right->number();
+            });
+
+  for (const FieldDescriptor* field : fields) {
     SetOutput(&cc_);
-    AppendFieldAppend(*md.field(i));
+    AppendFieldAppend(*field);
   }
   Unnest().Print("}").Print().Print("}  // namespace internal");
 }
diff --git a/tensorflow/tools/proto_text/gen_proto_text_functions_lib_test.cc b/tensorflow/tools/proto_text/gen_proto_text_functions_lib_test.cc
index b491295967..083f9fe188 100644
--- a/tensorflow/tools/proto_text/gen_proto_text_functions_lib_test.cc
+++ b/tensorflow/tools/proto_text/gen_proto_text_functions_lib_test.cc
@@ -69,6 +69,8 @@ T RoundtripParseProtoOrDie(const T& input, bool short_debug) {
 TEST(CreateProtoDebugStringLibTest, ValidSimpleTypes) {
   TestAllTypes proto;
 
+  // Note that this also tests that output of fields matches tag number order,
+  // since some of these fields have high tag numbers.
   proto.Clear();
   proto.set_optional_int32(-1);
   proto.set_optional_int64(-2);
@@ -469,11 +471,28 @@ TEST(CreateProtoDebugStringLibTest, Oneof) {
   proto.set_oneof_string("abc");
   EXPECT_TEXT_TRANSFORMS_MATCH();
 
+  // Empty oneof_string is printed, as the setting of the value in the oneof is
+  // meaningful.
+  proto.Clear();
+  proto.set_oneof_string("");
+  EXPECT_TEXT_TRANSFORMS_MATCH();
+
   proto.Clear();
   proto.set_oneof_string("abc");
   proto.set_oneof_uint32(123);
   EXPECT_TEXT_TRANSFORMS_MATCH();
 
+  // Zero uint32 is printed, as the setting of the value in the oneof is
+  // meaningful.
+  proto.Clear();
+  proto.set_oneof_uint32(0);
+  EXPECT_TEXT_TRANSFORMS_MATCH();
+
+  // Zero enum value is meaningful.
+  proto.Clear();
+  proto.set_oneof_enum(TestAllTypes::ZERO);
+  EXPECT_TEXT_TRANSFORMS_MATCH();
+
   // Parse a text format that lists multiple members of the oneof.
   EXPECT_PARSE_FAILURE("oneof_string: \"abc\" oneof_uint32: 13 ");
   EXPECT_PARSE_FAILURE("oneof_string: \"abc\" oneof_string: \"def\" ");
diff --git a/tensorflow/tools/proto_text/test.proto b/tensorflow/tools/proto_text/test.proto
index 9dd83e6393..e47a7b14e2 100644
--- a/tensorflow/tools/proto_text/test.proto
+++ b/tensorflow/tools/proto_text/test.proto
@@ -23,10 +23,10 @@ message TestAllTypes {
   }
 
   // Singular
-  int32 optional_int32 = 1;
+  int32 optional_int32 = 1000;  // use large tag to test output order.
   int64 optional_int64 = 2;
   uint32 optional_uint32 = 3;
-  uint64 optional_uint64 = 4;
+  uint64 optional_uint64 = 999;  // use large tag to test output order.
   sint32 optional_sint32 = 5;
   sint64 optional_sint64 = 6;
   fixed32 optional_fixed32 = 7;
@@ -74,6 +74,7 @@ message TestAllTypes {
     NestedMessage oneof_nested_message = 112;
     string oneof_string = 113;
     bytes oneof_bytes = 114;
+    NestedEnum oneof_enum = 100;
   }
 
   map<string, NestedMessage> map_string_to_message = 58;