Merge pull request #17019 from protocolbuffers/cp-27
Cherrypick editions fixes to 27.x
diff --git a/.github/workflows/test_ruby.yml b/.github/workflows/test_ruby.yml
index 38cb5ef..bbd76f4 100644
--- a/.github/workflows/test_ruby.yml
+++ b/.github/workflows/test_ruby.yml
@@ -40,7 +40,7 @@
- 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 @@
- 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: >
diff --git a/BUILD.bazel b/BUILD.bazel
index d9e2c44..301a046 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -418,6 +418,7 @@
name = "cc_toolchain",
blacklisted_protos = [
"//:compiler_plugin_proto",
+ "//:cpp_features_proto",
"//:descriptor_proto",
],
command_line = "--cpp_out=$(OUT)",
diff --git a/src/google/protobuf/compiler/command_line_interface.cc b/src/google/protobuf/compiler/command_line_interface.cc
index 82cce7b..690018f 100644
--- a/src/google/protobuf/compiler/command_line_interface.cc
+++ b/src/google/protobuf/compiler/command_line_interface.cc
@@ -2854,23 +2854,24 @@
}
// 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<Edition>(response.minimum_edition()),
static_cast<Edition>(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 6a2db03..8870dab 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 @@
"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());
diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc
index bf24e18..7accef3 100644
--- a/src/google/protobuf/feature_resolver.cc
+++ b/src/google/protobuf/feature_resolver.cc
@@ -189,17 +189,38 @@
return absl::OkStatus();
}
+void MaybeInsertEdition(Edition edition, Edition maximum_edition,
+ absl::btree_set<Edition>& 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<Edition>& 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 e5aaf05..bbd4750 100644
--- a/src/google/protobuf/feature_resolver_test.cc
+++ b/src/google/protobuf/feature_resolver_test.cc
@@ -1311,6 +1311,81 @@
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 d32f91c..ddf510c 100644
--- a/src/google/protobuf/unittest_features.proto
+++ b/src/google/protobuf/unittest_features.proto
@@ -193,7 +193,7 @@
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" },
