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, '{');