Skip to content

Commit

Permalink
protoc: support identifiers as reserved names in addition to string l…
Browse files Browse the repository at this point in the history 
…iterals (only in editions) (#13471)

This addresses #13440.

@mkruskal-google, I saw you are assigned the above issue. I hope it's okay that I took a stab at it.

Closes #13471

COPYBARA_INTEGRATE_REVIEW=#13471 from jhump:jh/reserved-names-support-identifiers a23f18e
PiperOrigin-RevId: 555610263
  • Loading branch information
@jhump
jhump authored and Copybara-Service committed Aug 10, 2023
1 parent 43b1365 commit e701f4f
Show file tree
Hide file tree
Showing 3 changed files with 186 additions and 19 deletions.
68 changes: 66 additions & 2 deletions src/google/protobuf/compiler/parser.cc
View file
Expand Up @@ -1796,10 +1796,27 @@ bool Parser::ParseReserved(DescriptorProto* message,
// Parse the declaration.
DO(Consume("reserved"));
if (LookingAtType(io::Tokenizer::TYPE_STRING)) {
if (syntax_identifier_ == "editions") {
RecordError(
"Reserved names must be identifiers in editions, not string "
"literals.");
return false;
}
LocationRecorder location(message_location,
DescriptorProto::kReservedNameFieldNumber);
location.StartAt(start_token);
return ParseReservedNames(message, location);
} else if (LookingAtType(io::Tokenizer::TYPE_IDENTIFIER)) {
if (syntax_identifier_ != "editions") {
RecordError(
"Reserved names must be string literals. (Only editions supports "
"identifiers.)");
return false;
}
LocationRecorder location(message_location,
DescriptorProto::kReservedNameFieldNumber);
location.StartAt(start_token);
return ParseReservedIdentifiers(message, location);
} else {
LocationRecorder location(message_location,
DescriptorProto::kReservedRangeFieldNumber);
Expand Down Expand Up @@ -1828,7 +1845,25 @@ bool Parser::ParseReservedNames(DescriptorProto* message,
const LocationRecorder& parent_location) {
do {
LocationRecorder location(parent_location, message->reserved_name_size());
DO(ParseReservedName(message->add_reserved_name(), "Expected field name."));
DO(ParseReservedName(message->add_reserved_name(),
"Expected field name string literal."));
} while (TryConsume(","));
DO(ConsumeEndOfDeclaration(";", &parent_location));
return true;
}

bool Parser::ParseReservedIdentifier(std::string* name,
absl::string_view error_message) {
DO(ConsumeIdentifier(name, error_message));
return true;
}

bool Parser::ParseReservedIdentifiers(DescriptorProto* message,
const LocationRecorder& parent_location) {
do {
LocationRecorder location(parent_location, message->reserved_name_size());
DO(ParseReservedIdentifier(message->add_reserved_name(),
"Expected field name identifier."));
} while (TryConsume(","));
DO(ConsumeEndOfDeclaration(";", &parent_location));
return true;
Expand Down Expand Up @@ -1889,10 +1924,27 @@ bool Parser::ParseReserved(EnumDescriptorProto* proto,
// Parse the declaration.
DO(Consume("reserved"));
if (LookingAtType(io::Tokenizer::TYPE_STRING)) {
if (syntax_identifier_ == "editions") {
RecordError(
"Reserved names must be identifiers in editions, not string "
"literals.");
return false;
}
LocationRecorder location(enum_location,
EnumDescriptorProto::kReservedNameFieldNumber);
location.StartAt(start_token);
return ParseReservedNames(proto, location);
} else if (LookingAtType(io::Tokenizer::TYPE_IDENTIFIER)) {
if (syntax_identifier_ != "editions") {
RecordError(
"Reserved names must be string literals. (Only editions supports "
"identifiers.)");
return false;
}
LocationRecorder location(enum_location,
EnumDescriptorProto::kReservedNameFieldNumber);
location.StartAt(start_token);
return ParseReservedIdentifiers(proto, location);
} else {
LocationRecorder location(enum_location,
EnumDescriptorProto::kReservedRangeFieldNumber);
Expand All @@ -1905,7 +1957,19 @@ bool Parser::ParseReservedNames(EnumDescriptorProto* proto,
const LocationRecorder& parent_location) {
do {
LocationRecorder location(parent_location, proto->reserved_name_size());
DO(ParseReservedName(proto->add_reserved_name(), "Expected enum value."));
DO(ParseReservedName(proto->add_reserved_name(),
"Expected enum value string literal."));
} while (TryConsume(","));
DO(ConsumeEndOfDeclaration(";", &parent_location));
return true;
}

bool Parser::ParseReservedIdentifiers(EnumDescriptorProto* proto,
const LocationRecorder& parent_location) {
do {
LocationRecorder location(parent_location, proto->reserved_name_size());
DO(ParseReservedIdentifier(proto->add_reserved_name(),
"Expected enum value identifier."));
} while (TryConsume(","));
DO(ConsumeEndOfDeclaration(";", &parent_location));
return true;
Expand Down
6 changes: 6 additions & 0 deletions src/google/protobuf/compiler/parser.h
View file
Expand Up @@ -403,12 +403,18 @@ class PROTOBUF_EXPORT Parser {
bool ParseReservedNames(DescriptorProto* message,
const LocationRecorder& parent_location);
bool ParseReservedName(std::string* name, absl::string_view error_message);
bool ParseReservedIdentifiers(DescriptorProto* message,
const LocationRecorder& parent_location);
bool ParseReservedIdentifier(std::string* name,
absl::string_view error_message);
bool ParseReservedNumbers(DescriptorProto* message,
const LocationRecorder& parent_location);
bool ParseReserved(EnumDescriptorProto* message,
const LocationRecorder& message_location);
bool ParseReservedNames(EnumDescriptorProto* message,
const LocationRecorder& parent_location);
bool ParseReservedIdentifiers(EnumDescriptorProto* message,
const LocationRecorder& parent_location);
bool ParseReservedNumbers(EnumDescriptorProto* message,
const LocationRecorder& parent_location);

Expand Down
131 changes: 114 additions & 17 deletions src/google/protobuf/compiler/parser_unittest.cc
View file
Expand Up @@ -871,6 +871,22 @@ TEST_F(ParseMessageTest, ReservedNames) {
"}");
}

TEST_F(ParseMessageTest, ReservedIdentifiers) {
ExpectParsesTo(
"edition = \"2023\";\n"
"message TestMessage {\n"
" reserved foo, bar;\n"
"}\n",

"syntax: \"editions\" "
"edition: \"2023\" "
"message_type {"
" name: \"TestMessage\""
" reserved_name: \"foo\""
" reserved_name: \"bar\""
"}");
}

TEST_F(ParseMessageTest, ExtensionRange) {
ExpectParsesTo(
"message TestMessage {\n"
Expand Down Expand Up @@ -1229,6 +1245,24 @@ TEST_F(ParseEnumTest, ReservedNames) {
"}");
}

TEST_F(ParseEnumTest, ReservedIdentifiers) {
ExpectParsesTo(
"edition = \"2023\";\n"
"enum TestEnum {\n"
" FOO = 0;\n"
" reserved foo, bar;\n"
"}\n",

"syntax: \"editions\" "
"edition: \"2023\" "
"enum_type {"
" name: \"TestEnum\""
" value { name:\"FOO\" number:0 }"
" reserved_name: \"foo\""
" reserved_name: \"bar\""
"}");
}

// ===================================================================

typedef ParserTest ParseServiceTest;
Expand Down Expand Up @@ -1502,6 +1536,44 @@ TEST_F(ParseErrorTest, DuplicateJsonName) {
"1:41: Already set option \"json_name\".\n");
}

TEST_F(ParseErrorTest, MsgReservedIdentifierOnlyInEditions) {
ExpectHasErrors(
"message TestMessage {\n"
" reserved foo, bar;\n"
"}\n",
"1:11: Reserved names must be string literals. (Only editions supports "
"identifiers.)\n");
}
TEST_F(ParseErrorTest, MsgReservedNameStringNotInEditions) {
ExpectHasErrors(
"edition = \"2023\";\n"
"message TestMessage {\n"
" reserved \"foo\", \"bar\";\n"
"}\n",
"2:11: Reserved names must be identifiers in editions, not string "
"literals.\n");
}

TEST_F(ParseErrorTest, EnumReservedIdentifierOnlyInEditions) {
ExpectHasErrors(
"enum TestEnum {\n"
" FOO = 0;\n"
" reserved foo, bar;\n"
"}\n",
"2:11: Reserved names must be string literals. (Only editions supports "
"identifiers.)\n");
}
TEST_F(ParseErrorTest, EnumReservedNameStringNotInEditions) {
ExpectHasErrors(
"edition = \"2023\";\n"
"enum TestEnum {\n"
" FOO = 0;\n"
" reserved \"foo\", \"bar\";\n"
"}\n",
"3:11: Reserved names must be identifiers in editions, not string "
"literals.\n");
}

TEST_F(ParseErrorTest, EnumValueOutOfRange) {
ExpectHasErrors(
"enum TestEnum {\n"
Expand Down Expand Up @@ -1701,13 +1773,16 @@ TEST_F(ParseErrorTest, EnumValueMissingNumber) {
"1:5: Missing numeric value for enum constant.\n");
}

// NB: with editions, this would be accepted and would reserve a value name of
// "max"
TEST_F(ParseErrorTest, EnumReservedStandaloneMaxNotAllowed) {
ExpectHasErrors(
"enum TestEnum {\n"
" FOO = 1;\n"
" reserved max;\n"
"}\n",
"2:11: Expected enum value or number range.\n");
"2:11: Reserved names must be string literals. (Only editions supports "
"identifiers.)\n");
}

TEST_F(ParseErrorTest, EnumReservedMixNameAndNumber) {
Expand All @@ -1718,6 +1793,15 @@ TEST_F(ParseErrorTest, EnumReservedMixNameAndNumber) {
"}\n",
"2:15: Expected enum number range.\n");
}
TEST_F(ParseErrorTest, EnumReservedMixNameAndNumberEditions) {
ExpectHasErrors(
"edition = \"2023\";\n"
"enum TestEnum {\n"
" FOO = 1;\n"
" reserved 10, foo;\n"
"}\n",
"3:15: Expected enum number range.\n");
}

TEST_F(ParseErrorTest, EnumReservedPositiveNumberOutOfRange) {
ExpectHasErrors(
Expand All @@ -1743,29 +1827,33 @@ TEST_F(ParseErrorTest, EnumReservedMissingQuotes) {
" FOO = 1;\n"
" reserved foo;\n"
"}\n",
"2:11: Expected enum value or number range.\n");
"2:11: Reserved names must be string literals. (Only editions supports "
"identifiers.)\n");
}

TEST_F(ParseErrorTest, EnumReservedInvalidIdentifier) {
ExpectHasWarnings(
R"(
enum TestEnum {
FOO = 1;
reserved "foo bar";
}
)",
"3:17: Reserved name \"foo bar\" is not a valid identifier.\n");
R"schema(
enum TestEnum {
FOO = 1;
reserved "foo bar";
}
)schema",
"3:19: Reserved name \"foo bar\" is not a valid identifier.\n");
}

// -------------------------------------------------------------------
// Reserved field number errors

// NB: with editions, this would be accepted and would reserve a field name of
// "max"
TEST_F(ParseErrorTest, ReservedStandaloneMaxNotAllowed) {
ExpectHasErrors(
"message Foo {\n"
" reserved max;\n"
"}\n",
"1:11: Expected field name or number range.\n");
"1:11: Reserved names must be string literals. (Only editions supports "
"identifiers.)\n");
}

TEST_F(ParseErrorTest, ReservedMixNameAndNumber) {
Expand All @@ -1775,23 +1863,32 @@ TEST_F(ParseErrorTest, ReservedMixNameAndNumber) {
"}\n",
"1:15: Expected field number range.\n");
}
TEST_F(ParseErrorTest, ReservedMixNameAndNumberEditions) {
ExpectHasErrors(
"edition = \"2023\";\n"
"message Foo {\n"
" reserved 10, foo;\n"
"}\n",
"2:15: Expected field number range.\n");
}

TEST_F(ParseErrorTest, ReservedMissingQuotes) {
ExpectHasErrors(
"message Foo {\n"
" reserved foo;\n"
"}\n",
"1:11: Expected field name or number range.\n");
"1:11: Reserved names must be string literals. (Only editions supports "
"identifiers.)\n");
}

TEST_F(ParseErrorTest, ReservedInvalidIdentifier) {
ExpectHasWarnings(
R"(
message Foo {
reserved "foo bar";
}
)",
"2:17: Reserved name \"foo bar\" is not a valid identifier.\n");
R"schema(
message Foo {
reserved "foo bar";
}
)schema",
"2:19: Reserved name \"foo bar\" is not a valid identifier.\n");
}

TEST_F(ParseErrorTest, ReservedNegativeNumber) {
Expand Down

0 comments on commit e701f4f

Please sign in to comment.