From 34ba95bb71946936e087986f5fe7eea7d96c7016 Mon Sep 17 00:00:00 2001
From: Josh Humphries <jhumphries@buf.build>
Date: Fri, 26 Aug 2022 16:36:36 -0400
Subject: [PATCH] json_name and default pseudo-options have source code info
 consistent with options

---
 src/google/protobuf/compiler/parser.cc        | 40 +++++++++++++++----
 src/google/protobuf/compiler/parser.h         |  4 ++
 .../protobuf/compiler/parser_unittest.cc      |  7 ++--
 3 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/src/google/protobuf/compiler/parser.cc b/src/google/protobuf/compiler/parser.cc
index f220fad8be..2ab823981e 100644
--- a/src/google/protobuf/compiler/parser.cc
+++ b/src/google/protobuf/compiler/parser.cc
@@ -408,6 +408,11 @@ Parser::LocationRecorder::LocationRecorder(const LocationRecorder& parent) {
   Init(parent, parent.source_code_info_);
 }
 
+Parser::LocationRecorder::LocationRecorder(const LocationRecorder& parent,
+                                           SourceCodeInfo* source_code_info) {
+  Init(parent, source_code_info);
+}
+
 Parser::LocationRecorder::LocationRecorder(const LocationRecorder& parent,
                                            int path1,
                                            SourceCodeInfo* source_code_info) {
@@ -1237,13 +1242,22 @@ bool Parser::ParseDefaultAssignment(
     field->clear_default_value();
   }
 
+  LocationRecorder location(field_location,
+                            FieldDescriptorProto::kDefaultValueFieldNumber);
+
   DO(Consume("default"));
   DO(Consume("="));
 
-  LocationRecorder location(field_location,
-                            FieldDescriptorProto::kDefaultValueFieldNumber);
-  location.RecordLegacyLocation(field,
-                                DescriptorPool::ErrorCollector::DEFAULT_VALUE);
+  // We don't need to create separate spans in source code info for name and value,
+  // since there's no way to represent them distinctly in a location path. But we will
+  // want a separate recorder for the value, just to have more precise location info
+  // in error messages. So we let it create a location in no_op, so it doesn't add a
+  // span to the file descriptor.
+  SourceCodeInfo no_op;
+  LocationRecorder value_location(location, &no_op);
+  value_location.RecordLegacyLocation(
+      field, DescriptorPool::ErrorCollector::DEFAULT_VALUE);
+
   std::string* default_value = field->mutable_default_value();
 
   if (!field->has_type()) {
@@ -1377,13 +1391,23 @@ bool Parser::ParseJsonName(FieldDescriptorProto* field,
 
   LocationRecorder location(field_location,
                             FieldDescriptorProto::kJsonNameFieldNumber);
-  location.RecordLegacyLocation(field,
-                                DescriptorPool::ErrorCollector::OPTION_NAME);
 
-  DO(Consume("json_name"));
+  // We don't need to create separate spans in source code info for name and value,
+  // since there's no way to represent them distinctly in a location path. But we will
+  // want a separate recorder for them, just to have more precise location info
+  // in error messages. So we let them create a location in no_op, so they don't
+  // add a span to the file descriptor.
+  SourceCodeInfo no_op;
+  {
+    LocationRecorder name_location(location, &no_op);
+    name_location.RecordLegacyLocation(
+        field, DescriptorPool::ErrorCollector::OPTION_NAME);
+
+    DO(Consume("json_name"));
+  }
   DO(Consume("="));
 
-  LocationRecorder value_location(location);
+  LocationRecorder value_location(location, &no_op);
   value_location.RecordLegacyLocation(
       field, DescriptorPool::ErrorCollector::OPTION_VALUE);
 
diff --git a/src/google/protobuf/compiler/parser.h b/src/google/protobuf/compiler/parser.h
index d4eb76302c..8b05d0e4ad 100644
--- a/src/google/protobuf/compiler/parser.h
+++ b/src/google/protobuf/compiler/parser.h
@@ -237,6 +237,10 @@ class PROTOBUF_EXPORT Parser {
     LocationRecorder(const LocationRecorder& parent, int path1, int path2);
 
     // Creates a recorder that generates locations into given source code info.
+    LocationRecorder(const LocationRecorder& parent,
+                     SourceCodeInfo* source_code_info);
+    // Creates a recorder that generates locations into given source code info
+    // and calls AddPath() one time.
     LocationRecorder(const LocationRecorder& parent, int path1,
                      SourceCodeInfo* source_code_info);
 
diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc
index 55ed7acb6f..a164d7d54a 100644
--- a/src/google/protobuf/compiler/parser_unittest.cc
+++ b/src/google/protobuf/compiler/parser_unittest.cc
@@ -3415,18 +3415,19 @@ TEST_F(SourceInfoTest, FieldOptions) {
   EXPECT_TRUE(
       Parse("message Foo {"
             "  optional int32 bar = 1 "
-            "$a$[default=$b$123$c$,$d$opt1=123$e$,"
-            "$f$opt2='hi'$g$]$h$;"
+            "$a$[$b$default=123$c$, $d$opt1=123$e$, "
+            "$f$opt2='hi'$g$, $h$json_name='barBar'$i$]$j$;"
             "}\n"));
 
   const FieldDescriptorProto& field = file_.message_type(0).field(0);
   const UninterpretedOption& option1 = field.options().uninterpreted_option(0);
   const UninterpretedOption& option2 = field.options().uninterpreted_option(1);
 
-  EXPECT_TRUE(HasSpan('a', 'h', field.options()));
+  EXPECT_TRUE(HasSpan('a', 'j', field.options()));
   EXPECT_TRUE(HasSpan('b', 'c', field, "default_value"));
   EXPECT_TRUE(HasSpan('d', 'e', option1));
   EXPECT_TRUE(HasSpan('f', 'g', option2));
+  EXPECT_TRUE(HasSpan('h', 'i', field, "json_name"));
 
   // Ignore these.
   EXPECT_TRUE(HasSpan(file_));