Merge pull request #17019 from protocolbuffers/cp-27

Cherrypick editions fixes to 27.x
protocolbuffers · Jun 4, 2024 · 368b9b2 · 368b9b2
2 parents d2edc49 + 2717ae7
commit 368b9b2
Show file tree

Hide file tree

Showing 7 changed files with 128 additions and 14 deletions.
diff --git 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: >

diff --git 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)",

diff --git 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<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
@@ -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());

diff --git 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<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
@@ -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
@@ -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" },