Skip to content

Commit

Permalink
Ruby: assigning 'nil' to submessage should clear the field. (#7397)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
haberman authored Apr 20, 2020
1 parent 81573b9 commit 1895045
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 17 deletions.
16 changes: 10 additions & 6 deletions ruby/ext/google/protobuf_c/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down
42 changes: 31 additions & 11 deletions ruby/ext/google/protobuf_c/storage.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -724,20 +725,23 @@ 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));
}

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,
Expand Down Expand Up @@ -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);
}
}
}

Expand Down Expand Up @@ -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;
Expand Down
11 changes: 11 additions & 0 deletions ruby/tests/basic_proto2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down

0 comments on commit 1895045

Please sign in to comment.