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

This reverts commit 37581380.

ruby/ext/google/protobuf_c/message.c

@@ -60,11 +60,6 @@ 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;
@@ -281,7 +276,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(self->descriptor->layout, f);
+    VALUE enum_type = field_type_class(f);
     VALUE method = rb_intern("const_get");
     VALUE raw_value = layout_get(self->descriptor->layout, Message_data(self), f);
 
@@ -325,13 +320,15 @@ VALUE Message_respond_to_missing(int argc, VALUE* argv, VALUE _self) {
   }
 }
 
-VALUE create_submsg_from_hash(const MessageLayout* layout,
-                              const upb_fielddef* f, VALUE hash) {
-  const upb_msgdef *d = upb_fielddef_msgsubdef(f);
+VALUE create_submsg_from_hash(const upb_fielddef *f, VALUE hash) {
+  const upb_def *d = upb_fielddef_subdef(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, field_type_class(layout, f));
+  return rb_class_new_instance(1, args, msgclass);
 }
 
 int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
@@ -381,14 +378,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(self->descriptor->layout, f, entry);
+        entry = create_submsg_from_hash(f, entry);
       }
 
       RepeatedField_push(ary, entry);
     }
   } else {
     if (TYPE(val) == T_HASH && upb_fielddef_issubmsg(f)) {
-      val = create_submsg_from_hash(self->descriptor->layout, f, val);
+      val = create_submsg_from_hash(f, val);
     }
 
     layout_set(self->descriptor->layout, Message_data(self), f, val);
@@ -633,11 +630,17 @@ VALUE Message_descriptor(VALUE klass) {
   return rb_ivar_get(klass, descriptor_instancevar_interned);
 }
 
-VALUE build_class_from_descriptor(VALUE descriptor) {
-  Descriptor* desc = ruby_to_Descriptor(descriptor);
+VALUE build_class_from_descriptor(Descriptor* desc) {
   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.");
@@ -648,7 +651,8 @@ VALUE build_class_from_descriptor(VALUE descriptor) {
       // their own toplevel constant class name.
       rb_intern("Message"),
       rb_cObject);
-  rb_ivar_set(klass, descriptor_instancevar_interned, descriptor);
+  rb_ivar_set(klass, descriptor_instancevar_interned,
+              get_def_obj(desc->msgdef));
   rb_define_alloc_func(klass, Message_alloc);
   rb_require("google/protobuf/message_exts");
   rb_include_module(klass, rb_eval_string("::Google::Protobuf::MessageExts"));
@@ -733,8 +737,7 @@ VALUE enum_descriptor(VALUE self) {
   return rb_ivar_get(self, descriptor_instancevar_interned);
 }
 
-VALUE build_module_from_enumdesc(VALUE _enumdesc) {
-  EnumDescriptor* enumdesc = ruby_to_EnumDescriptor(_enumdesc);
+VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) {
   VALUE mod = rb_define_module_id(
       rb_intern(upb_enumdef_fullname(enumdesc->enumdef)));
 
@@ -755,7 +758,8 @@ VALUE build_module_from_enumdesc(VALUE _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, _enumdesc);
+  rb_ivar_set(mod, descriptor_instancevar_interned,
+              get_def_obj(enumdesc->enumdef));
 
   return mod;
 }

ruby/ext/google/protobuf_c/protobuf.c

@@ -30,10 +30,26 @@
 
 #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;
-VALUE c_only_cookie;
+
+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));
+}
 
 // -----------------------------------------------------------------------------
 // Utilities.
@@ -100,6 +116,6 @@ void Init_protobuf_c() {
   kRubyStringASCIIEncoding = rb_usascii_encoding();
   kRubyString8bitEncoding = rb_ascii8bit_encoding();
 
-  rb_gc_register_address(&c_only_cookie);
-  c_only_cookie = rb_class_new_instance(0, NULL, rb_cObject);
+  rb_gc_register_address(&upb_def_to_ruby_obj_map);
+  upb_def_to_ruby_obj_map = rb_hash_new();
 }

ruby/ext/google/protobuf_c/storage.c

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

ruby/lib/google/protobuf.rb

@@ -50,72 +50,6 @@ 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,6 +17,7 @@ module BasicTest
     add_message "BadFieldNames" do
       optional :dup, :int32, 1
       optional :class, :int32, 2
+      optional :"a.b", :int32, 3
     end
   end
 
@@ -350,6 +351,11 @@ 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,6 +18,7 @@ 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_equal m, m2
+    assert m == m2
 
     data = Google::Protobuf.encode m
     m2 = Google::Protobuf.decode(proto_module::TestMessage, data)
@@ -902,6 +902,8 @@ 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
@@ -1082,7 +1084,9 @@ module CommonTests
 
     json_text = proto_module::TestMessage.encode_json(m)
     m2 = proto_module::TestMessage.decode_json(json_text)
-    assert_equal m, m2
+    puts m.inspect
+    puts m2.inspect
+    assert m == m2
 
     # Crash case from GitHub issue 283.
     bar = proto_module::Bar.new(msg: "bar")
@@ -1128,7 +1132,7 @@ module CommonTests
 
     actual = proto_module::TestMessage.encode_json(m, :emit_defaults => true)
 
-    assert_equal expected, JSON.parse(actual, :symbolize_names => true)
+    assert JSON.parse(actual, :symbolize_names => true) == expected
   end
 
   def test_json_emit_defaults_submsg
@@ -1163,7 +1167,7 @@ module CommonTests
 
     actual = proto_module::TestMessage.encode_json(m, :emit_defaults => true)
 
-    assert_equal expected, JSON.parse(actual, :symbolize_names => true)
+    assert JSON.parse(actual, :symbolize_names => true) == expected
   end
 
   def test_json_emit_defaults_repeated_submsg
@@ -1197,7 +1201,7 @@ module CommonTests
 
     actual = proto_module::TestMessage.encode_json(m, :emit_defaults => true)
 
-    assert_equal expected, JSON.parse(actual, :symbolize_names => true)
+    assert JSON.parse(actual, :symbolize_names => true) == expected
   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 Fixnum)." do
+                "Invalid argument for string field 'optional_string' (given Integer)." do
       A::B::C::TestMessage.new(optional_string: 4)
     end
     check_error Google::Protobuf::TypeError,
-                "Invalid argument for string field 'oneof_string' (given Fixnum)." do
+                "Invalid argument for string field 'oneof_string' (given Integer)." 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 Fixnum to assign to submessage field 'optional_msg'." do
+                "Invalid type Integer to assign to submessage field 'optional_msg'." do
       A::B::C::TestMessage.new(optional_msg: 2)
     end
     check_error Google::Protobuf::TypeError,