Updated upb from defcleanup branch and modified Ruby to use it (#5539)

ruby/ext/google/protobuf_c/message.c

@@ -60,6 +60,11 @@ rb_data_type_t Message_type = {
 VALUE Message_alloc(VALUE klass) {
   VALUE descriptor = rb_ivar_get(klass, descriptor_instancevar_interned);
   Descriptor* desc = ruby_to_Descriptor(descriptor);
+
+  if (desc->layout == NULL) {
+    desc->layout = create_layout(desc);
+  }
+
   MessageHeader* msg = (MessageHeader*)ALLOC_N(
       uint8_t, sizeof(MessageHeader) + desc->layout->size);
   VALUE ret;
@@ -276,7 +281,7 @@ VALUE Message_method_missing(int argc, VALUE* argv, VALUE _self) {
   } else if (accessor_type == METHOD_PRESENCE) {
     return layout_has(self->descriptor->layout, Message_data(self), f);
   } else if (accessor_type == METHOD_ENUM_GETTER) {
-    VALUE enum_type = field_type_class(f);
+    VALUE enum_type = field_type_class(self->descriptor->layout, f);
     VALUE method = rb_intern("const_get");
     VALUE raw_value = layout_get(self->descriptor->layout, Message_data(self), f);
 
@@ -320,15 +325,13 @@ VALUE Message_respond_to_missing(int argc, VALUE* argv, VALUE _self) {
   }
 }
 
-VALUE create_submsg_from_hash(const upb_fielddef *f, VALUE hash) {
-  const upb_def *d = upb_fielddef_subdef(f);
+VALUE create_submsg_from_hash(const MessageLayout* layout,
+                              const upb_fielddef* f, VALUE hash) {
+  const upb_msgdef *d = upb_fielddef_msgsubdef(f);
   assert(d != NULL);
 
-  VALUE descriptor = get_def_obj(d);
-  VALUE msgclass = rb_funcall(descriptor, rb_intern("msgclass"), 0, NULL);
-
   VALUE args[1] = { hash };
-  return rb_class_new_instance(1, args, msgclass);
+  return rb_class_new_instance(1, args, field_type_class(layout, f));
 }
 
 int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
@@ -378,14 +381,14 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
     for (int i = 0; i < RARRAY_LEN(val); i++) {
       VALUE entry = rb_ary_entry(val, i);
       if (TYPE(entry) == T_HASH && upb_fielddef_issubmsg(f)) {
-        entry = create_submsg_from_hash(f, entry);
+        entry = create_submsg_from_hash(self->descriptor->layout, f, entry);
       }
 
       RepeatedField_push(ary, entry);
     }
   } else {
     if (TYPE(val) == T_HASH && upb_fielddef_issubmsg(f)) {
-      val = create_submsg_from_hash(f, val);
+      val = create_submsg_from_hash(self->descriptor->layout, f, val);
     }
 
     layout_set(self->descriptor->layout, Message_data(self), f, val);
@@ -630,17 +633,11 @@ VALUE Message_descriptor(VALUE klass) {
   return rb_ivar_get(klass, descriptor_instancevar_interned);
 }
 
-VALUE build_class_from_descriptor(Descriptor* desc) {
+VALUE build_class_from_descriptor(VALUE descriptor) {
+  Descriptor* desc = ruby_to_Descriptor(descriptor);
   const char *name;
   VALUE klass;
 
-  if (desc->layout == NULL) {
-    desc->layout = create_layout(desc->msgdef);
-  }
-  if (desc->fill_method == NULL) {
-    desc->fill_method = new_fillmsg_decodermethod(desc, &desc->fill_method);
-  }
-
   name = upb_msgdef_fullname(desc->msgdef);
   if (name == NULL) {
     rb_raise(rb_eRuntimeError, "Descriptor does not have assigned name.");
@@ -651,8 +648,7 @@ VALUE build_class_from_descriptor(Descriptor* desc) {
       // their own toplevel constant class name.
       rb_intern("Message"),
       rb_cObject);
-  rb_ivar_set(klass, descriptor_instancevar_interned,
-              get_def_obj(desc->msgdef));
+  rb_ivar_set(klass, descriptor_instancevar_interned, descriptor);
   rb_define_alloc_func(klass, Message_alloc);
   rb_require("google/protobuf/message_exts");
   rb_include_module(klass, rb_eval_string("::Google::Protobuf::MessageExts"));
@@ -737,7 +733,8 @@ VALUE enum_descriptor(VALUE self) {
   return rb_ivar_get(self, descriptor_instancevar_interned);
 }
 
-VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) {
+VALUE build_module_from_enumdesc(VALUE _enumdesc) {
+  EnumDescriptor* enumdesc = ruby_to_EnumDescriptor(_enumdesc);
   VALUE mod = rb_define_module_id(
       rb_intern(upb_enumdef_fullname(enumdesc->enumdef)));
 
@@ -758,8 +755,7 @@ VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) {
   rb_define_singleton_method(mod, "lookup", enum_lookup, 1);
   rb_define_singleton_method(mod, "resolve", enum_resolve, 1);
   rb_define_singleton_method(mod, "descriptor", enum_descriptor, 0);
-  rb_ivar_set(mod, descriptor_instancevar_interned,
-              get_def_obj(enumdesc->enumdef));
+  rb_ivar_set(mod, descriptor_instancevar_interned, _enumdesc);
 
   return mod;
 }

ruby/ext/google/protobuf_c/protobuf.c

@@ -30,26 +30,10 @@
 
 #include "protobuf.h"
 
-// -----------------------------------------------------------------------------
-// Global map from upb {msg,enum}defs to wrapper Descriptor/EnumDescriptor
-// instances.
-// -----------------------------------------------------------------------------
-
-// This is a hash table from def objects (encoded by converting pointers to
-// Ruby integers) to MessageDef/EnumDef instances (as Ruby values).
-VALUE upb_def_to_ruby_obj_map;
-
 VALUE cError;
 VALUE cParseError;
 VALUE cTypeError;
-
-void add_def_obj(const void* def, VALUE value) {
-  rb_hash_aset(upb_def_to_ruby_obj_map, ULL2NUM((intptr_t)def), value);
-}
-
-VALUE get_def_obj(const void* def) {
-  return rb_hash_aref(upb_def_to_ruby_obj_map, ULL2NUM((intptr_t)def));
-}
+VALUE c_only_cookie;
 
 // -----------------------------------------------------------------------------
 // Utilities.
@@ -116,6 +100,6 @@ void Init_protobuf_c() {
   kRubyStringASCIIEncoding = rb_usascii_encoding();
   kRubyString8bitEncoding = rb_ascii8bit_encoding();
 
-  rb_gc_register_address(&upb_def_to_ruby_obj_map);
-  upb_def_to_ruby_obj_map = rb_hash_new();
+  rb_gc_register_address(&c_only_cookie);
+  c_only_cookie = rb_class_new_instance(0, NULL, rb_cObject);
 }

ruby/ext/google/protobuf_c/storage.c

@@ -430,8 +430,10 @@ static size_t align_up_to(size_t offset, size_t granularity) {
   return (offset + granularity - 1) & ~(granularity - 1);
 }
 
-MessageLayout* create_layout(const upb_msgdef* msgdef) {
+MessageLayout* create_layout(const Descriptor* desc) {
+  const upb_msgdef *msgdef = desc->msgdef;
   MessageLayout* layout = ALLOC(MessageLayout);
+  layout->desc = desc;
   int nfields = upb_msgdef_numfields(msgdef);
   upb_msg_field_iter it;
   upb_msg_oneof_iter oit;
@@ -448,7 +450,7 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
       layout->fields[upb_fielddef_index(field)].hasbit = hasbit++;
     } else {
       layout->fields[upb_fielddef_index(field)].hasbit =
-	  MESSAGE_FIELD_NO_HASBIT;
+          MESSAGE_FIELD_NO_HASBIT;
     }
   }
 
@@ -537,28 +539,25 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
   }
 
   layout->size = off;
-
   layout->msgdef = msgdef;
-  upb_msgdef_ref(layout->msgdef, &layout->msgdef);
 
   return layout;
 }
 
 void free_layout(MessageLayout* layout) {
   xfree(layout->fields);
-  upb_msgdef_unref(layout->msgdef, &layout->msgdef);
   xfree(layout);
 }
 
-VALUE field_type_class(const upb_fielddef* field) {
+VALUE field_type_class(const MessageLayout* layout, const upb_fielddef* field) {
   VALUE type_class = Qnil;
   if (upb_fielddef_type(field) == UPB_TYPE_MESSAGE) {
-    VALUE submsgdesc =
-        get_def_obj(upb_fielddef_subdef(field));
+    VALUE submsgdesc = get_msgdef_obj(layout->desc->descriptor_pool,
+                                      upb_fielddef_msgsubdef(field));
     type_class = Descriptor_msgclass(submsgdesc);
   } else if (upb_fielddef_type(field) == UPB_TYPE_ENUM) {
-    VALUE subenumdesc =
-        get_def_obj(upb_fielddef_subdef(field));
+    VALUE subenumdesc = get_enumdef_obj(layout->desc->descriptor_pool,
+                                        upb_fielddef_enumsubdef(field));
     type_class = EnumDescriptor_enummodule(subenumdesc);
   }
   return type_class;
@@ -632,7 +631,7 @@ void layout_clear(MessageLayout* layout,
 
     const upb_fielddef* key_field = map_field_key(field);
     const upb_fielddef* value_field = map_field_value(field);
-    VALUE type_class = field_type_class(value_field);
+    VALUE type_class = field_type_class(layout, value_field);
 
     if (type_class != Qnil) {
       VALUE args[3] = {
@@ -653,7 +652,7 @@ void layout_clear(MessageLayout* layout,
   } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) {
     VALUE ary = Qnil;
 
-    VALUE type_class = field_type_class(field);
+    VALUE type_class = field_type_class(layout, field);
 
     if (type_class != Qnil) {
       VALUE args[2] = {
@@ -668,9 +667,9 @@ void layout_clear(MessageLayout* layout,
 
     DEREF(memory, VALUE) = ary;
   } else {
-    native_slot_set(upb_fielddef_name(field),
-                    upb_fielddef_type(field), field_type_class(field),
-                    memory, layout_get_default(field));
+    native_slot_set(upb_fielddef_name(field), upb_fielddef_type(field),
+                    field_type_class(layout, field), memory,
+                    layout_get_default(field));
   }
 }
 
@@ -728,20 +727,19 @@ VALUE layout_get(MessageLayout* layout,
       return layout_get_default(field);
     }
     return native_slot_get(upb_fielddef_type(field),
-                           field_type_class(field),
-                           memory);
+                           field_type_class(layout, field), memory);
   } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) {
     return *((VALUE *)memory);
   } else if (!field_set) {
     return layout_get_default(field);
   } else {
     return native_slot_get(upb_fielddef_type(field),
-                           field_type_class(field),
-                           memory);
+                           field_type_class(layout, field), memory);
   }
 }
 
-static void check_repeated_field_type(VALUE val, const upb_fielddef* field) {
+static void check_repeated_field_type(const MessageLayout* layout, VALUE val,
+                                      const upb_fielddef* field) {
   RepeatedField* self;
   assert(upb_fielddef_label(field) == UPB_LABEL_REPEATED);
 
@@ -755,25 +753,13 @@ static void check_repeated_field_type(VALUE val, const upb_fielddef* field) {
     rb_raise(cTypeError, "Repeated field array has wrong element type");
   }
 
-  if (self->field_type == UPB_TYPE_MESSAGE) {
-    if (self->field_type_class !=
-        Descriptor_msgclass(get_def_obj(upb_fielddef_subdef(field)))) {
-      rb_raise(cTypeError,
-               "Repeated field array has wrong message class");
-    }
-  }
-
-
-  if (self->field_type == UPB_TYPE_ENUM) {
-    if (self->field_type_class !=
-        EnumDescriptor_enummodule(get_def_obj(upb_fielddef_subdef(field)))) {
-      rb_raise(cTypeError,
-               "Repeated field array has wrong enum class");
-    }
+  if (self->field_type_class != field_type_class(layout, field)) {
+    rb_raise(cTypeError, "Repeated field array has wrong message/enum class");
   }
 }
 
-static void check_map_field_type(VALUE val, const upb_fielddef* field) {
+static void check_map_field_type(const MessageLayout* layout, VALUE val,
+                                 const upb_fielddef* field) {
   const upb_fielddef* key_field = map_field_key(field);
   const upb_fielddef* value_field = map_field_value(field);
   Map* self;
@@ -790,17 +776,11 @@ static void check_map_field_type(VALUE val, const upb_fielddef* field) {
   if (self->value_type != upb_fielddef_type(value_field)) {
     rb_raise(cTypeError, "Map value type does not match field's value type");
   }
-  if (upb_fielddef_type(value_field) == UPB_TYPE_MESSAGE ||
-      upb_fielddef_type(value_field) == UPB_TYPE_ENUM) {
-    if (self->value_type_class !=
-        get_def_obj(upb_fielddef_subdef(value_field))) {
-      rb_raise(cTypeError,
-               "Map value type has wrong message/enum class");
-    }
+  if (self->value_type_class != field_type_class(layout, value_field)) {
+    rb_raise(cTypeError, "Map value type has wrong message/enum class");
   }
 }
 
-
 void layout_set(MessageLayout* layout,
                 void* storage,
                 const upb_fielddef* field,
@@ -828,20 +808,19 @@ void layout_set(MessageLayout* layout,
       // and case number are altered atomically (w.r.t. the Ruby VM).
       native_slot_set_value_and_case(
           upb_fielddef_name(field),
-          upb_fielddef_type(field), field_type_class(field),
+          upb_fielddef_type(field), field_type_class(layout, field),
           memory, val,
           oneof_case, upb_fielddef_number(field));
     }
   } else if (is_map_field(field)) {
-    check_map_field_type(val, field);
+    check_map_field_type(layout, val, field);
     DEREF(memory, VALUE) = val;
   } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) {
-    check_repeated_field_type(val, field);
+    check_repeated_field_type(layout, val, field);
     DEREF(memory, VALUE) = val;
   } else {
-    native_slot_set(upb_fielddef_name(field),
-                    upb_fielddef_type(field), field_type_class(field),
-                    memory, val);
+    native_slot_set(upb_fielddef_name(field), upb_fielddef_type(field),
+                    field_type_class(layout, field), memory, val);
   }
 
   if (layout->fields[upb_fielddef_index(field)].hasbit !=

ruby/lib/google/protobuf.rb

@@ -50,6 +50,72 @@ else
   rescue LoadError
     require 'google/protobuf_c'
   end
+
+  module Google
+    module Protobuf
+      module Internal
+        def self.infer_package(names)
+          # Package is longest common prefix ending in '.', if any.
+          min, max = names.minmax
+          last_common_dot = nil
+          min.size.times { |i|
+            if min[i] != max[i] then break end
+            if min[i] == ?. then last_common_dot = i end
+          }
+          if last_common_dot
+            return min.slice(0, last_common_dot)
+          end
+        end
+
+        class NestingBuilder
+          def initialize(msg_names, enum_names)
+            @to_pos = {nil=>nil}
+            @msg_children = Hash.new { |hash, key| hash[key] = [] }
+            @enum_children = Hash.new { |hash, key| hash[key] = [] }
+
+            msg_names.each_with_index { |name, idx| @to_pos[name] = idx }
+            enum_names.each_with_index { |name, idx| @to_pos[name] = idx }
+
+            msg_names.each { |name| @msg_children[parent(name)] << name }
+            enum_names.each { |name| @enum_children[parent(name)] << name }
+          end
+
+          def build(package)
+            return build_msg(package)
+          end
+
+          private
+          def build_msg(msg)
+            return {
+              :pos => @to_pos[msg],
+              :msgs => @msg_children[msg].map { |child| build_msg(child) },
+              :enums => @enum_children[msg].map { |child| @to_pos[child] },
+            }
+          end
+
+          private
+          def parent(name)
+            idx = name.rindex(?.)
+            if idx
+              return name.slice(0, idx)
+            else
+              return nil
+            end
+          end
+        end
+
+        def self.fixup_descriptor(package, msg_names, enum_names)
+          if package.nil?
+            package = self.infer_package(msg_names + enum_names)
+          end
+
+          nesting = NestingBuilder.new(msg_names, enum_names).build(package)
+
+          return package, nesting
+        end
+      end
+    end
+  end
 end
 
 require 'google/protobuf/repeated_field'

ruby/tests/basic.rb

@@ -17,7 +17,6 @@ module BasicTest
     add_message "BadFieldNames" do
       optional :dup, :int32, 1
       optional :class, :int32, 2
-      optional :"a.b", :int32, 3
     end
   end
 
@@ -351,11 +350,6 @@ module BasicTest
       assert nil != file_descriptor
       assert_equal "tests/basic_test.proto", file_descriptor.name
       assert_equal :proto3, file_descriptor.syntax
-
-      file_descriptor = BadFieldNames.descriptor.file_descriptor
-      assert nil != file_descriptor
-      assert_equal nil, file_descriptor.name
-      assert_equal :proto3, file_descriptor.syntax
     end
 
     def test_map_freeze

ruby/tests/basic_proto2.rb

@@ -18,7 +18,6 @@ module BasicTestProto2
       add_message "BadFieldNames" do
         optional :dup, :int32, 1
         optional :class, :int32, 2
-        optional :"a.b", :int32, 3
       end
     end
   end

ruby/tests/common_tests.rb

@@ -807,7 +807,7 @@ module CommonTests
                                                         proto_module::TestMessage2.new(:foo => 2)])
     data = proto_module::TestMessage.encode m
     m2 = proto_module::TestMessage.decode data
-    assert m == m2
+    assert_equal m, m2
 
     data = Google::Protobuf.encode m
     m2 = Google::Protobuf.decode(proto_module::TestMessage, data)
@@ -902,8 +902,6 @@ module CommonTests
     assert m['class'] == 2
     m['dup'] = 3
     assert m['dup'] == 3
-    m['a.b'] = 4
-    assert m['a.b'] == 4
   end
 
   def test_int_ranges
@@ -1084,9 +1082,7 @@ module CommonTests
 
     json_text = proto_module::TestMessage.encode_json(m)
     m2 = proto_module::TestMessage.decode_json(json_text)
-    puts m.inspect
-    puts m2.inspect
-    assert m == m2
+    assert_equal m, m2
 
     # Crash case from GitHub issue 283.
     bar = proto_module::Bar.new(msg: "bar")
@@ -1132,7 +1128,7 @@ module CommonTests
 
     actual = proto_module::TestMessage.encode_json(m, :emit_defaults => true)
 
-    assert JSON.parse(actual, :symbolize_names => true) == expected
+    assert_equal expected, JSON.parse(actual, :symbolize_names => true)
   end
 
   def test_json_emit_defaults_submsg
@@ -1167,7 +1163,7 @@ module CommonTests
 
     actual = proto_module::TestMessage.encode_json(m, :emit_defaults => true)
 
-    assert JSON.parse(actual, :symbolize_names => true) == expected
+    assert_equal expected, JSON.parse(actual, :symbolize_names => true)
   end
 
   def test_json_emit_defaults_repeated_submsg
@@ -1201,7 +1197,7 @@ module CommonTests
 
     actual = proto_module::TestMessage.encode_json(m, :emit_defaults => true)
 
-    assert JSON.parse(actual, :symbolize_names => true) == expected
+    assert_equal expected, JSON.parse(actual, :symbolize_names => true)
   end
 
   def value_from_ruby(value)

ruby/tests/type_errors.rb

@@ -10,11 +10,11 @@ require 'generated_code_pb'
 class TestTypeErrors < Test::Unit::TestCase
   def test_bad_string
     check_error Google::Protobuf::TypeError,
-                "Invalid argument for string field 'optional_string' (given Integer)." do
+                "Invalid argument for string field 'optional_string' (given Fixnum)." do
       A::B::C::TestMessage.new(optional_string: 4)
     end
     check_error Google::Protobuf::TypeError,
-                "Invalid argument for string field 'oneof_string' (given Integer)." do
+                "Invalid argument for string field 'oneof_string' (given Fixnum)." do
       A::B::C::TestMessage.new(oneof_string: 4)
     end
     check_error ArgumentError,
@@ -151,7 +151,7 @@ class TestTypeErrors < Test::Unit::TestCase
 
   def test_bad_msg
     check_error Google::Protobuf::TypeError,
-                "Invalid type Integer to assign to submessage field 'optional_msg'." do
+                "Invalid type Fixnum to assign to submessage field 'optional_msg'." do
       A::B::C::TestMessage.new(optional_msg: 2)
     end
     check_error Google::Protobuf::TypeError,