Fix cord extension check to apply to all editions.

This was mistakenly gated on edition and only applied to proto2/proto3 and edition 2023.

This also cleans up some of our validation logic for ctype/string_type.  Similar to other language features, ctype will only be validated by the C++ generator.  This means that protos that aren't used in C++ may end up with ctype incorrectly specified, but our Prototiller transformation for 2024 will strip those anyway (since we ban ctype in 2024).

PiperOrigin-RevId: 676893004
pull/18372/head
Mike Kruskal 5 months ago committed by Copybara-Service
parent 3abdf99eec
commit ac29f8c0cd
  1. 15
      src/google/protobuf/compiler/cpp/generator.cc
  2. 25
      src/google/protobuf/compiler/cpp/generator_unittest.cc
  3. 26
      src/google/protobuf/descriptor.cc
  4. 23
      src/google/protobuf/descriptor_unittest.cc

@ -355,8 +355,6 @@ static bool IsEnumMapType(const FieldDescriptor& field) {
absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const {
absl::Status status = absl::OkStatus();
auto edition = GetEdition(*file);
google::protobuf::internal::VisitDescriptors(*file, [&](const FieldDescriptor& field) {
const FeatureSet& resolved_features = GetResolvedSourceFeatures(field);
const pb::CppFeatures& unresolved_features =
@ -413,8 +411,7 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const {
}
}
// 'ctype' check has moved to DescriptorBuilder for Edition 2023 and above.
if (edition < Edition::EDITION_2023 && field.options().has_ctype()) {
if (field.options().has_ctype()) {
if (field.cpp_type() != FieldDescriptor::CPPTYPE_STRING) {
status = absl::FailedPreconditionError(absl::StrCat(
"Field ", field.full_name(),
@ -424,10 +421,18 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const {
if (field.is_extension()) {
status = absl::FailedPreconditionError(absl::StrCat(
"Extension ", field.full_name(),
" specifies ctype=CORD which is not supported for extensions."));
" specifies Cord type which is not supported for extensions."));
}
}
}
if (field.cpp_type() == FieldDescriptor::CPPTYPE_STRING &&
field.cpp_string_type() == FieldDescriptor::CppStringType::kCord &&
field.is_extension()) {
status = absl::FailedPreconditionError(absl::StrCat(
"Extension ", field.full_name(),
" specifies Cord type which is not supported for extensions."));
}
});
return status;
}

@ -255,10 +255,31 @@ TEST_F(CppGeneratorTest, StringTypeCordNotForExtension) {
"--experimental_editions foo.proto");
ExpectErrorSubstring(
"Extension bar specifies string_type=CORD which is not supported for "
"Extension bar specifies Cord type which is not supported for "
"extensions.");
}
TEST_F(CppGeneratorTest, InheritedStringTypeCordNotForExtension) {
CreateTempFile("foo.proto", R"schema(
edition = "2024";
import "google/protobuf/cpp_features.proto";
option features.(pb.cpp).string_type = CORD;
message Foo {
extensions 1 to max;
}
extend Foo {
bytes bar = 1;
}
)schema");
RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir "
"--experimental_editions foo.proto");
ExpectNoErrors();
}
TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) {
CreateTempFile("foo.proto",
R"schema(
@ -286,7 +307,7 @@ TEST_F(CppGeneratorTest, CtypeOnExtensionTest) {
RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir foo.proto");
ExpectErrorSubstring(
"Extension bar specifies ctype=CORD which is "
"Extension bar specifies Cord type which is "
"not supported for extensions.");
}
} // namespace

@ -7952,32 +7952,6 @@ void DescriptorBuilder::ValidateOptions(const FieldDescriptor* field,
ValidateFieldFeatures(field, proto);
auto edition = field->file()->edition();
auto& field_options = field->options();
if (field_options.has_ctype()) {
if (edition == Edition::EDITION_2023) {
if (field->cpp_type() != FieldDescriptor::CPPTYPE_STRING) {
AddError(field->full_name(), proto,
DescriptorPool::ErrorCollector::TYPE,
absl::StrFormat("Field %s specifies ctype, but is not a "
"string nor bytes field.",
field->full_name())
.c_str());
}
if (field_options.ctype() == FieldOptions::CORD) {
if (field->is_extension()) {
AddError(field->full_name(), proto,
DescriptorPool::ErrorCollector::TYPE,
absl::StrFormat("Extension %s specifies ctype=CORD which is "
"not supported for extensions.",
field->full_name())
.c_str());
}
}
}
}
// Only message type fields may be lazy.
if (field->options().lazy() || field->options().unverified_lazy()) {
if (field->type() != FieldDescriptor::TYPE_MESSAGE) {

@ -2957,29 +2957,6 @@ TEST_F(MiscTest, DefaultValues) {
EXPECT_EQ(enum_value_a, message->field(22)->default_value_enum());
}
TEST_F(MiscTest, InvalidFieldOptions) {
FileDescriptorProto file_proto;
file_proto.set_name("foo.proto");
file_proto.set_syntax("editions");
file_proto.set_edition(Edition::EDITION_2023);
DescriptorProto* message_proto = AddMessage(&file_proto, "TestMessage");
AddField(message_proto, "foo", 1, FieldDescriptorProto::LABEL_OPTIONAL,
FieldDescriptorProto::TYPE_INT32);
FieldDescriptorProto* bar_proto =
AddField(message_proto, "bar", 2, FieldDescriptorProto::LABEL_OPTIONAL,
FieldDescriptorProto::TYPE_INT32);
FieldOptions* options = bar_proto->mutable_options();
options->set_ctype(FieldOptions::CORD);
// Expects it to fail as int32 fields cannot have ctype.
DescriptorPool pool;
const FileDescriptor* file = pool.BuildFile(file_proto);
EXPECT_EQ(file, nullptr);
}
TEST_F(MiscTest, FieldOptions) {
// Try setting field options.

Loading…
Cancel
Save