Merge pull request #9039 from thomasvl/test_parse_simple

ObjC: Test the simple parse helper and fix an edge case with errors in the final line

src/google/protobuf/compiler/objectivec/objectivec_helpers.cc

@@ -1638,69 +1638,69 @@ class Parser {
   Parser(LineConsumer* line_consumer)
       : line_consumer_(line_consumer), line_(0) {}
 
-  // Parses a check of input, returning success/failure.
-  bool ParseChunk(StringPiece chunk);
+  // Feeds in some input, parse what it can, returning success/failure. Calling
+  // again after an error is undefined.
+  bool ParseChunk(StringPiece chunk, std::string* out_error);
 
   // Should be called to finish parsing (after all input has been provided via
-  // ParseChunk()).  Returns success/failure.
-  bool Finish();
+  // successful calls to ParseChunk(), calling after a ParseChunk() failure is
+  // undefined). Returns success/failure.
+  bool Finish(std::string* out_error);
 
   int last_line() const { return line_; }
-  std::string error_str() const { return error_str_; }
 
  private:
-  bool ParseLoop();
-
   LineConsumer* line_consumer_;
   int line_;
-  std::string error_str_;
-  StringPiece p_;
   std::string leftover_;
 };
 
-bool Parser::ParseChunk(StringPiece chunk) {
+bool Parser::ParseChunk(StringPiece chunk, std::string* out_error) {
+  StringPiece full_chunk;
   if (!leftover_.empty()) {
     leftover_ += std::string(chunk);
-    p_ = StringPiece(leftover_);
+    full_chunk = StringPiece(leftover_);
   } else {
-    p_ = chunk;
+    full_chunk = chunk;
+  }
+
+  StringPiece line;
+  while (ReadLine(&full_chunk, &line)) {
+    ++line_;
+    RemoveComment(&line);
+    TrimWhitespace(&line);
+    if (!line.empty() && !line_consumer_->ConsumeLine(line, out_error)) {
+      if (out_error->empty()) {
+        *out_error = "ConsumeLine failed without setting an error.";
+      }
+      leftover_.clear();
+      return false;
+    }
   }
-  bool result = ParseLoop();
-  if (p_.empty()) {
+
+  if (full_chunk.empty()) {
     leftover_.clear();
   } else {
-    leftover_ = std::string(p_);
+    leftover_ = std::string(full_chunk);
   }
-  return result;
+  return true;
 }
 
-bool Parser::Finish() {
-  if (leftover_.empty()) {
-    return true;
+bool Parser::Finish(std::string* out_error) {
+  // If there is still something to go, flush it with a newline.
+  if (!leftover_.empty() && !ParseChunk("\n", out_error)) {
+    return false;
   }
-  // Force a newline onto the end to finish parsing.
-  leftover_ += "\n";
-  p_ = StringPiece(leftover_);
-  if (!ParseLoop()) {
+  // This really should never fail if ParseChunk succeeded, but check to be sure.
+  if (!leftover_.empty()) {
+    *out_error = "ParseSimple Internal error: finished with pending data.";
     return false;
   }
-  return p_.empty();  // Everything used?
+  return true;
 }
 
-bool Parser::ParseLoop() {
-  StringPiece line;
-  while (ReadLine(&p_, &line)) {
-    ++line_;
-    RemoveComment(&line);
-    TrimWhitespace(&line);
-    if (line.empty()) {
-      continue;  // Blank line.
-    }
-    if (!line_consumer_->ConsumeLine(line, &error_str_)) {
-      return false;
-    }
-  }
-  return true;
+std::string FullErrorString(const std::string& name, int line_num, const std::string& msg) {
+  return std::string("error: ") + name + " Line " + StrCat(line_num) + ", " + msg;
 }
 
 }  // namespace
@@ -1723,22 +1723,33 @@ 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) {
+  std::string local_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<const char*>(buf), buf_len))) {
-      *out_error =
-          std::string("error: ") + path +
-          " Line " + StrCat(parser.last_line()) + ", " + parser.error_str();
+    if (!parser.ParseChunk(StringPiece(static_cast<const char*>(buf), buf_len),
+                           &local_error)) {
+      *out_error = FullErrorString(stream_name, parser.last_line(), local_error);
       return false;
     }
   }
-  return parser.Finish();
+  if (!parser.Finish(&local_error)) {
+    *out_error = FullErrorString(stream_name, parser.last_line(), local_error);
+    return false;
+  }
+  return true;
 }
 
 ImportWriter::ImportWriter(

src/google/protobuf/compiler/objectivec/objectivec_helpers.h

@@ -38,6 +38,7 @@
 
 #include <google/protobuf/descriptor.h>
 #include <google/protobuf/descriptor.pb.h>
+#include <google/protobuf/io/zero_copy_stream.h>
 
 #include <google/protobuf/port_def.inc>
 
@@ -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 {

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 <google/protobuf/compiler/objectivec/objectivec_helpers.h>
+#include <google/protobuf/io/zero_copy_stream_impl_lite.h>
 #include <google/protobuf/testing/googletest.h>
 #include <gtest/gtest.h>
 
@@ -242,6 +243,133 @@ TEST(ObjCHelperDeathTest, TextFormatDecodeData_Failures) {
 }
 #endif  // PROTOBUF_HAS_DEATH_TEST
 
+class TestLineCollector : public LineConsumer {
+ public:
+  TestLineCollector(std::vector<std::string>* inout_lines,
+                    const std::string* reject_line = nullptr,
+                    bool skip_msg = false)
+    : lines_(inout_lines), reject_(reject_line), skip_msg_(skip_msg) {}
+
+  bool ConsumeLine(const StringPiece& line, std::string* out_error) override {
+    if (reject_ && *reject_ == line) {
+      if (!skip_msg_) {
+        *out_error = std::string("Rejected '") + *reject_ + "'";
+      }
+      return false;
+    }
+    if (lines_) {
+      lines_->emplace_back(line);
+    }
+    return true;
+  }
+
+ private:
+  std::vector<std::string>* lines_;
+  const std::string* reject_;
+  bool skip_msg_;
+};
+
+const int kBlockSizes[] = {-1, 1, 2, 5, 64};
+const int kBlockSizeCount = GOOGLE_ARRAYSIZE(kBlockSizes);
+
+TEST(ObjCHelper, ParseSimple_BasicsSuccess) {
+  const std::vector<std::pair<std::string, std::vector<std::string>>> 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<std::string> 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<std::pair<std::string, std::vector<std::string>>> 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<std::string> 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<std::tuple<std::string, std::string, int>> tests = {
+    std::make_tuple("a\nb\nc", "a", 1),
+    std::make_tuple("a\nb\nc", "b", 2),
+    std::make_tuple("a\nb\nc", "c", 3),
+    std::make_tuple("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);
+    }
+  }
+}
+
+TEST(ObjCHelper, ParseSimple_RejectLinesNoMessage) {
+  const std::vector<std::tuple<std::string, std::string, int>> tests = {
+    std::make_tuple("a\nb\nc", "a", 1),
+    std::make_tuple("a\nb\nc", "b", 2),
+    std::make_tuple("a\nb\nc", "c", 3),
+    std::make_tuple("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), true /* skip msg */);
+      EXPECT_FALSE(ParseSimpleStream(input, "dummy", &collector, &err_str));
+      std::string expected_err =
+        StrCat("error: dummy Line ", std::get<2>(test),
+               ", ConsumeLine failed without setting an error.");
+      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