Skip to content

Commit

Permalink
Have the protoc CLI properly report any parser warnings.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@protobuf-github-bot @copybara-github
protobuf-github-bot authored and copybara-github committed Jan 16, 2025
1 parent 94ae26d commit cafeaa4
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 154 deletions.
25 changes: 17 additions & 8 deletions src/google/protobuf/compiler/command_line_interface_tester.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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__

Expand All @@ -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,
Expand Down
4 changes: 1 addition & 3 deletions src/google/protobuf/compiler/command_line_interface_tester.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down
76 changes: 65 additions & 11 deletions src/google/protobuf/compiler/command_line_interface_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
}

Expand All @@ -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_));
}
Expand Down Expand Up @@ -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_;
};

Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -4356,24 +4410,25 @@ TEST_P(EncodeDecodeTest, DecodeRaw) {
ExpectStdoutMatchesText(
"1: 123\n"
"14: \"foo\"\n");
ExpectStderrMatchesText("");
ExpectNoErrors();
}

TEST_P(EncodeDecodeTest, UnknownType) {
EXPECT_FALSE(
Run("google/protobuf/unittest.proto"
" --encode=NoSuchType"));
ExpectStdoutMatchesText("");
ExpectStderrMatchesText("Type not defined: NoSuchType\n");
ExpectError("Type not defined: NoSuchType");
}

TEST_P(EncodeDecodeTest, ProtoParseError) {
EXPECT_FALSE(
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) {
Expand All @@ -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) {
Expand All @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions src/google/protobuf/compiler/importer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down
Loading

0 comments on commit cafeaa4

Please sign in to comment.