Fix a bug in which proto code uses ctype instead of string_type inter…

…nally.

We change InferLegacyProtoFeatures to set ctype based on string_type when string_type is set. We need to update CppGenerator::ValidateFeatures to allow both ctype and string_type to be set because it runs after InferLegacyProtoFeatures.

PiperOrigin-RevId: 645480157

src/google/protobuf/BUILD.bazel

@@ -841,6 +841,7 @@ filegroup(
         "unittest_proto3_lite.proto",
         "unittest_proto3_optional.proto",
         "unittest_retention.proto",
+        "unittest_string_type.proto",
         "unittest_well_known_types.proto",
     ],
     visibility = ["//:__subpackages__"],
@@ -931,6 +932,7 @@ proto_library(
     deps = [
         ":any_proto",
         ":api_proto",
+        ":cpp_features_proto",
         ":descriptor_proto",
         ":duration_proto",
         ":empty_proto",

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

@@ -397,9 +397,19 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const {
                          " specifies string_type=CORD which is not supported "
                          "for extensions."));
       } else if (field.options().has_ctype()) {
-        status = absl::FailedPreconditionError(absl::StrCat(
-            field.full_name(),
-            " specifies both string_type and ctype which is not supported."));
+        // NOTE: this is just a sanity check. This case should never happen
+        // because descriptor builder makes string_type override ctype.
+        const FieldOptions::CType ctype = field.options().ctype();
+        const pb::CppFeatures::StringType string_type =
+            unresolved_features.string_type();
+        if ((ctype == FieldOptions::STRING &&
+             string_type != pb::CppFeatures::STRING) ||
+            (ctype == FieldOptions::CORD &&
+             string_type != pb::CppFeatures::CORD)) {
+          status = absl::FailedPreconditionError(
+              absl::StrCat(field.full_name(),
+                           " specifies inconsistent string_type and ctype."));
+        }
       }
     }
 

src/google/protobuf/descriptor.cc

@@ -3053,6 +3053,10 @@ void FieldDescriptor::CopyTo(FieldDescriptorProto* proto) const {
 
   if (&options() != &FieldOptions::default_instance()) {
     *proto->mutable_options() = options();
+    if (proto_features_->GetExtension(pb::cpp).has_string_type()) {
+      // ctype must have been set in InferLegacyProtoFeatures so avoid copying.
+      proto->mutable_options()->clear_ctype();
+    }
   }
 
   RestoreFeaturesToOptions(proto_features_, proto);
@@ -5466,6 +5470,24 @@ static void InferLegacyProtoFeatures(const FieldDescriptorProto& proto,
   }
 }
 
+// TODO: we should update proto code to not need ctype to be set
+// when string_type is set.
+static void EnforceCTypeStringTypeConsistency(
+    Edition edition, FieldDescriptor::CppType type,
+    const pb::CppFeatures& cpp_features, FieldOptions& options) {
+  if (&options == &FieldOptions::default_instance()) return;
+  if (edition < Edition::EDITION_2024 &&
+      type == FieldDescriptor::CPPTYPE_STRING) {
+    switch (cpp_features.string_type()) {
+      case pb::CppFeatures::CORD:
+        options.set_ctype(FieldOptions::CORD);
+        break;
+      default:
+        break;
+    }
+  }
+}
+
 template <class DescriptorT>
 void DescriptorBuilder::ResolveFeaturesImpl(
     Edition edition, const typename DescriptorT::Proto& proto,
@@ -6091,6 +6113,24 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
          iter != options_to_interpret_.end(); ++iter) {
       option_interpreter.InterpretNonExtensionOptions(&(*iter));
     }
+
+    // TODO: move this check back to generator.cc once we no longer
+    // need to set both ctype and string_type internally.
+    internal::VisitDescriptors(
+        *result, proto,
+        [&](const FieldDescriptor& field, const FieldDescriptorProto& proto) {
+          if (field.options_->has_ctype() && field.options_->features()
+                                                 .GetExtension(pb::cpp)
+                                                 .has_string_type()) {
+            AddError(
+                field.full_name(), proto, DescriptorPool::ErrorCollector::TYPE,
+                absl::StrFormat("Field %s specifies both string_type and ctype "
+                                "which is not supported.",
+                                field.full_name())
+                    .c_str());
+          }
+        });
+
     // Handle feature resolution.  This must occur after option interpretation,
     // but before validation.
     internal::VisitDescriptors(
@@ -6108,6 +6148,14 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
               alloc);
         });
 
+    internal::VisitDescriptors(*result, [&](const FieldDescriptor& field) {
+      EnforceCTypeStringTypeConsistency(
+          field.file()->edition(), field.cpp_type(),
+          field.merged_features_->GetExtension(pb::cpp),
+          const_cast<  // NOLINT(google3-runtime-proto-const-cast)
+              FieldOptions&>(*field.options_));
+    });
+
     // Post-process cleanup for field features.
     internal::VisitDescriptors(
         *result, proto,

src/google/protobuf/descriptor_unittest.cc

@@ -73,6 +73,7 @@
 #include "google/protobuf/unittest_lazy_dependencies_custom_option.pb.h"
 #include "google/protobuf/unittest_lazy_dependencies_enum.pb.h"
 #include "google/protobuf/unittest_proto3_arena.pb.h"
+#include "google/protobuf/unittest_string_type.pb.h"
 
 
 // Must be included last.
@@ -7856,12 +7857,11 @@ TEST_F(FeaturesTest, Edition2023InferredFeatures) {
         options { ctype: STRING_PIECE }
       }
       field {
-        name: "ctype_and_string_type"
+        name: "view"
         number: 4
         label: LABEL_OPTIONAL
         type: TYPE_STRING
         options {
-          ctype: CORD
           features {
             [pb.cpp] { string_type: VIEW }
           }
@@ -9685,7 +9685,7 @@ TEST_F(FeaturesTest, EnumFeatureHelpers) {
         type_name: "FooOpen"
         options {
           features {
-            [pb.cpp] { legacy_closed_enum: true string_type: STRING }
+            [pb.cpp] { legacy_closed_enum: true }
           }
         }
       }

src/google/protobuf/unittest_string_type.proto

@@ -0,0 +1,16 @@
+// Protocol Buffers - Google's data interchange format
+// Copyright 2008 Google Inc.  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 protobuf_unittest;
+
+import "google/protobuf/cpp_features.proto";
+
+message EntryProto {
+  bytes value = 3 [features.(pb.cpp).string_type = CORD];
+}