From cf3a6f5868222d31021e3835e5e7890a847f3d01 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 7 Nov 2023 12:22:10 -0800 Subject: [PATCH] Enabled editions support for upb generated code. This required enabling the feature in the code generator and fixing a few edge cases around label and type. Also added tests to verify the special cases, and to verify that required fields work as expected. PiperOrigin-RevId: 580263087 --- .../protobuf/compiler/allowlists/editions.cc | 1 + upb/reflection/field_def.c | 19 +++- upb/test/BUILD | 32 ++++++ upb/test/editions_test.cc | 62 +++++++++++ upb/test/editions_test.proto | 26 +++++ upb/util/BUILD | 6 +- upb/util/def_to_proto.c | 2 + upb/util/required_fields_editions_test.proto | 28 +++++ upb/util/required_fields_test.cc | 103 ++++++++++++------ upb_generator/file_layout.cc | 22 +++- upb_generator/file_layout.h | 8 +- upb_generator/plugin.h | 7 +- upb_generator/protoc-gen-upb.cc | 4 +- upb_generator/protoc-gen-upb_minitable.cc | 14 +-- 14 files changed, 277 insertions(+), 57 deletions(-) create mode 100644 upb/test/editions_test.cc create mode 100644 upb/test/editions_test.proto create mode 100644 upb/util/required_fields_editions_test.proto diff --git a/src/google/protobuf/compiler/allowlists/editions.cc b/src/google/protobuf/compiler/allowlists/editions.cc index 6ab81c0646..f883ece02a 100644 --- a/src/google/protobuf/compiler/allowlists/editions.cc +++ b/src/google/protobuf/compiler/allowlists/editions.cc @@ -20,6 +20,7 @@ static constexpr auto kEarlyEditionsFile = internal::MakeAllowlist( { // Intentionally left blank. "google/protobuf/", + "upb/", }, internal::AllowlistFlags::kMatchPrefix); diff --git a/upb/reflection/field_def.c b/upb/reflection/field_def.c index 5b67e209cc..0a4ae254eb 100644 --- a/upb/reflection/field_def.c +++ b/upb/reflection/field_def.c @@ -123,11 +123,26 @@ upb_CType upb_FieldDef_CType(const upb_FieldDef* f) { UPB_UNREACHABLE(); } -upb_FieldType upb_FieldDef_Type(const upb_FieldDef* f) { return f->type_; } +upb_FieldType upb_FieldDef_Type(const upb_FieldDef* f) { + // TODO: remove once we can deprecate kUpb_FieldType_Group. + if (f->type_ == kUpb_FieldType_Message && + UPB_DESC(FeatureSet_message_encoding)(f->resolved_features) == + UPB_DESC(FeatureSet_DELIMITED)) { + return kUpb_FieldType_Group; + } + return f->type_; +} uint32_t upb_FieldDef_Index(const upb_FieldDef* f) { return f->index_; } -upb_Label upb_FieldDef_Label(const upb_FieldDef* f) { return f->label_; } +upb_Label upb_FieldDef_Label(const upb_FieldDef* f) { + // TODO: remove once we can deprecate kUpb_Label_Required. + if (UPB_DESC(FeatureSet_field_presence)(f->resolved_features) == + UPB_DESC(FeatureSet_LEGACY_REQUIRED)) { + return kUpb_Label_Required; + } + return f->label_; +} uint32_t upb_FieldDef_Number(const upb_FieldDef* f) { return f->number_; } diff --git a/upb/test/BUILD b/upb/test/BUILD index 6ae451d029..b101237ddf 100644 --- a/upb/test/BUILD +++ b/upb/test/BUILD @@ -84,6 +84,24 @@ upb_c_proto_library( deps = [":test_proto"], ) +proto_library( + name = "editions_test_proto", + testonly = 1, + srcs = ["editions_test.proto"], +) + +upb_c_proto_library( + name = "editions_test_upb_c_proto", + testonly = 1, + deps = [":editions_test_proto"], +) + +upb_proto_reflection_library( + name = "editions_test_upb_proto_reflection", + testonly = 1, + deps = [":editions_test_proto"], +) + proto_library( name = "test_cpp_proto", srcs = ["test_cpp.proto"], @@ -166,6 +184,20 @@ cc_test( ], ) +cc_test( + name = "editions_test", + srcs = ["editions_test.cc"], + copts = UPB_DEFAULT_CPPOPTS, + deps = [ + ":editions_test_upb_c_proto", + ":editions_test_upb_proto_reflection", + "@com_google_googletest//:gtest_main", + "//upb:base", + "//upb:mem", + "//upb:reflection", + ], +) + cc_test( name = "test_cpp", srcs = ["test_cpp.cc"], diff --git a/upb/test/editions_test.cc b/upb/test/editions_test.cc new file mode 100644 index 0000000000..92f9922ebd --- /dev/null +++ b/upb/test/editions_test.cc @@ -0,0 +1,62 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google LLC. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file or at +// https://developers.google.com/open-source/licenses/bsd + +#include +#include "upb/base/descriptor_constants.h" +#include "upb/mem/arena.hpp" +#include "upb/reflection/def.hpp" +#include "upb/test/editions_test.upb.h" +#include "upb/test/editions_test.upbdefs.h" + +TEST(EditionsTest, PlainField) { + upb::DefPool defpool; + upb::MessageDefPtr md(upb_test_2023_EditionsMessage_getmsgdef(defpool.ptr())); + upb::FieldDefPtr f(md.FindFieldByName("plain_field")); + EXPECT_TRUE(f.has_presence()); +} + +TEST(EditionsTest, ImplicitPresenceField) { + upb::DefPool defpool; + upb::MessageDefPtr md(upb_test_2023_EditionsMessage_getmsgdef(defpool.ptr())); + upb::FieldDefPtr f(md.FindFieldByName("implicit_presence_field")); + EXPECT_FALSE(f.has_presence()); +} + +TEST(EditionsTest, DelimitedField) { + upb::DefPool defpool; + upb::MessageDefPtr md(upb_test_2023_EditionsMessage_getmsgdef(defpool.ptr())); + upb::FieldDefPtr f(md.FindFieldByName("delimited_field")); + EXPECT_EQ(kUpb_FieldType_Group, f.type()); +} + +TEST(EditionsTest, RequiredField) { + upb::DefPool defpool; + upb::MessageDefPtr md(upb_test_2023_EditionsMessage_getmsgdef(defpool.ptr())); + upb::FieldDefPtr f(md.FindFieldByName("required_field")); + EXPECT_EQ(kUpb_Label_Required, f.label()); +} + +TEST(EditionsTest, ClosedEnum) { + upb::DefPool defpool; + upb::MessageDefPtr md(upb_test_2023_EditionsMessage_getmsgdef(defpool.ptr())); + upb::FieldDefPtr f(md.FindFieldByName("enum_field")); + ASSERT_TRUE(f.enum_subdef().is_closed()); +} + +TEST(EditionsTest, PackedField) { + upb::DefPool defpool; + upb::MessageDefPtr md(upb_test_2023_EditionsMessage_getmsgdef(defpool.ptr())); + upb::FieldDefPtr f(md.FindFieldByName("unpacked_field")); + ASSERT_FALSE(f.packed()); +} + +TEST(EditionsTest, ConstructProto) { + // Doesn't do anything except construct the proto. This just verifies that + // the generated code compiles successfully. + upb::Arena arena; + upb_test_2023_EditionsMessage_new(arena.ptr()); +} diff --git a/upb/test/editions_test.proto b/upb/test/editions_test.proto new file mode 100644 index 0000000000..aeb2440c0d --- /dev/null +++ b/upb/test/editions_test.proto @@ -0,0 +1,26 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google LLC. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file or at +// https://developers.google.com/open-source/licenses/bsd + +edition = "2023"; + +package upb.test_2023; + +message EditionsMessage { + int32 plain_field = 1; + int32 implicit_presence_field = 2 [features.field_presence = IMPLICIT]; + int32 required_field = 3 [features.field_presence = LEGACY_REQUIRED]; + EditionsMessage delimited_field = 4 [features.message_encoding = DELIMITED]; + EditionsEnum enum_field = 5; + repeated int32 unpacked_field = 6 + [features.repeated_field_encoding = EXPANDED]; +} + +enum EditionsEnum { + option features.enum_type = CLOSED; + + ONE = 1; +} diff --git a/upb/util/BUILD b/upb/util/BUILD index f354be10d8..8efb0b1f68 100644 --- a/upb/util/BUILD +++ b/upb/util/BUILD @@ -106,7 +106,10 @@ cc_library( proto_library( name = "required_fields_test_proto", - srcs = ["required_fields_test.proto"], + srcs = [ + "required_fields_editions_test.proto", + "required_fields_test.proto", + ], ) upb_c_proto_library( @@ -131,6 +134,7 @@ cc_test( "//upb:json", "//upb:mem", "//upb:reflection", + "//upb:reflection_internal", "@com_google_absl//absl/strings", ], ) diff --git a/upb/util/def_to_proto.c b/upb/util/def_to_proto.c index 29331d59bb..2c0ecbe484 100644 --- a/upb/util/def_to_proto.c +++ b/upb/util/def_to_proto.c @@ -511,6 +511,8 @@ static google_protobuf_FileDescriptorProto* filedef_toproto(upb_ToProto_Context* if (upb_FileDef_Syntax(f) == kUpb_Syntax_Proto3) { google_protobuf_FileDescriptorProto_set_syntax(proto, strviewdup(ctx, "proto3")); + } else if (upb_FileDef_Syntax(f) == kUpb_Syntax_Editions) { + google_protobuf_FileDescriptorProto_set_syntax(proto, strviewdup(ctx, "editions")); } size_t n; diff --git a/upb/util/required_fields_editions_test.proto b/upb/util/required_fields_editions_test.proto new file mode 100644 index 0000000000..b0f41008da --- /dev/null +++ b/upb/util/required_fields_editions_test.proto @@ -0,0 +1,28 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google LLC. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file or at +// https://developers.google.com/open-source/licenses/bsd + +edition = "2023"; + +package upb_util_2023_test; + +message EmptyMessage {} + +message HasRequiredField { + int32 required_int32 = 1 [features.field_presence = LEGACY_REQUIRED]; +} + +message TestRequiredFields { + EmptyMessage required_message = 1 [features.field_presence = LEGACY_REQUIRED]; + TestRequiredFields optional_message = 2; + repeated HasRequiredField repeated_message = 3; + map map_int32_message = 4; + map map_int64_message = 5; + map map_uint32_message = 6; + map map_uint64_message = 7; + map map_bool_message = 8; + map map_string_message = 9; +} diff --git a/upb/util/required_fields_test.cc b/upb/util/required_fields_test.cc index 167fa7e71c..70f0a95bf6 100644 --- a/upb/util/required_fields_test.cc +++ b/upb/util/required_fields_test.cc @@ -9,13 +9,16 @@ #include -#include #include #include "absl/strings/string_view.h" #include "upb/base/status.hpp" #include "upb/json/decode.h" +#include "upb/mem/arena.h" #include "upb/mem/arena.hpp" +#include "upb/reflection/common.h" #include "upb/reflection/def.hpp" +#include "upb/util/required_fields_editions_test.upb.h" +#include "upb/util/required_fields_editions_test.upbdefs.h" #include "upb/util/required_fields_test.upb.h" #include "upb/util/required_fields_test.upbdefs.h" @@ -40,31 +43,61 @@ std::vector PathsToText(upb_FieldPathEntry* entry) { return ret; } -void CheckRequired(absl::string_view json, - const std::vector& missing) { - upb::Arena arena; - upb::DefPool defpool; - upb_util_test_TestRequiredFields* test_msg = - upb_util_test_TestRequiredFields_new(arena.ptr()); - upb::MessageDefPtr m( - upb_util_test_TestRequiredFields_getmsgdef(defpool.ptr())); - upb::Status status; - EXPECT_TRUE(upb_JsonDecode(json.data(), json.size(), test_msg, m.ptr(), - defpool.ptr(), 0, arena.ptr(), status.ptr())) - << status.error_message(); - upb_FieldPathEntry* entries = nullptr; - EXPECT_EQ(!missing.empty(), upb_util_HasUnsetRequired( - test_msg, m.ptr(), defpool.ptr(), &entries)); - if (entries) { - EXPECT_EQ(missing, PathsToText(entries)); - free(entries); +template +class RequiredFieldsTest : public testing::Test { + public: + void CheckRequired(absl::string_view json, + const std::vector& missing) { + upb::Arena arena; + upb::DefPool defpool; + auto* test_msg = T::NewMessage(arena.ptr()); + upb::MessageDefPtr m = T::GetMessageDef(defpool.ptr()); + upb::Status status; + EXPECT_TRUE(upb_JsonDecode(json.data(), json.size(), test_msg, m.ptr(), + defpool.ptr(), 0, arena.ptr(), status.ptr())) + << status.error_message(); + upb_FieldPathEntry* entries = nullptr; + EXPECT_EQ( + !missing.empty(), + upb_util_HasUnsetRequired(test_msg, m.ptr(), defpool.ptr(), &entries)); + if (entries) { + EXPECT_EQ(missing, PathsToText(entries)); + free(entries); + } + + // Verify that we can pass a NULL pointer to entries when we don't care + // about them. + EXPECT_EQ(!missing.empty(), upb_util_HasUnsetRequired( + test_msg, m.ptr(), defpool.ptr(), nullptr)); } +}; - // Verify that we can pass a NULL pointer to entries when we don't care about - // them. - EXPECT_EQ(!missing.empty(), upb_util_HasUnsetRequired( - test_msg, m.ptr(), defpool.ptr(), nullptr)); -} +class Proto2Type { + public: + using MessageType = upb_util_test_TestRequiredFields; + static MessageType* NewMessage(upb_Arena* arena) { + return upb_util_test_TestRequiredFields_new(arena); + } + static upb::MessageDefPtr GetMessageDef(upb_DefPool* defpool) { + return upb::MessageDefPtr( + upb_util_test_TestRequiredFields_getmsgdef(defpool)); + } +}; + +class Edition2023Type { + public: + using MessageType = upb_util_2023_test_TestRequiredFields; + static MessageType* NewMessage(upb_Arena* arena) { + return upb_util_2023_test_TestRequiredFields_new(arena); + } + static upb::MessageDefPtr GetMessageDef(upb_DefPool* defpool) { + return upb::MessageDefPtr( + upb_util_2023_test_TestRequiredFields_getmsgdef(defpool)); + } +}; + +using MyTypes = ::testing::Types; +TYPED_TEST_SUITE(RequiredFieldsTest, MyTypes); // message HasRequiredField { // required int32 required_int32 = 1; @@ -76,10 +109,10 @@ void CheckRequired(absl::string_view json, // repeated HasRequiredField repeated_message = 3; // map map_int32_message = 4; // } -TEST(RequiredFieldsTest, TestRequired) { - CheckRequired(R"json({})json", {"required_message"}); - CheckRequired(R"json({"required_message": {}}")json", {}); - CheckRequired( +TYPED_TEST(RequiredFieldsTest, TestRequired) { + TestFixture::CheckRequired(R"json({})json", {"required_message"}); + TestFixture::CheckRequired(R"json({"required_message": {}}")json", {}); + TestFixture::CheckRequired( R"json( { "optional_message": {} @@ -88,7 +121,7 @@ TEST(RequiredFieldsTest, TestRequired) { {"required_message", "optional_message.required_message"}); // Repeated field. - CheckRequired( + TestFixture::CheckRequired( R"json( { "optional_message": { @@ -104,7 +137,7 @@ TEST(RequiredFieldsTest, TestRequired) { "optional_message.repeated_message[1].required_int32"}); // Int32 map key. - CheckRequired( + TestFixture::CheckRequired( R"json( { "required_message": {}, @@ -118,7 +151,7 @@ TEST(RequiredFieldsTest, TestRequired) { {"map_int32_message[5].required_int32"}); // Int64 map key. - CheckRequired( + TestFixture::CheckRequired( R"json( { "required_message": {}, @@ -132,7 +165,7 @@ TEST(RequiredFieldsTest, TestRequired) { {"map_int64_message[5].required_int32"}); // Uint32 map key. - CheckRequired( + TestFixture::CheckRequired( R"json( { "required_message": {}, @@ -146,7 +179,7 @@ TEST(RequiredFieldsTest, TestRequired) { {"map_uint32_message[5].required_int32"}); // Uint64 map key. - CheckRequired( + TestFixture::CheckRequired( R"json( { "required_message": {}, @@ -160,7 +193,7 @@ TEST(RequiredFieldsTest, TestRequired) { {"map_uint64_message[5].required_int32"}); // Bool map key. - CheckRequired( + TestFixture::CheckRequired( R"json( { "required_message": {}, @@ -173,7 +206,7 @@ TEST(RequiredFieldsTest, TestRequired) { {"map_bool_message[true].required_int32"}); // String map key. - CheckRequired( + TestFixture::CheckRequired( R"json( { "required_message": {}, diff --git a/upb_generator/file_layout.cc b/upb_generator/file_layout.cc index 9fa03d2635..56067c5818 100644 --- a/upb_generator/file_layout.cc +++ b/upb_generator/file_layout.cc @@ -32,8 +32,10 @@ #include #include +#include #include "upb/mini_table/internal/extension.h" +#include "upb/reflection/def.hpp" #include "upb_generator/common.h" namespace upb { @@ -43,24 +45,32 @@ const char* kEnumsInit = "enums_layout"; const char* kExtensionsInit = "extensions_layout"; const char* kMessagesInit = "messages_layout"; -void AddEnums(upb::MessageDefPtr message, std::vector* enums) { +void AddEnums(upb::MessageDefPtr message, std::vector* enums, + WhichEnums which) { enums->reserve(enums->size() + message.enum_type_count()); for (int i = 0; i < message.enum_type_count(); i++) { - enums->push_back(message.enum_type(i)); + upb::EnumDefPtr enum_type = message.enum_type(i); + if (which == kAllEnums || enum_type.is_closed()) { + enums->push_back(message.enum_type(i)); + } } for (int i = 0; i < message.nested_message_count(); i++) { - AddEnums(message.nested_message(i), enums); + AddEnums(message.nested_message(i), enums, which); } } -std::vector SortedEnums(upb::FileDefPtr file) { +std::vector SortedEnums(upb::FileDefPtr file, + WhichEnums which) { std::vector enums; enums.reserve(file.toplevel_enum_count()); for (int i = 0; i < file.toplevel_enum_count(); i++) { - enums.push_back(file.toplevel_enum(i)); + upb::EnumDefPtr top_level_enum = file.toplevel_enum(i); + if (which == kAllEnums || top_level_enum.is_closed()) { + enums.push_back(file.toplevel_enum(i)); + } } for (int i = 0; i < file.toplevel_message_count(); i++) { - AddEnums(file.toplevel_message(i), &enums); + AddEnums(file.toplevel_message(i), &enums, which); } std::sort(enums.begin(), enums.end(), [](upb::EnumDefPtr a, upb::EnumDefPtr b) { diff --git a/upb_generator/file_layout.h b/upb_generator/file_layout.h index a5b9416a7d..d428674098 100644 --- a/upb_generator/file_layout.h +++ b/upb_generator/file_layout.h @@ -57,7 +57,13 @@ namespace upb { namespace generator { -std::vector SortedEnums(upb::FileDefPtr file); +enum WhichEnums { + kAllEnums = 0, + kClosedEnums = 1, +}; + +std::vector SortedEnums(upb::FileDefPtr file, + WhichEnums which); // Ordering must match upb/def.c! // diff --git a/upb_generator/plugin.h b/upb_generator/plugin.h index 1500d4097c..50ff5498bf 100644 --- a/upb_generator/plugin.h +++ b/upb_generator/plugin.h @@ -187,9 +187,12 @@ class Plugin { ABSL_LOG(FATAL) << "Failed to parse CodeGeneratorRequest"; } response_ = UPB_DESC(compiler_CodeGeneratorResponse_new)(arena_.ptr()); + + int features = + UPB_DESC(compiler_CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL) | + UPB_DESC(compiler_CodeGeneratorResponse_FEATURE_SUPPORTS_EDITIONS); UPB_DESC(compiler_CodeGeneratorResponse_set_supported_features) - (response_, - UPB_DESC(compiler_CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL)); + (response_, features); } void WriteResponse() { diff --git a/upb_generator/protoc-gen-upb.cc b/upb_generator/protoc-gen-upb.cc index dac5bd6383..1e97a78370 100644 --- a/upb_generator/protoc-gen-upb.cc +++ b/upb_generator/protoc-gen-upb.cc @@ -868,7 +868,7 @@ void WriteHeader(const DefPoolPair& pools, upb::FileDefPtr file, const std::vector this_file_messages = SortedMessages(file); const std::vector this_file_exts = SortedExtensions(file); - std::vector this_file_enums = SortedEnums(file); + std::vector this_file_enums = SortedEnums(file, kAllEnums); std::vector forward_messages = SortedForwardMessages(this_file_messages, this_file_exts); @@ -1097,7 +1097,7 @@ void WriteMiniDescriptorSource(const DefPoolPair& pools, upb::FileDefPtr file, WriteMessageMiniDescriptorInitializer(msg, options, output); } - for (const auto msg : SortedEnums(file)) { + for (const auto msg : SortedEnums(file, kClosedEnums)) { WriteEnumMiniDescriptorInitializer(msg, options, output); } } diff --git a/upb_generator/protoc-gen-upb_minitable.cc b/upb_generator/protoc-gen-upb_minitable.cc index e3064adadc..7a1d1d6dfb 100644 --- a/upb_generator/protoc-gen-upb_minitable.cc +++ b/upb_generator/protoc-gen-upb_minitable.cc @@ -145,12 +145,11 @@ void WriteHeader(const DefPoolPair& pools, upb::FileDefPtr file, output("\n"); - std::vector this_file_enums = SortedEnums(file); + std::vector this_file_enums = + SortedEnums(file, kClosedEnums); - if (file.syntax() == kUpb_Syntax_Proto2) { - for (const auto enumdesc : this_file_enums) { - output("extern const upb_MiniTableEnum $0;\n", EnumInit(enumdesc)); - } + for (const auto enumdesc : this_file_enums) { + output("extern const upb_MiniTableEnum $0;\n", EnumInit(enumdesc)); } output("extern const upb_MiniTableFile $0;\n\n", FileLayoutName(file)); @@ -536,9 +535,8 @@ void WriteEnum(upb::EnumDefPtr e, Output& output) { } int WriteEnums(const DefPoolPair& pools, upb::FileDefPtr file, Output& output) { - if (file.syntax() != kUpb_Syntax_Proto2) return 0; - - std::vector this_file_enums = SortedEnums(file); + std::vector this_file_enums = + SortedEnums(file, kClosedEnums); for (const auto e : this_file_enums) { WriteEnum(e, output);