diff --git a/src/google/protobuf/compiler/cpp/generator.cc b/src/google/protobuf/compiler/cpp/generator.cc index 14a0d8768b6f..4284cec120e7 100644 --- a/src/google/protobuf/compiler/cpp/generator.cc +++ b/src/google/protobuf/compiler/cpp/generator.cc @@ -386,55 +386,28 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const { } } - if (unresolved_features.has_string_type()) { - if (field.cpp_type() != FieldDescriptor::CPPTYPE_STRING) { - status = absl::FailedPreconditionError(absl::StrCat( - "Field ", field.full_name(), - " specifies string_type, but is not a string nor bytes field.")); - } else if (unresolved_features.string_type() == pb::CppFeatures::CORD && - field.is_extension()) { - status = absl::FailedPreconditionError( - absl::StrCat("Extension ", field.full_name(), - " specifies string_type=CORD which is not supported " - "for extensions.")); - } else if (field.options().has_ctype()) { - // 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.")); - } - } + if ((unresolved_features.string_type() == pb::CppFeatures::CORD || + field.legacy_proto_ctype() == FieldOptions::CORD) && + field.is_extension()) { + status = absl::FailedPreconditionError( + absl::StrCat("Extension ", field.full_name(), + " specifies CORD string type which is not supported " + "for extensions.")); } - if (field.options().has_ctype()) { - if (field.cpp_type() != FieldDescriptor::CPPTYPE_STRING) { - status = absl::FailedPreconditionError(absl::StrCat( - "Field ", field.full_name(), - " specifies ctype, but is not a string nor bytes field.")); - } - if (field.options().ctype() == FieldOptions::CORD) { - if (field.is_extension()) { - status = absl::FailedPreconditionError(absl::StrCat( - "Extension ", field.full_name(), - " specifies Cord type which is not supported for extensions.")); - } - } + if ((unresolved_features.has_string_type() || + field.has_legacy_proto_ctype()) && + field.cpp_type() != FieldDescriptor::CPPTYPE_STRING) { + status = absl::FailedPreconditionError(absl::StrCat( + "Field ", field.full_name(), + " specifies string_type, but is not a string nor bytes field.")); } - if (field.cpp_type() == FieldDescriptor::CPPTYPE_STRING && - field.cpp_string_type() == FieldDescriptor::CppStringType::kCord && - field.is_extension()) { + if (unresolved_features.has_string_type() && + field.has_legacy_proto_ctype()) { status = absl::FailedPreconditionError(absl::StrCat( - "Extension ", field.full_name(), - " specifies Cord type which is not supported for extensions.")); + "Field ", field.full_name(), + " specifies both string_type and ctype which is not supported.")); } }); return status; diff --git a/src/google/protobuf/compiler/cpp/generator_unittest.cc b/src/google/protobuf/compiler/cpp/generator_unittest.cc index 4f105e620c11..432074884314 100644 --- a/src/google/protobuf/compiler/cpp/generator_unittest.cc +++ b/src/google/protobuf/compiler/cpp/generator_unittest.cc @@ -255,8 +255,8 @@ TEST_F(CppGeneratorTest, StringTypeCordNotForExtension) { "--experimental_editions foo.proto"); ExpectErrorSubstring( - "Extension bar specifies Cord type which is not supported for " - "extensions."); + "Extension bar specifies CORD string type which is not supported for " + "extensions"); } TEST_F(CppGeneratorTest, InheritedStringTypeCordNotForExtension) { @@ -280,7 +280,7 @@ TEST_F(CppGeneratorTest, InheritedStringTypeCordNotForExtension) { ExpectNoErrors(); } -TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) { +TEST_F(CppGeneratorTest, CtypeOnNonStringFieldTest) { CreateTempFile("foo.proto", R"schema( edition = "2023"; @@ -290,8 +290,8 @@ TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) { RunProtoc( "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir foo.proto"); ExpectErrorSubstring( - "Field Foo.bar specifies ctype, but is not " - "a string nor bytes field."); + "Field Foo.bar specifies string_type, but is not a string nor bytes " + "field."); } TEST_F(CppGeneratorTest, CtypeOnExtensionTest) { @@ -307,8 +307,8 @@ TEST_F(CppGeneratorTest, CtypeOnExtensionTest) { RunProtoc( "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir foo.proto"); ExpectErrorSubstring( - "Extension bar specifies Cord type which is " - "not supported for extensions."); + "Extension bar specifies CORD string type which is not supported for " + "extensions"); } } // namespace } // namespace cpp diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 26581320e09d..dad7ca75d9af 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -3053,10 +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(); - } + } + if (has_legacy_proto_ctype()) { + proto->mutable_options()->set_ctype( + static_cast(legacy_proto_ctype())); } RestoreFeaturesToOptions(proto_features_, proto); @@ -3663,6 +3663,10 @@ void FieldDescriptor::DebugString( FieldOptions full_options = options(); CopyFeaturesToOptions(proto_features_, &full_options); + if (has_legacy_proto_ctype()) { + full_options.set_ctype( + static_cast(legacy_proto_ctype())); + } std::string formatted_options; if (FormatBracketedOptions(depth, full_options, file()->pool(), &formatted_options)) { @@ -3904,6 +3908,10 @@ void MethodDescriptor::DebugString( // Feature methods =============================================== +bool FieldDescriptor::has_legacy_proto_ctype() const { + return legacy_proto_ctype_ <= FieldOptions::CType_MAX; +} + bool EnumDescriptor::is_closed() const { return features().enum_type() == FeatureSet::CLOSED; } @@ -5527,23 +5535,6 @@ 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 (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, @@ -5635,6 +5626,13 @@ void DescriptorBuilder::PostProcessFieldFeatures( field.type_ = FieldDescriptor::TYPE_GROUP; } } + + if (field.options_->has_ctype()) { + field.legacy_proto_ctype_ = field.options_->ctype(); + const_cast( // NOLINT(google3-runtime-proto-const-cast) + field.options_) + ->clear_ctype(); + } } // A common pattern: We want to convert a repeated field in the descriptor @@ -6167,24 +6165,6 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl( 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, [&] { - return absl::StrFormat( - "Field %s specifies both string_type and ctype " - "which is not supported.", - field.full_name()); - }); - } - }); - // Handle feature resolution. This must occur after option interpretation, // but before validation. { @@ -6206,22 +6186,6 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl( }); } - internal::VisitDescriptors(*result, [&](const FieldDescriptor& field) { - if (result->edition() >= Edition::EDITION_2024 && - field.options().has_ctype()) { - // "ctype" is no longer supported in edition 2024 and beyond. - AddError( - field.full_name(), proto, DescriptorPool::ErrorCollector::NAME, - "ctype option is not allowed under edition 2024 and beyond. Use " - "the feature string_type = VIEW|CORD|STRING|... instead."); - } - 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, @@ -6617,6 +6581,7 @@ void DescriptorBuilder::BuildFieldOrExtension(const FieldDescriptorProto& proto, result->is_oneof_ = false; result->in_real_oneof_ = false; result->proto3_optional_ = proto.proto3_optional(); + result->legacy_proto_ctype_ = FieldOptions::CType_MAX + 1; if (proto.proto3_optional() && file_->edition() != Edition::EDITION_PROTO3) { AddError(result->full_name(), proto, DescriptorPool::ErrorCollector::TYPE, @@ -7965,6 +7930,13 @@ void DescriptorBuilder::ValidateOptions(const FieldDescriptor* field, ValidateFieldFeatures(field, proto); + if (field->file()->edition() >= Edition::EDITION_2024 && + field->has_legacy_proto_ctype()) { + AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::TYPE, + "ctype option is not allowed under edition 2024 and beyond. Use " + "the feature string_type = VIEW|CORD|STRING|... instead."); + } + // Only message type fields may be lazy. if (field->options().lazy() || field->options().unverified_lazy()) { if (field->type() != FieldDescriptor::TYPE_MESSAGE) { diff --git a/src/google/protobuf/descriptor.h b/src/google/protobuf/descriptor.h index 135765ffcbf0..3a7348130838 100644 --- a/src/google/protobuf/descriptor.h +++ b/src/google/protobuf/descriptor.h @@ -130,6 +130,7 @@ namespace compiler { class CodeGenerator; class CommandLineInterface; namespace cpp { +class CppGenerator; // Defined in helpers.h class Formatter; } // namespace cpp @@ -1078,6 +1079,14 @@ class PROTOBUF_EXPORT FieldDescriptor : private internal::SymbolBase, friend const std::string& internal::DefaultValueStringAsString( const FieldDescriptor* field); + // Returns the original ctype specified in the .proto file. This should not + // be relied on, as it no longer uniquely determines behavior. The + // cpp_string_type() method should be used instead, which takes feature + // settings into account. Needed by CppGenerator for validation only. + friend class compiler::cpp::CppGenerator; + int legacy_proto_ctype() const { return legacy_proto_ctype_; } + bool has_legacy_proto_ctype() const; + // Returns true if this field was syntactically written with "optional" in the // .proto file. Excludes singular proto3 fields that do not have a label. bool has_optional_keyword() const; @@ -1141,6 +1150,10 @@ class PROTOBUF_EXPORT FieldDescriptor : private internal::SymbolBase, // Located here for bitpacking. bool in_real_oneof_ : 1; + // Actually an optional `CType`, but stored as uint8_t to save space. This + // contains the original ctype option specified in the .proto file. + uint8_t legacy_proto_ctype_ : 2; + // Sadly, `number_` located here to reduce padding. Unrelated to all_names_ // and its indices above. int number_; @@ -1198,7 +1211,7 @@ class PROTOBUF_EXPORT FieldDescriptor : private internal::SymbolBase, friend class OneofDescriptor; }; -PROTOBUF_INTERNAL_CHECK_CLASS_SIZE(FieldDescriptor, 88); +PROTOBUF_INTERNAL_CHECK_CLASS_SIZE(FieldDescriptor, 96); // Describes a oneof defined in a message type. class PROTOBUF_EXPORT OneofDescriptor : private internal::SymbolBase { diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index c15f3a9b6be7..94f3fd2088fc 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -10295,7 +10295,7 @@ TEST_F(FeaturesTest, NoCtypeFromEdition2024) { } } )pb", - "foo.proto: Foo.bar: NAME: ctype option is not allowed under edition " + "foo.proto: Foo.bar: TYPE: ctype option is not allowed under edition " "2024 and beyond. Use the feature string_type = VIEW|CORD|STRING|... " "instead.\n"); } diff --git a/src/google/protobuf/no_field_presence_test.cc b/src/google/protobuf/no_field_presence_test.cc index 7f7c492a4e26..62f8a2a639b1 100644 --- a/src/google/protobuf/no_field_presence_test.cc +++ b/src/google/protobuf/no_field_presence_test.cc @@ -68,6 +68,7 @@ void CheckDefaultValues(const TestAllTypes& m) { EXPECT_EQ(TestAllTypes::FOO, m.optional_nested_enum()); EXPECT_EQ(FOREIGN_FOO, m.optional_foreign_enum()); + EXPECT_EQ(0, m.optional_string_piece().size()); EXPECT_EQ(0, m.repeated_int32_size()); EXPECT_EQ(0, m.repeated_int64_size()); @@ -89,6 +90,7 @@ void CheckDefaultValues(const TestAllTypes& m) { EXPECT_EQ(0, m.repeated_proto2_message_size()); EXPECT_EQ(0, m.repeated_nested_enum_size()); EXPECT_EQ(0, m.repeated_foreign_enum_size()); + EXPECT_EQ(0, m.repeated_string_piece_size()); EXPECT_EQ(0, m.repeated_lazy_message_size()); EXPECT_EQ(TestAllTypes::ONEOF_FIELD_NOT_SET, m.oneof_field_case()); } @@ -114,6 +116,7 @@ void FillValues(TestAllTypes* m) { m->mutable_optional_proto2_message()->set_optional_int32(44); m->set_optional_nested_enum(TestAllTypes::BAZ); m->set_optional_foreign_enum(FOREIGN_BAZ); + m->set_optional_string_piece("test"); m->mutable_optional_lazy_message()->set_bb(45); m->add_repeated_int32(100); m->add_repeated_int64(101); @@ -135,6 +138,7 @@ void FillValues(TestAllTypes* m) { m->add_repeated_proto2_message()->set_optional_int32(48); m->add_repeated_nested_enum(TestAllTypes::BAZ); m->add_repeated_foreign_enum(FOREIGN_BAZ); + m->add_repeated_string_piece("test"); m->add_repeated_lazy_message()->set_bb(49); m->set_oneof_uint32(1); @@ -166,6 +170,7 @@ void CheckNonDefaultValues(const TestAllTypes& m) { EXPECT_EQ(44, m.optional_proto2_message().optional_int32()); EXPECT_EQ(TestAllTypes::BAZ, m.optional_nested_enum()); EXPECT_EQ(FOREIGN_BAZ, m.optional_foreign_enum()); + EXPECT_EQ("test", m.optional_string_piece()); EXPECT_EQ(true, m.has_optional_lazy_message()); EXPECT_EQ(45, m.optional_lazy_message().bb()); @@ -209,6 +214,8 @@ void CheckNonDefaultValues(const TestAllTypes& m) { EXPECT_EQ(TestAllTypes::BAZ, m.repeated_nested_enum(0)); EXPECT_EQ(1, m.repeated_foreign_enum_size()); EXPECT_EQ(FOREIGN_BAZ, m.repeated_foreign_enum(0)); + EXPECT_EQ(1, m.repeated_string_piece_size()); + EXPECT_EQ("test", m.repeated_string_piece(0)); EXPECT_EQ(1, m.repeated_lazy_message_size()); EXPECT_EQ(49, m.repeated_lazy_message(0).bb()); @@ -733,7 +740,7 @@ TEST(NoFieldPresenceTest, ReflectionHasFieldTest) { if (field->is_repeated() || field->containing_oneof()) { continue; } - if (field->options().ctype() != FieldOptions::STRING) { + if (internal::cpp::IsStringFieldWithPrivatizedAccessors(*field)) { continue; } EXPECT_EQ(true, r->HasField(message, field)); @@ -1027,6 +1034,7 @@ TYPED_TEST(NoFieldPresenceSerializeTest, DontSerializeDefaultValuesTest) { message.set_optional_bytes(""); message.set_optional_nested_enum(TestAllTypes::FOO); // first enum entry message.set_optional_foreign_enum(FOREIGN_FOO); // first enum entry + message.set_optional_string_piece(""); ASSERT_TRUE(TestSerialize(message, &output_sink)); EXPECT_EQ(0, this->GetOutput().size());