Merge pull request #10586 from jhump/jh/validate-reserved-names

protoc: validate reserved names are identifiers

src/google/protobuf/compiler/parser.cc

@@ -51,6 +51,7 @@
 #include "absl/strings/ascii.h"
 #include "absl/strings/escaping.h"
 #include "absl/strings/str_cat.h"
+#include "absl/strings/str_format.h"
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/descriptor.pb.h"
 #include "google/protobuf/io/tokenizer.h"
@@ -405,13 +406,16 @@ void Parser::AddError(const std::string& error) {
   AddError(input_->current().line, input_->current().column, error);
 }
 
-void Parser::AddWarning(const std::string& warning) {
+void Parser::AddWarning(int line, int column, const std::string& warning) {
   if (error_collector_ != nullptr) {
-    error_collector_->AddWarning(input_->current().line,
+    error_collector_->AddWarning(line, column, warning);
-                                 input_->current().column, warning);
   }
 }
 
+void Parser::AddWarning(const std::string& warning) {
+  AddWarning(input_->current().line, input_->current().column, warning);
+}
+
 // -------------------------------------------------------------------
 
 Parser::LocationRecorder::LocationRecorder(Parser* parser)
@@ -1770,11 +1774,23 @@ bool Parser::ParseReserved(DescriptorProto* message,
   }
 }
 
+bool Parser::ParseReservedName(std::string* name, const char* error_message) {
+  // Capture the position of the token, in case we have to report an
+  // error after it is consumed.
+  int line = input_->current().line;
+  int col = input_->current().column;
+  DO(ConsumeString(name, error_message));
+  if (!io::Tokenizer::IsIdentifier(*name)) {
+    AddWarning(line, col, absl::StrFormat("Reserved name \"%s\" is not a valid identifier.", *name));
+  }
+  return true;
+}
+
 bool Parser::ParseReservedNames(DescriptorProto* message,
                                 const LocationRecorder& parent_location) {
   do {
     LocationRecorder location(parent_location, message->reserved_name_size());
-    DO(ConsumeString(message->add_reserved_name(), "Expected field name."));
+    DO(ParseReservedName(message->add_reserved_name(), "Expected field name."));
   } while (TryConsume(","));
   DO(ConsumeEndOfDeclaration(";", &parent_location));
   return true;
@@ -1829,42 +1845,42 @@ bool Parser::ParseReservedNumbers(DescriptorProto* message,
   return true;
 }
 
-bool Parser::ParseReserved(EnumDescriptorProto* message,
+bool Parser::ParseReserved(EnumDescriptorProto* proto,
-                           const LocationRecorder& message_location) {
+                           const LocationRecorder& enum_location) {
   io::Tokenizer::Token start_token = input_->current();
   // Parse the declaration.
   DO(Consume("reserved"));
   if (LookingAtType(io::Tokenizer::TYPE_STRING)) {
-    LocationRecorder location(message_location,
+    LocationRecorder location(enum_location,
                               EnumDescriptorProto::kReservedNameFieldNumber);
     location.StartAt(start_token);
-    return ParseReservedNames(message, location);
+    return ParseReservedNames(proto, location);
   } else {
-    LocationRecorder location(message_location,
+    LocationRecorder location(enum_location,
                               EnumDescriptorProto::kReservedRangeFieldNumber);
     location.StartAt(start_token);
-    return ParseReservedNumbers(message, location);
+    return ParseReservedNumbers(proto, location);
   }
 }
 
-bool Parser::ParseReservedNames(EnumDescriptorProto* message,
+bool Parser::ParseReservedNames(EnumDescriptorProto* proto,
                                 const LocationRecorder& parent_location) {
   do {
-    LocationRecorder location(parent_location, message->reserved_name_size());
+    LocationRecorder location(parent_location, proto->reserved_name_size());
-    DO(ConsumeString(message->add_reserved_name(), "Expected enum value."));
+    DO(ParseReservedName(proto->add_reserved_name(), "Expected enum value."));
   } while (TryConsume(","));
   DO(ConsumeEndOfDeclaration(";", &parent_location));
   return true;
 }
 
-bool Parser::ParseReservedNumbers(EnumDescriptorProto* message,
+bool Parser::ParseReservedNumbers(EnumDescriptorProto* proto,
                                   const LocationRecorder& parent_location) {
   bool first = true;
   do {
-    LocationRecorder location(parent_location, message->reserved_range_size());
+    LocationRecorder location(parent_location, proto->reserved_range_size());
 
     EnumDescriptorProto::EnumReservedRange* range =
-        message->add_reserved_range();
+        proto->add_reserved_range();
     int start, end;
     io::Tokenizer::Token start_token;
     {

src/google/protobuf/compiler/parser.h

@@ -216,6 +216,9 @@ class PROTOBUF_EXPORT Parser {
   // of the current token.
   void AddError(const std::string& error);
 
+  // Invokes error_collector_->AddWarning(), if error_collector_ is not NULL.
+  void AddWarning(int line, int column, const std::string& warning);
+
   // Invokes error_collector_->AddWarning() with the line and column number
   // of the current token.
   void AddWarning(const std::string& warning);
@@ -401,6 +404,7 @@ class PROTOBUF_EXPORT Parser {
                      const LocationRecorder& message_location);
   bool ParseReservedNames(DescriptorProto* message,
                           const LocationRecorder& parent_location);
+  bool ParseReservedName(std::string* name, const char* error_message);
   bool ParseReservedNumbers(DescriptorProto* message,
                             const LocationRecorder& parent_location);
   bool ParseReserved(EnumDescriptorProto* message,

src/google/protobuf/compiler/parser_unittest.cc

@@ -147,6 +147,16 @@ class ParserTest : public testing::Test {
     EXPECT_EQ(io::Tokenizer::TYPE_END, input_->current().type);
   }
 
+  // Parse the text and expect that the given warnings are reported.
+  void ExpectHasWarnings(const char* text, const char* expected_warnings) {
+    SetupParser(text);
+    FileDescriptorProto file;
+    ASSERT_TRUE(parser_->Parse(input_.get(), &file));
+    EXPECT_EQ(io::Tokenizer::TYPE_END, input_->current().type);
+    ASSERT_EQ("", error_collector_.text_);
+    EXPECT_EQ(expected_warnings, error_collector_.warning_);
+  }
+
   // Same as above but does not expect that the parser parses the complete
   // input.
   void ExpectHasEarlyExitErrors(const char* text, const char* expected_errors) {
@@ -1725,6 +1735,17 @@ TEST_F(ParseErrorTest, EnumReservedMissingQuotes) {
       "2:11: Expected enum value or number range.\n");
 }
 
+TEST_F(ParseErrorTest, EnumReservedInvalidIdentifier) {
+  ExpectHasWarnings(
+      R"pb(
+      enum TestEnum {
+        FOO = 1;
+        reserved "foo bar";
+      }
+      )pb",
+      "3:17: Reserved name \"foo bar\" is not a valid identifier.\n");
+}
+
 // -------------------------------------------------------------------
 // Reserved field number errors
 
@@ -1752,6 +1773,16 @@ TEST_F(ParseErrorTest, ReservedMissingQuotes) {
       "1:11: Expected field name or number range.\n");
 }
 
+TEST_F(ParseErrorTest, ReservedInvalidIdentifier) {
+  ExpectHasWarnings(
+      R"pb(
+      message Foo {
+        reserved "foo bar";
+      }
+      )pb",
+      "2:17: Reserved name \"foo bar\" is not a valid identifier.\n");
+}
+
 TEST_F(ParseErrorTest, ReservedNegativeNumber) {
   ExpectHasErrors(
       "message Foo {\n"