From cafeaa41fb3c8d67eef0de11ed60ff539fd3318f Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 16 Jan 2025 07:12:56 -0800 Subject: [PATCH] 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 --- .../compiler/command_line_interface_tester.cc | 25 +++-- .../compiler/command_line_interface_tester.h | 4 +- .../command_line_interface_unittest.cc | 76 +++++++++++-- src/google/protobuf/compiler/importer.cc | 7 ++ src/google/protobuf/compiler/parser.cc | 100 +----------------- .../protobuf/compiler/parser_unittest.cc | 37 ------- 6 files changed, 95 insertions(+), 154 deletions(-) diff --git a/src/google/protobuf/compiler/command_line_interface_tester.cc b/src/google/protobuf/compiler/command_line_interface_tester.cc index e965d27348493..290227864ed68 100644 --- a/src/google/protobuf/compiler/command_line_interface_tester.cc +++ b/src/google/protobuf/compiler/command_line_interface_tester.cc @@ -80,7 +80,7 @@ void CommandLineInterfaceTester::RunProtocWithArgs( return_code_ = cli_.Run(static_cast(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, diff --git a/src/google/protobuf/compiler/command_line_interface_tester.h b/src/google/protobuf/compiler/command_line_interface_tester.h index 5f428c81933fb..4aa42bad02a54 100644 --- a/src/google/protobuf/compiler/command_line_interface_tester.h +++ b/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> generators_; diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index 848ce650460c5..480f7cd76b729 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -19,6 +19,7 @@ #include #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 { 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 { 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 { int duped_stdin_; std::string captured_stdout_; std::string captured_stderr_; + std::vector captured_warnings_; + std::vector 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, diff --git a/src/google/protobuf/compiler/importer.cc b/src/google/protobuf/compiler/importer.cc index 7a84e7d604b40..b38c28cd70c23 100644 --- a/src/google/protobuf/compiler/importer.cc +++ b/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_; diff --git a/src/google/protobuf/compiler/parser.cc b/src/google/protobuf/compiler/parser.cc index bc17e7a70ac43..298f0714560a9 100644 --- a/src/google/protobuf/compiler/parser.cc +++ b/src/google/protobuf/compiler/parser.cc @@ -13,7 +13,6 @@ #include "google/protobuf/compiler/parser.h" -#include #include #include @@ -23,7 +22,6 @@ #include #include -#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); diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 46f3db1633817..46d9d899fce70 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/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, '{');