Reserve unknown in Ruby (#3763)

* Reserve unknown in ruby

* Revert ruby tests. Wait for cpp impl for conformance test

* Add conformance test for preserving unknown

* Add unknown field conformance test to csharp failure list.

* Fix comments

* Fix comment

* Fix comments

* Fix typo

* Use stringsink_string directly

* Mark hd unused

* Remove unused encodeunknown_handlerfunc

.gitignore

@@ -176,3 +176,7 @@ js/testproto_libs2.js
 ruby/lib/
 ruby/tests/generated_code_pb.rb
 ruby/tests/test_import_pb.rb
+ruby/Gemfile.lock
+ruby/compatibility_tests/v3.0.0/protoc
+ruby/compatibility_tests/v3.0.0/tests/generated_code_pb.rb
+ruby/compatibility_tests/v3.0.0/tests/test_import_pb.rb

conformance/conformance_test.cc

@@ -290,6 +290,28 @@ void ConformanceTestSuite::RunValidInputTest(
       TextFormat::ParseFromString(equivalent_text_format, reference_message))
           << "Failed to parse data for test case: " << test_name
           << ", data: " << equivalent_text_format;
+  const string equivalent_wire_format = reference_message->SerializeAsString();
+  RunValidBinaryInputTest(test_name, level, input, input_format,
+                          equivalent_wire_format, requested_output, isProto3);
+}
+
+void ConformanceTestSuite::RunValidBinaryInputTest(
+    const string& test_name, ConformanceLevel level, const string& input,
+    WireFormat input_format, const string& equivalent_wire_format,
+    WireFormat requested_output, bool isProto3) {
+  auto newTestMessage = [&isProto3]() {
+    Message* newMessage;
+    if (isProto3) {
+      newMessage = new TestAllTypesProto3;
+    } else {
+      newMessage = new TestAllTypesProto2;
+    }
+    return newMessage;
+  };
+  Message* reference_message = newTestMessage();
+  GOOGLE_CHECK(
+      reference_message->ParseFromString(equivalent_wire_format))
+          << "Failed to parse wire data for test case: " << test_name;
 
   ConformanceRequest request;
   ConformanceResponse response;
@@ -493,6 +515,19 @@ void ConformanceTestSuite::RunValidProtobufTest(
   }
 }
 
+void ConformanceTestSuite::RunValidBinaryProtobufTest(
+    const string& test_name, ConformanceLevel level,
+    const string& input_protobuf, bool isProto3) {
+  string rname = ".Proto3";
+  if (!isProto3) {
+    rname = ".Proto2";
+  }
+  RunValidBinaryInputTest(
+      ConformanceLevelToString(level) + rname + ".ProtobufInput." + test_name +
+      ".ProtobufOutput", level, input_protobuf, conformance::PROTOBUF,
+      input_protobuf, conformance::PROTOBUF, isProto3);
+}
+
 void ConformanceTestSuite::RunValidProtobufTestWithMessage(
     const string& test_name, ConformanceLevel level, const Message *input,
     const string& equivalent_text_format, bool isProto3) {
@@ -811,6 +846,14 @@ void ConformanceTestSuite::TestOneofMessage (MessageType &message,
       "OneofZeroEnum", RECOMMENDED, &message, "oneof_enum: FOO", isProto3);
 }
 
+template <class MessageType>
+void ConformanceTestSuite::TestUnknownMessage(MessageType& message,
+                                              bool isProto3) {
+  message.ParseFromString("\xA8\x1F\x01");
+  RunValidBinaryProtobufTest("UnknownVarint", REQUIRED,
+                             message.SerializeAsString(), isProto3);
+}
+
 bool ConformanceTestSuite::RunSuite(ConformanceTestRunner* runner,
                                     std::string* output) {
   runner_ = runner;
@@ -1847,6 +1890,14 @@ bool ConformanceTestSuite::RunSuite(ConformanceTestRunner* runner,
       "StringFieldSingleQuoteBoth", RECOMMENDED,
       R"({'optionalString': 'Hello world!'})");
 
+  // Unknown fields.
+  {
+    TestAllTypesProto3 messageProto3;
+    TestAllTypesProto2 messageProto2;
+    TestUnknownMessage(messageProto3, true);
+    TestUnknownMessage(messageProto2, false);
+  }
+
   // Wrapper types.
   RunValidJsonTest(
       "OptionalBoolWrapper", REQUIRED,

conformance/conformance_test.h

@@ -167,6 +167,13 @@ class ConformanceTestSuite {
                          const string& equivalent_text_format,
                          conformance::WireFormat requested_output,
                          bool isProto3);
+  void RunValidBinaryInputTest(const string& test_name,
+                               ConformanceLevel level,
+                               const string& input,
+                               conformance::WireFormat input_format,
+                               const string& equivalent_wire_format,
+                               conformance::WireFormat requested_output,
+                               bool isProto3);
   void RunValidJsonTest(const string& test_name,
                         ConformanceLevel level,
                         const string& input_json,
@@ -180,6 +187,10 @@ class ConformanceTestSuite {
                             const string& input_protobuf,
                             const string& equivalent_text_format,
                             bool isProto3);
+  void RunValidBinaryProtobufTest(const string& test_name,
+                                  ConformanceLevel level,
+                                  const string& input_protobuf,
+                                  bool isProto3);
   void RunValidProtobufTestWithMessage(
       const string& test_name, ConformanceLevel level,
       const Message *input,
@@ -212,6 +223,9 @@ class ConformanceTestSuite {
   template <class MessageType>
   void TestOneofMessage (MessageType &message,
                          bool isProto3);
+  template <class MessageType>
+  void TestUnknownMessage (MessageType &message,
+                           bool isProto3);
   void TestValidDataForType(
       google::protobuf::FieldDescriptor::Type,
       std::vector<std::pair<std::string, std::string>> values);

conformance/failure_list_csharp.txt

 Recommended.Proto3.JsonInput.BytesFieldBase64Url.JsonOutput
 Recommended.Proto3.JsonInput.BytesFieldBase64Url.ProtobufOutput
+Required.Proto3.ProtobufInput.UnknownVarint.ProtobufOutput

conformance/failure_list_ruby.txt

@@ -59,8 +59,6 @@ Required.Proto3.JsonInput.FieldMask.ProtobufOutput
 Required.Proto3.JsonInput.FloatFieldInfinity.JsonOutput
 Required.Proto3.JsonInput.FloatFieldNan.JsonOutput
 Required.Proto3.JsonInput.FloatFieldNegativeInfinity.JsonOutput
-Required.Proto3.JsonInput.FloatFieldTooLarge
-Required.Proto3.JsonInput.FloatFieldTooSmall
 Required.Proto3.JsonInput.OneofFieldDuplicate
 Required.Proto3.JsonInput.OptionalBoolWrapper.JsonOutput
 Required.Proto3.JsonInput.OptionalBoolWrapper.ProtobufOutput

ruby/ext/google/protobuf_c/encode_decode.c

@@ -44,6 +44,56 @@ VALUE noleak_rb_str_cat(VALUE rb_str, const char *str, long len) {
   return rb_str;
 }
 
+// The code below also comes from upb's prototype Ruby binding, developed by
+// haberman@.
+
+/* stringsink *****************************************************************/
+
+static void *stringsink_start(void *_sink, const void *hd, size_t size_hint) {
+  stringsink *sink = _sink;
+  sink->len = 0;
+  return sink;
+}
+
+static size_t stringsink_string(void *_sink, const void *hd, const char *ptr,
+                                size_t len, const upb_bufhandle *handle) {
+  stringsink *sink = _sink;
+  size_t new_size = sink->size;
+
+  UPB_UNUSED(hd);
+  UPB_UNUSED(handle);
+
+  while (sink->len + len > new_size) {
+    new_size *= 2;
+  }
+
+  if (new_size != sink->size) {
+    sink->ptr = realloc(sink->ptr, new_size);
+    sink->size = new_size;
+  }
+
+  memcpy(sink->ptr + sink->len, ptr, len);
+  sink->len += len;
+
+  return len;
+}
+
+void stringsink_init(stringsink *sink) {
+  upb_byteshandler_init(&sink->handler);
+  upb_byteshandler_setstartstr(&sink->handler, stringsink_start, NULL);
+  upb_byteshandler_setstring(&sink->handler, stringsink_string, NULL);
+
+  upb_bytessink_reset(&sink->sink, &sink->handler, sink);
+
+  sink->size = 32;
+  sink->ptr = malloc(sink->size);
+  sink->len = 0;
+}
+
+void stringsink_uninit(stringsink *sink) {
+  free(sink->ptr);
+}
+
 // -----------------------------------------------------------------------------
 // Parsing.
 // -----------------------------------------------------------------------------
@@ -613,6 +663,20 @@ static void add_handlers_for_oneof_field(upb_handlers *h,
   upb_handlerattr_uninit(&attr);
 }
 
+static bool unknown_field_handler(void* closure, const void* hd,
+                                  const char* buf, size_t size) {
+  UPB_UNUSED(hd);
+
+  MessageHeader* msg = (MessageHeader*)closure;
+  if (msg->unknown_fields == NULL) {
+    msg->unknown_fields = malloc(sizeof(stringsink));
+    stringsink_init(msg->unknown_fields);
+  }
+
+  stringsink_string(msg->unknown_fields, NULL, buf, size, NULL);
+
+  return true;
+}
 
 static void add_handlers_for_message(const void *closure, upb_handlers *h) {
   const upb_msgdef* msgdef = upb_handlers_msgdef(h);
@@ -634,6 +698,9 @@ static void add_handlers_for_message(const void *closure, upb_handlers *h) {
     desc->layout = create_layout(desc->msgdef);
   }
 
+  upb_handlerattr attr = UPB_HANDLERATTR_INITIALIZER;
+  upb_handlers_setunknown(h, unknown_field_handler, &attr);
+
   for (upb_msg_field_begin(&i, desc->msgdef);
        !upb_msg_field_done(&i);
        upb_msg_field_next(&i)) {
@@ -831,65 +898,6 @@ VALUE Message_decode_json(VALUE klass, VALUE data) {
 // -----------------------------------------------------------------------------
 // Serializing.
 // -----------------------------------------------------------------------------
-//
-// The code below also comes from upb's prototype Ruby binding, developed by
-// haberman@.
-
-/* stringsink *****************************************************************/
-
-// This should probably be factored into a common upb component.
-
-typedef struct {
-  upb_byteshandler handler;
-  upb_bytessink sink;
-  char *ptr;
-  size_t len, size;
-} stringsink;
-
-static void *stringsink_start(void *_sink, const void *hd, size_t size_hint) {
-  stringsink *sink = _sink;
-  sink->len = 0;
-  return sink;
-}
-
-static size_t stringsink_string(void *_sink, const void *hd, const char *ptr,
-                                size_t len, const upb_bufhandle *handle) {
-  stringsink *sink = _sink;
-  size_t new_size = sink->size;
-
-  UPB_UNUSED(hd);
-  UPB_UNUSED(handle);
-
-  while (sink->len + len > new_size) {
-    new_size *= 2;
-  }
-
-  if (new_size != sink->size) {
-    sink->ptr = realloc(sink->ptr, new_size);
-    sink->size = new_size;
-  }
-
-  memcpy(sink->ptr + sink->len, ptr, len);
-  sink->len += len;
-
-  return len;
-}
-
-void stringsink_init(stringsink *sink) {
-  upb_byteshandler_init(&sink->handler);
-  upb_byteshandler_setstartstr(&sink->handler, stringsink_start, NULL);
-  upb_byteshandler_setstring(&sink->handler, stringsink_string, NULL);
-
-  upb_bytessink_reset(&sink->sink, &sink->handler, sink);
-
-  sink->size = 32;
-  sink->ptr = malloc(sink->size);
-  sink->len = 0;
-}
-
-void stringsink_uninit(stringsink *sink) {
-  free(sink->ptr);
-}
 
 /* msgvisitor *****************************************************************/
 
@@ -1171,6 +1179,11 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc,
     }
   }
 
+  stringsink* unknown = msg->unknown_fields;
+  if (unknown != NULL) {
+    upb_sink_putunknown(sink, unknown->ptr, unknown->len);
+  }
+
   upb_sink_endmsg(sink, &status);
 }
 

ruby/ext/google/protobuf_c/message.c

@@ -44,6 +44,11 @@ void Message_mark(void* _self) {
 }
 
 void Message_free(void* self) {
+  stringsink* unknown = ((MessageHeader *)self)->unknown_fields;
+  if (unknown != NULL) {
+    stringsink_uninit(unknown);
+    free(unknown);
+  }
   xfree(self);
 }
 
@@ -67,6 +72,8 @@ VALUE Message_alloc(VALUE klass) {
   msg->descriptor = desc;
   rb_ivar_set(ret, descriptor_instancevar_interned, descriptor);
 
+  msg->unknown_fields = NULL;
+
   layout_init(desc->layout, Message_data(msg));
 
   return ret;

ruby/ext/google/protobuf_c/protobuf.h

@@ -475,8 +475,20 @@ VALUE layout_inspect(MessageLayout* layout, void* storage);
 // Message class creation.
 // -----------------------------------------------------------------------------
 
+// This should probably be factored into a common upb component.
+
+typedef struct {
+  upb_byteshandler handler;
+  upb_bytessink sink;
+  char *ptr;
+  size_t len, size;
+} stringsink;
+
+void stringsink_uninit(stringsink *sink);
+
 struct MessageHeader {
   Descriptor* descriptor;      // kept alive by self.class.descriptor reference.
+  stringsink* unknown_fields;  // store unknown fields in decoding.
   // Data comes after this.
 };
 

src/google/protobuf/test_messages_proto3.proto

@@ -215,6 +215,9 @@ message TestAllTypesProto3 {
   int32 field__Name16 = 416;
   int32 field_name17__ = 417;
   int32 Field_name18__ = 418;
+
+  // Reserved for testing unknown fields
+  reserved 501 to 510;
 }
 
 message ForeignMessage {