Fix python codegen crash when C++ features are used. (#20577)

What we actually want to prevent is any future use of non-enum features or python-specific features which don't exist today.  If other language features are present we can safely strip them from the python gencode.  We also reserve an extension number for the future python features in descriptor.proto

PiperOrigin-RevId: 733885839

Author: mkruskal-google

src/google/protobuf/compiler/cpp/BUILD.bazel

@@ -105,6 +105,7 @@ cc_library(
     visibility = [
         "//pkg:__pkg__",
         "//src/google/protobuf/compiler:__pkg__",
+        "//src/google/protobuf/compiler/python:__pkg__",  # For testing only.
         "@io_kythe//kythe/cxx/tools:__subpackages__",
     ],
     deps = [

src/google/protobuf/compiler/python/BUILD.bazel

@@ -51,7 +51,11 @@ cc_test(
     copts = COPTS,
     deps = [
         ":python",
+        "//src/google/protobuf",
+        "//src/google/protobuf/compiler:code_generator",
         "//src/google/protobuf/compiler:command_line_interface",
+        "//src/google/protobuf/compiler:command_line_interface_tester",
+        "//src/google/protobuf/compiler/cpp",
         "//src/google/protobuf/io",
         "//src/google/protobuf/io:printer",
         "//src/google/protobuf/testing",

src/google/protobuf/compiler/python/generator.cc

@@ -479,8 +479,17 @@ std::string Generator::GetResolvedFeatures(
     // Assume these are all enums.  If we add non-enum global features or any
     // python-specific features, we will need to come back and improve this
     // logic.
-    ABSL_CHECK(field->enum_type() != nullptr)
-        << "Unexpected non-enum field found!";
+    if (field->type() != FieldDescriptor::TYPE_ENUM) {
+      ABSL_CHECK(field->is_extension())
+          << "Unsupported non-enum global feature found: "
+          << field->full_name();
+      // Placeholder for python-specific features.
+      ABSL_CHECK(field->number() != 1003)
+          << "Unsupported python-specific feature found: "
+          << field->full_name();
+      // Skip any non-python language-specific features.
+      continue;
+    }
     if (field->options().retention() == FieldOptions::RETENTION_SOURCE) {
       // Skip any source-retention features.
       continue;

src/google/protobuf/compiler/python/plugin_unittest.cc

@@ -9,15 +9,20 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "google/protobuf/testing/file.h"
-#include "google/protobuf/testing/file.h"
-#include "google/protobuf/compiler/command_line_interface.h"
-#include "google/protobuf/compiler/python/generator.h"
 #include <gtest/gtest.h>
 #include "absl/log/absl_check.h"
+#include "absl/strings/str_cat.h"
 #include "absl/strings/str_split.h"
+#include "absl/strings/substitute.h"
+#include "google/protobuf/compiler/code_generator.h"
+#include "google/protobuf/compiler/command_line_interface_tester.h"
+#include "google/protobuf/compiler/cpp/generator.h"
+#include "google/protobuf/compiler/python/generator.h"
+#include "google/protobuf/cpp_features.pb.h"
 #include "google/protobuf/io/printer.h"
 #include "google/protobuf/io/zero_copy_stream.h"
 
@@ -100,6 +105,58 @@ TEST(PythonPluginTest, ImportTest) {
   EXPECT_TRUE(found_expected_import);
 }
 
+class PythonGeneratorTest : public CommandLineInterfaceTester,
+                            public testing::WithParamInterface<bool> {
+ protected:
+  PythonGeneratorTest() {
+    auto generator = std::make_unique<Generator>();
+    generator->set_opensource_runtime(GetParam());
+    RegisterGenerator("--python_out", "--python_opt", std::move(generator),
+                      "Python test generator");
+
+    // Generate built-in protos.
+    CreateTempFile(
+        google::protobuf::DescriptorProto::descriptor()->file()->name(),
+        google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
+  }
+};
+
+TEST_P(PythonGeneratorTest, PythonWithCppFeatures) {
+  // Test that the presence of C++ features does not break Python generation.
+  RegisterGenerator("--cpp_out", "--cpp_opt",
+                    std::make_unique<cpp::CppGenerator>(),
+                    "C++ test generator");
+  CreateTempFile("google/protobuf/cpp_features.proto",
+                 pb::CppFeatures::descriptor()->file()->DebugString());
+  CreateTempFile("foo.proto",
+                 R"schema(
+    edition = "2023";
+
+    import "google/protobuf/cpp_features.proto";
+
+    package foo;
+    
+    enum Bar {
+      AAA = 0;
+      BBB = 1;
+    }
+
+    message Foo {
+      Bar bar_enum = 1 [features.(pb.cpp).legacy_closed_enum = true];
+    })schema");
+
+  RunProtoc(absl::Substitute(
+      "protocol_compiler --proto_path=$$tmpdir --cpp_out=$$tmpdir "
+      "--python_out=$$tmpdir foo.proto $0 "
+      "google/protobuf/cpp_features.proto",
+      google::protobuf::DescriptorProto::descriptor()->file()->name()));
+
+  ExpectNoErrors();
+}
+
+INSTANTIATE_TEST_SUITE_P(PythonGeneratorTest, PythonGeneratorTest,
+                         testing::Bool());
+
 }  // namespace
 }  // namespace python
 }  // namespace compiler

src/google/protobuf/descriptor.cc

@@ -5537,7 +5537,7 @@ static void InferLegacyProtoFeatures(const ProtoT& proto,
 static void InferLegacyProtoFeatures(const FieldDescriptorProto& proto,
                                      const FieldOptions& options,
                                      Edition edition, FeatureSet& features) {
-  if (!features.MutableExtension(pb::cpp)->has_string_type()) {
+  if (!features.GetExtension(pb::cpp).has_string_type()) {
     if (options.ctype() == FieldOptions::CORD) {
       features.MutableExtension(pb::cpp)->set_string_type(
           pb::CppFeatures::CORD);

src/google/protobuf/descriptor.proto

@@ -1135,6 +1135,11 @@ message FeatureSet {
       type: ".pb.JavaFeatures"
     },
     declaration = { number: 1002, full_name: ".pb.go", type: ".pb.GoFeatures" },
+    declaration = {
+      number: 1003,
+      full_name: ".pb.python",
+      type: ".pb.PythonFeatures"
+    },
     declaration = {
       number: 9990,
       full_name: ".pb.proto1",

src/google/protobuf/descriptor_unittest.cc

@@ -11799,6 +11799,49 @@ TEST_F(DescriptorPoolFeaturesTest, OverrideDefaults) {
               )pb"));
 }
 
+TEST_F(DescriptorPoolFeaturesTest, OverrideFieldDefaults) {
+  FeatureSetDefaults defaults = ParseTextOrDie(R"pb(
+    defaults {
+      edition: EDITION_PROTO2
+      overridable_features {
+        field_presence: EXPLICIT
+        enum_type: CLOSED
+        repeated_field_encoding: EXPANDED
+        utf8_validation: VERIFY
+        message_encoding: LENGTH_PREFIXED
+        json_format: ALLOW
+        enforce_naming_style: STYLE_LEGACY
+      }
+    }
+    minimum_edition: EDITION_PROTO2
+    maximum_edition: EDITION_2023
+  )pb");
+  EXPECT_OK(pool_.SetFeatureSetDefaults(std::move(defaults)));
+
+  FileDescriptorProto file_proto = ParseTextOrDie(R"pb(
+    name: "foo.proto"
+    syntax: "editions"
+    edition: EDITION_PROTO3
+    message_type {
+      name: "Foo"
+      field { name: "bar" number: 1 label: LABEL_OPTIONAL type: TYPE_INT64 }
+    }
+  )pb");
+
+  BuildDescriptorMessagesInTestPool();
+  const FileDescriptor* file = ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto));
+  const FieldDescriptor* field = file->message_type(0)->field(0);
+  EXPECT_THAT(GetFeatures(field), EqualsProto(R"pb(
+                field_presence: EXPLICIT
+                enum_type: CLOSED
+                repeated_field_encoding: EXPANDED
+                utf8_validation: VERIFY
+                message_encoding: LENGTH_PREFIXED
+                json_format: ALLOW
+                enforce_naming_style: STYLE_LEGACY
+              )pb"));
+}
+
 TEST_F(DescriptorPoolFeaturesTest, ResolvesFeaturesForCppDefault) {
   EXPECT_FALSE(pool_.ResolvesFeaturesFor(pb::test));
   EXPECT_FALSE(pool_.ResolvesFeaturesFor(pb::TestMessage::test_message));