protoc: parser rejects explicit use of map_entry option (#13479)

This addresses #13441.

This preserves the similar check at the _point of use_ of invalid messages in `DescriptorBuilder` (and there's an existing test that verifies that check still works).

But it adds another check in the parser, to catch this error at the _point of definition_ of an invalid message. And the corresponding test is updated: we no longer need a usage of the message to catch the error, and the reported position is the definition of the option, not the usage site of the message.

The way this works feels kinda gross, but I wasn't sure of a better way to do it. The only place we know for certain that it was an explicit option (vs. auto-added by the parser when synthesizing a map entry message) is when after processing the message body, we can look at the uninterpreted options. So that's what this does. If you have ideas on better/cleaner approaches, I'd be happy to revise.

Closes #13479

COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/13479 from jhump:jh/map-option-not-allowed 4d95e9b839
PiperOrigin-RevId: 555577734

src/google/protobuf/compiler/parser.cc

@@ -571,6 +571,26 @@ void Parser::SkipRestOfBlock() {
 
 // ===================================================================
 
+bool Parser::ValidateMessage(const DescriptorProto* proto) {
+  for (int i = 0; i < proto->options().uninterpreted_option_size(); i++) {
+    const UninterpretedOption& option =
+        proto->options().uninterpreted_option(i);
+    if (option.name_size() > 0 && !option.name(0).is_extension() &&
+        option.name(0).name_part() == "map_entry") {
+      int line = -1, col = 0;  // indicates line and column not known
+      if (source_location_table_ != nullptr) {
+        source_location_table_->Find(
+            &option, DescriptorPool::ErrorCollector::OPTION_NAME, &line, &col);
+      }
+      RecordError(line, col,
+                  "map_entry should not be set explicitly. "
+                  "Use map<KeyType, ValueType> instead.");
+      return false;
+    }
+  }
+  return true;
+}
+
 bool Parser::ValidateEnum(const EnumDescriptorProto* proto) {
   bool has_allow_alias = false;
   bool allow_alias = false;
@@ -661,9 +681,8 @@ bool Parser::Parse(io::Tokenizer* input, FileDescriptorProto* file) {
     root_location.RecordLegacyLocation(file,
                                        DescriptorPool::ErrorCollector::OTHER);
 
-    if (require_syntax_identifier_ || LookingAt("syntax")
-        || LookingAt("edition")
-    ) {
+    if (require_syntax_identifier_ || LookingAt("syntax") ||
+        LookingAt("edition")) {
       if (!ParseSyntaxIdentifier(file, root_location)) {
         // Don't attempt to parse the file if we didn't recognize the syntax
         // identifier.
@@ -867,6 +886,7 @@ bool IsMessageSetWireFormatMessage(const DescriptorProto& message) {
   for (int i = 0; i < options.uninterpreted_option_size(); ++i) {
     const UninterpretedOption& uninterpreted = options.uninterpreted_option(i);
     if (uninterpreted.name_size() == 1 &&
+        !uninterpreted.name(0).is_extension() &&
         uninterpreted.name(0).name_part() == "message_set_wire_format" &&
         uninterpreted.identifier_value() == "true") {
       return true;
@@ -931,6 +951,9 @@ bool Parser::ParseMessageBlock(DescriptorProto* message,
   if (message->reserved_range_size() > 0) {
     AdjustReservedRangesWithMaxEndNumber(message);
   }
+
+  DO(ValidateMessage(message));
+
   return true;
 }
 

src/google/protobuf/compiler/parser.h

@@ -534,6 +534,7 @@ class PROTOBUF_EXPORT Parser {
     return syntax_identifier_ == "proto3";
   }
 
+  bool ValidateMessage(const DescriptorProto* proto);
   bool ValidateEnum(const EnumDescriptorProto* proto);
 
   // =================================================================

src/google/protobuf/compiler/parser_unittest.cc

@@ -171,6 +171,8 @@ class ParserTest : public testing::Test {
   // input.
   void ExpectHasEarlyExitErrors(const char* text, const char* expected_errors) {
     SetupParser(text);
+    SourceLocationTable source_locations;
+    parser_->RecordSourceLocationsTo(&source_locations);
     FileDescriptorProto file;
     EXPECT_FALSE(parser_->Parse(input_.get(), &file));
     EXPECT_EQ(expected_errors, error_collector_.text_);
@@ -1746,12 +1748,12 @@ TEST_F(ParseErrorTest, EnumReservedMissingQuotes) {
 
 TEST_F(ParseErrorTest, EnumReservedInvalidIdentifier) {
   ExpectHasWarnings(
-      R"pb(
+      R"(
       enum TestEnum {
         FOO = 1;
         reserved "foo bar";
       }
-      )pb",
+      )",
       "3:17: Reserved name \"foo bar\" is not a valid identifier.\n");
 }
 
@@ -1784,11 +1786,11 @@ TEST_F(ParseErrorTest, ReservedMissingQuotes) {
 
 TEST_F(ParseErrorTest, ReservedInvalidIdentifier) {
   ExpectHasWarnings(
-      R"pb(
+      R"(
       message Foo {
         reserved "foo bar";
       }
-      )pb",
+      )",
       "2:17: Reserved name \"foo bar\" is not a valid identifier.\n");
 }
 
@@ -2235,7 +2237,7 @@ TEST_F(ParserValidationErrorTest, EnumValueAliasError) {
 }
 
 TEST_F(ParserValidationErrorTest, ExplicitlyMapEntryError) {
-  ExpectHasValidationErrors(
+  ExpectHasErrors(
       "message Foo {\n"
       "  message ValueEntry {\n"
       "    option map_entry = true;\n"
@@ -2243,9 +2245,8 @@ TEST_F(ParserValidationErrorTest, ExplicitlyMapEntryError) {
       "    optional int32 value = 2;\n"
       "    extensions 99 to 999;\n"
       "  }\n"
-      "  repeated ValueEntry value = 1;\n"
       "}",
-      "7:11: map_entry should not be set explicitly. Use "
+      "2:11: map_entry should not be set explicitly. Use "
       "map<KeyType, ValueType> instead.\n");
 }
 
@@ -2950,7 +2951,7 @@ class SourceInfoTest : public ParserTest {
       }
     }
 
-      return false;
+    return false;
   }
 
  private:
@@ -4143,7 +4144,6 @@ TEST_F(ParseEditionsTest, FeaturesWithoutEditions) {
 }
 
 
-
 }  // anonymous namespace
 
 }  // namespace compiler