Merge pull request #209 from cfallin/ruby-protoc-maps

Properly support maps in Ruby protoc and another bugfix.

ruby/ext/google/protobuf_c/encode_decode.c

@@ -208,7 +208,11 @@ typedef struct {
   size_t ofs;
   upb_fieldtype_t key_field_type;
   upb_fieldtype_t value_field_type;
-  VALUE value_field_typeclass;
+
+  // We know that we can hold this reference because the handlerdata has the
+  // same lifetime as the upb_handlers struct, and the upb_handlers struct holds
+  // a reference to the upb_msgdef, which in turn has references to its subdefs.
+  const upb_def* value_field_subdef;
 } map_handlerdata_t;
 
 // Temporary frame for map parsing: at the beginning of a map entry message, a
@@ -248,8 +252,15 @@ static bool endmap_handler(void *closure, const void *hd, upb_status* s) {
   VALUE key = native_slot_get(
       mapdata->key_field_type, Qnil,
       &frame->key_storage);
+
+  VALUE value_field_typeclass = Qnil;
+  if (mapdata->value_field_type == UPB_TYPE_MESSAGE ||
+      mapdata->value_field_type == UPB_TYPE_ENUM) {
+    value_field_typeclass = get_def_obj(mapdata->value_field_subdef);
+  }
+
   VALUE value = native_slot_get(
-      mapdata->value_field_type, mapdata->value_field_typeclass,
+      mapdata->value_field_type, value_field_typeclass,
       &frame->value_storage);
 
   Map_index_set(frame->map, key, value);
@@ -280,17 +291,7 @@ static map_handlerdata_t* new_map_handlerdata(
                                                     MAP_VALUE_FIELD);
   assert(value_field != NULL);
   hd->value_field_type = upb_fielddef_type(value_field);
-  hd->value_field_typeclass = field_type_class(value_field);
-
-  // Ensure that value_field_typeclass is properly GC-rooted. We must do this
-  // because we hold a reference to the Ruby class in the handlerdata, which is
-  // owned by the handlers. The handlers are owned by *this* message's Ruby
-  // object, but each Ruby object is rooted independently at the def -> Ruby
-  // object map. So we have to ensure that the Ruby objects we depend on will
-  // stick around as long as we're around.
-  if (hd->value_field_typeclass != Qnil) {
-    rb_ary_push(desc->typeclass_references, hd->value_field_typeclass);
-  }
+  hd->value_field_subdef = upb_fielddef_subdef(value_field);
 
   return hd;
 }

ruby/tests/generated_code.rb

@@ -27,16 +27,16 @@ Google::Protobuf::DescriptorPool.generated_pool.build do
     repeated :repeated_bytes, :string, 29
     repeated :repeated_enum, :enum, 30, "A.B.C.TestEnum"
     repeated :repeated_msg, :message, 31, "A.B.C.TestMessage"
-    repeated :map_int32_string, :message, 61, "A.B.C.TestMessage.MapInt32StringEntry"
-    repeated :map_int64_string, :message, 62, "A.B.C.TestMessage.MapInt64StringEntry"
-    repeated :map_uint32_string, :message, 63, "A.B.C.TestMessage.MapUint32StringEntry"
-    repeated :map_uint64_string, :message, 64, "A.B.C.TestMessage.MapUint64StringEntry"
-    repeated :map_bool_string, :message, 65, "A.B.C.TestMessage.MapBoolStringEntry"
-    repeated :map_string_string, :message, 66, "A.B.C.TestMessage.MapStringStringEntry"
-    repeated :map_string_msg, :message, 67, "A.B.C.TestMessage.MapStringMsgEntry"
-    repeated :map_string_enum, :message, 68, "A.B.C.TestMessage.MapStringEnumEntry"
-    repeated :map_string_int32, :message, 69, "A.B.C.TestMessage.MapStringInt32Entry"
-    repeated :map_string_bool, :message, 70, "A.B.C.TestMessage.MapStringBoolEntry"
+    map :map_int32_string, :int32, :string, 61
+    map :map_int64_string, :int64, :string, 62
+    map :map_uint32_string, :uint32, :string, 63
+    map :map_uint64_string, :uint64, :string, 64
+    map :map_bool_string, :bool, :string, 65
+    map :map_string_string, :string, :string, 66
+    map :map_string_msg, :string, :message, 67, "A.B.C.TestMessage"
+    map :map_string_enum, :string, :enum, 68, "A.B.C.TestEnum"
+    map :map_string_int32, :string, :int32, 69
+    map :map_string_bool, :string, :bool, 70
     optional :nested_message, :message, 80, "A.B.C.TestMessage.NestedMessage"
     oneof :my_oneof do
       optional :oneof_int32, :int32, 41
@@ -52,46 +52,6 @@ Google::Protobuf::DescriptorPool.generated_pool.build do
       optional :oneof_msg, :message, 51, "A.B.C.TestMessage"
     end
   end
-  add_message "A.B.C.TestMessage.MapInt32StringEntry" do
-    optional :key, :int32, 1
-    optional :value, :string, 2
-  end
-  add_message "A.B.C.TestMessage.MapInt64StringEntry" do
-    optional :key, :int64, 1
-    optional :value, :string, 2
-  end
-  add_message "A.B.C.TestMessage.MapUint32StringEntry" do
-    optional :key, :uint32, 1
-    optional :value, :string, 2
-  end
-  add_message "A.B.C.TestMessage.MapUint64StringEntry" do
-    optional :key, :uint64, 1
-    optional :value, :string, 2
-  end
-  add_message "A.B.C.TestMessage.MapBoolStringEntry" do
-    optional :key, :bool, 1
-    optional :value, :string, 2
-  end
-  add_message "A.B.C.TestMessage.MapStringStringEntry" do
-    optional :key, :string, 1
-    optional :value, :string, 2
-  end
-  add_message "A.B.C.TestMessage.MapStringMsgEntry" do
-    optional :key, :string, 1
-    optional :value, :message, 2, "A.B.C.TestMessage"
-  end
-  add_message "A.B.C.TestMessage.MapStringEnumEntry" do
-    optional :key, :string, 1
-    optional :value, :enum, 2, "A.B.C.TestEnum"
-  end
-  add_message "A.B.C.TestMessage.MapStringInt32Entry" do
-    optional :key, :string, 1
-    optional :value, :int32, 2
-  end
-  add_message "A.B.C.TestMessage.MapStringBoolEntry" do
-    optional :key, :string, 1
-    optional :value, :bool, 2
-  end
   add_message "A.B.C.TestMessage.NestedMessage" do
     optional :foo, :int32, 1
   end
@@ -107,16 +67,6 @@ module A
   module B
     module C
       TestMessage = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage").msgclass
-      TestMessage::MapInt32StringEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapInt32StringEntry").msgclass
-      TestMessage::MapInt64StringEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapInt64StringEntry").msgclass
-      TestMessage::MapUint32StringEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapUint32StringEntry").msgclass
-      TestMessage::MapUint64StringEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapUint64StringEntry").msgclass
-      TestMessage::MapBoolStringEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapBoolStringEntry").msgclass
-      TestMessage::MapStringStringEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapStringStringEntry").msgclass
-      TestMessage::MapStringMsgEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapStringMsgEntry").msgclass
-      TestMessage::MapStringEnumEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapStringEnumEntry").msgclass
-      TestMessage::MapStringInt32Entry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapStringInt32Entry").msgclass
-      TestMessage::MapStringBoolEntry = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.MapStringBoolEntry").msgclass
       TestMessage::NestedMessage = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestMessage.NestedMessage").msgclass
       TestEnum = Google::Protobuf::DescriptorPool.generated_pool.lookup("A.B.C.TestEnum").enummodule
     end

src/google/protobuf/compiler/ruby/ruby_generator.cc

@@ -102,24 +102,53 @@ std::string TypeName(const google::protobuf::FieldDescriptor* field) {
 
 void GenerateField(const google::protobuf::FieldDescriptor* field,
                    google::protobuf::io::Printer* printer) {
-  printer->Print(
-    "$label$ :$name$, ",
-    "label", LabelForField(field),
-    "name", field->name());
-  printer->Print(
-    ":$type$, $number$",
-    "type", TypeName(field),
-    "number", IntToString(field->number()));
-  if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
-    printer->Print(
-      ", \"$subtype$\"\n",
-     "subtype", field->message_type()->full_name());
-  } else if (field->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) {
+
+  if (field->is_map()) {
+    const FieldDescriptor* key_field =
+        field->message_type()->FindFieldByNumber(1);
+    const FieldDescriptor* value_field =
+        field->message_type()->FindFieldByNumber(2);
+
     printer->Print(
-      ", \"$subtype$\"\n",
-      "subtype", field->enum_type()->full_name());
+      "map :$name$, :$key_type$, :$value_type$, $number$",
+      "name", field->name(),
+      "key_type", TypeName(key_field),
+      "value_type", TypeName(value_field),
+      "number", IntToString(field->number()));
+
+    if (value_field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
+      printer->Print(
+        ", \"$subtype$\"\n",
+        "subtype", value_field->message_type()->full_name());
+    } else if (value_field->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) {
+      printer->Print(
+        ", \"$subtype$\"\n",
+        "subtype", value_field->enum_type()->full_name());
+    } else {
+      printer->Print("\n");
+    }
   } else {
-    printer->Print("\n");
+
+    printer->Print(
+      "$label$ :$name$, ",
+      "label", LabelForField(field),
+      "name", field->name());
+    printer->Print(
+      ":$type$, $number$",
+      "type", TypeName(field),
+      "number", IntToString(field->number()));
+
+    if (field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
+      printer->Print(
+        ", \"$subtype$\"\n",
+       "subtype", field->message_type()->full_name());
+    } else if (field->cpp_type() == FieldDescriptor::CPPTYPE_ENUM) {
+      printer->Print(
+        ", \"$subtype$\"\n",
+        "subtype", field->enum_type()->full_name());
+    } else {
+      printer->Print("\n");
+    }
   }
 }
 
@@ -141,6 +170,13 @@ void GenerateOneof(const google::protobuf::OneofDescriptor* oneof,
 
 void GenerateMessage(const google::protobuf::Descriptor* message,
                      google::protobuf::io::Printer* printer) {
+
+  // Don't generate MapEntry messages -- we use the Ruby extension's native
+  // support for map fields instead.
+  if (message->options().map_entry()) {
+    return;
+  }
+
   printer->Print(
     "add_message \"$name$\" do\n",
     "name", message->full_name());
@@ -213,6 +249,13 @@ void GenerateMessageAssignment(
     const std::string& prefix,
     const google::protobuf::Descriptor* message,
     google::protobuf::io::Printer* printer) {
+
+  // Don't generate MapEntry messages -- we use the Ruby extension's native
+  // support for map fields instead.
+  if (message->options().map_entry()) {
+    return;
+  }
+
   printer->Print(
     "$prefix$$name$ = ",
     "prefix", prefix,