Skip to content

Commit

Permalink
Breaking change: Strip ctype from options in C++
Browse files Browse the repository at this point in the history 
This has been replaced with the cpp_string_type helper on FieldDescriptor, which returns the actual behavior of the field rather than the specification.  This also handles merging of ctype and string_type in edition 2023 where both are allowed.  ctype will be banned from 2024 onward.

PiperOrigin-RevId: 709226325
  • Loading branch information
@mkruskal-google @copybara-github
mkruskal-google authored and copybara-github committed Dec 24, 2024
1 parent 1223341 commit aebf8b9
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 109 deletions.
61 changes: 17 additions & 44 deletions src/google/protobuf/compiler/cpp/generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 7 additions & 7 deletions src/google/protobuf/compiler/cpp/generator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -280,7 +280,7 @@ TEST_F(CppGeneratorTest, InheritedStringTypeCordNotForExtension) {
ExpectNoErrors();
}

TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) {
TEST_F(CppGeneratorTest, CtypeOnNonStringFieldTest) {
CreateTempFile("foo.proto",
R"schema(
edition = "2023";
Expand All @@ -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) {
Expand All @@ -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
Expand Down
82 changes: 27 additions & 55 deletions src/google/protobuf/descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<FieldOptions::CType>(legacy_proto_ctype()));
}

RestoreFeaturesToOptions(proto_features_, proto);
Expand Down Expand Up @@ -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<FieldOptions::CType>(legacy_proto_ctype()));
}
std::string formatted_options;
if (FormatBracketedOptions(depth, full_options, file()->pool(),
&formatted_options)) {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 <class DescriptorT>
void DescriptorBuilder::ResolveFeaturesImpl(
Edition edition, const typename DescriptorT::Proto& proto,
Expand Down Expand Up @@ -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<FieldOptions*>( // NOLINT(google3-runtime-proto-const-cast)
field.options_)
->clear_ctype();
}
}

// A common pattern: We want to convert a repeated field in the descriptor
Expand Down Expand Up @@ -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.
{
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
15 changes: 14 additions & 1 deletion src/google/protobuf/descriptor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ namespace compiler {
class CodeGenerator;
class CommandLineInterface;
namespace cpp {
class CppGenerator;
// Defined in helpers.h
class Formatter;
} // namespace cpp
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/google/protobuf/descriptor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
10 changes: 9 additions & 1 deletion src/google/protobuf/no_field_presence_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
}
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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());
Expand Down

0 comments on commit aebf8b9

Please sign in to comment.