Skip to content

Commit

Permalink
Lock down ctype=CORD in proto file.
Browse files Browse the repository at this point in the history 
ctype can not be used for none string/bytes fields. ctye=CORD can not be used for extensions. A new macro PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE is added that will be flipped for our next breaking release.

PiperOrigin-RevId: 555615362
  • Loading branch information
@anandolee
anandolee authored and Copybara-Service committed Aug 10, 2023
1 parent e701f4f commit b359e50
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 13 deletions.
6 changes: 3 additions & 3 deletions csharp/protos/unittest.proto
View file
Expand Up @@ -258,7 +258,7 @@ extend TestAllExtensions {
optional_import_enum_extension = 23;

optional string optional_string_piece_extension = 24 [ctype = STRING_PIECE];
optional string optional_cord_extension = 25 [ctype = CORD];
optional string optional_cord_extension = 25;

optional protobuf_unittest_import_proto2.PublicImportMessage
optional_public_import_message_extension = 26;
Expand Down Expand Up @@ -298,7 +298,7 @@ extend TestAllExtensions {
repeated_import_enum_extension = 53;

repeated string repeated_string_piece_extension = 54 [ctype = STRING_PIECE];
repeated string repeated_cord_extension = 55 [ctype = CORD];
repeated string repeated_cord_extension = 55;

repeated TestAllTypes.NestedMessage repeated_lazy_message_extension = 57
[lazy = true];
Expand Down Expand Up @@ -329,7 +329,7 @@ extend TestAllExtensions {

optional string default_string_piece_extension = 84
[ctype = STRING_PIECE, default = "abc"];
optional string default_cord_extension = 85 [ctype = CORD, default = "123"];
optional string default_cord_extension = 85 [default = "123"];

// For oneof test
optional uint32 oneof_uint32_extension = 111;
Expand Down
6 changes: 3 additions & 3 deletions objectivec/Tests/unittest.proto
View file
Expand Up @@ -244,7 +244,7 @@ extend TestAllExtensions {
optional_import_enum_extension = 23;

optional string optional_string_piece_extension = 24 [ctype=STRING_PIECE];
optional string optional_cord_extension = 25 [ctype=CORD];
optional string optional_cord_extension = 25;

optional objc.protobuf.tests.public_import.Message
optional_public_import_message_extension = 26;
Expand Down Expand Up @@ -286,7 +286,7 @@ extend TestAllExtensions {
repeated_import_enum_extension = 53;

repeated string repeated_string_piece_extension = 54 [ctype=STRING_PIECE];
repeated string repeated_cord_extension = 55 [ctype=CORD];
repeated string repeated_cord_extension = 55;

repeated TestAllTypes.NestedMessage
repeated_lazy_message_extension = 57 [lazy=true];
Expand Down Expand Up @@ -317,7 +317,7 @@ extend TestAllExtensions {

optional string default_string_piece_extension = 84 [ctype=STRING_PIECE,
default="abc"];
optional string default_cord_extension = 85 [ctype=CORD, default="123"];
optional string default_cord_extension = 85 [default="123"];

// For oneof test
optional uint32 oneof_uint32_extension = 111;
Expand Down
16 changes: 16 additions & 0 deletions src/google/protobuf/compiler/cpp/generator.cc
View file
Expand Up @@ -379,6 +379,22 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const {
absl::StrCat("Field ", field.full_name(),
" has a closed enum type with implicit presence."));
}
#ifdef PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE
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 ctype=CORD which is not supported for extensions."));
}
}
}
#endif // !PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE
});
return status;
}
Expand Down
34 changes: 34 additions & 0 deletions src/google/protobuf/compiler/cpp/generator_unittest.cc
View file
Expand Up @@ -172,6 +172,40 @@ TEST_F(CppGeneratorTest, LegacyClosedEnumImplicit) {
"Field Foo.bar has a closed enum type with implicit presence.");
}

#ifdef PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE
TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) {
CreateTempFile("foo.proto",
R"schema(
edition = "2023";
message Foo {
int32 bar = 1 [ctype=STRING];
})schema");
RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir "
"--experimental_editions foo.proto");
ExpectErrorSubstring(
"Field Foo.bar specifies ctype, but is not "
"a string nor bytes field.");
}

TEST_F(CppGeneratorTest, CtypeOnExtensionTest) {
CreateTempFile("foo.proto",
R"schema(
edition = "2023";
message Foo {
extensions 1 to max;
}
extend Foo {
bytes bar = 1 [ctype=CORD];
})schema");
RunProtoc(
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir "
"--experimental_editions foo.proto");
ExpectErrorSubstring(
"Extension bar specifies ctype=CORD which is "
"not supported for extensions.");
}
#endif // !PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE
} // namespace
} // namespace cpp
} // namespace compiler
Expand Down
4 changes: 4 additions & 0 deletions src/google/protobuf/port_def.inc
View file
Expand Up @@ -246,6 +246,10 @@ static_assert(PROTOBUF_ABSL_MIN(20230125, 3),
// Owner: mkruskal@
#define PROTOBUF_FUTURE_EXTENSION_RANGE_CLASS 1

// Used to lock down wrong ctype usages in proto file.
// Owner: jieluo@
#define PROTOBUF_FUTURE_REMOVE_WRONG_CTYPE 1

#endif

#ifdef PROTOBUF_VERSION
Expand Down
12 changes: 9 additions & 3 deletions src/google/protobuf/unittest.proto
View file
Expand Up @@ -267,7 +267,9 @@ extend TestAllExtensions {
optional_import_enum_extension = 23;

optional string optional_string_piece_extension = 24 [ctype=STRING_PIECE];
optional string optional_cord_extension = 25 [ctype=CORD];
// TODO(b/294946958): ctype=CORD is not supported for extension. Add
// ctype=CORD option back after it is supported.
optional string optional_cord_extension = 25;

optional protobuf_unittest_import.PublicImportMessage
optional_public_import_message_extension = 26;
Expand Down Expand Up @@ -309,7 +311,9 @@ extend TestAllExtensions {
repeated_import_enum_extension = 53;

repeated string repeated_string_piece_extension = 54 [ctype=STRING_PIECE];
repeated string repeated_cord_extension = 55 [ctype=CORD];
// TODO(b/294946958): ctype=CORD is not supported for extension. Add
// ctype=CORD option back after it is supported.
repeated string repeated_cord_extension = 55;

repeated TestAllTypes.NestedMessage
repeated_lazy_message_extension = 57 [lazy=true];
Expand Down Expand Up @@ -340,7 +344,9 @@ extend TestAllExtensions {

optional string default_string_piece_extension = 84 [ctype=STRING_PIECE,
default="abc"];
optional string default_cord_extension = 85 [ctype=CORD, default="123"];
// TODO(b/294946958): ctype=CORD is not supported for extension. Add
// ctype=CORD option back after it is supported.
optional string default_cord_extension = 85 [default="123"];

// For oneof test
optional uint32 oneof_uint32_extension = 111;
Expand Down
13 changes: 9 additions & 4 deletions src/google/protobuf/unittest_lite.proto
View file
Expand Up @@ -243,7 +243,9 @@ extend TestAllExtensionsLite {

optional string optional_string_piece_extension_lite = 24
[ctype = STRING_PIECE];
optional string optional_cord_extension_lite = 25 [ctype = CORD];
// TODO(b/294946958): ctype=CORD is not supported for extension. Add
// ctype=CORD option back after it is supported.
optional string optional_cord_extension_lite = 25;

optional protobuf_unittest_import.PublicImportMessageLite
optional_public_import_message_extension_lite = 26;
Expand Down Expand Up @@ -288,7 +290,9 @@ extend TestAllExtensionsLite {

repeated string repeated_string_piece_extension_lite = 54
[ctype = STRING_PIECE];
repeated string repeated_cord_extension_lite = 55 [ctype = CORD];
// TODO(b/294946958): ctype=CORD is not supported for extension. Add
// ctype=CORD option back after it is supported.
repeated string repeated_cord_extension_lite = 55;

repeated TestAllTypesLite.NestedMessage repeated_lazy_message_extension_lite =
57 [lazy = true];
Expand Down Expand Up @@ -319,8 +323,9 @@ extend TestAllExtensionsLite {

optional string default_string_piece_extension_lite = 84
[ctype = STRING_PIECE, default = "abc"];
optional string default_cord_extension_lite = 85
[ctype = CORD, default = "123"];
// TODO(b/294946958): ctype=CORD is not supported for extension. Add
// ctype=CORD option back after it is supported.
optional string default_cord_extension_lite = 85 [default = "123"];

// For oneof test
optional uint32 oneof_uint32_extension_lite = 111;
Expand Down

0 comments on commit b359e50

Please sign in to comment.