From ad2a557d9608e375f6f5a0fefc7442cebe47bf63 Mon Sep 17 00:00:00 2001 From: Eric Salo Date: Thu, 3 Aug 2023 14:54:49 -0700 Subject: [PATCH] Fix an upb bug When build a FileDescriptorProto into pool, FieldDescriptorProto.type may not set if type_name is set. Runtime Change. To make copybara happy, tests will be added in a separate change PiperOrigin-RevId: 553598520 --- upb/reflection/field_def.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/upb/reflection/field_def.c b/upb/reflection/field_def.c index b7bc264c4c..06851cf087 100644 --- a/upb/reflection/field_def.c +++ b/upb/reflection/field_def.c @@ -592,16 +592,14 @@ static void _upb_FieldDef_Create(upb_DefBuilder* ctx, const char* prefix, f->full_name, (int)f->type_); } } + } else if (has_type_name) { + f->type_ = + UPB_FIELD_TYPE_UNSPECIFIED; // We'll assign this in resolve_fielddef() } - if (!has_type && has_type_name) { - f->type_ = - UPB_FIELD_TYPE_UNSPECIFIED; // We'll assign this in resolve_subdef() - } else { - if (f->type_ < kUpb_FieldType_Double || f->type_ > kUpb_FieldType_SInt64) { - _upb_DefBuilder_Errf(ctx, "invalid type for field %s (%d)", f->full_name, - f->type_); - } + if (f->type_ < kUpb_FieldType_Double || f->type_ > kUpb_FieldType_SInt64) { + _upb_DefBuilder_Errf(ctx, "invalid type for field %s (%d)", f->full_name, + f->type_); } if (f->label_ < kUpb_Label_Optional || f->label_ > kUpb_Label_Repeated) { @@ -650,15 +648,14 @@ static void _upb_FieldDef_Create(upb_DefBuilder* ctx, const char* prefix, f->is_packed = UPB_DESC(FieldOptions_packed)(f->opts); } else { // Repeated fields default to packed for proto3 only. - f->is_packed = has_type && upb_FieldDef_IsPrimitive(f) && + f->is_packed = upb_FieldDef_IsPrimitive(f) && f->label_ == kUpb_Label_Repeated && upb_FileDef_Syntax(f->file) == kUpb_Syntax_Proto3; } f->has_presence = (!upb_FieldDef_IsRepeated(f)) && - (f->type_ == kUpb_FieldType_Message || f->type_ == kUpb_FieldType_Group || - upb_FieldDef_ContainingOneof(f) || + (upb_FieldDef_IsSubMessage(f) || upb_FieldDef_ContainingOneof(f) || (upb_FileDef_Syntax(f->file) == kUpb_Syntax_Proto2)); } @@ -766,22 +763,16 @@ static void resolve_subdef(upb_DefBuilder* ctx, const char* prefix, case UPB_DEFTYPE_ENUM: f->sub.enumdef = def; f->type_ = kUpb_FieldType_Enum; - if (!UPB_DESC(FieldOptions_has_packed)(f->opts)) { - f->is_packed = f->label_ == kUpb_Label_Repeated && - upb_FileDef_Syntax(f->file) == kUpb_Syntax_Proto3; - } break; case UPB_DEFTYPE_MSG: f->sub.msgdef = def; f->type_ = kUpb_FieldType_Message; // It appears there is no way of // this being a group. - f->has_presence = !upb_FieldDef_IsRepeated(f); break; default: _upb_DefBuilder_Errf(ctx, "Couldn't resolve type name for field %s", f->full_name); } - break; } case kUpb_FieldType_Message: case kUpb_FieldType_Group: