Nearly all known cases (map, repeated field, and top-level) have been addressed.

The only case that doesn't work is decoding a wrapper type from JSON
at the top level.  This doesn't make sense and probably no users do it
I changed it to throw.

ruby/ext/google/protobuf_c/encode_decode.c

@@ -278,6 +278,17 @@ static void *appendsubmsg_handler(void *closure, const void *hd) {
   return submsg;
 }
 
+// Appends a wrapper to a repeated field (a regular Ruby array for now).
+static void *appendwrapper_handler(void *closure, const void *hd) {
+  VALUE ary = (VALUE)closure;
+  int size = RepeatedField_size(ary);
+  (void)hd;
+
+  RepeatedField_push(ary, Qnil);
+
+  return RepeatedField_index_native(ary, size);
+}
+
 // Sets a non-repeated submessage field in a message.
 static void *submsg_handler(void *closure, const void *hd) {
   MessageHeader* msg = closure;
@@ -503,6 +514,23 @@ static void *oneofsubmsg_handler(void *closure,
   return submsg;
 }
 
+bool is_wrapper(const upb_msgdef* m) {
+  switch (upb_msgdef_wellknowntype(m)) {
+    case UPB_WELLKNOWN_DOUBLEVALUE:
+    case UPB_WELLKNOWN_FLOATVALUE:
+    case UPB_WELLKNOWN_INT64VALUE:
+    case UPB_WELLKNOWN_UINT64VALUE:
+    case UPB_WELLKNOWN_INT32VALUE:
+    case UPB_WELLKNOWN_UINT32VALUE:
+    case UPB_WELLKNOWN_STRINGVALUE:
+    case UPB_WELLKNOWN_BYTESVALUE:
+    case UPB_WELLKNOWN_BOOLVALUE:
+      return true;
+    default:
+      return false;
+  }
+}
+
 // Set up handlers for a repeated field.
 static void add_handlers_for_repeated_field(upb_handlers *h,
                                             const Descriptor* desc,
@@ -544,7 +572,11 @@ static void add_handlers_for_repeated_field(upb_handlers *h,
       VALUE subklass = field_type_class(desc->layout, f);
       upb_handlerattr attr = UPB_HANDLERATTR_INIT;
       attr.handler_data = newsubmsghandlerdata(h, 0, -1, subklass);
+      if (is_wrapper(upb_fielddef_msgsubdef(f))) {
+        upb_handlers_setstartsubmsg(h, f, appendwrapper_handler, &attr);
+      } else {
         upb_handlers_setstartsubmsg(h, f, appendsubmsg_handler, &attr);
+      }
       break;
     }
   }
@@ -612,23 +644,6 @@ static bool boolwrapper_handler(void* closure, const void* hd, bool val) {
   return true;
 }
 
-bool is_wrapper(const upb_msgdef* m) {
-  switch (upb_msgdef_wellknowntype(m)) {
-    case UPB_WELLKNOWN_DOUBLEVALUE:
-    case UPB_WELLKNOWN_FLOATVALUE:
-    case UPB_WELLKNOWN_INT64VALUE:
-    case UPB_WELLKNOWN_UINT64VALUE:
-    case UPB_WELLKNOWN_INT32VALUE:
-    case UPB_WELLKNOWN_UINT32VALUE:
-    case UPB_WELLKNOWN_STRINGVALUE:
-    case UPB_WELLKNOWN_BYTESVALUE:
-    case UPB_WELLKNOWN_BOOLVALUE:
-      return true;
-    default:
-      return false;
-  }
-}
-
 // Set up handlers for a singular field.
 static void add_handlers_for_singular_field(const Descriptor* desc,
                                             upb_handlers* h,
@@ -811,6 +826,10 @@ static bool unknown_field_handler(void* closure, const void* hd,
   return true;
 }
 
+size_t get_field_offset(MessageLayout* layout, const upb_fielddef* f) {
+  return layout->fields[upb_fielddef_index(f)].offset + sizeof(MessageHeader);
+}
+
 void add_handlers_for_message(const void *closure, upb_handlers *h) {
   const VALUE descriptor_pool = (VALUE)closure;
   const upb_msgdef* msgdef = upb_handlers_msgdef(h);
@@ -847,8 +866,7 @@ void add_handlers_for_message(const void *closure, upb_handlers *h) {
        upb_msg_field_next(&i)) {
     const upb_fielddef *f = upb_msg_iter_field(&i);
     const upb_oneofdef *oneof = upb_fielddef_containingoneof(f);
-    size_t offset = desc->layout->fields[upb_fielddef_index(f)].offset +
-        sizeof(MessageHeader);
+    size_t offset = get_field_offset(desc->layout, f);
 
     if (oneof) {
       size_t oneof_case_offset =
@@ -960,17 +978,28 @@ VALUE Message_decode(VALUE klass, VALUE data) {
   {
     const upb_pbdecodermethod* method = msgdef_decodermethod(desc);
     const upb_handlers* h = upb_pbdecodermethod_desthandlers(method);
+    const upb_msgdef* m = upb_handlers_msgdef(h);
+    VALUE wrapper = Qnil;
+    void* ptr = msg;
     stackenv se;
     upb_sink sink;
     upb_pbdecoder* decoder;
     stackenv_init(&se, "Error occurred during parsing: %" PRIsVALUE);
 
-    upb_sink_reset(&sink, h, msg);
+    if (is_wrapper(m)) {
+      ptr = &wrapper;
+    }
+
+    upb_sink_reset(&sink, h, ptr);
     decoder = upb_pbdecoder_create(se.arena, method, sink, &se.status);
     upb_bufsrc_putbuf(RSTRING_PTR(data), RSTRING_LEN(data),
                       upb_pbdecoder_input(decoder));
 
     stackenv_uninit(&se);
+
+    if (is_wrapper(m)) {
+      msg_rb = ruby_wrapper_type(msgklass, wrapper);
+    }
   }
 
   return msg_rb;
@@ -1024,13 +1053,21 @@ VALUE Message_decode_json(int argc, VALUE* argv, VALUE klass) {
 
   {
     const upb_json_parsermethod* method = msgdef_jsonparsermethod(desc);
+    const upb_handlers* h = get_fill_handlers(desc);
+    const upb_msgdef* m = upb_handlers_msgdef(h);
     stackenv se;
     upb_sink sink;
     upb_json_parser* parser;
     DescriptorPool* pool = ruby_to_DescriptorPool(generated_pool);
     stackenv_init(&se, "Error occurred during parsing: %" PRIsVALUE);
 
-    upb_sink_reset(&sink, get_fill_handlers(desc), msg);
+    if (is_wrapper(m)) {
+      rb_raise(
+          rb_eRuntimeError,
+          "Parsing a wrapper type from JSON at the top level does not work.");
+    }
+
+    upb_sink_reset(&sink, h, msg);
     parser = upb_json_parser_create(se.arena, method, pool->symtab, sink,
                                     &se.status, RTEST(ignore_unknown_fields));
     upb_bufsrc_putbuf(RSTRING_PTR(data), RSTRING_LEN(data),
@@ -1103,6 +1140,7 @@ static void putary(VALUE ary, const upb_fielddef* f, upb_sink sink, int depth,
   upb_selector_t sel = 0;
   int size;
   int i;
+  VALUE type_class = ruby_to_RepeatedField(ary)->field_type_class;
 
   if (ary == Qnil) return;
   if (!emit_defaults && NUM2INT(RepeatedField_length(ary)) == 0) return;
@@ -1137,9 +1175,11 @@ static void putary(VALUE ary, const upb_fielddef* f, upb_sink sink, int depth,
       case UPB_TYPE_BYTES:
         putstr(*((VALUE *)memory), f, subsink);
         break;
-      case UPB_TYPE_MESSAGE:
-        putsubmsg(*((VALUE*)memory), f, subsink, depth, emit_defaults, is_json);
+      case UPB_TYPE_MESSAGE: {
+        VALUE val = native_slot_get(UPB_TYPE_MESSAGE, type_class, memory);
+        putsubmsg(val, f, subsink, depth, emit_defaults, is_json);
         break;
+      }
 
 #undef T
 
@@ -1440,13 +1480,9 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc,
         putstr(str, f, sink);
       }
     } else if (upb_fielddef_issubmsg(f)) {
-      VALUE val = DEREF(msg, offset, VALUE);
-      int type = TYPE(val);
-      if (type != T_DATA && type != T_NIL && is_wrapper_type_field(f)) {
-        // OPT: could try to avoid expanding the wrapper here.
-        val = ruby_wrapper_type(field_type_class(desc->layout, f), val);
-        DEREF(msg, offset, VALUE) = val;
-      }
+      // OPT: could try to avoid the layout_get() (which will expand lazy
+      // wrappers).
+      VALUE val = layout_get(desc->layout, Message_data(msg), f);
       putsubmsg(val, f, sink, depth, emit_defaults, is_json);
     } else {
       upb_selector_t sel = getsel(f, upb_handlers_getprimitivehandlertype(f));

ruby/ext/google/protobuf_c/map.c

@@ -559,7 +559,8 @@ VALUE Map_deep_copy(VALUE _self) {
     void* mem = value_memory(&v);
     upb_value dup;
     void* dup_mem = value_memory(&dup);
-    native_slot_deep_copy(self->value_type, dup_mem, mem);
+    native_slot_deep_copy(self->value_type, self->value_type_class, dup_mem,
+                          mem);
 
     if (!upb_strtable_insert2(&new_self->table,
                               upb_strtable_iter_key(&it),

ruby/ext/google/protobuf_c/protobuf.h

@@ -363,7 +363,8 @@ VALUE native_slot_get(upb_fieldtype_t type,
 void native_slot_init(upb_fieldtype_t type, void* memory);
 void native_slot_mark(upb_fieldtype_t type, void* memory);
 void native_slot_dup(upb_fieldtype_t type, void* to, void* from);
-void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from);
+void native_slot_deep_copy(upb_fieldtype_t type, VALUE type_class, void* to,
+                           void* from);
 bool native_slot_eq(upb_fieldtype_t type, VALUE type_class, void* mem1,
                     void* mem2);
 

ruby/ext/google/protobuf_c/repeated_field.c

@@ -378,7 +378,7 @@ VALUE RepeatedField_deep_copy(VALUE _self) {
   for (i = 0; i < self->size; i++, off += elem_size) {
     void* to_mem = (uint8_t *)new_rptfield_self->elements + off;
     void* from_mem = (uint8_t *)self->elements + off;
-    native_slot_deep_copy(field_type, to_mem, from_mem);
+    native_slot_deep_copy(field_type, self->field_type_class, to_mem, from_mem);
     new_rptfield_self->size++;
   }
 

ruby/ext/google/protobuf_c/storage.c

@@ -297,12 +297,15 @@ VALUE native_slot_get(upb_fieldtype_t type,
       return DEREF(memory, VALUE);
     case UPB_TYPE_MESSAGE: {
       VALUE val = DEREF(memory, VALUE);
+
+      // Lazily expand wrapper type if necessary.
       int type = TYPE(val);
       if (type != T_DATA && type != T_NIL) {
         // This must be a wrapper type.
         val = ruby_wrapper_type(type_class, val);
         DEREF(memory, VALUE) = val;
       }
+
       return val;
     }
     case UPB_TYPE_ENUM: {
@@ -381,7 +384,8 @@ void native_slot_dup(upb_fieldtype_t type, void* to, void* from) {
   memcpy(to, from, native_slot_size(type));
 }
 
-void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from) {
+void native_slot_deep_copy(upb_fieldtype_t type, VALUE type_class, void* to,
+                           void* from) {
   switch (type) {
     case UPB_TYPE_STRING:
     case UPB_TYPE_BYTES: {
@@ -391,7 +395,7 @@ void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from) {
       break;
     }
     case UPB_TYPE_MESSAGE: {
-      VALUE from_val = DEREF(from, VALUE);
+      VALUE from_val = native_slot_get(type, type_class, from);
       DEREF(to, VALUE) = (from_val != Qnil) ?
           Message_deep_copy(from_val) : Qnil;
       break;
@@ -1035,7 +1039,9 @@ void layout_deep_copy(MessageLayout* layout, void* to, void* from) {
       if (slot_read_oneof_case(layout, from, oneof) ==
           upb_fielddef_number(field)) {
         *to_oneof_case = *from_oneof_case;
-        native_slot_deep_copy(upb_fielddef_type(field), to_memory, from_memory);
+        native_slot_deep_copy(upb_fielddef_type(field),
+                              field_type_class(layout, field), to_memory,
+                              from_memory);
       }
     } else if (is_map_field(field)) {
       DEREF(to_memory, VALUE) =
@@ -1049,7 +1055,9 @@ void layout_deep_copy(MessageLayout* layout, void* to, void* from) {
         slot_set_hasbit(layout, to, field);
       }
 
-      native_slot_deep_copy(upb_fielddef_type(field), to_memory, from_memory);
+      native_slot_deep_copy(upb_fielddef_type(field),
+                            field_type_class(layout, field), to_memory,
+                            from_memory);
     }
   }
 }

ruby/tests/basic.rb

@@ -234,6 +234,48 @@ module BasicTest
       m.map_string_int32['aaa'] = 3
     end
 
+    def test_map_wrappers
+      run_asserts = ->(m) {
+        assert_equal 2.0, m.map_double[0].value
+        assert_equal 4.0, m.map_float[0].value
+        assert_equal 3, m.map_int32[0].value
+        assert_equal 4, m.map_int64[0].value
+        assert_equal 5, m.map_uint32[0].value
+        assert_equal 6, m.map_uint64[0].value
+        assert_equal true, m.map_bool[0].value
+        assert_equal 'str', m.map_string[0].value
+        assert_equal 'fun', m.map_bytes[0].value
+      }
+
+      m = proto_module::Wrapper.new(
+        map_double: {0 => Google::Protobuf::DoubleValue.new(value: 2.0)},
+        map_float: {0 => Google::Protobuf::FloatValue.new(value: 4.0)},
+        map_int32: {0 => Google::Protobuf::Int32Value.new(value: 3)},
+        map_int64: {0 => Google::Protobuf::Int64Value.new(value: 4)},
+        map_uint32: {0 => Google::Protobuf::UInt32Value.new(value: 5)},
+        map_uint64: {0 => Google::Protobuf::UInt64Value.new(value: 6)},
+        map_bool: {0 => Google::Protobuf::BoolValue.new(value: true)},
+        map_string: {0 => Google::Protobuf::StringValue.new(value: 'str')},
+        map_bytes: {0 => Google::Protobuf::BytesValue.new(value: 'fun')},
+      )
+
+      run_asserts.call(m)
+      serialized = proto_module::Wrapper::encode(m)
+      m2 = proto_module::Wrapper::decode(serialized)
+      run_asserts.call(m2)
+
+      # Test the case where we are serializing directly from the parsed form
+      # (before anything lazy is materialized).
+      m3 = proto_module::Wrapper::decode(serialized)
+      serialized2 = proto_module::Wrapper::encode(m3)
+      m4 = proto_module::Wrapper::decode(serialized2)
+      run_asserts.call(m4)
+
+      # Test that the lazy form compares equal to the expanded form.
+      m5 = proto_module::Wrapper::decode(serialized2)
+      assert_equal m5, m
+    end
+
     def test_concurrent_decoding
       o = Outer.new
       o.items[0] = Inner.new

ruby/tests/basic_test.proto

@@ -140,6 +140,18 @@ message Wrapper {
   repeated google.protobuf.BoolValue repeated_bool = 17;
   repeated google.protobuf.StringValue repeated_string = 18;
   repeated google.protobuf.BytesValue repeated_bytes = 19;
+
+  // Wrappers as map keys don't really make sense, but we still need to make
+  // sure they work and don't crash.
+  map<int32, google.protobuf.DoubleValue> map_double = 21;
+  map<int32, google.protobuf.FloatValue> map_float = 22;
+  map<int32, google.protobuf.Int32Value> map_int32 = 23;
+  map<int32, google.protobuf.Int64Value> map_int64 = 24;
+  map<int32, google.protobuf.UInt32Value> map_uint32 = 25;
+  map<int32, google.protobuf.UInt64Value> map_uint64 = 26;
+  map<int32, google.protobuf.BoolValue> map_bool = 27;
+  map<int32, google.protobuf.StringValue> map_string = 28;
+  map<int32, google.protobuf.BytesValue> map_bytes = 29;
 }
 
 message TimeMessage {

ruby/tests/common_tests.rb

@@ -1334,6 +1334,81 @@ module CommonTests
     assert_equal m5, m
   end
 
+  def test_repeated_wrappers
+    run_asserts = ->(m) {
+      assert_equal 2.0, m.repeated_double[0].value
+      assert_equal 4.0, m.repeated_float[0].value
+      assert_equal 3, m.repeated_int32[0].value
+      assert_equal 4, m.repeated_int64[0].value
+      assert_equal 5, m.repeated_uint32[0].value
+      assert_equal 6, m.repeated_uint64[0].value
+      assert_equal true, m.repeated_bool[0].value
+      assert_equal 'str', m.repeated_string[0].value
+      assert_equal 'fun', m.repeated_bytes[0].value
+    }
+
+    m = proto_module::Wrapper.new(
+      repeated_double: [Google::Protobuf::DoubleValue.new(value: 2.0)],
+      repeated_float: [Google::Protobuf::FloatValue.new(value: 4.0)],
+      repeated_int32: [Google::Protobuf::Int32Value.new(value: 3)],
+      repeated_int64: [Google::Protobuf::Int64Value.new(value: 4)],
+      repeated_uint32: [Google::Protobuf::UInt32Value.new(value: 5)],
+      repeated_uint64: [Google::Protobuf::UInt64Value.new(value: 6)],
+      repeated_bool: [Google::Protobuf::BoolValue.new(value: true)],
+      repeated_string: [Google::Protobuf::StringValue.new(value: 'str')],
+      repeated_bytes: [Google::Protobuf::BytesValue.new(value: 'fun')],
+    )
+
+    run_asserts.call(m)
+    serialized = proto_module::Wrapper::encode(m)
+    m2 = proto_module::Wrapper::decode(serialized)
+    run_asserts.call(m2)
+
+    # Test the case where we are serializing directly from the parsed form
+    # (before anything lazy is materialized).
+    m3 = proto_module::Wrapper::decode(serialized)
+    serialized2 = proto_module::Wrapper::encode(m3)
+    m4 = proto_module::Wrapper::decode(serialized2)
+    run_asserts.call(m4)
+
+    # Test that the lazy form compares equal to the expanded form.
+    m5 = proto_module::Wrapper::decode(serialized2)
+    assert_equal m5, m
+  end
+
+  def test_top_level_wrappers
+    # We don't expect anyone to do this, but we should also make sure it does
+    # the right thing.
+    run_test = ->(klass, val) {
+      m = klass.new(value: val)
+      serialized = klass::encode(m)
+      m2 = klass::decode(serialized)
+      assert_equal m, m2
+
+      # Encode directly from lazy form.
+      serialized2 = klass::encode(m2)
+
+      assert_equal m, m2
+      assert_equal serialized, serialized2
+
+      serialized_json = klass::encode_json(m)
+
+      # This is nonsensical to do and does not work.  There is no good reason
+      # to parse a wrapper type directly.
+      assert_raise(RuntimeError) { klass::decode_json(serialized_json) }
+    }
+
+    run_test.call(Google::Protobuf::DoubleValue, 2.0)
+    run_test.call(Google::Protobuf::FloatValue, 4.0)
+    run_test.call(Google::Protobuf::Int32Value, 3)
+    run_test.call(Google::Protobuf::Int64Value, 4)
+    run_test.call(Google::Protobuf::UInt32Value, 5)
+    run_test.call(Google::Protobuf::UInt64Value, 6)
+    run_test.call(Google::Protobuf::BoolValue, true)
+    run_test.call(Google::Protobuf::StringValue, 'str')
+    run_test.call(Google::Protobuf::BytesValue, 'fun')
+  end
+
   def test_wrapper_setters_as_value
     run_asserts = ->(m) {
       m.double_as_value = 4.8