From a7823bbd459e2edbc3bdbc4997ccd31b1a48a75b Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Thu, 30 Sep 2021 15:46:52 -0400 Subject: [PATCH] Ensure errors are reported from the last chunk of a file. - Fix case where no error was provide if the consumer rejected something in the final chunk of input. - Expose a more testable interface for the parser. - Test all the basic behaviors of the parser. --- .../compiler/objectivec/objectivec_helpers.cc | 36 ++++--- .../compiler/objectivec/objectivec_helpers.h | 6 ++ .../objectivec/objectivec_helpers_unittest.cc | 102 ++++++++++++++++++ 3 files changed, 131 insertions(+), 13 deletions(-) diff --git a/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc b/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc index a90adbad9..9a443c7c9 100644 --- a/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc +++ b/src/google/protobuf/compiler/objectivec/objectivec_helpers.cc @@ -1675,16 +1675,12 @@ bool Parser::ParseChunk(StringPiece chunk) { } bool Parser::Finish() { - if (leftover_.empty()) { - return true; - } - // Force a newline onto the end to finish parsing. - leftover_ += "\n"; - p_ = StringPiece(leftover_); - if (!ParseLoop()) { + // If there is still something to go, flush it with a newline. + if (!leftover_.empty() && !ParseChunk("\n")) { return false; } - return p_.empty(); // Everything used? + // This really should never fail if ParseChunk succeeded, but check to be sure. + return leftover_.empty(); } bool Parser::ParseLoop() { @@ -1703,6 +1699,11 @@ bool Parser::ParseLoop() { return true; } +std::string ParserErrorString(const Parser& parser, const std::string& name) { + return std::string("error: ") + name + " Line " + + StrCat(parser.last_line()) + ", " + parser.error_str(); +} + } // namespace LineConsumer::LineConsumer() {} @@ -1723,22 +1724,31 @@ bool ParseSimpleFile(const std::string& path, LineConsumer* line_consumer, io::FileInputStream file_stream(fd); file_stream.SetCloseOnDelete(true); + return ParseSimpleStream(file_stream, path, line_consumer, out_error); +} + +bool ParseSimpleStream(io::ZeroCopyInputStream& input_stream, + const std::string& stream_name, + LineConsumer* line_consumer, + std::string* out_error) { Parser parser(line_consumer); const void* buf; int buf_len; - while (file_stream.Next(&buf, &buf_len)) { + while (input_stream.Next(&buf, &buf_len)) { if (buf_len == 0) { continue; } if (!parser.ParseChunk(StringPiece(static_cast(buf), buf_len))) { - *out_error = - std::string("error: ") + path + - " Line " + StrCat(parser.last_line()) + ", " + parser.error_str(); + *out_error = ParserErrorString(parser, stream_name); return false; } } - return parser.Finish(); + if (!parser.Finish()) { + *out_error = ParserErrorString(parser, stream_name); + return false; + } + return true; } ImportWriter::ImportWriter( diff --git a/src/google/protobuf/compiler/objectivec/objectivec_helpers.h b/src/google/protobuf/compiler/objectivec/objectivec_helpers.h index aa2211e5e..c5f948c84 100644 --- a/src/google/protobuf/compiler/objectivec/objectivec_helpers.h +++ b/src/google/protobuf/compiler/objectivec/objectivec_helpers.h @@ -38,6 +38,7 @@ #include #include +#include #include @@ -287,6 +288,11 @@ bool PROTOC_EXPORT ParseSimpleFile(const std::string& path, LineConsumer* line_consumer, std::string* out_error); +bool PROTOC_EXPORT ParseSimpleStream(io::ZeroCopyInputStream& input_stream, + const std::string& stream_name, + LineConsumer* line_consumer, + std::string* out_error); + // Helper class for parsing framework import mappings and generating // import statements. class PROTOC_EXPORT ImportWriter { diff --git a/src/google/protobuf/compiler/objectivec/objectivec_helpers_unittest.cc b/src/google/protobuf/compiler/objectivec/objectivec_helpers_unittest.cc index 0aef94fe3..bfbdf5a8e 100644 --- a/src/google/protobuf/compiler/objectivec/objectivec_helpers_unittest.cc +++ b/src/google/protobuf/compiler/objectivec/objectivec_helpers_unittest.cc @@ -29,6 +29,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include +#include #include #include @@ -242,6 +243,107 @@ TEST(ObjCHelperDeathTest, TextFormatDecodeData_Failures) { } #endif // PROTOBUF_HAS_DEATH_TEST +class TestLineCollector : public LineConsumer { + public: + TestLineCollector(std::vector* inout_lines, + const std::string* reject_line = nullptr) + : lines_(inout_lines), reject_(reject_line) {} + + bool ConsumeLine(const StringPiece& line, std::string* out_error) override { + if (reject_) { + if (*reject_ == line) { + *out_error = std::string("Rejected '") + *reject_ + "'"; + return false; + } + } + if (lines_) { + lines_->emplace_back(line); + } + return true; + } + + private: + std::vector* lines_; + const std::string* reject_; +}; + +const int kBlockSizes[] = {-1, 1, 2, 5, 64}; +const int kBlockSizeCount = GOOGLE_ARRAYSIZE(kBlockSizes); + +TEST(ObjCHelper, ParseSimple_BasicsSuccess) { + const std::vector>> tests = { + {"", {}}, + {"a", {"a"}}, + {"a c", {"a c"}}, + {" a c ", {"a c"}}, + {"\ta c ", {"a c"}}, + {"abc\n", {"abc"}}, + {"abc\nd f", {"abc", "d f"}}, + {"\n abc \n def \n\n", {"abc", "def"}}, + }; + + for (const auto& test : tests) { + for (int i = 0; i < kBlockSizeCount; i++) { + io::ArrayInputStream input(test.first.data(), test.first.size(), kBlockSizes[i]); + std::string err_str; + std::vector lines; + TestLineCollector collector(&lines); + EXPECT_TRUE(ParseSimpleStream(input, "dummy", &collector, &err_str)); + EXPECT_EQ(lines, test.second); + EXPECT_TRUE(err_str.empty()); + } + } +} + +TEST(ObjCHelper, ParseSimple_DropsComments) { + const std::vector>> tests = { + {"# nothing", {}}, + {"#", {}}, + {"##", {}}, + {"\n# nothing\n", {}}, + {"a # same line", {"a"}}, + {"a # same line\n", {"a"}}, + {"a\n# line\nc", {"a", "c"}}, + {"# n o t # h i n g #", {}}, + {"## n o # t h i n g #", {}}, + {"a# n o t # h i n g #", {"a"}}, + {"a\n## n o # t h i n g #", {"a"}}, + }; + + for (const auto& test : tests) { + for (int i = 0; i < kBlockSizeCount; i++) { + io::ArrayInputStream input(test.first.data(), test.first.size(), kBlockSizes[i]); + std::string err_str; + std::vector lines; + TestLineCollector collector(&lines); + EXPECT_TRUE(ParseSimpleStream(input, "dummy", &collector, &err_str)); + EXPECT_EQ(lines, test.second); + EXPECT_TRUE(err_str.empty()); + } + } +} + +TEST(ObjCHelper, ParseSimple_RejectLines) { + const std::vector> tests = { + {"a\nb\nc", "a", 1}, + {"a\nb\nc", "b", 2}, + {"a\nb\nc", "c", 3}, + {"a\nb\nc\n", "c", 3}, + }; + + for (const auto& test : tests) { + for (int i = 0; i < kBlockSizeCount; i++) { + io::ArrayInputStream input(std::get<0>(test).data(), std::get<0>(test).size(), + kBlockSizes[i]); + std::string err_str; + TestLineCollector collector(nullptr, &std::get<1>(test)); + EXPECT_FALSE(ParseSimpleStream(input, "dummy", &collector, &err_str)); + std::string expected_err = StrCat("error: dummy Line ", std::get<2>(test), ", Rejected '", std::get<1>(test), "'"); + EXPECT_EQ(err_str, expected_err); + } + } +} + // TODO(thomasvl): Should probably add some unittests for all the special cases // of name mangling (class name, field name, enum names). Rather than doing // this with an ObjC test in the objectivec directory, we should be able to