From 887e95dade3b93daf6cc14e1ac7ccbccc66053a9 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Thu, 30 May 2024 22:18:22 -0700 Subject: [PATCH 1/4] Fix a bug in edition defaults calculation. Since the fixed/overridable split can occur whenever a feature is introduced or removed, we need to include those editions in the resulting compiled defaults. This does bug only affects edition 2024 and later, where features may be removed or introduced in isolation. PiperOrigin-RevId: 638903990 --- src/google/protobuf/feature_resolver.cc | 27 ++++++- src/google/protobuf/feature_resolver_test.cc | 75 ++++++++++++++++++++ src/google/protobuf/unittest_features.proto | 2 +- 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index bf24e18a17b7..7accef385257 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -189,17 +189,38 @@ absl::Status ValidateExtension(const Descriptor& feature_set, return absl::OkStatus(); } +void MaybeInsertEdition(Edition edition, Edition maximum_edition, + absl::btree_set& editions) { + if (edition <= maximum_edition) { + editions.insert(edition); + } +} + +// This collects all of the editions that are relevant to any features defined +// in a message descriptor. We only need to consider editions where something +// has changed. void CollectEditions(const Descriptor& descriptor, Edition maximum_edition, absl::btree_set& editions) { for (int i = 0; i < descriptor.field_count(); ++i) { - for (const auto& def : descriptor.field(i)->options().edition_defaults()) { - if (maximum_edition < def.edition()) continue; + const FieldOptions& options = descriptor.field(i)->options(); + // Editions where a new feature is introduced should be captured. + MaybeInsertEdition(options.feature_support().edition_introduced(), + maximum_edition, editions); + + // Editions where a feature is removed should be captured. + if (options.feature_support().has_edition_removed()) { + MaybeInsertEdition(options.feature_support().edition_removed(), + maximum_edition, editions); + } + + // Any edition where a default value changes should be captured. + for (const auto& def : options.edition_defaults()) { // TODO Remove this once all features use EDITION_LEGACY. if (def.edition() == Edition::EDITION_LEGACY) { editions.insert(Edition::EDITION_PROTO2); continue; } - editions.insert(def.edition()); + MaybeInsertEdition(def.edition(), maximum_edition, editions); } } } diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index e5aaf05ffe6d..bbd4750c4e0f 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -1311,6 +1311,81 @@ TEST_F(FeatureResolverPoolTest, CompileDefaultsMinimumTooEarly) { HasError(HasSubstr("edition 1_TEST_ONLY is earlier than the oldest"))); } +TEST_F(FeatureResolverPoolTest, CompileDefaultsRemovedOnly) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum Bar { + TEST_ENUM_FEATURE_UNKNOWN = 0; + VALUE1 = 1; + VALUE2 = 2; + } + message Foo { + optional Bar file_feature = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + feature_support.edition_removed = EDITION_99998_TEST_ONLY, + edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + auto compiled_defaults = FeatureResolver::CompileDefaults( + feature_set_, {ext}, EDITION_99997_TEST_ONLY, EDITION_99999_TEST_ONLY); + ASSERT_OK(compiled_defaults); + const auto& defaults = *compiled_defaults->defaults().rbegin(); + EXPECT_THAT(defaults.edition(), EDITION_99998_TEST_ONLY); + EXPECT_THAT(defaults.fixed_features().GetExtension(pb::test).file_feature(), + pb::VALUE1); + EXPECT_FALSE(defaults.overridable_features() + .GetExtension(pb::test) + .has_file_feature()); +} + +TEST_F(FeatureResolverPoolTest, CompileDefaultsIntroducedOnly) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum Bar { + TEST_ENUM_FEATURE_UNKNOWN = 0; + VALUE1 = 1; + VALUE2 = 2; + } + message Foo { + optional Bar file_feature = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_99998_TEST_ONLY, + edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + auto compiled_defaults = FeatureResolver::CompileDefaults( + feature_set_, {ext}, EDITION_99997_TEST_ONLY, EDITION_99999_TEST_ONLY); + ASSERT_OK(compiled_defaults); + const auto& defaults = *compiled_defaults->defaults().rbegin(); + EXPECT_THAT(defaults.edition(), EDITION_99998_TEST_ONLY); + EXPECT_THAT( + defaults.overridable_features().GetExtension(pb::test).file_feature(), + pb::VALUE1); + EXPECT_FALSE( + defaults.fixed_features().GetExtension(pb::test).has_file_feature()); +} + TEST_F(FeatureResolverPoolTest, CompileDefaultsMinimumCovered) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; diff --git a/src/google/protobuf/unittest_features.proto b/src/google/protobuf/unittest_features.proto index d32f91c38abf..ddf510c6b6ce 100644 --- a/src/google/protobuf/unittest_features.proto +++ b/src/google/protobuf/unittest_features.proto @@ -193,7 +193,7 @@ message TestFeatures { targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, feature_support = { - edition_introduced: EDITION_PROTO2 + edition_introduced: EDITION_PROTO3 edition_removed: EDITION_2023 }, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" }, From f61d89cf9ec051450cf9cb54182da40706fd1753 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Mon, 3 Jun 2024 13:26:12 -0700 Subject: [PATCH 2/4] Avoid ODR violations from bootstrapped protos PiperOrigin-RevId: 639892103 --- BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/BUILD.bazel b/BUILD.bazel index d9e2c4476a31..301a04656f3f 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -418,6 +418,7 @@ proto_lang_toolchain( name = "cc_toolchain", blacklisted_protos = [ "//:compiler_plugin_proto", + "//:cpp_features_proto", "//:descriptor_proto", ], command_line = "--cpp_out=$(OUT)", From 9a37881bc8520d0dfdd33bb96fbd3139c56ec7ee Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Mon, 3 Jun 2024 19:17:08 -0700 Subject: [PATCH 3/4] Always report plugin support errors from protoc. If a plugin doesn't support a feature used by a proto file, we can't trust any errors reported by that plugin. We should always report these types of errors first. There could be cases where the plugin doesn't correctly specify its support when it hits an error though, so we should *also* report any plugin errors to avoid masking them. PiperOrigin-RevId: 639984803 --- .../protobuf/compiler/command_line_interface.cc | 17 +++++++++-------- .../compiler/command_line_interface_unittest.cc | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/google/protobuf/compiler/command_line_interface.cc b/src/google/protobuf/compiler/command_line_interface.cc index 82cce7b73abe..690018f5d524 100644 --- a/src/google/protobuf/compiler/command_line_interface.cc +++ b/src/google/protobuf/compiler/command_line_interface.cc @@ -2854,23 +2854,24 @@ bool CommandLineInterface::GeneratePluginOutput( } // Check for errors. - if (!response.error().empty()) { - // Generator returned an error. - *error = response.error(); - return false; - } + bool success = true; if (!EnforceProto3OptionalSupport(plugin_name, response.supported_features(), parsed_files)) { - return false; + success = false; } if (!EnforceEditionsSupport(plugin_name, response.supported_features(), static_cast(response.minimum_edition()), static_cast(response.maximum_edition()), parsed_files)) { - return false; + success = false; + } + if (!response.error().empty()) { + // Generator returned an error. + *error = response.error(); + success = false; } - return true; + return success; } bool CommandLineInterface::EncodeOrDecode(const DescriptorPool* pool) { diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index 6a2db03bafda..8870dabae30f 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -1854,6 +1854,22 @@ TEST_F(CommandLineInterfaceTest, PluginNoEditionsSupport) { "code generator prefix-gen-plug hasn't been updated to support editions"); } +TEST_F(CommandLineInterfaceTest, PluginErrorAndNoEditionsSupport) { + CreateTempFile("foo.proto", R"schema( + edition = "2023"; + message MockCodeGenerator_Error { } + )schema"); + + SetMockGeneratorTestCase("no_editions"); + Run("protocol_compiler " + "--proto_path=$tmpdir foo.proto --plug_out=$tmpdir"); + + ExpectErrorSubstring( + "code generator prefix-gen-plug hasn't been updated to support editions"); + ExpectErrorSubstring( + "--plug_out: foo.proto: Saw message type MockCodeGenerator_Error."); +} + TEST_F(CommandLineInterfaceTest, EditionDefaults) { CreateTempFile("google/protobuf/descriptor.proto", google::protobuf::DescriptorProto::descriptor()->file()->DebugString()); From 2717ae77f6d38f5bb685bec05678a487815d1e0d Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Mon, 3 Jun 2024 13:06:20 -0700 Subject: [PATCH 4/4] Internal change PiperOrigin-RevId: 639885959 --- .github/workflows/test_ruby.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test_ruby.yml b/.github/workflows/test_ruby.yml index 38cb5ef5b236..bbd76f499146 100644 --- a/.github/workflows/test_ruby.yml +++ b/.github/workflows/test_ruby.yml @@ -40,7 +40,7 @@ jobs: - name: Run tests uses: protocolbuffers/protobuf-ci/bazel-docker@v3 with: - image: ${{ matrix.image || format('us-docker.pkg.dev/protobuf-build/containers/test/linux/ruby:{0}-6.3.0-a6940b1421a71325ef4c7828ec72d404f56bbf30', matrix.ruby) }} + image: ${{ matrix.image || format('us-docker.pkg.dev/protobuf-build/containers/test/linux/ruby:{0}-6.3.0-9848710ff1370795ee7517570a20b81e140112ec', matrix.ruby) }} credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} bazel-cache: ruby_linux/${{ matrix.ruby }}_${{ matrix.bazel }} bazel: test //ruby/... //ruby/tests:ruby_version --test_env=KOKORO_RUBY_VERSION --test_env=BAZEL=true ${{ matrix.ffi == 'FFI' && '--//ruby:ffi=enabled --test_env=PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION=FFI' || '' }} @@ -172,7 +172,7 @@ jobs: - name: Run tests uses: protocolbuffers/protobuf-ci/bazel-docker@v3 with: - image: us-docker.pkg.dev/protobuf-build/containers/test/linux/ruby:${{ matrix.ruby }}-6.3.0-a6940b1421a71325ef4c7828ec72d404f56bbf30 + image: us-docker.pkg.dev/protobuf-build/containers/test/linux/ruby:${{ matrix.ruby }}-6.3.0-9848710ff1370795ee7517570a20b81e140112ec credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} bazel-cache: ruby_install/${{ matrix.ruby }}_${{ matrix.bazel }} bash: >