Ruby: assigning 'nil' to submessage should clear the field. (#7397)

Previously if you assigned 'nil' to a submessage in proto2
the field would be set to 'nil' but would still have its hasbit
set. This was a clear bug so I'm fixing it outright, even though
it is an observable behavior change.

ruby/ext/google/protobuf_c/message.c

@@ -608,6 +608,12 @@ VALUE Message_to_h(VALUE _self) {
   VALUE hash;
   upb_msg_field_iter it;
   TypedData_Get_Struct(_self, MessageHeader, &Message_type, self);
+  // We currently have a few behaviors that are specific to proto2.
+  // This is unfortunate, we should key behaviors off field attributes (like
+  // whether a field has presence), not proto2 vs. proto3. We should see if we
+  // can change this without breaking users.
+  bool is_proto2 =
+      upb_msgdef_syntax(self->descriptor->msgdef) == UPB_SYNTAX_PROTO2;
 
   hash = rb_hash_new();
 
@@ -618,10 +624,9 @@ VALUE Message_to_h(VALUE _self) {
     VALUE msg_value;
     VALUE msg_key;
 
-    // For proto2, do not include fields which are not set.
-    if (upb_msgdef_syntax(self->descriptor->msgdef) == UPB_SYNTAX_PROTO2 &&
-       field_contains_hasbit(self->descriptor->layout, field) &&
-       !layout_has(self->descriptor->layout, Message_data(self), field)) {
+    // Do not include fields that are not present (oneof or optional fields).
+    if (is_proto2 && upb_fielddef_haspresence(field) &&
+        !layout_has(self->descriptor->layout, Message_data(self), field)) {
       continue;
     }
 
@@ -631,8 +636,7 @@ VALUE Message_to_h(VALUE _self) {
       msg_value = Map_to_h(msg_value);
     } else if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) {
       msg_value = RepeatedField_to_ary(msg_value);
-      if (upb_msgdef_syntax(self->descriptor->msgdef) == UPB_SYNTAX_PROTO2 &&
-          RARRAY_LEN(msg_value) == 0) {
+      if (is_proto2 && RARRAY_LEN(msg_value) == 0) {
         continue;
       }
 

ruby/ext/google/protobuf_c/storage.c

@@ -517,7 +517,8 @@ void create_layout(Descriptor* desc) {
        !upb_msg_field_done(&it);
        upb_msg_field_next(&it)) {
     const upb_fielddef* field = upb_msg_iter_field(&it);
-    if (upb_fielddef_haspresence(field)) {
+    if (upb_fielddef_haspresence(field) &&
+        !upb_fielddef_containingoneof(field)) {
       layout->fields[upb_fielddef_index(field)].hasbit = hasbit++;
     } else {
       layout->fields[upb_fielddef_index(field)].hasbit =
@@ -724,11 +725,8 @@ static void slot_clear_hasbit(MessageLayout* layout,
 static bool slot_is_hasbit_set(MessageLayout* layout,
                             const void* storage,
                             const upb_fielddef* field) {
+  assert(field_contains_hasbit(layout, field));
   size_t hasbit = layout->fields[upb_fielddef_index(field)].hasbit;
-  if (hasbit == MESSAGE_FIELD_NO_HASBIT) {
-    return false;
-  }
-
   return DEREF_OFFSET(
       (uint8_t*)storage, hasbit / 8, char) & (1 << (hasbit % 8));
 }
@@ -736,8 +734,14 @@ static bool slot_is_hasbit_set(MessageLayout* layout,
 VALUE layout_has(MessageLayout* layout,
                  const void* storage,
                  const upb_fielddef* field) {
-  assert(field_contains_hasbit(layout, field));
-  return slot_is_hasbit_set(layout, storage, field) ? Qtrue : Qfalse;
+  assert(upb_fielddef_haspresence(field));
+  const upb_oneofdef* oneof = upb_fielddef_containingoneof(field);
+  if (oneof) {
+    uint32_t oneof_case = slot_read_oneof_case(layout, storage, oneof);
+    return oneof_case == upb_fielddef_number(field);
+  } else {
+    return slot_is_hasbit_set(layout, storage, field) ? Qtrue : Qfalse;
+  }
 }
 
 void layout_clear(MessageLayout* layout,
@@ -953,7 +957,16 @@ void layout_set(MessageLayout* layout,
 
   if (layout->fields[upb_fielddef_index(field)].hasbit !=
       MESSAGE_FIELD_NO_HASBIT) {
-    slot_set_hasbit(layout, storage, field);
+    if (val == Qnil) {
+      // No other field type has a hasbit and allows nil assignment.
+      if (upb_fielddef_type(field) != UPB_TYPE_MESSAGE) {
+        fprintf(stderr, "field: %s\n", upb_fielddef_fullname(field));
+      }
+      assert(upb_fielddef_type(field) == UPB_TYPE_MESSAGE);
+      slot_clear_hasbit(layout, storage, field);
+    } else {
+      slot_set_hasbit(layout, storage, field);
+    }
   }
 }
 
@@ -1095,9 +1108,16 @@ VALUE layout_eq(MessageLayout* layout, void* msg1, void* msg2) {
         return Qfalse;
       }
     } else {
-      if (slot_is_hasbit_set(layout, msg1, field) !=
-              slot_is_hasbit_set(layout, msg2, field) ||
-          !native_slot_eq(upb_fielddef_type(field),
+      if (field_contains_hasbit(layout, field) &&
+          slot_is_hasbit_set(layout, msg1, field) !=
+              slot_is_hasbit_set(layout, msg2, field)) {
+        // TODO(haberman): I don't think we should actually care about hasbits
+        // here: an unset default should be able to equal a set default. But we
+        // can address this later (will also have to make sure defaults are
+        // being properly set when hasbit is clear).
+        return Qfalse;
+      }
+      if (!native_slot_eq(upb_fielddef_type(field),
                           field_type_class(layout, field), msg1_memory,
                           msg2_memory)) {
         return Qfalse;

ruby/tests/basic_proto2.rb

@@ -197,6 +197,17 @@ module BasicTestProto2
       assert !m.has_my_oneof?
     end
 
+    def test_assign_nil
+      m = TestMessageDefaults.new
+      m.optional_msg = TestMessage2.new(:foo => 42)
+
+      assert_equal TestMessage2.new(:foo => 42), m.optional_msg
+      assert m.has_optional_msg?
+      m.optional_msg = nil
+      assert_equal nil, m.optional_msg
+      assert !m.has_optional_msg?
+    end
+
     def test_initialization_map_errors
       e = assert_raise ArgumentError do
         TestMessage.new(:hello => "world")