From 5e7b709564403f1fe22476f7efa1eeff61e995cb Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Tue, 24 May 2022 15:52:18 -0700 Subject: [PATCH] Revert "Various fixups to warnings flags (#9344)" (#10037) This reverts commit 448d421250569811a3e656cd4c87fe42e857ed6f. Unfortunately we have to revert this because we're finding that it introduces too much new build log spam for existing proto files that are out of compliance with the warnings. We might be able to roll it forward again if we can figure out a way to do so without so many new log messages. --- .../command_line_interface_unittest.cc | 25 +++++-------------- src/google/protobuf/compiler/importer.cc | 6 ----- src/google/protobuf/compiler/parser.cc | 12 ++++----- .../protobuf/compiler/parser_unittest.cc | 3 ++- 4 files changed, 13 insertions(+), 33 deletions(-) diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index 99fa0ace39ddd..7bde1b403e3e2 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -1217,8 +1217,7 @@ TEST_F(CommandLineInterfaceTest, InsertWithAnnotationFixup) { "--plug_out=insert_endlines=test_generator,test_plugin:$tmpdir " "--proto_path=$tmpdir foo.proto"); - ExpectWarningSubstring( - "foo.proto:2:36: warning: Message name should be in UpperCamelCase."); + ExpectNoErrors(); CheckGeneratedAnnotations("test_generator", "foo.proto"); CheckGeneratedAnnotations("test_plugin", "foo.proto"); } @@ -2372,21 +2371,6 @@ 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 @@ -2707,6 +2691,7 @@ TEST_P(EncodeDecodeTest, Encode) { EXPECT_TRUE(Run(args + " --encode=protobuf_unittest.TestAllTypes")); ExpectStdoutMatchesBinaryFile(TestUtil::GetTestDataPath( "third_party/protobuf/testdata/golden_message_oneof_implemented")); + ExpectStderrMatchesText(""); } TEST_P(EncodeDecodeTest, Decode) { @@ -2718,6 +2703,7 @@ TEST_P(EncodeDecodeTest, Decode) { ExpectStdoutMatchesTextFile(TestUtil::GetTestDataPath( "third_party/protobuf/" "testdata/text_format_unittest_data_oneof_implemented.txt")); + ExpectStderrMatchesText(""); } TEST_P(EncodeDecodeTest, Partial) { @@ -2726,7 +2712,7 @@ TEST_P(EncodeDecodeTest, Partial) { Run(TestUtil::MaybeTranslatePath("net/proto2/internal/unittest.proto") + " --encode=protobuf_unittest.TestRequired")); ExpectStdoutMatchesText(""); - ExpectStderrContainsText( + ExpectStderrMatchesText( "warning: Input message is missing required fields: a, b, c\n"); } @@ -2750,7 +2736,7 @@ TEST_P(EncodeDecodeTest, UnknownType) { Run(TestUtil::MaybeTranslatePath("net/proto2/internal/unittest.proto") + " --encode=NoSuchType")); ExpectStdoutMatchesText(""); - ExpectStderrContainsText("Type not defined: NoSuchType\n"); + ExpectStderrMatchesText("Type not defined: NoSuchType\n"); } TEST_P(EncodeDecodeTest, ProtoParseError) { @@ -2775,6 +2761,7 @@ TEST_P(EncodeDecodeTest, EncodeDeterministicOutput) { args + " --encode=protobuf_unittest.TestAllTypes --deterministic_output")); ExpectStdoutMatchesBinaryFile(TestUtil::GetTestDataPath( "third_party/protobuf/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 index 595bcd1276376..f1e26f8bdd1d3 100644 --- a/src/google/protobuf/compiler/importer.cc +++ b/src/google/protobuf/compiler/importer.cc @@ -105,12 +105,6 @@ 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 index 472394dbba04d..5bd37d147bc44 100644 --- a/src/google/protobuf/compiler/parser.cc +++ b/src/google/protobuf/compiler/parser.cc @@ -643,11 +643,10 @@ 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_) { - AddWarning( - "No syntax specified. Please use 'syntax = \"proto2\";' or " - "'syntax = \"proto3\";' to specify a syntax version. " - "(Defaulted to proto2 syntax.)" - ); + 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.)"; syntax_identifier_ = "proto2"; } @@ -1020,8 +1019,7 @@ bool Parser::ParseMessageFieldNoLabel( location.RecordLegacyLocation(field, DescriptorPool::ErrorCollector::NAME); DO(ConsumeIdentifier(field->mutable_name(), "Expected field name.")); - if (field->type() != FieldDescriptorProto::TYPE_GROUP && - !IsLowerUnderscore(field->name())) { + if (!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 index 83f591dc3c74f..2d681d957f7ee 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -221,8 +221,9 @@ TEST_F(ParserTest, StopAfterSyntaxIdentifierWithErrors) { TEST_F(ParserTest, WarnIfSyntaxIdentifierOmmitted) { SetupParser("message A {}"); FileDescriptorProto file; + CaptureTestStderr(); EXPECT_TRUE(parser_->Parse(input_.get(), &file)); - EXPECT_TRUE(error_collector_.warning_.find("No syntax specified") != + EXPECT_TRUE(GetCapturedTestStderr().find("No syntax specified") != std::string::npos); }