diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index f57501cc65c..db5d856d751 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/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; } diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 739c0a77650..56f95216699 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/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; diff --git a/ruby/tests/basic_proto2.rb b/ruby/tests/basic_proto2.rb index 4c7ddd55b98..986a31b2293 100755 --- a/ruby/tests/basic_proto2.rb +++ b/ruby/tests/basic_proto2.rb @@ -197,6 +197,17 @@ def test_set_clear_defaults 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")