Hide upb_MiniTableField.submsg_index with new `UPB_PRIVATE()` macro
The fields of upb_MiniTableField are intended to be internal-only, accessed only through public functions like `upb_MiniTable_GetSubMessageTable()`. But over time, clients have started accessing many of these fields directly. This is an easy mistake to make, as there is no clear signal that the fields should not be used in applications. This makes the implementation difficult to change without breaking users.
The new `UPB_PRIVATE()` macro appends an unpredictable string to each private symbol. This makes it very difficult to accidentally use a private symbol, since users would need to write something like `field->submsg_index_dont_copy_me__upb_internal_use_only`. This is still possible to do, but it leaves a clear wart in the code showing that an an encapsulation break has occurred. The `UPB_PRIVATE()` macro itself is defined in `port/def.inc`, which users cannot include directly.
Once we land this, more such CLs will follow for the other fields of `upb_MiniTable*`. We will add inline functions as needed to provide the semantic functionality needed by users.
PiperOrigin-RevId: 523166901
diff --git a/BUILD b/BUILD
index 24bde07..3f06ac3 100644
--- a/BUILD
+++ b/BUILD
@@ -329,6 +329,7 @@
":collections",
":message_accessors",
":mini_table_internal",
+ ":port",
":upb",
"//upb/test:test_messages_proto2_upb_proto",
"//upb/test:test_messages_proto3_upb_proto",
diff --git a/upb/message/accessors.c b/upb/message/accessors.c
index c10ffec..dc3ea01 100644
--- a/upb/message/accessors.c
+++ b/upb/message/accessors.c
@@ -199,7 +199,8 @@
upb_Message* message = NULL;
// Callers should check that message is not set first before calling
// PromotoUnknownToMessage.
- UPB_ASSERT(mini_table->subs[field->submsg_index].submsg == sub_mini_table);
+ UPB_ASSERT(upb_MiniTable_GetSubMessageTable(mini_table, field) ==
+ sub_mini_table);
bool is_oneof = _upb_MiniTableField_InOneOf(field);
if (!is_oneof || _upb_getoneofcase_field(msg, field) == field->number) {
UPB_ASSERT(upb_Message_GetMessage(msg, field, NULL) == NULL);
@@ -286,7 +287,7 @@
upb_Message* map_entry_message,
upb_Arena* arena) {
const upb_MiniTable* map_entry_mini_table =
- mini_table->subs[field->submsg_index].submsg;
+ mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(map_entry_mini_table);
UPB_ASSERT(map_entry_mini_table->field_count == 2);
const upb_MiniTableField* map_entry_key_field =
@@ -311,7 +312,7 @@
upb_Message* msg, const upb_MiniTable* mini_table,
const upb_MiniTableField* field, int decode_options, upb_Arena* arena) {
const upb_MiniTable* map_entry_mini_table =
- mini_table->subs[field->submsg_index].submsg;
+ mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(map_entry_mini_table);
UPB_ASSERT(map_entry_mini_table);
UPB_ASSERT(map_entry_mini_table->field_count == 2);
diff --git a/upb/message/accessors.h b/upb/message/accessors.h
index 74ca1f2..141a97d 100644
--- a/upb/message/accessors.h
+++ b/upb/message/accessors.h
@@ -552,7 +552,7 @@
UPB_ASSUME(!upb_IsRepeatedOrMap(field));
UPB_ASSUME(_upb_MiniTableField_GetRep(field) ==
UPB_SIZE(kUpb_FieldRep_4Byte, kUpb_FieldRep_8Byte));
- UPB_ASSERT(mini_table->subs[field->submsg_index].submsg);
+ UPB_ASSERT(mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg);
_upb_Message_SetNonExtensionField(msg, field, &sub_message);
}
@@ -564,7 +564,7 @@
upb_Message* sub_message = *UPB_PTR_AT(msg, field->offset, upb_Message*);
if (!sub_message) {
const upb_MiniTable* sub_mini_table =
- mini_table->subs[field->submsg_index].submsg;
+ mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(sub_mini_table);
sub_message = _upb_Message_New(sub_mini_table, arena);
*UPB_PTR_AT(msg, field->offset, upb_Message*) = sub_message;
diff --git a/upb/message/accessors_test.cc b/upb/message/accessors_test.cc
index a0dece4..910a700 100644
--- a/upb/message/accessors_test.cc
+++ b/upb/message/accessors_test.cc
@@ -49,6 +49,9 @@
#include "upb/wire/common.h"
#include "upb/wire/decode.h"
+// Must be last
+#include "upb/port/def.inc"
+
namespace {
// Proto2 test messages field numbers used for reflective access.
@@ -580,10 +583,10 @@
// Initialize sub table to null. Not using upb_MiniTable_SetSubMessage
// since it checks ->ext on parameter.
upb_MiniTableSub* sub = const_cast<upb_MiniTableSub*>(
- &table->subs[table->fields[1].submsg_index]);
+ &table->subs[table->fields[1].UPB_PRIVATE(submsg_index)]);
sub->submsg = nullptr;
sub = const_cast<upb_MiniTableSub*>(
- &table->subs[table->fields[2].submsg_index]);
+ &table->subs[table->fields[2].UPB_PRIVATE(submsg_index)]);
sub->submsg = nullptr;
return table;
}
@@ -605,10 +608,10 @@
// Initialize sub table to null. Not using upb_MiniTable_SetSubMessage
// since it checks ->ext on parameter.
upb_MiniTableSub* sub = const_cast<upb_MiniTableSub*>(
- &table->subs[table->fields[1].submsg_index]);
+ &table->subs[table->fields[1].UPB_PRIVATE(submsg_index)]);
sub->submsg = nullptr;
sub = const_cast<upb_MiniTableSub*>(
- &table->subs[table->fields[2].submsg_index]);
+ &table->subs[table->fields[2].UPB_PRIVATE(submsg_index)]);
sub->submsg = nullptr;
return table;
}
diff --git a/upb/message/copy.c b/upb/message/copy.c
index 570e6c1..3f2c607 100644
--- a/upb/message/copy.c
+++ b/upb/message/copy.c
@@ -101,7 +101,7 @@
while (upb_Map_Next(map, &key, &val, &iter)) {
const upb_MiniTableField* value_field = &map_entry_table->fields[1];
const upb_MiniTable* value_sub =
- (value_field->submsg_index != kUpb_NoSub)
+ (value_field->UPB_PRIVATE(submsg_index) != kUpb_NoSub)
? upb_MiniTable_GetSubMessageTable(map_entry_table, value_field)
: NULL;
upb_CType value_field_type = upb_MiniTableField_CType(value_field);
@@ -122,7 +122,7 @@
upb_Message* clone,
upb_Arena* arena) {
const upb_MiniTable* map_entry_table =
- mini_table->subs[field->submsg_index].submsg;
+ mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(map_entry_table);
const upb_MiniTableField* key_field = &map_entry_table->fields[0];
@@ -166,7 +166,7 @@
_upb_MiniTableField_CheckIsArray(field);
upb_Array* cloned_array = upb_Array_DeepClone(
array, upb_MiniTableField_CType(field),
- field->submsg_index != kUpb_NoSub
+ field->UPB_PRIVATE(submsg_index) != kUpb_NoSub
? upb_MiniTable_GetSubMessageTable(mini_table, field)
: NULL,
arena);
diff --git a/upb/mini_table/common.h b/upb/mini_table/common.h
index 5169d1c..4fc9473 100644
--- a/upb/mini_table/common.h
+++ b/upb/mini_table/common.h
@@ -112,14 +112,27 @@
}
}
+// Returns the MiniTable for this message field. If the field is unlinked,
+// returns NULL.
UPB_API_INLINE const upb_MiniTable* upb_MiniTable_GetSubMessageTable(
const upb_MiniTable* mini_table, const upb_MiniTableField* field) {
- return mini_table->subs[field->submsg_index].submsg;
+ UPB_ASSERT(upb_MiniTableField_CType(field) == kUpb_CType_Message);
+ return mini_table->subs[field->UPB_PRIVATE(submsg_index)].submsg;
}
+// Returns the MiniTableEnum for this enum field. If the field is unlinked,
+// returns NULL.
UPB_API_INLINE const upb_MiniTableEnum* upb_MiniTable_GetSubEnumTable(
const upb_MiniTable* mini_table, const upb_MiniTableField* field) {
- return mini_table->subs[field->submsg_index].subenum;
+ UPB_ASSERT(upb_MiniTableField_CType(field) == kUpb_CType_Enum);
+ return mini_table->subs[field->UPB_PRIVATE(submsg_index)].subenum;
+}
+
+// Returns true if this MiniTable field is linked to a MiniTable for the
+// sub-message.
+UPB_API_INLINE bool upb_MiniTable_MessageFieldIsLinked(
+ const upb_MiniTable* mini_table, const upb_MiniTableField* field) {
+ return upb_MiniTable_GetSubMessageTable(mini_table, field) != NULL;
}
// If this field is in a oneof, returns the first field in the oneof.
diff --git a/upb/mini_table/decode.c b/upb/mini_table/decode.c
index 72f6dd0..71e1589 100644
--- a/upb/mini_table/decode.c
+++ b/upb/mini_table/decode.c
@@ -172,9 +172,9 @@
}
if (upb_MiniTable_HasSub(field, msg_modifiers)) {
- field->submsg_index = sub_count ? (*sub_count)++ : 0;
+ field->UPB_PRIVATE(submsg_index) = sub_count ? (*sub_count)++ : 0;
} else {
- field->submsg_index = kUpb_NoSub;
+ field->UPB_PRIVATE(submsg_index) = kUpb_NoSub;
}
if (upb_MtDecoder_FieldIsPackable(field) &&
@@ -1046,7 +1046,8 @@
return false;
}
- upb_MiniTableSub* table_sub = (void*)&table->subs[field->submsg_index];
+ upb_MiniTableSub* table_sub =
+ (void*)&table->subs[field->UPB_PRIVATE(submsg_index)];
table_sub->submsg = sub;
return true;
}
@@ -1058,7 +1059,8 @@
(uintptr_t)(table->fields + table->field_count));
UPB_ASSERT(sub);
- upb_MiniTableSub* table_sub = (void*)&table->subs[field->submsg_index];
+ upb_MiniTableSub* table_sub =
+ (void*)&table->subs[field->UPB_PRIVATE(submsg_index)];
table_sub->subenum = sub;
return true;
}
diff --git a/upb/mini_table/field_internal.h b/upb/mini_table/field_internal.h
index a258c10..0e36a07 100644
--- a/upb/mini_table/field_internal.h
+++ b/upb/mini_table/field_internal.h
@@ -40,7 +40,11 @@
uint32_t number;
uint16_t offset;
int16_t presence; // If >0, hasbit_index. If <0, ~oneof_index
- uint16_t submsg_index; // kUpb_NoSub if descriptortype != MESSAGE/GROUP/ENUM
+
+ // Indexes into `upb_MiniTable.subs`
+ // Will be set to `kUpb_NoSub` if `descriptortype` != MESSAGE/GROUP/ENUM
+ uint16_t UPB_PRIVATE(submsg_index);
+
uint8_t descriptortype;
// upb_FieldMode | upb_LabelFlags | (upb_FieldRep << kUpb_FieldRep_Shift)
diff --git a/upb/port/def.inc b/upb/port/def.inc
index 7c36a03..c1cf36f 100644
--- a/upb/port/def.inc
+++ b/upb/port/def.inc
@@ -200,6 +200,8 @@
/* UPB_PTRADD(ptr, ofs): add pointer while avoiding "NULL + 0" UB */
#define UPB_PTRADD(ptr, ofs) ((ofs) ? (ptr) + (ofs) : (ptr))
+#define UPB_PRIVATE(x) x##_dont_copy_me__upb_internal_use_only
+
/* Configure whether fasttable is switched on or not. *************************/
#ifdef __has_attribute
diff --git a/upb/port/undef.inc b/upb/port/undef.inc
index 12e05d2..e1752f7 100644
--- a/upb/port/undef.inc
+++ b/upb/port/undef.inc
@@ -70,3 +70,4 @@
#undef UPB_IS_GOOGLE3
#undef UPB_ATOMIC
#undef UPB_USE_C11_ATOMICS
+#undef UPB_PRIVATE
diff --git a/upb/wire/decode.c b/upb/wire/decode.c
index c2ab7e0..4a33689 100644
--- a/upb/wire/decode.c
+++ b/upb/wire/decode.c
@@ -220,7 +220,7 @@
static upb_Message* _upb_Decoder_NewSubMessage(
upb_Decoder* d, const upb_MiniTableSub* subs,
const upb_MiniTableField* field) {
- const upb_MiniTable* subl = subs[field->submsg_index].submsg;
+ const upb_MiniTable* subl = subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(subl);
upb_Message* msg = _upb_Message_New(subl, &d->arena);
if (!msg) _upb_Decoder_ErrorJmp(d, kUpb_DecodeStatus_OutOfMemory);
@@ -259,7 +259,7 @@
upb_Decoder* d, const char* ptr, upb_Message* submsg,
const upb_MiniTableSub* subs, const upb_MiniTableField* field, int size) {
int saved_delta = upb_EpsCopyInputStream_PushLimit(&d->input, ptr, size);
- const upb_MiniTable* subl = subs[field->submsg_index].submsg;
+ const upb_MiniTable* subl = subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(subl);
ptr = _upb_Decoder_RecurseSubMessage(d, ptr, submsg, subl, DECODE_NOGROUP);
upb_EpsCopyInputStream_PopLimit(&d->input, ptr, saved_delta);
@@ -290,7 +290,7 @@
static const char* _upb_Decoder_DecodeKnownGroup(
upb_Decoder* d, const char* ptr, upb_Message* submsg,
const upb_MiniTableSub* subs, const upb_MiniTableField* field) {
- const upb_MiniTable* subl = subs[field->submsg_index].submsg;
+ const upb_MiniTable* subl = subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(subl);
return _upb_Decoder_DecodeGroup(d, ptr, submsg, subl, field->number);
}
@@ -354,7 +354,7 @@
const upb_MiniTableSub* subs,
const upb_MiniTableField* field,
wireval* val) {
- const upb_MiniTableEnum* e = subs[field->submsg_index].subenum;
+ const upb_MiniTableEnum* e = subs[field->UPB_PRIVATE(submsg_index)].subenum;
if (!_upb_Decoder_CheckEnum(d, ptr, msg, e, field, val)) return ptr;
void* mem = UPB_PTR_AT(_upb_array_ptr(arr), arr->size * 4, void);
arr->size++;
@@ -425,7 +425,7 @@
upb_Decoder* d, const char* ptr, upb_Message* msg, upb_Array* arr,
const upb_MiniTableSub* subs, const upb_MiniTableField* field,
wireval* val) {
- const upb_MiniTableEnum* e = subs[field->submsg_index].subenum;
+ const upb_MiniTableEnum* e = subs[field->UPB_PRIVATE(submsg_index)].subenum;
int saved_limit = upb_EpsCopyInputStream_PushLimit(&d->input, ptr, val->size);
char* out = UPB_PTR_AT(_upb_array_ptr(arr), arr->size * 4, void);
while (!_upb_Decoder_IsDone(d, &ptr)) {
@@ -586,7 +586,7 @@
upb_Map* map = *map_p;
upb_MapEntry ent;
UPB_ASSERT(upb_MiniTableField_Type(field) == kUpb_FieldType_Message);
- const upb_MiniTable* entry = subs[field->submsg_index].submsg;
+ const upb_MiniTable* entry = subs[field->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(entry->field_count == 2);
UPB_ASSERT(!upb_IsRepeatedOrMap(&entry->fields[0]));
@@ -647,7 +647,8 @@
int type = field->descriptortype;
if (UPB_UNLIKELY(op == kUpb_DecodeOp_Enum) &&
- !_upb_Decoder_CheckEnum(d, ptr, msg, subs[field->submsg_index].subenum,
+ !_upb_Decoder_CheckEnum(d, ptr, msg,
+ subs[field->UPB_PRIVATE(submsg_index)].subenum,
field, val)) {
return ptr;
}
@@ -963,7 +964,7 @@
int* op) {
// If sub-message is not linked, treat as unknown.
if (field->mode & kUpb_LabelFlags_IsExtension) return;
- const upb_MiniTableSub* sub = &mt->subs[field->submsg_index];
+ const upb_MiniTableSub* sub = &mt->subs[field->UPB_PRIVATE(submsg_index)];
if (!sub->submsg) *op = kUpb_DecodeOp_UnknownField;
}
diff --git a/upb/wire/encode.c b/upb/wire/encode.c
index ef931a7..c75bf6f 100644
--- a/upb/wire/encode.c
+++ b/upb/wire/encode.c
@@ -263,7 +263,7 @@
case kUpb_FieldType_Group: {
size_t size;
void* submsg = *(void**)field_mem;
- const upb_MiniTable* subm = subs[f->submsg_index].submsg;
+ const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg;
if (submsg == NULL) {
return;
}
@@ -277,7 +277,7 @@
case kUpb_FieldType_Message: {
size_t size;
void* submsg = *(void**)field_mem;
- const upb_MiniTable* subm = subs[f->submsg_index].submsg;
+ const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg;
if (submsg == NULL) {
return;
}
@@ -366,7 +366,7 @@
case kUpb_FieldType_Group: {
const void* const* start = _upb_array_constptr(arr);
const void* const* ptr = start + arr->size;
- const upb_MiniTable* subm = subs[f->submsg_index].submsg;
+ const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg;
if (--e->depth == 0) encode_err(e, kUpb_EncodeStatus_MaxDepthExceeded);
do {
size_t size;
@@ -381,7 +381,7 @@
case kUpb_FieldType_Message: {
const void* const* start = _upb_array_constptr(arr);
const void* const* ptr = start + arr->size;
- const upb_MiniTable* subm = subs[f->submsg_index].submsg;
+ const upb_MiniTable* subm = subs[f->UPB_PRIVATE(submsg_index)].submsg;
if (--e->depth == 0) encode_err(e, kUpb_EncodeStatus_MaxDepthExceeded);
do {
size_t size;
@@ -420,7 +420,7 @@
const upb_MiniTableSub* subs,
const upb_MiniTableField* f) {
const upb_Map* map = *UPB_PTR_AT(msg, f->offset, const upb_Map*);
- const upb_MiniTable* layout = subs[f->submsg_index].submsg;
+ const upb_MiniTable* layout = subs[f->UPB_PRIVATE(submsg_index)].submsg;
UPB_ASSERT(layout->field_count == 2);
if (map == NULL) return;
diff --git a/upbc/protoc-gen-upb.cc b/upbc/protoc-gen-upb.cc
index 9fa67c9..71ebd7f 100644
--- a/upbc/protoc-gen-upb.cc
+++ b/upbc/protoc-gen-upb.cc
@@ -1115,7 +1115,7 @@
}
if (field.ctype() == kUpb_CType_Message) {
- uint64_t idx = mt_f->submsg_index;
+ uint64_t idx = mt_f->UPB_PRIVATE(submsg_index);
if (idx > 255) return false;
data |= idx << 16;
@@ -1263,9 +1263,9 @@
"{$0, $1, $2, $3, $4, $5}", field64->number,
ArchDependentSize(field32->offset, field64->offset),
ArchDependentSize(field32->presence, field64->presence),
- field64->submsg_index == kUpb_NoSub
+ field64->UPB_PRIVATE(submsg_index) == kUpb_NoSub
? "kUpb_NoSub"
- : absl::StrCat(field64->submsg_index).c_str(),
+ : absl::StrCat(field64->UPB_PRIVATE(submsg_index)).c_str(),
field64->descriptortype, GetModeInit(field32, field64));
}
}
@@ -1311,7 +1311,7 @@
for (int i = 0; i < mt_64->field_count; i++) {
const upb_MiniTableField* f = &mt_64->fields[i];
- if (f->submsg_index != kUpb_NoSub) {
+ if (f->UPB_PRIVATE(submsg_index) != kUpb_NoSub) {
subs.push_back(GetSub(message.FindFieldByNumber(f->number)));
}
}
