From dfbe98781c5ac2c427aa4406705f8201c7364fcf Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Fri, 21 Jun 2024 13:12:05 -0700 Subject: [PATCH] Fix a bug in which proto code uses ctype instead of string_type internally. 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 | 2 + src/google/protobuf/compiler/cpp/generator.cc | 16 +++++-- src/google/protobuf/descriptor.cc | 48 +++++++++++++++++++ src/google/protobuf/descriptor_unittest.cc | 6 +-- .../protobuf/unittest_string_type.proto | 16 +++++++ 5 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 src/google/protobuf/unittest_string_type.proto diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index 711546a6276f..040bead4f1ef 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/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", diff --git a/src/google/protobuf/compiler/cpp/generator.cc b/src/google/protobuf/compiler/cpp/generator.cc index 6b166e74fb5e..8ee87e546d30 100644 --- a/src/google/protobuf/compiler/cpp/generator.cc +++ b/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.")); + } } } diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index f668ae252226..1619d34daa6b 100644 --- a/src/google/protobuf/descriptor.cc +++ b/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 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, diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index d9628fbbc36c..9eb7011acd7b 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/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 } } } } diff --git a/src/google/protobuf/unittest_string_type.proto b/src/google/protobuf/unittest_string_type.proto new file mode 100644 index 000000000000..611588f19185 --- /dev/null +++ b/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]; +}