Various fixups to warnings flags (#9344)

* Don't drop parser warnings on the floor Fix #9343. * Convert "missing syntax" warning to an actual warning For some reason this warning was emitted as a log message rather than a structured warning. Convert it to use the AddWarning API so that it gets emitted with a file and line number by protoc, and is visible via the error collection interface during programmatic use. * Remove CaptureTestStderr() call CaptureTestStderr() and GetCapturedTestStderr() have to be paired with each other. * Adjust tests for new warnings A few tests now produce warnings that they didn't before, but were expecting not to see any stderr output. Adjust the tests accordingly. Co-authored-by: Adam Cozzette <[email protected]>
protocolbuffers · May 13, 2022 · 448d421 · 448d421
1 parent 366cb84
commit 448d421
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 13 deletions.
diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc
@@ -1217,7 +1217,8 @@ TEST_F(CommandLineInterfaceTest, InsertWithAnnotationFixup) {
       "--plug_out=insert_endlines=test_generator,test_plugin:$tmpdir "
       "--proto_path=$tmpdir foo.proto");
 
-  ExpectNoErrors();
+  ExpectWarningSubstring(
+      "foo.proto:2:36: warning: Message name should be in UpperCamelCase.");
   CheckGeneratedAnnotations("test_generator", "foo.proto");
   CheckGeneratedAnnotations("test_plugin", "foo.proto");
 }
@@ -2371,6 +2372,21 @@ TEST_F(CommandLineInterfaceTest, Warnings) {
   ExpectErrorSubstring("foo.proto:2:1: warning: Import bar.proto is unused.");
 }
 
+TEST_F(CommandLineInterfaceTest, ParserWarnings) {
+  // Test that parser warnings are propagated. See #9343.
+
+  CreateTempFile("foo.proto",
+                 "syntax = \"proto2\";\n"
+                 "message bad_to_the_bone {};\n");
+
+  Run("protocol_compiler --test_out=$tmpdir "
+      "--proto_path=$tmpdir foo.proto");
+  ExpectCapturedStderrSubstringWithZeroReturnCode(
+      "foo.proto:2:25: warning: Message name should be in UpperCamelCase. "
+      "Found: bad_to_the_bone. "
+      "See https://developers.google.com/protocol-buffers/docs/style");
+}
+
 // -------------------------------------------------------------------
 // Flag parsing tests
 
@@ -2691,7 +2707,6 @@ TEST_P(EncodeDecodeTest, Encode) {
   EXPECT_TRUE(Run(args + " --encode=protobuf_unittest.TestAllTypes"));
   ExpectStdoutMatchesBinaryFile(TestUtil::GetTestDataPath(
       "net/proto2/internal/testdata/golden_message_oneof_implemented"));
-  ExpectStderrMatchesText("");
 }
 
 TEST_P(EncodeDecodeTest, Decode) {
@@ -2703,7 +2718,6 @@ TEST_P(EncodeDecodeTest, Decode) {
   ExpectStdoutMatchesTextFile(TestUtil::GetTestDataPath(
       "net/proto2/internal/"
       "testdata/text_format_unittest_data_oneof_implemented.txt"));
-  ExpectStderrMatchesText("");
 }
 
 TEST_P(EncodeDecodeTest, Partial) {
@@ -2712,7 +2726,7 @@ TEST_P(EncodeDecodeTest, Partial) {
       Run(TestUtil::MaybeTranslatePath("net/proto2/internal/unittest.proto") +
           " --encode=protobuf_unittest.TestRequired"));
   ExpectStdoutMatchesText("");
-  ExpectStderrMatchesText(
+  ExpectStderrContainsText(
       "warning:  Input message is missing required fields:  a, b, c\n");
 }
 
@@ -2736,7 +2750,7 @@ TEST_P(EncodeDecodeTest, UnknownType) {
       Run(TestUtil::MaybeTranslatePath("net/proto2/internal/unittest.proto") +
           " --encode=NoSuchType"));
   ExpectStdoutMatchesText("");
-  ExpectStderrMatchesText("Type not defined: NoSuchType\n");
+  ExpectStderrContainsText("Type not defined: NoSuchType\n");
 }
 
 TEST_P(EncodeDecodeTest, ProtoParseError) {
@@ -2761,7 +2775,6 @@ TEST_P(EncodeDecodeTest, EncodeDeterministicOutput) {
       args + " --encode=protobuf_unittest.TestAllTypes --deterministic_output"));
   ExpectStdoutMatchesBinaryFile(TestUtil::GetTestDataPath(
       "net/proto2/internal/testdata/golden_message_oneof_implemented"));
-  ExpectStderrMatchesText("");
 }
 
 TEST_P(EncodeDecodeTest, DecodeDeterministicOutput) {

diff --git a/src/google/protobuf/compiler/importer.cc b/src/google/protobuf/compiler/importer.cc
@@ -105,6 +105,12 @@ class SourceTreeDescriptorDatabase::SingleFileErrorCollector
     had_errors_ = true;
   }
 
+  void AddWarning(int line, int column, const std::string& message) override {
+    if (multi_file_error_collector_ != NULL) {
+      multi_file_error_collector_->AddWarning(filename_, line, column, message);
+    }
+  }
+
  private:
   std::string filename_;
   MultiFileErrorCollector* multi_file_error_collector_;

diff --git a/src/google/protobuf/compiler/parser.cc b/src/google/protobuf/compiler/parser.cc
@@ -643,10 +643,11 @@ bool Parser::Parse(io::Tokenizer* input, FileDescriptorProto* file) {
       // Store the syntax into the file.
       if (file != nullptr) file->set_syntax(syntax_identifier_);
     } else if (!stop_after_syntax_identifier_) {
-      GOOGLE_LOG(WARNING) << "No syntax specified for the proto file: " << file->name()
-                   << ". Please use 'syntax = \"proto2\";' "
-                   << "or 'syntax = \"proto3\";' to specify a syntax "
-                   << "version. (Defaulted to proto2 syntax.)";
+      AddWarning(
+        "No syntax specified. Please use 'syntax = \"proto2\";' or "
+        "'syntax = \"proto3\";' to specify a syntax version. "
+        "(Defaulted to proto2 syntax.)"
+      );
       syntax_identifier_ = "proto2";
     }
 
@@ -1019,7 +1020,8 @@ bool Parser::ParseMessageFieldNoLabel(
     location.RecordLegacyLocation(field, DescriptorPool::ErrorCollector::NAME);
     DO(ConsumeIdentifier(field->mutable_name(), "Expected field name."));
 
-    if (!IsLowerUnderscore(field->name())) {
+    if (field->type() != FieldDescriptorProto::TYPE_GROUP &&
+        !IsLowerUnderscore(field->name())) {
       AddWarning(
           "Field name should be lowercase. Found: " + field->name() +
           ". See: https://developers.google.com/protocol-buffers/docs/style");

diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc
@@ -221,9 +221,8 @@ TEST_F(ParserTest, StopAfterSyntaxIdentifierWithErrors) {
 TEST_F(ParserTest, WarnIfSyntaxIdentifierOmmitted) {
   SetupParser("message A {}");
   FileDescriptorProto file;
-  CaptureTestStderr();
   EXPECT_TRUE(parser_->Parse(input_.get(), &file));
-  EXPECT_TRUE(GetCapturedTestStderr().find("No syntax specified") !=
+  EXPECT_TRUE(error_collector_.warning_.find("No syntax specified") !=
               std::string::npos);
 }