Skip to content

Commit

Permalink
require identifier in editions
Browse files Browse the repository at this point in the history
  • Loading branch information
@jhump
jhump committed Aug 8, 2023
1 parent e60fea8 commit a23f18e
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 20 deletions.
26 changes: 20 additions & 6 deletions src/google/protobuf/compiler/parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1773,12 +1773,19 @@ 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) && syntax_identifier_ == "editions") {
// As of first edition, we also accept identifiers instead of string literals.
} 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);
Expand Down Expand Up @@ -1811,7 +1818,7 @@ 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;
Expand Down Expand Up @@ -1888,12 +1895,19 @@ 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) && syntax_identifier_ == "editions") {
// As of first edition, we also accept identifiers instead of string literals.
} 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);
Expand All @@ -1910,7 +1924,7 @@ 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;
Expand Down
45 changes: 31 additions & 14 deletions src/google/protobuf/compiler/parser_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1539,16 +1539,15 @@ TEST_F(ParseErrorTest, MsgReservedIdentifierOnlyInEditions) {
"message TestMessage {\n"
" reserved foo, bar;\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, MsgReservedIdentifierCantMixWithStrings) {
TEST_F(ParseErrorTest, MsgReservedNameStringNotInEditions) {
ExpectHasErrors(
"edition = \"2023\";\n"
"message TestMessage {\n"
" reserved foo, \"bar\";\n"
" reserved \"foo\", \"bar\";\n"
"}\n",
"2:16: Expected field name identifier.\n");
"2:11: Reserved names must be identifiers in editions, not string literals.\n");
}

TEST_F(ParseErrorTest, EnumReservedIdentifierOnlyInEditions) {
Expand All @@ -1557,17 +1556,16 @@ TEST_F(ParseErrorTest, EnumReservedIdentifierOnlyInEditions) {
" FOO = 0;\n"
" reserved foo, bar;\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, EnumReservedIdentifierCantMixWithStrings) {
TEST_F(ParseErrorTest, EnumReservedNameStringNotInEditions) {
ExpectHasErrors(
"edition = \"2023\";\n"
"enum TestEnum {\n"
" FOO = 0;\n"
" reserved foo, \"bar\";\n"
" reserved \"foo\", \"bar\";\n"
"}\n",
"3:16: Expected enum value identifier.\n");
"3:11: Reserved names must be identifiers in editions, not string literals.\n");
}

TEST_F(ParseErrorTest, EnumValueOutOfRange) {
Expand Down Expand Up @@ -1769,13 +1767,14 @@ 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 @@ -1786,6 +1785,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 @@ -1811,7 +1819,7 @@ 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) {
Expand All @@ -1828,12 +1836,13 @@ TEST_F(ParseErrorTest, EnumReservedInvalidIdentifier) {
// -------------------------------------------------------------------
// 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 @@ -1843,13 +1852,21 @@ 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) {
Expand Down

0 comments on commit a23f18e

Please sign in to comment.