Have the protoc CLI properly report any parser warnings.

Remove the naming warnings that would be considered noisy if they were suddenly enabled with no easy way to disable the warnings. The upcoming Edition 2024 naming style enforcement feature and a planned corresponding change to the linter which respects NOLINT comments will cover the cases that these warnings would.

The parser appears to have been accidentally silently suppressed on the CLI path all along; while they were reachable on other usages of the parser, those cases are secondary compared to what we consider the main entry point and so no longer having the naming warnings on those should not locally be a major loss.

PiperOrigin-RevId: 716223703

src/google/protobuf/compiler/command_line_interface_tester.cc

@@ -80,7 +80,7 @@ void CommandLineInterfaceTester::RunProtocWithArgs(
 
   return_code_ = cli_.Run(static_cast<int>(args.size()), argv.data());
 
-  error_text_ = GetCapturedTestStderr();
+  captured_stderr_ = GetCapturedTestStderr();
 #if !defined(__CYGWIN__)
   captured_stdout_ = GetCapturedTestStdout();
 #endif
@@ -116,33 +116,42 @@ void CommandLineInterfaceTester::CreateTempDir(absl::string_view name) {
 
 void CommandLineInterfaceTester::ExpectNoErrors() {
   EXPECT_EQ(0, return_code_);
-  EXPECT_EQ("", error_text_);
+
+  // Note: since warnings and errors are both simply printed to stderr, we
+  // can't holistically distinguish them here; in practice we don't have
+  // multiline warnings so just counting any line with 'warning:' in it
+  // is sufficient to separate warnings and errors in practice.
+  for (const auto& line :
+       absl::StrSplit(captured_stderr_, '\n', absl::SkipEmpty())) {
+    EXPECT_THAT(line, HasSubstr("warning:"));
+  }
 }
 
 void CommandLineInterfaceTester::ExpectErrorText(
     absl::string_view expected_text) {
   EXPECT_NE(0, return_code_);
-  EXPECT_THAT(error_text_, HasSubstr(absl::StrReplaceAll(
-                               expected_text, {{"$tmpdir", temp_directory_}})));
+  EXPECT_THAT(captured_stderr_,
+              HasSubstr(absl::StrReplaceAll(expected_text,
+                                            {{"$tmpdir", temp_directory_}})));
 }
 
 void CommandLineInterfaceTester::ExpectErrorSubstring(
     absl::string_view expected_substring) {
   EXPECT_NE(0, return_code_);
-  EXPECT_THAT(error_text_, HasSubstr(expected_substring));
+  EXPECT_THAT(captured_stderr_, HasSubstr(expected_substring));
 }
 
 void CommandLineInterfaceTester::ExpectWarningSubstring(
     absl::string_view expected_substring) {
   EXPECT_EQ(0, return_code_);
-  EXPECT_THAT(error_text_, HasSubstr(expected_substring));
+  EXPECT_THAT(captured_stderr_, HasSubstr(expected_substring));
 }
 
 #if defined(_WIN32) && !defined(__CYGWIN__)
 bool CommandLineInterfaceTester::HasAlternateErrorSubstring(
     const std::string& expected_substring) {
   EXPECT_NE(0, return_code_);
-  return error_text_.find(expected_substring) != std::string::npos;
+  return captured_stderr_.find(expected_substring) != std::string::npos;
 }
 #endif  // _WIN32 && !__CYGWIN__
 
@@ -162,7 +171,7 @@ void CommandLineInterfaceTester::
     ExpectCapturedStderrSubstringWithZeroReturnCode(
         absl::string_view expected_substring) {
   EXPECT_EQ(0, return_code_);
-  EXPECT_THAT(error_text_, HasSubstr(expected_substring));
+  EXPECT_THAT(captured_stderr_, HasSubstr(expected_substring));
 }
 
 void CommandLineInterfaceTester::ExpectFileContent(absl::string_view filename,

src/google/protobuf/compiler/command_line_interface_tester.h

@@ -136,10 +136,8 @@ class CommandLineInterfaceTester : public testing::Test {
   // The result of Run().
   int return_code_;
 
-  // The captured stderr output.
-  std::string error_text_;
+  std::string captured_stderr_;
 
-  // The captured stdout.
   std::string captured_stdout_;
 
   std::vector<std::unique_ptr<CodeGenerator>> generators_;

src/google/protobuf/compiler/command_line_interface_unittest.cc

@@ -19,6 +19,7 @@
 #include <gmock/gmock.h>
 #include "absl/log/absl_check.h"
 #include "absl/strings/escaping.h"
+#include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
 #include "absl/types/span.h"
 #include "google/protobuf/compiler/command_line_interface_tester.h"
@@ -4022,6 +4023,20 @@ TEST_F(CommandLineInterfaceTest,
       "extendee message foo.Foo");
 }
 
+TEST_F(CommandLineInterfaceTest, WarningForReservedNameNotIdentifier) {
+  CreateTempFile("foo.proto", R"schema(
+    syntax = "proto2";
+    package foo;
+    message Foo {
+      reserved "not ident";
+    })schema");
+
+  Run("protocol_compiler --test_out=$tmpdir --proto_path=$tmpdir foo.proto");
+  ExpectNoErrors();
+  ExpectWarningSubstring(
+      "Reserved name \"not ident\" is not a valid identifier.");
+}
+
 TEST_F(CommandLineInterfaceTest,
        ExtensionDeclarationVerificationDeclarationUndeclaredError) {
   CreateTempFile("foo.proto", R"schema(
@@ -4236,6 +4251,19 @@ class EncodeDecodeTest : public testing::TestWithParam<EncodeDecodeTestMode> {
     captured_stdout_ = GetCapturedTestStdout();
     captured_stderr_ = GetCapturedTestStderr();
 
+    // Note: since warnings and errors are both simply printed to stderr, we
+    // can't holistically distinguish them here; in practice we don't have
+    // multiline warnings so just counting any line with 'warning:' in it
+    // is sufficient to separate warnings and errors in practice.
+    for (const auto& line :
+         absl::StrSplit(StripCR(captured_stderr_), '\n', absl::SkipEmpty())) {
+      if (absl::StrContains(line, "warning:")) {
+        captured_warnings_.push_back(std::string(line));
+      } else {
+        captured_errors_.push_back(std::string(line));
+      }
+    }
+
     return result == 0;
   }
 
@@ -4257,6 +4285,30 @@ class EncodeDecodeTest : public testing::TestWithParam<EncodeDecodeTestMode> {
     ExpectStdoutMatchesText(expected_output);
   }
 
+  void ExpectNoErrors() { EXPECT_THAT(captured_errors_, testing::IsEmpty()); }
+
+  void ExpectNoWarnings() {
+    EXPECT_THAT(captured_warnings_, testing::IsEmpty());
+  }
+
+  void ExpectError(absl::string_view expected_text) {
+    EXPECT_THAT(captured_errors_, testing::Contains(expected_text));
+  }
+
+  void ExpectErrorSubstring(absl::string_view expected_substring) {
+    EXPECT_THAT(captured_errors_,
+                testing::Contains(testing::HasSubstr(expected_substring)));
+  }
+
+  void ExpectWarning(absl::string_view expected_text) {
+    EXPECT_THAT(captured_warnings_, testing::Contains(expected_text));
+  }
+
+  void ExpectWarningSubstring(absl::string_view expected_substring) {
+    EXPECT_THAT(captured_warnings_,
+                testing::Contains(testing::HasSubstr(expected_substring)));
+  }
+
   void ExpectStdoutMatchesText(const std::string& expected_text) {
     EXPECT_EQ(StripCR(expected_text), StripCR(captured_stdout_));
   }
@@ -4295,6 +4347,9 @@ class EncodeDecodeTest : public testing::TestWithParam<EncodeDecodeTestMode> {
   int duped_stdin_;
   std::string captured_stdout_;
   std::string captured_stderr_;
+  std::vector<std::string> captured_warnings_;
+  std::vector<std::string> captured_errors_;
+
   std::string unittest_proto_descriptor_set_filename_;
 };
 
@@ -4318,7 +4373,7 @@ TEST_P(EncodeDecodeTest, Encode) {
   EXPECT_TRUE(
       Run(absl::StrCat(args, " --encode=protobuf_unittest.TestAllTypes")));
   ExpectStdoutMatchesBinaryFile(golden_path);
-  ExpectStderrMatchesText("");
+  ExpectNoErrors();
 }
 
 TEST_P(EncodeDecodeTest, Decode) {
@@ -4331,7 +4386,7 @@ TEST_P(EncodeDecodeTest, Decode) {
   ExpectStdoutMatchesTextFile(TestUtil::GetTestDataPath(
       "google/protobuf/"
       "testdata/text_format_unittest_data_oneof_implemented.txt"));
-  ExpectStderrMatchesText("");
+  ExpectNoErrors();
 }
 
 TEST_P(EncodeDecodeTest, Partial) {
@@ -4340,8 +4395,7 @@ TEST_P(EncodeDecodeTest, Partial) {
       Run("google/protobuf/unittest.proto"
           " --encode=protobuf_unittest.TestRequired"));
   ExpectStdoutMatchesText("");
-  ExpectStderrMatchesText(
-      "warning:  Input message is missing required fields:  a, b, c\n");
+  ExpectWarning("warning:  Input message is missing required fields:  a, b, c");
 }
 
 TEST_P(EncodeDecodeTest, DecodeRaw) {
@@ -4356,7 +4410,7 @@ TEST_P(EncodeDecodeTest, DecodeRaw) {
   ExpectStdoutMatchesText(
       "1: 123\n"
       "14: \"foo\"\n");
-  ExpectStderrMatchesText("");
+  ExpectNoErrors();
 }
 
 TEST_P(EncodeDecodeTest, UnknownType) {
@@ -4364,7 +4418,7 @@ TEST_P(EncodeDecodeTest, UnknownType) {
       Run("google/protobuf/unittest.proto"
           " --encode=NoSuchType"));
   ExpectStdoutMatchesText("");
-  ExpectStderrMatchesText("Type not defined: NoSuchType\n");
+  ExpectError("Type not defined: NoSuchType");
 }
 
 TEST_P(EncodeDecodeTest, ProtoParseError) {
@@ -4372,8 +4426,9 @@ TEST_P(EncodeDecodeTest, ProtoParseError) {
       Run("net/proto2/internal/no_such_file.proto "
           "--encode=NoSuchType"));
   ExpectStdoutMatchesText("");
-  ExpectStderrContainsText(
-      "net/proto2/internal/no_such_file.proto: No such file or directory\n");
+  ExpectErrorSubstring(
+      "net/proto2/internal/no_such_file.proto: "
+      "No such file or directory");
 }
 
 TEST_P(EncodeDecodeTest, EncodeDeterministicOutput) {
@@ -4389,7 +4444,7 @@ TEST_P(EncodeDecodeTest, EncodeDeterministicOutput) {
   EXPECT_TRUE(Run(absl::StrCat(
       args, " --encode=protobuf_unittest.TestAllTypes --deterministic_output")));
   ExpectStdoutMatchesBinaryFile(golden_path);
-  ExpectStderrMatchesText("");
+  ExpectNoErrors();
 }
 
 TEST_P(EncodeDecodeTest, DecodeDeterministicOutput) {
@@ -4399,8 +4454,7 @@ TEST_P(EncodeDecodeTest, DecodeDeterministicOutput) {
   EXPECT_FALSE(
       Run("google/protobuf/unittest.proto"
           " --decode=protobuf_unittest.TestAllTypes --deterministic_output"));
-  ExpectStderrMatchesText(
-      "Can only use --deterministic_output with --encode.\n");
+  ExpectError("Can only use --deterministic_output with --encode.");
 }
 
 INSTANTIATE_TEST_SUITE_P(FileDescriptorSetSource, EncodeDecodeTest,

src/google/protobuf/compiler/importer.cc

@@ -94,6 +94,13 @@ class SourceTreeDescriptorDatabase::SingleFileErrorCollector
     had_errors_ = true;
   }
 
+  void RecordWarning(int line, int column, absl::string_view message) override {
+    if (multi_file_error_collector_ != nullptr) {
+      multi_file_error_collector_->RecordWarning(filename_, line, column,
+                                                 message);
+    }
+  }
+
  private:
   std::string filename_;
   MultiFileErrorCollector* multi_file_error_collector_;

src/google/protobuf/compiler/parser.cc

@@ -13,7 +13,6 @@
 
 #include "google/protobuf/compiler/parser.h"
 
-#include <float.h>
 
 #include <cstddef>
 #include <cstdint>
@@ -23,7 +22,6 @@
 #include <utility>
 #include <vector>
 
-#include "absl/base/casts.h"
 #include "absl/cleanup/cleanup.h"
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
@@ -39,8 +37,6 @@
 #include "google/protobuf/io/strtod.h"
 #include "google/protobuf/io/tokenizer.h"
 #include "google/protobuf/message_lite.h"
-#include "google/protobuf/port.h"
-#include "google/protobuf/wire_format.h"
 
 // Must be included last.
 #include "google/protobuf/port_def.inc"
@@ -106,57 +102,6 @@ std::string MapEntryName(absl::string_view field_name) {
   return result;
 }
 
-bool IsUppercase(char c) { return c >= 'A' && c <= 'Z'; }
-
-bool IsLowercase(char c) { return c >= 'a' && c <= 'z'; }
-
-bool IsNumber(char c) { return c >= '0' && c <= '9'; }
-
-bool IsUpperCamelCase(absl::string_view name) {
-  if (name.empty()) {
-    return true;
-  }
-  // Name must start with an upper case character.
-  if (!IsUppercase(name[0])) {
-    return false;
-  }
-  // Must not contains underscore.
-  for (const char c : name) {
-    if (c == '_') {
-      return false;
-    }
-  }
-  return true;
-}
-
-bool IsUpperUnderscore(absl::string_view name) {
-  for (const char c : name) {
-    if (!IsUppercase(c) && c != '_' && !IsNumber(c)) {
-      return false;
-    }
-  }
-  return true;
-}
-
-bool IsLowerUnderscore(absl::string_view name) {
-  for (const char c : name) {
-    if (!IsLowercase(c) && c != '_' && !IsNumber(c)) {
-      return false;
-    }
-  }
-  return true;
-}
-
-bool IsNumberFollowUnderscore(absl::string_view name) {
-  for (int i = 1; i < name.length(); i++) {
-    const char c = name[i];
-    if (IsNumber(c) && name[i - 1] == '_') {
-      return true;
-    }
-  }
-  return false;
-}
-
 }  // anonymous namespace
 
 // Makes code slightly more readable.  The meaning of "DO(foo)" is
@@ -626,22 +571,6 @@ bool Parser::ValidateEnum(const EnumDescriptorProto* proto) {
     return false;
   }
 
-  // Enforce that enum constants must be UPPER_CASE except in case of
-  // enum_alias.
-  if (!allow_alias) {
-    for (const auto& enum_value : proto->value()) {
-      if (!IsUpperUnderscore(enum_value.name())) {
-        RecordWarning([&] {
-          return absl::StrCat(
-              "Enum constant should be in UPPER_CASE. Found: ",
-              enum_value.name(),
-              ". See "
-              "https://developers.google.com/protocol-buffers/docs/style");
-        });
-      }
-    }
-  }
-
   return true;
 }
 
@@ -866,14 +795,6 @@ bool Parser::ParseMessageDefinition(
     location.RecordLegacyLocation(message,
                                   DescriptorPool::ErrorCollector::NAME);
     DO(ConsumeIdentifier(message->mutable_name(), "Expected message name."));
-    if (!IsUpperCamelCase(message->name())) {
-      RecordWarning([=] {
-        return absl::StrCat(
-            "Message name should be in UpperCamelCase. Found: ",
-            message->name(),
-            ". See https://developers.google.com/protocol-buffers/docs/style");
-      });
-    }
   }
   DO(ParseMessageBlock(message, message_location, containing_file));
 
@@ -1101,22 +1022,6 @@ bool Parser::ParseMessageFieldNoLabel(
                               FieldDescriptorProto::kNameFieldNumber);
     location.RecordLegacyLocation(field, DescriptorPool::ErrorCollector::NAME);
     DO(ConsumeIdentifier(field->mutable_name(), "Expected field name."));
-
-    if (!IsLowerUnderscore(field->name())) {
-      RecordWarning([=] {
-        return absl::StrCat(
-            "Field name should be lowercase. Found: ", field->name(),
-            ". See: https://developers.google.com/protocol-buffers/docs/style");
-      });
-    }
-    if (IsNumberFollowUnderscore(field->name())) {
-      RecordWarning([=] {
-        return absl::StrCat(
-            "Number should not come right after an underscore. Found: ",
-            field->name(),
-            ". See: https://developers.google.com/protocol-buffers/docs/style");
-      });
-    }
   }
   DO(Consume("=", "Missing field number."));
 
@@ -1861,6 +1766,11 @@ bool Parser::ParseReservedName(std::string* name, ErrorMaker error_message) {
   int col = input_->current().column;
   DO(ConsumeString(name, error_message));
   if (!io::Tokenizer::IsIdentifier(*name)) {
+    // Before Edition 2023, it was possible to reserve any string literal. This
+    // doesn't really make sense if the string literal wasn't a valid
+    // identifier, so warn about it here.
+    // Note that this warning is also load-bearing for tests that intend to
+    // verify warnings work as expected today.
     RecordWarning(line, col, [=] {
       return absl::StrFormat("Reserved name \"%s\" is not a valid identifier.",
                              *name);

src/google/protobuf/compiler/parser_unittest.cc

@@ -235,43 +235,6 @@ TEST_F(ParserTest, WarnIfSyntaxIdentifierOmitted) {
               std::string::npos);
 }
 
-TEST_F(ParserTest, WarnIfFieldNameIsNotUpperCamel) {
-  SetupParser(
-      "syntax = \"proto2\";"
-      "message abc {}");
-  FileDescriptorProto file;
-  EXPECT_TRUE(parser_->Parse(input_.get(), &file));
-  EXPECT_TRUE(error_collector_.warning_.find(
-                  "Message name should be in UpperCamelCase. Found: abc.") !=
-              std::string::npos);
-}
-
-TEST_F(ParserTest, WarnIfFieldNameIsNotLowerUnderscore) {
-  SetupParser(
-      "syntax = \"proto2\";"
-      "message A {"
-      "  optional string SongName = 1;"
-      "}");
-  FileDescriptorProto file;
-  EXPECT_TRUE(parser_->Parse(input_.get(), &file));
-  EXPECT_TRUE(error_collector_.warning_.find(
-                  "Field name should be lowercase. Found: SongName") !=
-              std::string::npos);
-}
-
-TEST_F(ParserTest, WarnIfFieldNameContainsNumberImmediatelyFollowUnderscore) {
-  SetupParser(
-      "syntax = \"proto2\";"
-      "message A {"
-      "  optional string song_name_1 = 1;"
-      "}");
-  FileDescriptorProto file;
-  EXPECT_TRUE(parser_->Parse(input_.get(), &file));
-  EXPECT_TRUE(error_collector_.warning_.find(
-                  "Number should not come right after an underscore. Found: "
-                  "song_name_1.") != std::string::npos);
-}
-
 TEST_F(ParserTest, RegressionNestedOpenBraceDoNotStackOverflow) {
   std::string input("edition=\"a\000;", 12);
   input += std::string(100000, '{');